739616 - Check or remove rv from GetUrlForUri() call, in nsMsgAttachmentHandler.cpp

Status: NEW

(MailNews Core :: Composition, defect)

Description

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from bug 736789 comment #22)
> >       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
> [I wonder why kaie never checked this rv]

Right.

***

{
588       nsCOMPtr<nsIURI> aURL;
589       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
590 
591       rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL, nsnull);
592       if (NS_FAILED(rv))
593         goto done;
}

***

Maybe check the other calls too?
http://mxr.mozilla.org/comm-central/search?string=GetUrlForUri&case=1&find=%2Fmail
"Found 23 matching lines in 22 files"

Comment 1

[David :Bienvenu]

please please be careful - injudicious checking of return values are what led to the original bug.

Comment 2

[Serge Gautherie (:sgautherie)]

Sure, I'm not saying that rv must be checked: just removing rv if the other option.
It's only the unchecked rv that looks "bad".

Yet, in this case, I would assume it should be checked:
I assume that currently it does something like "if GetUrlForUri() fails, then NS_NewInputStreamChannel(null) fails too", doesn't it?

Comment 3

[David :Bienvenu]

(In reply to Serge Gautherie (:sgautherie) from comment #2)
> Sure, I'm not saying that rv must be checked: just removing rv if the other
> option.
> It's only the unchecked rv that looks "bad".
> 
> Yet, in this case, I would assume it should be checked:
> I assume that currently it does something like "if GetUrlForUri() fails,
> then NS_NewInputStreamChannel(null) fails too", doesn't it?

I'm referring specifically to :

Maybe check the other calls too?
http://mxr.mozilla.org/comm-central/search?string=GetUrlForUri&case=1&find=%2Fmail
"Found 23 matching lines in 22 files"

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to David :Bienvenu from comment #3)
> I'm referring specifically to :
> 
> Maybe check the other calls too?

Ah! I meant "1- check whether existing rv are actually used, 2- maybe check whether calls without rv should have one" (not that all calls must have a checked rv) ;-)

Comment 5

[Atul Jangra [:atuljangra]]

Is someone working on it right now? If not, I would like to work on this bug :-)

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to Atul Jangra from comment #5)
> Is someone working on it right now? If not, I would like to work on this bug
> :-)

No so please take it. you can ask questions here, but it's faster on irc.mozilla.org in #maildev.

Comment 7

[Atul Jangra [:atuljangra]]

I should also rename aURL to url, as this is not our convention. aURL is generally used as a parameter to the routine.

{
588       nsCOMPtr<nsIURI> aURL;
589       rv = messageService->GetUrlForUri(uri.get(), getter_AddRefs(aURL), nsnull);
590 

Usul, Bienvenu should I go for this?

Comment 8

[Atul Jangra [:atuljangra]]

Attachment: First attempt

Created first version of the patch.
Gone through this: http://mxr.mozilla.org/comm-central/search?string=GetUrlForUri&case=1&find=%2Fmail. 
Looked up all the search results, and to be on safer side(as pointed out by bienvenu) I have just removed rv, where it is not used or checked.
I have also renamed aURL, as pointed out in earlier comment.
Obviously we need to do more, so just a Version 1 patch.
Cheers!!

Comment 9

[Ludovic Hirlimann [:Usul]]

Comment on attachment 640849 [details] [diff] [review]
First attempt

Jim knows this code way better than I do. I can't review anyway - passing the review along.

Comment 10

[Atul Jangra [:atuljangra]]

Attachment: Version 1

a small mistake. 
rest all same as last one.

Comment 11

[Atul Jangra [:atuljangra]]

Thanks Usul:-)

Comment 12

[Serge Gautherie (:sgautherie)]

(While here) I'm a little confused: is it expected that .idl has void whereas .cpp have NS_IMETHODIMP?

{
/mailnews/base/public/nsIMsgMessageService.idl
    * line 135 -- void GetUrlForUri(in string aMessageURI, out nsIURI aURL, in nsIMsgWindow aMsgWindow);
/mailnews/imap/src/nsImapService.cpp
    * line 241 -- NS_IMETHODIMP nsImapService::GetUrlForUri(const char *aMessageURI,
/mailnews/local/src/nsMailboxService.cpp
    * line 404 -- NS_IMETHODIMP nsMailboxService::GetUrlForUri(const char *aMessageURI, nsIURI **aURL, nsIMsgWindow *aMsgWindow)
/mailnews/news/src/nsNntpService.cpp
    * line 469 -- NS_IMETHODIMP nsNntpService::GetUrlForUri(const char *aMessageURI, nsIURI **aURL, nsIMsgWindow *aMsgWindow)

}

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 640983 [details] [diff] [review]
Version 1

Review of attachment 640983 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Atul Jangra from comment #8)

> Looked up all the search results, and to be on safer side(as pointed out by

That's a good step.

> bienvenu) I have just removed rv, where it is not used or checked.

But I'm not fond of such simple cleanup:
I meant that this should be actually analyzed and '(void)' cast should be used where we don't (want to) check the return value,
even adding a comment as need be.

Comment 14

[Jim Porter (:squib)]

(In reply to Serge Gautherie (:sgautherie) from comment #13)
> But I'm not fond of such simple cleanup:
> I meant that this should be actually analyzed and '(void)' cast should be
> used where we don't (want to) check the return value,
> even adding a comment as need be.

We definitely don't want to start adding more checks unless we're absolutely sure they won't return failure codes for legitimate values (i.e. stuff that works currently). We've already been bitten by overzealous error checking in similar code recently.

Comment 15

[Jim Porter (:squib)]

Comment on attachment 640983 [details] [diff] [review]
Version 1

Review of attachment 640983 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing out review for now. I'd r+ anything that casts those return values to void, though. Like so:

  (void)msgService->GetUrlForUri(...);

Comments might be nice, but I have no strong opinions either way for that.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this still active? Should it still be listed as a good first/mentored bug? I'm checking up on mentored bugs that haven't been changed in a while. Thanks!

Comment 17

[Atul Jangra [:atuljangra]]

Unassigning the bug if someone else wants to work on this :-)

Comment 18

[Amod [:AMoz]]

I would like to work on the bug. Will upload the patch within a while.

Comment 19

[Alexey Bugrov]

Attachment: If not checked, remove rv from GetUrlForUri() call and cast it to void

Comment 20

[Alexey Bugrov]

Anyone still alive here?.. Can I take the bug?..

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to Alexey Bugrov from comment #20)
> Anyone still alive here?.. Can I take the bug?..

please do. Helping peeps will be available in #maildev on irc.mozilla.org ask standard8 neilaway or mconley

Comment 22

[Tony Mechelynck [:tonymec]]

AFAICT, this bug is now ASSIGNED, so I'm marking it as such. This also answers lizzard's questions in comment #16, except I'm not sure sgautherie is still available for mentoring. (In reply to comment #21) Usul, do you think someone else should be written in as mentor?

Comment 23

[Ludovic Hirlimann [:Usul]]

(In reply to Tony Mechelynck [:tonymec] from comment #22)
> AFAICT, this bug is now ASSIGNED, so I'm marking it as such. This also
> answers lizzard's questions in comment #16, except I'm not sure sgautherie
> is still available for mentoring. (In reply to comment #21) Usul, do you
> think someone else should be written in as mentor?

I don't know Serge is active in SM more than TB. I think reveiw should be redirected to say standard8 if tony doesn't find more about serge's status.

Comment 24

[Tony Mechelynck [:tonymec]]

(In reply to Ludovic Hirlimann [:Usul] from comment #23)
> (In reply to Tony Mechelynck [:tonymec] from comment #22)
> > AFAICT, this bug is now ASSIGNED, so I'm marking it as such. This also
> > answers lizzard's questions in comment #16, except I'm not sure sgautherie
> > is still available for mentoring. (In reply to comment #21) Usul, do you
> > think someone else should be written in as mentor?
> 
> I don't know Serge is active in SM more than TB. I think reveiw should be
> redirected to say standard8 if tony doesn't find more about serge's status.

I think Serge is active in seaMonkey, but his all-around activity seems to come in bursts: see for yourself, https://bugzilla.mozilla.org/page.cgi?id=user_activity.html&action=run&who=sgautherie.bz%40free.fr&from=2013-04-13&to=2013-08-12&sort=when

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 778868 [details] [diff] [review]
If not checked, remove rv from GetUrlForUri() call and cast it to void

(Sorry for the delay.)

I'm not a reviewer: passing it back to :squib.

Ftr, could someone answer my comment 12?
(The C++ methods do return a value. Is it expected to "silently" ignore/mask it on IDL/JS side?)

You may want to re-add your s/aURL/url/ nit fix (later).

Comment 26

[Jim Porter (:squib)]

Comment on attachment 778868 [details] [diff] [review]
If not checked, remove rv from GetUrlForUri() call and cast it to void

Review of attachment 778868 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about taking so long on this review! rs=me.

Comment 27

[Alexey Bugrov]

Should I also add that s/aURL/url/ fix? It was Atul Jangra who added it, not me. I didn't add it because I don't know about those conventions, I'm just trying to do my first bug. :)

Comment 28

[Wayne Mery (:wsmwk)]

(In reply to Alexey Bugrov from comment #27)
> Should I also add that s/aURL/url/ fix? It was Atul Jangra who added it, not
> me. I didn't add it because I don't know about those conventions, I'm just
> trying to do my first bug. :)

squib, I suspect this question was directed at you

Comment 29

[Jim Porter (:squib)]

It would be reasonable to do that, yes. "aFoo" means that the variable is a function parameter.

Comment 30

[Jim Porter (:squib)]

Alexey: Is this patch ready to land, or did you want to do the s/aURL/url/ fix as mentioned in comment 27?

Comment 31

[Shyam Venkatesh]

So what I get from the comments is I should apply a (void) cast to all instances where GetUrlForUri()? 
I'd like to work on this bug.

Comment 32

[Jim Porter (:squib)]

No. You'll just want to merge the two most-recent patches on this bug (one of them is marked obsolete). The goal is:

1) Use a (void) cast in places where we're not checking the return value (already done in the latest patch).
2) Rename local variables named "aURL" to "url" (done in the previous patch).

Comment 33

[Jim Porter (:squib)]

To be honest, we could probably just land this patch as-is, but I didn't want to mark the patch as checkin-needed until Alexey gave the ok.

Comment 34

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 35

[Kevin Mather [:kmather73]]

Is anyone working on this right now? If not, I would like to work on this bug.

Comment 36

[Magnus Melin [:mkmelin]]

Looks abandoned, so feel free to dust it off! Instructions in comment 32

Comment 37

[Jim Porter (:squib)]

I think the patch is basically done (see comment 33), but it'd be helpful if someone could check to see if we missed anything before landing.

Comment 38

[Shaif]

Attachment: bug739616.diff

1) Used (void) cast in places where we're not checking the return value.
2) Renamed local variables named "aURL" to "url".

Comment 39

[realstealthninja]

The file nsMsgAttachmentHandler.cpp does not exist anymore.
I have found cases of aUri should these be named to uri?
as of the rv variable from what i found all the places seem to check if it succeed, will do further inspections
I have also found one case of aUrl in the file mimedrft.cpp which i have renamed to url

Comment 40

[realstealthninja]

aUri was in nsMsgComposeContentHandler.cpp @ line 102
I've also found two cases where the return value isnt being checked
nsMsgComposeService.cpp @ line 784

    rv = inStream->Read(readBuf + readOffset, maxReadCount, &readCount);

nsMsgCompose.cpp

  rv = GetMsgDBHdrFromURI(msgURI, getter_AddRefs(msgHdr));

Comment 41

[realstealthninja]

wondering if we should check them instead of simply casting them into void

Comment 42

[realstealthninja]

all mentions of GetUrlForUri()'s return value is checked

Comment 43

[realstealthninja]

i have done the required changes to the above mentioned return values if anyone would like to add anything please do.
will be posting the diff after a confirmation