Closed
Bug 1297133
Opened 8 years ago
Closed 8 years ago
Port |Bug 1295825 - Add a [must_use] attribute to XPIDL| to Mailnews
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 51.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
rkent
:
review+
|
Details | Diff | Splinter Review |
Looks like more warnings got turned into errors, so we're busted.
Assignee | ||
Comment 1•8 years ago
|
||
Aleth or Richard, I don't have a Mac, so I can only do this very painfully a few errors at a time, all I can see on treeherder. Right now, I saw eight which I fixed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5d376f556adc We can speed things up if you can give me a full list of errors from a local compile. You can just place (void) in front of the call for now and keep going. I can add error handling later. Or Aleth, you can take this bug.
Flags: needinfo?(richard.marti)
Flags: needinfo?(aleth)
Comment 2•8 years ago
|
||
My Mac Mini is a really slow one from 2009. If aleth isn't faster I can try it tomorrow evening.
Flags: needinfo?(richard.marti)
Comment 3•8 years ago
|
||
(In reply to Jorg K (GMT+2, PTO during summer) from comment #0) > Looks like more warnings got turned into errors, so we're busted. Looking at the regressing bug, it's not that these warnings weren't enabled before, it's that the certain interfaces got a must_use added, to make sure the return value is checked.
Assignee | ||
Comment 5•8 years ago
|
||
Here is another one: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0a862425193c With some luck that could be all: https://dxr.mozilla.org/comm-central/search?q=pipe-%3EGetInput&redirect=false
Assignee | ||
Comment 6•8 years ago
|
||
Assuming that I covered all the call sites, let's get some review here.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8783710 -
Flags: review?(rkent)
Attachment #8783710 -
Flags: review?(aleth)
Assignee | ||
Comment 7•8 years ago
|
||
Oops, wrong patch.
Attachment #8783710 -
Attachment is obsolete: true
Attachment #8783710 -
Flags: review?(rkent)
Attachment #8783710 -
Flags: review?(aleth)
Attachment #8783713 -
Flags: review?(rkent)
Attachment #8783713 -
Flags: review?(aleth)
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
Thanks, that's the six files I have. But I actually check the errors now.
Comment 10•8 years ago
|
||
Comment on attachment 8783713 [details] [diff] [review] v1a. Review of attachment 8783713 [details] [diff] [review]: ----------------------------------------------------------------- This basically looks r+ to me, but see what you think about these comments: ::: mailnews/addrbook/src/nsAddbookProtocolHandler.cpp @@ +162,5 @@ > > + rv = pipe->GetInputStream(getter_AddRefs(pipeIn)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = pipe->GetOutputStream(getter_AddRefs(pipeOut)); > + NS_ENSURE_SUCCESS(rv, rv); I think you can use MOZ_ALWAYS_SUCCEEDS here together with a comment, following https://hg.mozilla.org/mozilla-central/rev/44f472638f79#l2.51 Not sure if it matters though. ::: mailnews/base/util/nsMsgProtocol.cpp @@ +1425,5 @@ > mInStream = dont_AddRef(static_cast<nsIInputStream *>(inputStream)); > > nsIAsyncOutputStream *outputStream = nullptr; > + rv = pipe->GetOutputStream(&outputStream); > + NS_ENSURE_SUCCESS(rv, rv); Same here. ::: mailnews/imap/src/nsImapProtocol.cpp @@ +3027,5 @@ > // we create an "infinite" pipe in case we get extremely long lines from the imap server, > // and the consumer is waiting for a whole line > nsCOMPtr<nsIPipe> pipe = do_CreateInstance("@mozilla.org/pipe;1"); > rv = pipe->Init(false, false, 4096, PR_UINT32_MAX); > NS_ASSERTION(NS_SUCCEEDED(rv), "nsIPipe->Init failed!"); Not sure why this is an assertion - that won't help in opt builds. @@ +3032,5 @@ > > + rv = pipe->GetInputStream(getter_AddRefs(m_channelInputStream)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = pipe->GetOutputStream(getter_AddRefs(m_channelOutputStream)); > + NS_ENSURE_SUCCESS(rv, rv); Could be MOZ_ALWAYS_SUCCEEDS if the pipe->Init rv check was made reliable. ::: mailnews/mime/src/nsStreamConverter.cpp @@ +628,5 @@ > { > + rv = pipe->GetInputStream(getter_AddRefs(mInputStream)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = pipe->GetOutputStream(getter_AddRefs(mOutputStream)); > + NS_ENSURE_SUCCESS(rv, rv); see above ::: mailnews/news/src/nsNNTPProtocol.cpp @@ +2086,5 @@ > // > if (m_channelListener) { > nsCOMPtr<nsIPipe> pipe = do_CreateInstance("@mozilla.org/pipe;1"); > mozilla::DebugOnly<nsresult> rv = pipe->Init(false, false, 4096, PR_UINT32_MAX); > NS_ASSERTION(NS_SUCCEEDED(rv), "failed to create pipe"); Removing the NS_ASSERTION here in a way that doesn't cause a mess is what the //TODO comment was about I think.
Attachment #8783713 -
Flags: review?(aleth) → feedback+
Comment 11•8 years ago
|
||
Also interesting to see what the pipe->Init failure handler looks like here, compared to the c-c code: https://hg.mozilla.org/mozilla-central/rev/44f472638f79#l2.43
Assignee | ||
Updated•8 years ago
|
Attachment #8783715 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8783713 -
Attachment is obsolete: true
Attachment #8783713 -
Flags: review?(rkent)
Attachment #8783729 -
Flags: review?(rkent)
Assignee | ||
Comment 13•8 years ago
|
||
Did a little more clean-up. Why create a pipe in nsStreamConverter.cpp which we don't use?
Attachment #8783729 -
Attachment is obsolete: true
Attachment #8783729 -
Flags: review?(rkent)
Attachment #8783731 -
Flags: review?(rkent)
Comment 14•8 years ago
|
||
Comment on attachment 8783731 [details] [diff] [review] v2b. Review of attachment 8783731 [details] [diff] [review]: ----------------------------------------------------------------- I can't confirm the issues on OSX and Linux, as my builders are now all obsolete for comm-central on those platforms. But the changes make sense, others (and try) have confirmed the changes there, so this is fine.
Attachment #8783731 -
Flags: review?(rkent) → review+
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/fca207401880
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
You need to log in
before you can comment on or make changes to this bug.
Description
•