i'm not sure how serious this is, but most implementations check.

This adds the additional check to see if SetSpec succeeds. Notice that since rv is guaranteed to be true in this scope, doing the NS_ENSURE_SUCCESS does not change its value.

Taking...

Comment on attachment 110159 [details] [diff] [review] This fixes the "problem" >Index: src/nsMailboxService.cpp >+ rv = aUrl->SetSpec(aSpec); >+ NS_ENSURE_SUCCESS(rv, rv); > aMsgUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) _retval); nit, while you're at it, it would be good to replace QueryInterface with: CallQueryInterface(aMsgUrl, _retval); r=darin (w/ or w/o suggested change)

Comment on attachment 110159 [details] [diff] [review] This fixes the "problem" It doesn't look to me as though this patch will actually apply as-is; it'll need a bit of updating. With that updating, and the CallQI change suggested by darin, sr=dmose

BTW: This has been checked in but backed out again, since it did not compile: nsMailboxService.cpp: In method `nsresult nsMailboxService::NewURI(const class nsACString_internal &, const char *, class nsIURI *, class nsIURI **)': nsMailboxService.cpp:554: `aUrl' undeclared (first use this function) nsMailboxService.cpp:554: (Each undeclared identifier is reported only once nsMailboxService.cpp:554: for each function it appears in.)

Well, probably the patch just needs to be changed from aUri to aMsgUri, if you want to check in this again timeless.

This patch was made against r1.99. (In reply to comment #6) > the patch just needs to be changed from aUri to aMsgUri This change came from r1.108, which removed the |QueryInterface()| call too; but it added another |SetSpec()| call. (In reply to comment #5) > BTW: This has been checked in but backed out again This was r1.119 and r1.120. *** From <http://mxr.mozilla.org/comm-central/search?string=%3ANewURI&case=on&find=Service%5C.cpp%24> Check: <http://mxr.mozilla.org/comm-central/source/mailnews/news/src/nsNntpService.cpp#1379> Don't check: <http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapService.cpp#2416> <http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMailboxService.cpp#536> <http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsSmtpService.cpp#318> Don't use |SetSpec()|: <http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Service.cpp#296> (But has |QueryInterface()|) What do we want to do ? *Remove 1 check ? *Add 3 checks ? *Other places to look at ?

Is this still relevant? Is Mark Anderson working on it?

Whether it's still relevant or not (and no, I haven't checked), I haven't worked on it in about a decade. (Thanks for making me feel old. ;) )

Will you continue? If not, please de-assign yourself. Can anybody answer comment 7? I think I could work on the spots from that comment.

(In reply to Serge Gautherie (:sgautherie) from comment #7) http://mxr.mozilla.org/comm-central/search?string=SetSpec&case=1&find=%2Fmailnews%2F At first glance, this bug is still relevant, and there may be other occurrences to add a check to too.

OK, I can add the checks to all the missing places.

Comment on attachment 598318 [details] [diff] [review] patch, checks all spots thx this is the right thing to do, but a bit scary because some of these failures may have been harmless. In any case, I'll build and run with this. But a few comments: don't need space after NS_ENSURE_SUCCESS and before (rv, rv) (occurs a bunch of places). Please add a space between the rv,rv and then just delete the NS_ENSURE_TRUE line - if QI fails, it should set an error rv. nsCOMPtr<nsIMsgMailNewsUrl> mailnewsurl = do_QueryInterface(nntpUrl, &rv); NS_ENSURE_SUCCESS(rv,rv); - if (!mailnewsurl) return NS_ERROR_FAILURE; + NS_ENSURE_TRUE(mailnewsurl, NS_ERROR_FAILURE); same thing here (if CreateInstance fails it will set the rv): NS_ENSURE_SUCCESS(rv,rv); - if (!post) return NS_ERROR_FAILURE; + NS_ENSURE_TRUE(post, NS_ERROR_FAILURE); along with getting rid of the space after nsCOMPtr...

Comment on attachment 603016 [details] [diff] [review] patch v2 looks good, thx - one nit: + NS_ENSURE_FALSE(*newsgroupsNames == '\0', NS_ERROR_INVALID_ARG); can be NS_ENSURE_TRUE(*newsgroupsNames, NS_ERROR_INVALID_ARG);

Thanks, fixed. Carrying over r=bienvenu.

(In reply to David :Bienvenu from comment #16) > can be NS_ENSURE_TRUE(*newsgroupsNames, NS_ERROR_INVALID_ARG); Even better: NS_ENSURE_ARG(*newsgroupsNames);

(In reply to Serge Gautherie (:sgautherie) from comment #18) > (In reply to David :Bienvenu from comment #16) > > can be NS_ENSURE_TRUE(*newsgroupsNames, NS_ERROR_INVALID_ARG); > > Even better: NS_ENSURE_ARG(*newsgroupsNames); Why did you remove the checking needed serge ?

(In reply to Ludovic Hirlimann [:Usul] from comment #19) > (In reply to Serge Gautherie (:sgautherie) from comment #18) > > (In reply to David :Bienvenu from comment #16) > > > can be NS_ENSURE_TRUE(*newsgroupsNames, NS_ERROR_INVALID_ARG); > > > > Even better: NS_ENSURE_ARG(*newsgroupsNames); > > Why did you remove the checking needed serge ? To wait for patch update with my nit, obviously.

You could have written it more obviously.

This patch fails to apply to comm-central.

Comment on attachment 603706 [details] [diff] [review] patch v4 [Checked in: Comment 23] http://hg.mozilla.org/comm-central/rev/53fcee1f9ec3

That explains that!

What was the problem?

It had already landed and I had been too distracted to remember to get back to updating the bug. Sorry folks.

A few calls remain unchecked and without comment+void, in nsMsgMaildirStore.cpp and nsImapService.cpp Shouldn't we document/fix them too?

I think I intentionally skipped that one in nsMsgMaildirStore.cpp as it returned void, so I didn't know what to do. Any idea? I don't know why I missed the two in nsImapService.cpp. I can add them.

(In reply to :aceman from comment #28) > I think I intentionally skipped that one in nsMsgMaildirStore.cpp as it > returned void, so I didn't know what to do. Any idea? This file was added in bug 402392 by David: he should know. 2 hints, fwiw: *http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1620 a comment + "(void)". *http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgMaildirStore.cpp#1160 "// ### TODO - what if this fails?". > I don't know why I missed the two in nsImapService.cpp. I can add them. Yes please.

Fixes the leftover spots. The large changes in NewURI are just removing one indentation level as the whole function was in one large if (NS_SUCCEEDED(rv)) {} so I converted it to early return (NS_ENSURE_SUCCESS). I hope it is OK.

Comment on attachment 606974 [details] [diff] [review] patch, part 2 Review of attachment 606974 [details] [diff] [review]: ----------------------------------------------------------------- You should attach a 'diff -w' too, to ease review. ::: mailnews/local/src/nsMsgMaildirStore.cpp @@ +1145,5 @@ > nsCOMPtr<nsIMsgMailNewsUrl> url = do_QueryInterface(mailboxurl); > url->SetUpdatingFolder(true); > nsCAutoString uriSpec("mailbox://"); > + // ### TODO - what if SetSpec fails? > + (void)url->SetSpec(uriSpec); Nit Add a space after ')'.

With nit.

The changes in nsImapService in diff -w.

Comment on attachment 607318 [details] [diff] [review] part 2, v2 (nsImapService.cpp, diff -w) Review of attachment 607318 [details] [diff] [review]: ----------------------------------------------------------------- ::: nsImapService.cpp.org @@ +2596,3 @@ > } > else > + rv = mailnewsUrl->SetSpec(aSpec); While here, add {} to else.

Same as v2, just added the brackets.

Comment on attachment 603706 [details] [diff] [review] patch v4 [Checked in: Comment 23] this caused an error forwarding certain messages as an attachment, in particular, this change: - if (aURL) - aURL->SetSpec(nsDependentCString(uri.get())); + if (aURL) { + rv = aURL->SetSpec(nsDependentCString(uri.get())); + if (NS_FAILED(rv)) + goto done; + } I think I remember seeing a bug go by about this. The uri in question has a ? at the end, which is probably why setspec failed (though I haven't debugged that part).

Another possible regression is bug 736782.

Comment on attachment 607326 [details] [diff] [review] part 2, v3 [Checked in: Comment 39] thx, this looks reasonable. The SetSpec failure handling scares me a little, but not as much as the mailbox url set spec code that caused so much trouble. We'll have to keep watch for any new imap problems.