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

NEW
Assigned to

Status

MailNews Core
Composition
6 years ago
10 months ago

People

(Reporter: sgautherie, Assigned: kmather73, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove][good first bug][lang=c++], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
(In reply to neil@parkwaycc.co.uk 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

6 years ago
please please be careful - injudicious checking of return values are what led to the original bug.
(Reporter)

Comment 2

6 years ago
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

6 years ago
(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"
(Reporter)

Comment 4

6 years ago
(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) ;-)
Is someone working on it right now? If not, I would like to work on this bug :-)
(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.
Assignee: nobody → atuljangra66
Status: NEW → ASSIGNED
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?
Created attachment 640849 [details] [diff] [review]
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!!
Attachment #640849 - Flags: review?(sgautherie.bz)
Attachment #640849 - Flags: review?(ludovic)
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.
Attachment #640849 - Flags: review?(ludovic) → review?(squibblyflabbetydoo)
Created attachment 640983 [details] [diff] [review]
Version 1

a small mistake. 
rest all same as last one.
Attachment #640849 - Attachment is obsolete: true
Attachment #640849 - Flags: review?(squibblyflabbetydoo)
Attachment #640849 - Flags: review?(sgautherie.bz)
Attachment #640983 - Flags: review?(squibblyflabbetydoo)
Attachment #640983 - Flags: review?(sgautherie.bz)
Thanks Usul:-)
(Reporter)

Comment 12

5 years ago
(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)

}
(Reporter)

Comment 13

5 years ago
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.
Attachment #640983 - Flags: review?(sgautherie.bz) → review-
(Reporter)

Updated

5 years ago
Attachment #640983 - Flags: review- → feedback-

Comment 14

5 years ago
(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

5 years ago
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.
Attachment #640983 - Flags: review?(squibblyflabbetydoo)
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!
Flags: needinfo?(sgautherie.bz)
Unassigning the bug if someone else wants to work on this :-)
Status: ASSIGNED → NEW
Assignee: atuljangra66 → nobody

Comment 18

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

Comment 19

4 years ago
Created attachment 778868 [details] [diff] [review]
If not checked, remove rv from GetUrlForUri() call and cast it to void

Comment 20

4 years ago
Anyone still alive here?.. Can I take the bug?..
(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

Updated

4 years ago
Attachment #778868 - Flags: review?(sgautherie.bz)
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?
Assignee: nobody → alexey0bugrov
Status: NEW → ASSIGNED
Flags: needinfo?(ludovic)
(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.
Flags: needinfo?(ludovic)
(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
(Reporter)

Comment 25

4 years ago
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).
Attachment #778868 - Flags: review?(sgautherie.bz) → review?(squibblyflabbetydoo)
(Reporter)

Updated

4 years ago
Flags: needinfo?(sgautherie.bz)

Comment 26

4 years ago
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.
Attachment #778868 - Flags: review?(squibblyflabbetydoo) → review+

Comment 27

4 years ago
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. :)
Mentor: bugzillamozillaorg_serge_20140323@gautherie.fr
Whiteboard: [good first bug][mentor=sgautherie][lang=c++] → [good first bug][lang=c++]

Comment 28

3 years ago
(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
Flags: needinfo?(squibblyflabbetydoo)

Comment 29

3 years ago
It would be reasonable to do that, yes. "aFoo" means that the variable is a function parameter.
Flags: needinfo?(squibblyflabbetydoo)

Comment 30

3 years ago
Alexey: Is this patch ready to land, or did you want to do the s/aURL/url/ fix as mentioned in comment 27?
Flags: needinfo?(alexey0bugrov)

Updated

3 years ago
Attachment #640983 - Attachment is obsolete: true

Updated

3 years ago
Assignee: alexey0bugrov → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(alexey0bugrov)

Updated

3 years ago
Whiteboard: [good first bug][lang=c++] → [patchlove][good first bug][lang=c++]

Comment 31

3 years ago
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.

Updated

3 years ago
Assignee: nobody → shyamvenkatesh95
Flags: needinfo?(squibblyflabbetydoo)

Comment 32

3 years ago
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).
Flags: needinfo?(squibblyflabbetydoo)

Comment 33

3 years ago
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.
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
(Assignee)

Comment 35

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

Comment 36

2 years ago
Looks abandoned, so feel free to dust it off! Instructions in comment 32
Assignee: shyamvenkatesh95 → kmather73

Comment 37

2 years ago
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

2 years ago
Created attachment 8729806 [details] [diff] [review]
bug739616.diff

1) Used (void) cast in places where we're not checking the return value.
2) Renamed local variables named "aURL" to "url".
Attachment #8729806 - Flags: review?(bugzillamozillaorg_serge_20140323)
Attachment #8729806 - Flags: feedback?

Updated

2 years ago
Attachment #8729806 - Flags: review?(bugzillamozillaorg_serge_20140323)
Attachment #8729806 - Flags: review?
Attachment #8729806 - Flags: feedback?

Updated

2 years ago
Attachment #8729806 - Flags: review?
You need to log in before you can comment on or make changes to this bug.