Closed
Bug 1301706
Opened 8 years ago
Closed 8 years ago
nsIURI.getSpec is fallible. Add missing checks to GetSpec() calls in /mailnews
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 51.0
People
(Reporter: philip.chee, Assigned: jorgk-bmo)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
(In reply to Nicholas Nethercote [:njn] from Bug 1297300 comment #0) > nsIURI.getSpec is fallible, but plenty of call sites don't check the return > value. Adding [must_use] to the IDL will identify those call sites. > > See bug 1290350 comment 6 for an example of how this can cause problems.
Assignee | ||
Comment 1•8 years ago
|
||
Aleth, if you have time, could you get me the full list of compilation errors on Mac. I've just done a push that will show me some at least. Of course I can also look through DXR. Many times we check the return value, but some times we don't.
Flags: needinfo?(aleth)
Comment 2•8 years ago
|
||
In this case dxr is easier and faster than a compile log: https://dxr.mozilla.org/comm-central/search?q=getspec(+ext%3Acpp+path%3Amailnews&redirect=true
Flags: needinfo?(aleth)
Comment 3•8 years ago
|
||
I'll be very curious to see if this helps with some of our OOM | large crashes, which I was just looking at a few days ago.
Assignee | ||
Comment 4•8 years ago
|
||
I went through in DXR and looks for "Getspec()" without checking the return value. Some where in #ifdef DEBUG ... #endif and I put a (void) in front. Some weren't used, so I remove them. Some functions calling Getspec() were void or bool, so I returned or returned the "default" value. I don't claim I found them all. This can currently not be tested since the M-C part hasn't landed. If I do a push with [must_use] as an M-C patch, I'm afraid that M-C won't compile since bug 1301607 isn't done yet. So we could land this now and hope that we don't get bustage later or at least very little. Or we wait and land this as a bustage fix later.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8789917 -
Flags: review?(rkent)
Attachment #8789917 -
Flags: review?(mkmelin+mozilla)
Comment 5•8 years ago
|
||
Comment on attachment 8789917 [details] [diff] [review] First attempt. Review of attachment 8789917 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +5212,5 @@ > if (session) > session->IsFolderOpenInWindow(this, &folderOpen); > #ifdef DEBUG_bienvenu1 > nsCString urlSpec; > + (void) aUrl->GetSpec(getter_Copies(urlSpec)); this wasn't the way to fix these... but it's ifdef bienvenue1 so you might want to remove all of the ifdef block ::: mailnews/imap/src/nsImapProtocol.cpp @@ +2128,5 @@ > if (aURL) > { > #ifdef DEBUG_bienvenu > nsAutoCString urlSpec; > + (void) aURL->GetSpec(urlSpec); same here ::: mailnews/mime/src/mimedrft.cpp @@ +143,5 @@ > > if ( tmp->m_url ) > { > nsAutoCString spec; > + (void) tmp->m_url->GetSpec(spec); shouldn't you use the "Unused" thing?
Assignee | ||
Comment 6•8 years ago
|
||
Yes, in the #ifdef DEBUG_bienvenu1 I was slack ;-) The one in mimedrft.cpp is in #ifdef NS_DEBUG extern "C" void mime_dump_attachments ( nsMsgAttachmentData *attachData ) but the patch context doesn't show that. The Β«"Unused" thingΒ» and (void) are actually equivalent, only that (quoting from dev-platform): cast-to-void is commonly suggested as an alternative to an explicit unused marking, and it is something that I wanted to use originally. Unfortunately, we have not been able to make that work: this is primarily because compilers often remove the cast-to-void as part of the parsing phase, so it's not visible in the parse tree for static checkers. In debug I think (void) is the way to go.
Comment 7•8 years ago
|
||
I think that'll be confusing when someone's looking at code. I wonder if anyone ever uses the NS_DEBUG portions and such though...
Assignee | ||
Comment 8•8 years ago
|
||
If you want to remove the debug sections, I can remove them. I find them useful as an indication where D/B used to have his debug. And I personally wouldn't remove mime_dump_attachments(). I don't want to #include "mozilla/Unused.h" conditionally only to use it in debug. I also don't see which confusion you're talking about. So what do you want to do: Remove, use "unused" or can we leave the (void)?
Comment 10•8 years ago
|
||
(In reply to Jorg K (GMT+2, PTO during summer) from comment #9) > One of the (void)s is in #ifdef PR_LOGGING. iirc that's enabled in debug builds.
Comment 11•8 years ago
|
||
I'd say you can include "mozilla/Unused.h" unconditionally and use Unused where needed, even in ifdef blocks. It's confusing (kind of a trap) to have code that's wrong "just because it's in the debug section and we don't care".
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Magnus Melin from comment #11) > It's confusing (kind of a trap) to have code that's wrong "just because it's > in the debug section and we don't care". It's not wrong. It's 100% fine only that "static checkers" don't like it. But guess what: They'll never see it since it's debug only.
Assignee | ||
Comment 14•8 years ago
|
||
r+ as per comment #12. In the #ifdef DEBUG_bienvenu I put a comment so no one gets confused. In mime_dump_attachments() which is C code the Unused can't be used, so I added another comment. In the file with #ifdef PR_LOGGING I included "mozilla/Unused.h" conditionally. I hope everyone is satisfied this way. I'll do a try run tomorrow when bug 1300152 is backed out from M-C because currently there are so many X failures that I can't see the forest for the trees :-(
Attachment #8789917 -
Attachment is obsolete: true
Attachment #8789917 -
Flags: review?(rkent)
Attachment #8789917 -
Flags: review?(mkmelin+mozilla)
Attachment #8789943 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
I found the solution that will make all the purists happy ;-) GetSpecOrDefault() can be used for debugging or printing purposes. It's infallible. Use interdiff to see it. While I was there, I removed some more (void)s found elsewhere in debug.
Attachment #8789943 -
Attachment is obsolete: true
Attachment #8789982 -
Flags: review+
Assignee | ||
Comment 16•8 years ago
|
||
No additional failures on try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=366b057a0fe8ff7fef00f211cbb7b3788eb88e1b Landed: https://hg.mozilla.org/comm-central/rev/33b093acd0a6 Let's leave this bug open for a few days so I can check DXR again after it refreshed. Perhaps M-C make the return value check mandatory in the meantime.
Keywords: leave-open
Assignee | ||
Comment 17•8 years ago
|
||
On second thought, the URL is pretty much optional here, so let's just kick on if it can't be retrieved instead of skipping the attachment. Do you agree?
Attachment #8790034 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 18•8 years ago
|
||
(changed comment.)
Attachment #8790034 -
Attachment is obsolete: true
Attachment #8790034 -
Flags: review?(mkmelin+mozilla)
Attachment #8790037 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 19•8 years ago
|
||
OK, DXR has refreshed now and I missed three cases. These are included here together with the change in mimemoz2.cpp where I'd like to ignore the error.
Attachment #8790037 -
Attachment is obsolete: true
Attachment #8790037 -
Flags: review?(mkmelin+mozilla)
Attachment #8790059 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 20•8 years ago
|
||
More clean-up.
Attachment #8790059 -
Attachment is obsolete: true
Attachment #8790059 -
Flags: review?(mkmelin+mozilla)
Attachment #8790077 -
Flags: review?(mkmelin+mozilla)
Updated•8 years ago
|
Attachment #8790077 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Landed follow-up: https://hg.mozilla.org/comm-central/rev/13758326e126 I believe it's all covered now. Should there still be bustage when M-C adds must_use to the IDL, we can reopen this bug or open a new one after the next branch date Sept. 19th, 2016.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
You need to log in
before you can comment on or make changes to this bug.
Description
•