186724 - add checks to see if SetSpec() failed in various spots

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[timeless]

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

Comment 1

[Mark Anderson]

Attachment: 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]

Taking...

Comment 3

[Darin Fisher]

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 (:dmosedale, :dmose)]

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]]

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]]

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)]

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]

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

Comment 9

[Mark Anderson]

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]

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)]

(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]

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

Comment 13

[:aceman]

Attachment: patch, checks all spots

Comment 14

[David :Bienvenu]

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]

Attachment: patch v2

Comment 16

[David :Bienvenu]

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]

Attachment: patch v3

Thanks, fixed. Carrying over r=bienvenu.

Comment 18

[Serge Gautherie (:sgautherie)]

(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]]

(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)]

(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]

Attachment: patch v4 [Checked in: Comment 23]

You could have written it more obviously.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This patch fails to apply to comm-central.

Comment 23

[Serge Gautherie (:sgautherie)]

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]]

That explains that!

Comment 25

[:aceman]

What was the problem?

Comment 26

[Mark Banner (:standard8)]

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)]

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]

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)]

(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]

Attachment: 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)]

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]

Attachment: part 2, v2

With nit.

Comment 33

[:aceman]

Attachment: part 2, v2 (nsImapService.cpp, diff -w)

The changes in nsImapService in diff -w.

Comment 34

[Serge Gautherie (:sgautherie)]

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]

Attachment: part 2, v3 [Checked in: Comment 39]

Same as v2, just added the brackets.

Comment 36

[David :Bienvenu]

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]

Another possible regression is bug 736782.

Comment 38

[David :Bienvenu]

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]]

https://hg.mozilla.org/comm-central/rev/10f68e70c91a