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

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Philip Chee]

(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.

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[aleth [:aleth]]

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

Comment 3

[Wayne Mery (:wsmwk)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 5

[Magnus Melin [:mkmelin]]

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?

Comment 6

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

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...

Comment 8

[Jorg K (CEST = GMT+2)]

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 9

[Jorg K (CEST = GMT+2)]

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

Comment 10

[aleth [:aleth]]

(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

[Magnus Melin [:mkmelin]]

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

[Magnus Melin [:mkmelin]]

Feel free to take that as a r+ once sorted out.

Comment 13

[Jorg K (CEST = GMT+2)]

(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.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 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 :-(

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 16

[Jorg K (CEST = GMT+2)]

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.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 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?

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Follow up (v1b).

(changed comment.)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: Follow up (v2b).

More clean-up.

Comment 21

[Jorg K (CEST = GMT+2)]

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.