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

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
--
minor
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: timeless, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 13.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed in TB13.0: part 1][fixed in TB15.0: part 2], URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

15 years ago
Taking...
Assignee: mscott → andersma
(Reporter)

Updated

15 years ago
Attachment #110159 - Flags: superreview?(sspitzer)
Attachment #110159 - Flags: review?(darin)

Comment 3

15 years ago
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)
Attachment #110159 - Flags: review?(darin) → review+
Product: MailNews → Core
(Reporter)

Updated

12 years ago
Attachment #110159 - Flags: superreview?(sspitzer) → superreview?(dmose)

Comment 4

12 years ago
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
Attachment #110159 - Flags: superreview?(dmose) → superreview+

Comment 5

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

12 years ago
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 ?
Severity: normal → minor
OS: Windows 2000 → All
QA Contact: grylchan → mailnews.networking
Hardware: PC → All
Summary: mailbox service NewURI doesn't check to see if SetSpec failed.. → mailbox service NewURI() should check to see if SetSpec() failed
Product: Core → MailNews Core
(Assignee)

Comment 8

6 years ago
Is this still relevant? Is Mark Anderson working on it?

Comment 9

6 years ago
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. ;) )
(Assignee)

Comment 10

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

Updated

6 years ago
Assignee: hamfastgamgee → nobody
(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.
(Assignee)

Comment 12

6 years ago
OK, I can add the checks to all the missing places.
Assignee: nobody → acelists
(Assignee)

Updated

6 years ago
Component: Networking → Backend
QA Contact: networking → backend
Summary: mailbox service NewURI() should check to see if SetSpec() failed → add checks to see if SetSpec() failed in various spots
(Assignee)

Comment 13

6 years ago
Created attachment 598318 [details] [diff] [review]
patch, checks all spots
Attachment #110159 - Attachment is obsolete: true
Attachment #598318 - Flags: review?(dbienvenu)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED

Comment 14

6 years ago
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...
(Assignee)

Comment 15

6 years ago
Created attachment 603016 [details] [diff] [review]
patch v2
Attachment #598318 - Attachment is obsolete: true
Attachment #603016 - Flags: review?(dbienvenu)
Attachment #598318 - Flags: review?(dbienvenu)

Comment 16

6 years ago
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);
Attachment #603016 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 17

6 years ago
Created attachment 603032 [details] [diff] [review]
patch v3

Thanks, fixed. Carrying over r=bienvenu.
Attachment #603016 - Attachment is obsolete: true
Attachment #603032 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(In reply to David :Bienvenu from comment #16)
> can be NS_ENSURE_TRUE(*newsgroupsNames, NS_ERROR_INVALID_ARG);

Even better: NS_ENSURE_ARG(*newsgroupsNames);
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 13.0
(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.
(Assignee)

Comment 21

6 years ago
Created attachment 603706 [details] [diff] [review]
patch v4
[Checked in: Comment 23]

You could have written it more obviously.
Attachment #603032 - Attachment is obsolete: true
Attachment #603706 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
This patch fails to apply to comm-central.
Keywords: checkin-needed
Comment on attachment 603706 [details] [diff] [review]
patch v4
[Checked in: Comment 23]

http://hg.mozilla.org/comm-central/rev/53fcee1f9ec3
Attachment #603706 - Attachment description: patch v4 → patch v4 [Checked in: Comment 23]
That explains that!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

6 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

6 years ago
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.
(Assignee)

Comment 30

6 years ago
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.
Attachment #606974 - Flags: review?(dbienvenu)
Attachment #606974 - Flags: feedback?(sgautherie.bz)
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 ')'.
Attachment #606974 - Flags: feedback?(sgautherie.bz)
(Assignee)

Updated

6 years ago
Blocks: 447994
(Assignee)

Comment 32

6 years ago
Created attachment 607316 [details] [diff] [review]
part 2, v2

With nit.
Attachment #606974 - Attachment is obsolete: true
Attachment #607316 - Flags: review?(dbienvenu)
Attachment #606974 - Flags: review?(dbienvenu)
(Assignee)

Comment 33

6 years ago
Created attachment 607318 [details] [diff] [review]
part 2, v2 (nsImapService.cpp, diff -w)

The changes in nsImapService in diff -w.
Attachment #607318 - Flags: feedback?(dbienvenu)
Attachment #607318 - Attachment is patch: true
Attachment #607318 - Attachment description: part 2, v2 (diff -w) → part 2, v2 (nsImapService.cpp, 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.
Attachment #607318 - Flags: feedback+
Attachment #607316 - Flags: feedback+
(Assignee)

Comment 35

6 years ago
Created attachment 607326 [details] [diff] [review]
part 2, v3 [Checked in: Comment 39]

Same as v2, just added the brackets.
Attachment #607316 - Attachment is obsolete: true
Attachment #607326 - Flags: review?(dbienvenu)
Attachment #607316 - Flags: review?(dbienvenu)
Attachment #607326 - Flags: feedback+
(Assignee)

Updated

6 years ago
Status: REOPENED → ASSIGNED

Comment 36

6 years ago
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).
(Assignee)

Comment 37

6 years ago
Another possible regression is bug 736782.
Depends on: 736782
Whiteboard: [fixed in TB13.0: part 1]

Updated

6 years ago
Depends on: 736789

Updated

5 years ago
Attachment #607318 - Flags: feedback?(dbienvenu) → feedback+

Comment 38

5 years ago
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.
Attachment #607326 - Flags: review?(dbienvenu) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [fixed in TB13.0: part 1] → [fixed in TB13.0: part 1][checkin patch "part 2, v3"]
https://hg.mozilla.org/comm-central/rev/10f68e70c91a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in TB13.0: part 1][checkin patch "part 2, v3"] → [fixed in TB13.0: part 1][fixed in TB15.0: part 2]
Keywords: checkin-needed
Attachment #607318 - Attachment is obsolete: true
Attachment #607326 - Attachment description: part 2, v3 → part 2, v3 [Checked in: Comment 39]

Updated

5 years ago
Depends on: 763319
You need to log in before you can comment on or make changes to this bug.