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 User image timeless 2002-12-24 23:28:44 PST
i'm not sure how serious this is, but most implementations check.
Comment 1 User image 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 User image Mark Anderson 2002-12-26 12:23:28 PST
Taking...
Comment 3 User image 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 User image 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 User image 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 User image 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 User image 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 User image :aceman 2012-01-23 08:38:12 PST
Is this still relevant? Is Mark Anderson working on it?
Comment 9 User image 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 User image :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 User image 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 User image :aceman 2012-02-06 12:08:22 PST
OK, I can add the checks to all the missing places.
Comment 13 User image :aceman 2012-02-17 11:56:23 PST
Created attachment 598318 [details] [diff] [review]
patch, checks all spots
Comment 14 User image 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 User image :aceman 2012-03-05 12:24:36 PST
Created attachment 603016 [details] [diff] [review]
patch v2
Comment 16 User image 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 User image :aceman 2012-03-05 13:09:27 PST
Created attachment 603032 [details] [diff] [review]
patch v3

Thanks, fixed. Carrying over r=bienvenu.
Comment 18 User image 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 User image 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 User image 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 User image :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 User image Ryan VanderMeulen [:RyanVM] 2012-03-07 16:40:17 PST
This patch fails to apply to comm-central.
Comment 23 User image 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 User image Ryan VanderMeulen [:RyanVM] 2012-03-07 16:50:29 PST
That explains that!
Comment 25 User image :aceman 2012-03-07 22:50:32 PST
What was the problem?
Comment 26 User image Mark Banner (:standard8) 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 User image 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 User image :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 User image 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 User image :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 User image 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 User image :aceman 2012-03-19 14:14:29 PDT
Created attachment 607316 [details] [diff] [review]
part 2, v2

With nit.
Comment 33 User image :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 User image 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 User image :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 User image 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 User image :aceman 2012-03-21 08:57:49 PDT
Another possible regression is bug 736782.
Comment 38 User image 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 User image 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.