Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsIURI.getSpec is fallible. Add missing checks to GetSpec() calls in /mailnews

RESOLVED FIXED in Thunderbird 51.0

Status

MailNews Core
Backend
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Philip Chee, Assigned: Jorg K (GMT+2))

Tracking

unspecified
Thunderbird 51.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

11 months ago
(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

11 months 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

11 months 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)
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

11 months ago
Created attachment 8789917 [details] [diff] [review]
First attempt.

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

11 months 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

11 months 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

11 months 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

11 months 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)?
(Assignee)

Comment 9

11 months ago
One of the (void)s is in #ifdef PR_LOGGING.

Comment 10

11 months 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

11 months 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".

Comment 12

11 months ago
Feel free to take that as a r+ once sorted out.
(Assignee)

Comment 13

11 months 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

11 months ago
Created attachment 8789943 [details] [diff] [review]
Second attempt.

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 #8789943 - Flags: review+
Attachment #8789917 - Flags: review?(rkent)
Attachment #8789917 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 15

11 months ago
Created attachment 8789982 [details] [diff] [review]
Third attempt.

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

11 months 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

11 months ago
Created attachment 8790034 [details] [diff] [review]
Follow up.

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

11 months ago
Created attachment 8790037 [details] [diff] [review]
Follow up (v1b).

(changed comment.)
Attachment #8790034 - Attachment is obsolete: true
Attachment #8790034 - Flags: review?(mkmelin+mozilla)
Attachment #8790037 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 19

11 months ago
Created attachment 8790059 [details] [diff] [review]
Follow up (v2a).

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

11 months ago
Created attachment 8790077 [details] [diff] [review]
Follow up (v2b).

More clean-up.
Attachment #8790059 - Attachment is obsolete: true
Attachment #8790059 - Flags: review?(mkmelin+mozilla)
Attachment #8790077 - Flags: review?(mkmelin+mozilla)

Updated

11 months ago
Attachment #8790077 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 21

11 months 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
Last Resolved: 11 months 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.