Last Comment Bug 186724 - add checks to see if SetSpec() failed in various spots
: add checks to see if SetSpec() failed in various spots
Status: RESOLVED FIXED
[fixed in TB13.0: part 1][fixed in TB...
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 736782 736789 763319
Blocks: 447994 186719
  Show dependency treegraph
 
Reported: 2002-12-24 23:28 PST by timeless
Modified: 2012-06-10 13:56 PDT (History)
7 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
This fixes the "problem" (659 bytes, patch)
2002-12-26 11:57 PST, Mark Anderson
darin.moz: review+
dmose: superreview+
Details | Diff | Splinter Review
patch, checks all spots (11.47 KB, patch)
2012-02-17 11:56 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (12.58 KB, patch)
2012-03-05 12:24 PST, :aceman
mozilla: review+
Details | Diff | Splinter Review
patch v3 (12.57 KB, patch)
2012-03-05 13:09 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
patch v4 [Checked in: Comment 23] (12.55 KB, patch)
2012-03-07 07:01 PST, :aceman
acelists: review+
Details | Diff | Splinter Review
patch, part 2 (7.15 KB, patch)
2012-03-18 08:21 PDT, :aceman
no flags Details | Diff | Splinter Review
part 2, v2 (7.17 KB, patch)
2012-03-19 14:14 PDT, :aceman
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
part 2, v2 (nsImapService.cpp, diff -w) (1.33 KB, patch)
2012-03-19 14:15 PDT, :aceman
mozilla: feedback+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
part 2, v3 [Checked in: Comment 39] (7.17 KB, patch)
2012-03-19 14:48 PDT, :aceman
mozilla: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review

Description timeless 2002-12-24 23:28:44 PST
i'm not sure how serious this is, but most implementations check.
Comment 1 Mark Anderson 2002-12-26 11:57:12 PST
Created attachment 110159 [details] [diff] [review]
This fixes the "problem"

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.
Comment 2 Mark Anderson 2002-12-26 12:23:28 PST
Taking...
Comment 3 Darin Fisher 2003-01-02 14:19:52 PST
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 4 Dan Mosedale (:dmose) 2005-10-18 15:15:15 PDT
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
Comment 5 Frank Wein [:mcsmurf] 2005-11-14 08:46:52 PST
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.)
Comment 6 Frank Wein [:mcsmurf] 2005-11-14 08:48:31 PST
Well, probably the patch just needs to be changed from aUri to aMsgUri, if you want to check in this again timeless.
Comment 7 Serge Gautherie (:sgautherie) 2008-07-25 10:01:33 PDT
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 ?
Comment 8 :aceman 2012-01-23 08:38:12 PST
Is this still relevant? Is Mark Anderson working on it?
Comment 9 Mark Anderson 2012-01-23 08:41:33 PST
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. ;) )
Comment 10 :aceman 2012-01-23 08:52:50 PST
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.
Comment 11 Serge Gautherie (:sgautherie) 2012-01-23 09:21:15 PST
(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.
Comment 12 :aceman 2012-02-06 12:08:22 PST
OK, I can add the checks to all the missing places.
Comment 13 :aceman 2012-02-17 11:56:23 PST
Created attachment 598318 [details] [diff] [review]
patch, checks all spots
Comment 14 David :Bienvenu 2012-03-05 10:38:29 PST
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 15 :aceman 2012-03-05 12:24:36 PST
Created attachment 603016 [details] [diff] [review]
patch v2
Comment 16 David :Bienvenu 2012-03-05 12:53:57 PST
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);
Comment 17 :aceman 2012-03-05 13:09:27 PST
Created attachment 603032 [details] [diff] [review]
patch v3

Thanks, fixed. Carrying over r=bienvenu.
Comment 18 Serge Gautherie (:sgautherie) 2012-03-05 13:29:31 PST
(In reply to David :Bienvenu from comment #16)
> can be NS_ENSURE_TRUE(*newsgroupsNames, NS_ERROR_INVALID_ARG);

Even better: NS_ENSURE_ARG(*newsgroupsNames);
Comment 19 Ludovic Hirlimann [:Usul] 2012-03-07 06:19:20 PST
(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 ?
Comment 20 Serge Gautherie (:sgautherie) 2012-03-07 06:56:46 PST
(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.
Comment 21 :aceman 2012-03-07 07:01:00 PST
Created attachment 603706 [details] [diff] [review]
patch v4
[Checked in: Comment 23]

You could have written it more obviously.
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-03-07 16:40:17 PST
This patch fails to apply to comm-central.
Comment 23 Serge Gautherie (:sgautherie) 2012-03-07 16:49:39 PST
Comment on attachment 603706 [details] [diff] [review]
patch v4
[Checked in: Comment 23]

http://hg.mozilla.org/comm-central/rev/53fcee1f9ec3
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-03-07 16:50:29 PST
That explains that!
Comment 25 :aceman 2012-03-07 22:50:32 PST
What was the problem?
Comment 26 Mark Banner (:standard8, limited time in Dec) 2012-03-08 02:36:33 PST
It had already landed and I had been too distracted to remember to get back to updating the bug. Sorry folks.
Comment 27 Serge Gautherie (:sgautherie) 2012-03-08 06:05:14 PST
A few calls remain unchecked and without comment+void, in
nsMsgMaildirStore.cpp and nsImapService.cpp

Shouldn't we document/fix them too?
Comment 28 :aceman 2012-03-12 14:10:56 PDT
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.
Comment 29 Serge Gautherie (:sgautherie) 2012-03-12 16:50:29 PDT
(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.
Comment 30 :aceman 2012-03-18 08:21:22 PDT
Created attachment 606974 [details] [diff] [review]
patch, part 2

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 31 Serge Gautherie (:sgautherie) 2012-03-18 20:31:56 PDT
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 ')'.
Comment 32 :aceman 2012-03-19 14:14:29 PDT
Created attachment 607316 [details] [diff] [review]
part 2, v2

With nit.
Comment 33 :aceman 2012-03-19 14:15:40 PDT
Created attachment 607318 [details] [diff] [review]
part 2, v2 (nsImapService.cpp, diff -w)

The changes in nsImapService in diff -w.
Comment 34 Serge Gautherie (:sgautherie) 2012-03-19 14:40:32 PDT
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.
Comment 35 :aceman 2012-03-19 14:48:54 PDT
Created attachment 607326 [details] [diff] [review]
part 2, v3 [Checked in: Comment 39]

Same as v2, just added the brackets.
Comment 36 David :Bienvenu 2012-03-21 08:54:40 PDT
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).
Comment 37 :aceman 2012-03-21 08:57:49 PDT
Another possible regression is bug 736782.
Comment 38 David :Bienvenu 2012-05-08 14:09:18 PDT
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.
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-05-09 15:32:44 PDT
https://hg.mozilla.org/comm-central/rev/10f68e70c91a

Note You need to log in before you can comment on or make changes to this bug.