Last Comment Bug 779130 - Fix nsresult abuses in comm-central
: Fix nsresult abuses in comm-central
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
:
Mentors:
: 736766 (view as bug list)
Depends on:
Blocks: 736766 nsresult-enum 783289
  Show dependency treegraph
 
Reported: 2012-07-31 07:44 PDT by Joshua Cranmer [:jcranmer]
Modified: 2013-10-28 07:19 PDT (History)
6 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch part 1 -- Fix type of constants (99.30 KB, patch)
2012-08-08 06:28 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review
Patch part 2 -- Use MimeConverterOutputCallback more (33.74 KB, patch)
2012-08-08 06:32 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
standard8: review+
Details | Diff | Splinter Review
Patch part 3 -- Fix nsresult |= (7.98 KB, patch)
2012-08-08 06:36 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review
Patch part 4 -- Change types to nsresult (40.18 KB, patch)
2012-08-08 06:40 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review
Patch part 5 -- Add lots of static_cast<nsresult> (29.66 KB, patch)
2012-08-08 06:46 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
standard8: review-
Details | Diff | Splinter Review
Patch part 6 -- Change types away from nsresult (18.33 KB, patch)
2012-08-08 06:47 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review
Patch part 7 -- Remove unnecessary nsresults (7.54 KB, patch)
2012-08-08 06:52 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
standard8: review+
Details | Diff | Splinter Review
Part part 5, v2 -- Add lots of static_cast<nsresult> (28.45 KB, patch)
2012-08-14 03:43 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
standard8: review+
Details | Diff | Splinter Review
Interdiff for patch part 5, v1 -> v2 (14.62 KB, patch)
2012-08-14 03:43 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Patch part 8 -- Make more nsSmtpProtocol methods return nsresult (20.27 KB, patch)
2012-08-15 07:36 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review
Patch part 9 -- Fix one more bad constant type (1.15 KB, patch)
2012-08-15 07:51 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review
Patch part 6, v2 -- Change types away from nsresult (18.03 KB, patch)
2012-08-16 05:26 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
irving: review+
Details | Diff | Splinter Review

Description Joshua Cranmer [:jcranmer] 2012-07-31 07:44:42 PDT

    
Comment 1 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-02 09:42:09 PDT
I have to say, I found plenty of bugs and other weird stuff in m-c when converting nsresult to enum, but so far, c-c is vastly more horrible.  You don't have this kind of rampant abandon in interchanging integer types and nsresult anywhere in m-c.
Comment 2 Mike Conley (:mconley) 2012-08-02 10:11:17 PDT
Ha! Though, given mailnews's history, it really shouldn't be much of a surprise.
Comment 3 Joshua Cranmer [:jcranmer] 2012-08-02 10:17:53 PDT
FWIW, some of the code was mostly directly copied from the NS 5 version of mozilla and predates the use of nsresult codes and instead uses the "negative result for error codes." Replacing this stuff fell into the category of "fix it when you're already mucking around there," so it never was a particularly big priority to fix.
Comment 4 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:28:25 PDT
Created attachment 650090 [details] [diff] [review]
Patch part 1 -- Fix type of constants

Apparently successful try run for the series I'm posting now: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=76b8cfcd1809  All failures starred, or successful on retrigger.

I believe every single change in this patch is just switching between 0, NS_OK, nullptr, and false.  In some cases this might actually be indicative of an underlying bug, such as if a function tries to return false (indicating error) but actually returns NS_OK (indicating success).  But in any event, this patch shouldn't change any behavior.

Let me know if you're the right person to ask to review this stuff.
Comment 5 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:32:54 PDT
Created attachment 650094 [details] [diff] [review]
Patch part 2 -- Use MimeConverterOutputCallback more

We have a typedef; let's use it, please.  This is used by later patches, because MimeConverterOutputCallback is defined to return nsresult, but actual functions of that type really return int, so I want to change the typedef to int in one fell swoop.

In this patch I also change mime_output_fn() and mime_multipart_related_output_fn() to return nsresult instead of int so they match the new MimeConverterOutputCallback definition.  This is to avoid compiler errors for returning signed vs. unsigned, since at this point nsresult is still PRUint32.  In a later patch I'll change them back.
Comment 6 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:36:35 PDT
Created attachment 650096 [details] [diff] [review]
Patch part 3 -- Fix nsresult |=

You can't do

  nsresult rv = Foo();
  rv |= Bar();

when nsresult is an enum.  This uses additional variables instead.  Or, if there are many such |= in a row or rv is returned, it makes rv contain the last failure code.
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:40:36 PDT
Created attachment 650098 [details] [diff] [review]
Patch part 4 -- Change types to nsresult

This changes lots of int, PRUint32, etc. to nsresult -- variables, return values, parameters, and so on.  I tried to make a judgment call in each case as to whether it was used more like an nsresult or integer, but sometimes it was used as both.  Then I picked one arbitrarily, and added casts to avoid eventual compilation errors.

Changing PRUint32 to nsresult should have no consequence at all except added type safety (once nsresult is an enum).  However, changing int to nsresult makes it unsigned, which can change behavior -- e.g., "if (status < 0)" has to be changed to "if (NS_FAILED(status))".  I tried to check for this.
Comment 8 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:46:01 PDT
Created attachment 650101 [details] [diff] [review]
Patch part 5 -- Add lots of static_cast<nsresult>

In some cases, something is just used as the wrong type, and I saw no easy way to fix it.  In these cases, there was no easy option other than just adding a cast.  This patch shouldn't change behavior, but it highlights dubious code so that at least the person reading it will hopefully notice.  In a lot of cases the intent seems clear, but I don't want to risk changing behavior.  In many cases it's just "static_cast<nsresult>(-1)", which could probably be safely changed to "NS_ERROR_FAILURE" or something.  I can't realistically spend the time to fix many of these, particularly since I don't know the code well.

(The result of NS_ERROR_GENERATE_FAILURE probably shouldn't need to be cast, but my current patches for m-c require it to be.  If I wind up changing that, these parts of the patchset can be dropped.)
Comment 9 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:47:31 PDT
Created attachment 650103 [details] [diff] [review]
Patch part 6 -- Change types away from nsresult

This is the inverse of part 4.  In cases where I judged values weren't really being used as nsresults, I changed the type to something else.
Comment 10 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-08 06:52:46 PDT
Created attachment 650105 [details] [diff] [review]
Patch part 7 -- Remove unnecessary nsresults

In some cases, I was able to avoid casting nsresults by just removing them entirely.  This includes nsresults that are never checked, or that are only checked in a way where the result is always the same (e.g., assigning a boolean to an nsresult and then checking NS_FAILED).

In EnumerateCards(), a bool was being used as an nsresult, so the NS_FAILED() call would always return false.  I changed it here to match the intent, because it was only used in an assertion, so it doesn't change behavior in production.  (I'm avoiding actual changes to behavior across the board here, because I'm changing so many things at once and I don't understand them.)

In the case of DeleteSmtpServer, I verified that there were no C++ callers, so changing it to always return NS_OK instead of a boolean-converted-to-nsresult won't change anything (no exception either way).

These seven patches fix all the nsresult misuse that's picked up by making nsresult an enum.  Making it an enum class will doubtless find more -- I'm still working on that patch set for m-c.
Comment 11 Mark Banner (:standard8, afk until Dec) 2012-08-08 13:25:24 PDT
Comment on attachment 650090 [details] [diff] [review]
Patch part 1 -- Fix type of constants

Ok, I'm going to punt some of these in irving's direction, and I'll look at the rest soon.
Comment 12 Mark Banner (:standard8, afk until Dec) 2012-08-13 13:36:51 PDT
Comment on attachment 650094 [details] [diff] [review]
Patch part 2 -- Use MimeConverterOutputCallback more

Review of attachment 650094 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, much cleaner. I'd probably comment about improving some of the style on the lines you're changing, however, as this is large amounts of replacement, and it was all bad before, I think we can skip that this time.
Comment 13 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 02:13:35 PDT
Yes, generally I fix style on code I change, but not when I'm changing so much at once.  Especially because it can make extremely long, repetitive patches a lot harder to review.
Comment 14 Mark Banner (:standard8, afk until Dec) 2012-08-14 02:48:10 PDT
Comment on attachment 650105 [details] [diff] [review]
Patch part 7 -- Remove unnecessary nsresults

Review of attachment 650105 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsSmtpService.cpp
@@ +669,5 @@
>      aServer->ClearAllValues();
>  
>      mServerKeyList = newServerList;
>      saveKeyList();
> +    return rv;

You removed the definition of rv above, so this is actually broken. I'd suggest just leaving the return NS_OK as that's what we've been doing up until now.
Comment 15 Mark Banner (:standard8, afk until Dec) 2012-08-14 02:56:17 PDT
Comment on attachment 650101 [details] [diff] [review]
Patch part 5 -- Add lots of static_cast<nsresult>

Review of attachment 650101 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbLDAPListenerBase.cpp
@@ +337,5 @@
>      }
>  
>      // Don't know how to handle this, so use the message error code in
>      // the failure return value so we hopefully get it back to the UI.
> +    return static_cast<nsresult>(NS_ERROR_GENERATE_FAILURE(

Isn't NS_ERROR_GENERATE_FAILURE already cast to nsresult?

::: mailnews/compose/src/nsMsgSendPart.cpp
@@ +234,5 @@
>    if (m_encoder_data)
>    {
>      nsresult rv = MIME_EncoderWrite(m_encoder_data, encoded_data, length);
>      if (NS_FAILED(rv))
> +      status = static_cast<nsresult>(-1);

Can you add XXX statements for these as well please (several places in the patch)?
Comment 16 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 03:42:42 PDT
(In reply to Mark Banner (:standard8) from comment #14)
> You removed the definition of rv above, so this is actually broken. I'd
> suggest just leaving the return NS_OK as that's what we've been doing up
> until now.

Oops!  Dunno how that snuck in -- it wasn't in my push to try (obviously).
Comment 17 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 03:43:22 PDT
Created attachment 651679 [details] [diff] [review]
Part part 5, v2 -- Add lots of static_cast<nsresult>
Comment 18 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 03:43:53 PDT
Created attachment 651680 [details] [diff] [review]
Interdiff for patch part 5, v1 -> v2
Comment 19 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 03:47:04 PDT
Parts 2 and 7:

https://hg.mozilla.org/comm-central/rev/78f676e4f2d3
https://hg.mozilla.org/comm-central/rev/3285912ee959
Comment 20 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 04:16:06 PDT
Comment on attachment 650090 [details] [diff] [review]
Patch part 1 -- Fix type of constants

Review of attachment 650090 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsSmtpService.cpp
@@ +669,5 @@
>      aServer->ClearAllValues();
>  
>      mServerKeyList = newServerList;
>      saveKeyList();
> +    return NS_OK;

Ignore this change -- it was already pushed as part of a separate patch to fix build breakage:

https://hg.mozilla.org/comm-central/rev/7387d34b4335
Comment 21 Mark Banner (:standard8, afk until Dec) 2012-08-14 06:35:58 PDT
Comment on attachment 651679 [details] [diff] [review]
Part part 5, v2 -- Add lots of static_cast<nsresult>

Review of attachment 651679 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. In case you hadn't realised, this patch does depend on the other ones in this bug.
Comment 22 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 06:40:17 PDT
I'll hold off on pushing any more patches until they're all reviewed, then.  No rush.
Comment 23 :aceman 2012-08-14 06:53:57 PDT
-        rv = m_readSet->Add(messageKey);
-        if (NS_FAILED(rv)) return false;
+        if (m_readSet->Add(messageKey) < 0) return false;

Why is this change good? Isn't NS_FAILED() preferred and future-proof?
Comment 24 :aceman 2012-08-14 06:58:51 PDT
This may also help bug 736766.
Comment 25 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-14 08:34:53 PDT
(In reply to :aceman from comment #23)
> -        rv = m_readSet->Add(messageKey);
> -        if (NS_FAILED(rv)) return false;
> +        if (m_readSet->Add(messageKey) < 0) return false;
> 
> Why is this change good? Isn't NS_FAILED() preferred and future-proof?

Yes, if rv is actually an nsresult.  In this case, the function returns an int:

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgKeySet.cpp#641

If the function were changed to return nsresult, which it probably should be, NS_FAILED() would be correct.  But I'm not trying to clean up all the functions that sometimes return int and sometimes nsresult; that would add a lot of work to an already fairly large job.
Comment 26 :aceman 2012-08-14 08:56:21 PDT
Thanks.
Comment 27 :Irving Reid (No longer working on Firefox) 2012-08-14 11:19:35 PDT
Comment on attachment 650090 [details] [diff] [review]
Patch part 1 -- Fix type of constants

Review of attachment 650090 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, I'll attach a separate patch that fixes all the places where we were returning an incorrect value in the first place (for example returning 'false' which == NS_OK when we meant to indicate failure)
Comment 28 :Irving Reid (No longer working on Firefox) 2012-08-14 11:59:00 PDT
Comment on attachment 650096 [details] [diff] [review]
Patch part 3 -- Fix nsresult |=

Review of attachment 650096 [details] [diff] [review]:
-----------------------------------------------------------------

I'm OK with this patch going ahead as is, or with the changes I suggest; r+ either way.

::: mailnews/base/src/nsMsgContentPolicy.cpp
@@ +335,4 @@
>  
>    NS_ENSURE_SUCCESS(rv, false);
> +  NS_ENSURE_SUCCESS(rv2, false);
> +  NS_ENSURE_SUCCESS(rv3, false);

I think we can take early exits on these instead of having extra variables:

nsresult rv = aRequestingLocation->SchemeIs("chrome", &isChrome);
NS_ENSURE_SUCCESS(rv, false);
rv = aRequestingLocation->SchemeIs("resource", &isRes);
NS_ENSURE_SUCCESS(rv, false);
rv = aRequestingLocation->SchemeIs("file", &isFile);
NS_ENSURE_SUCCESS(rv, false);

@@ +389,4 @@
>  
>    NS_ENSURE_SUCCESS(rv, false);
> +  NS_ENSURE_SUCCESS(rv2, false);
> +  NS_ENSURE_SUCCESS(rv3, false);

Early exits again:

nsresult rv = aRequestingLocation->SchemeIs("chrome", &isChrome);
NS_ENSURE_SUCCESS(rv, false);
rv = aRequestingLocation->SchemeIs("resource", &isRes);
NS_ENSURE_SUCCESS(rv, false);
rv = aRequestingLocation->SchemeIs("data", &isData);
NS_ENSURE_SUCCESS(rv, false);

@@ +409,5 @@
>  
>    // Error condition - we must return true so that we block.
>    NS_ENSURE_SUCCESS(rv, true);
> +  NS_ENSURE_SUCCESS(rv2, true);
> +  NS_ENSURE_SUCCESS(rv3, true);

And again:

nsresult rv = aRequestingLocation->SchemeIs("http", &isHttp);
NS_ENSURE_SUCCESS(rv, false);
rv = aRequestingLocation->SchemeIs("https", &isHttps);
NS_ENSURE_SUCCESS(rv, false);
rv = aRequestingLocation->SchemeIs("file", &isFile);
NS_ENSURE_SUCCESS(rv, false);
Comment 29 :Irving Reid (No longer working on Firefox) 2012-08-14 14:23:02 PDT
Comment on attachment 650098 [details] [diff] [review]
Patch part 4 -- Change types to nsresult

Review of attachment 650098 [details] [diff] [review]:
-----------------------------------------------------------------

Standard8 can overrule me on these nits if he wants; otherwise, I'd like to give it a quick re-review after the patch is updated.

::: mailnews/base/util/nsMsgLineBuffer.cpp
@@ +93,5 @@
>          /* The last buffer ended with a CR.  The new buffer does not start
>             with a LF.  This old buffer should be shipped out and discarded. */
>          PR_ASSERT(m_bufferSize > m_bufferPos);
> +        // XXX -1 is not a valid nsresult
> +        if (m_bufferSize <= m_bufferPos) return static_cast<nsresult>(-1);

The only callers that don't ignore the result treat it as an nsresult (aside from casting it back and forth to int), so let's use valid nsresult codes here:

if (m_bufferSize <= m_bufferPos)
  return NS_ERROR_UNEXPECTED;

@@ +101,1 @@
>             return status;

Similarly,

if (ConvertAndSendBuffer() == -1)
  return NS_ERROR_FAILURE;

@@ +170,5 @@
> +            return NS_OK;
> +
> +        // XXX This returns -1 on error, not an nsresult
> +        status = static_cast<nsresult>(ConvertAndSendBuffer());
> +        if (NS_FAILED(status)) return status;

if (ConvertAndSendBuffer() == -1)
  return NS_ERROR_FAILURE;

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +410,1 @@
>        return rv;

I'd prefer to leave AuthLoginResponse() returning PRInt32, so

PRInt32 rv = AuthLoginReponse(nullptr, 0);
if (rv < 0)
  return NS_ERROR_FAILURE;

@@ +1054,5 @@
>  }
>  
>  
>  
> +nsresult nsSmtpProtocol::AuthLoginResponse(nsIInputStream * stream, PRUint32 length)

All the other internal methods in this class return integers, not nsresult; I'd prefer to not change this one.

::: mailnews/compose/src/nsSmtpProtocol.h
@@ +173,5 @@
>      PRInt32 AuthLoginStep0();
>      PRInt32 AuthLoginStep0Response();
>      PRInt32 AuthLoginStep1();
>      PRInt32 AuthLoginStep2();
> +    nsresult AuthLoginResponse(nsIInputStream * stream, PRUint32 length);

Leave this as PRInt32 to be consistent with the others

::: mailnews/local/src/nsParseMailbox.h
@@ +155,5 @@
>  
>    void    SetDB (nsIMsgDatabase *mailDB) {m_mailDB = mailDB; }
>  
>    // message socket libnet callbacks, which come through folder pane
> +  nsresult ProcessMailboxInputStream(nsIURI* aURL, nsIInputStream *aIStream, PRUint32 aLength);

Was the change from virtual to non-virtual deliberate?
Comment 30 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 06:32:30 PDT
(In reply to Irving Reid (:irving) from comment #29)
> The only callers that don't ignore the result treat it as an nsresult (aside
> from casting it back and forth to int), so let's use valid nsresult codes
> here:
> 
> if (m_bufferSize <= m_bufferPos)
>   return NS_ERROR_UNEXPECTED;

How do you figure?  It's called by nsMsgMailboxParser::ProcessMailboxInputStream, which just returns the result.  That's called by nsMsgMailboxParser::OnDataAvailable, which just returns the result.  That's a virtual XPCOM method with like a zillion callers.  Do all of them really treat the result properly?

I really don't want to make this kind of change if it might change behavior.  Likewise for the other two comments on the file.

> ::: mailnews/compose/src/nsSmtpProtocol.cpp
> @@ +410,1 @@
> >        return rv;
> 
> I'd prefer to leave AuthLoginResponse() returning PRInt32, so
> 
> PRInt32 rv = AuthLoginReponse(nullptr, 0);
> if (rv < 0)
>   return NS_ERROR_FAILURE;
> 
> @@ +1054,5 @@
> >  }
> >  
> >  
> >  
> > +nsresult nsSmtpProtocol::AuthLoginResponse(nsIInputStream * stream, PRUint32 length)
> 
> All the other internal methods in this class return integers, not nsresult;
> I'd prefer to not change this one.

Instead, I changed all the others to return nsresult (or void, for the ones that always returned NS_OK).  Currently, changing things that really return nsresult to return int works, because nsresult can still be implicitly converted to int.  When nsresult is made an enum class rather than a regular enum, this will no longer be possible, so they'll have to be ported at that point -- unless you change every single "return NS_ERROR_FAILURE;" to "return static_cast<int>(NS_ERROR_FAILURE);".  May as well do it now.

> ::: mailnews/local/src/nsParseMailbox.h
> @@ +155,5 @@
> >  
> >    void    SetDB (nsIMsgDatabase *mailDB) {m_mailDB = mailDB; }
> >  
> >    // message socket libnet callbacks, which come through folder pane
> > +  nsresult ProcessMailboxInputStream(nsIURI* aURL, nsIInputStream *aIStream, PRUint32 aLength);
> 
> Was the change from virtual to non-virtual deliberate?

Yes.  If it were really used as a virtual function, I'd have to be a lot more careful in changing its return type, lest it mask a function it previously overrode.  In fact, the method doesn't override anything nor is it overridden by anything, so I made it non-virtual while I was changing the signature anyway.
Comment 31 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 07:36:41 PDT
Created attachment 652095 [details] [diff] [review]
Patch part 8 -- Make more nsSmtpProtocol methods return nsresult

This makes it consistent again.  The methods changed are all private, so I checked callers where necessary and don't think any of this changes behavior.
Comment 32 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 07:51:16 PDT
Created attachment 652101 [details] [diff] [review]
Patch part 9 -- Fix one more bad constant type

I don't know how this slipped through the cracks before.  Possibly it's related to the institution of nullptr, but returning nsnull here should have been caught too, since numeric types don't implicitly convert to enum.
Comment 33 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 07:54:00 PDT
Comment on attachment 650098 [details] [diff] [review]
Patch part 4 -- Change types to nsresult

Re-requesting review per comment 30.
Comment 34 :Irving Reid (No longer working on Firefox) 2012-08-15 08:04:42 PDT
Comment on attachment 650098 [details] [diff] [review]
Patch part 4 -- Change types to nsresult

(In reply to :Aryeh Gregor from comment #30)
> I really don't want to make this kind of change if it might change behavior.
> Likewise for the other two comments on the file.

OK, I'll move the change to a separate patch - I've been accumulating a patch containing all the places where I think our old return values were incorrect, and I'll put that up for separate review later.

> Instead, I changed all the others to return nsresult (or void, for the ones
> that always returned NS_OK).

OK.

> > Was the change from virtual to non-virtual deliberate?
> 
> Yes.  If it were really used as a virtual function, I'd have to be a lot
> more careful in changing its return type, lest it mask a function it
> previously overrode.  In fact, the method doesn't override anything nor is
> it overridden by anything, so I made it non-virtual while I was changing the
> signature anyway.

Great, thanks.
Comment 35 :Irving Reid (No longer working on Firefox) 2012-08-15 08:32:02 PDT
(In reply to :Aryeh Gregor from comment #30)

(in the context of returning static_cast<nsresult>(-1) )

> How do you figure?  It's called by
> nsMsgMailboxParser::ProcessMailboxInputStream, which just returns the
> result.  That's called by nsMsgMailboxParser::OnDataAvailable, which just
> returns the result.  That's a virtual XPCOM method with like a zillion
> callers.  Do all of them really treat the result properly?
> 
> I really don't want to make this kind of change if it might change behavior.
> Likewise for the other two comments on the file.

Just for my own understanding, the result of static_cast<nsresult>(-1) will show up as a failure when tested with the NS_FAILED() macro, right?
Comment 36 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-15 08:46:25 PDT
Correct.  NS_FAILED just tests if the high bit is set when the argument is interpreted as PRUint32, and -1 has all bits set.
Comment 37 :Irving Reid (No longer working on Firefox) 2012-08-15 09:06:11 PDT
Comment on attachment 650103 [details] [diff] [review]
Patch part 6 -- Change types away from nsresult

Review of attachment 650103 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgSend.cpp
@@ +1050,5 @@
>  PRInt32
>  nsMsgComposeAndSend::PreProcessPart(nsMsgAttachmentHandler  *ma,
>                                      nsMsgSendPart           *toppart) // The very top most container of the message
>  {
> +  int             status;

This function still uses NS_FAILED(status) to test the return values of lots of calls - should we clean that up here or in a separate patch (or just leave it)?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +4518,5 @@
> +    if (request) {
> +      nsresult rv;
> +      request->GetStatus(&rv);
> +      returnValue = static_cast<bool>(rv);
> +    }

Wow, the old code here was really ugly, almost certainly broken, and might be the cause of some long-standing bugs with not handling dead IMAP connections correctly; I'll investigate further and file a separate bug if necessary.

That said, as long as we don't lose bits casting the nsresult to bool and back, this preserves the behaviour of the existing code.
Comment 38 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-16 05:26:02 PDT
(In reply to Irving Reid (:irving) from comment #37)
> This function still uses NS_FAILED(status) to test the return values of lots
> of calls - should we clean that up here or in a separate patch (or just
> leave it)?

NS_FAILED() is soon going to be an inline function that takes nsresult as its arg (bug 768865, which I hope to land today-ish).  This means when we convert nsresult to an enum, all attempts to use it on non-nsresults will fail.  So this will have to be cleaned up separately before the enum conversion lands.  I think we can leave it for a later bug.

> ::: mailnews/imap/src/nsImapProtocol.cpp
> @@ +4518,5 @@
> > +    if (request) {
> > +      nsresult rv;
> > +      request->GetStatus(&rv);
> > +      returnValue = static_cast<bool>(rv);
> > +    }
> 
> Wow, the old code here was really ugly, almost certainly broken, and might
> be the cause of some long-standing bugs with not handling dead IMAP
> connections correctly; I'll investigate further and file a separate bug if
> necessary.
> 
> That said, as long as we don't lose bits casting the nsresult to bool and
> back, this preserves the behaviour of the existing code.

Oops -- this change is broken.  The NS_SUCCEEDED() check just below now will return different results.  Here's a better diff for that file:

   if (NS_SUCCEEDED(returnValue)) // check the other way of cancelling.
   {
     ReentrantMonitorAutoEnter threadDeathMon(m_threadDeathMonitor);
-    returnValue = m_threadShouldDie;
-  }
-  return returnValue;
+    // XXX Casting bool to nsresult
+    returnValue = static_cast<nsresult>(m_threadShouldDie);
+  }
+  // XXX Casting nsresult to bool
+  return static_cast<bool>(returnValue);
 }

This should preserve existing behavior exactly, just making implicit conversions explicit.  I'll attach a new patch in a sec.
Comment 39 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-16 05:26:56 PDT
Created attachment 652406 [details] [diff] [review]
Patch part 6, v2 -- Change types away from nsresult

Same as last patch except for the difference detailed in the previous comment.
Comment 40 :Irving Reid (No longer working on Firefox) 2012-08-16 06:34:28 PDT
Comment on attachment 652406 [details] [diff] [review]
Patch part 6, v2 -- Change types away from nsresult

Review of attachment 652406 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +4524,5 @@
> +    // XXX Casting bool to nsresult
> +    returnValue = static_cast<nsresult>(m_threadShouldDie);
> +  }
> +  // XXX Casting nsresult to bool
> +  return static_cast<bool>(returnValue);

I like this.
Comment 42 :Irving Reid (No longer working on Firefox) 2013-10-28 07:19:33 PDT
*** Bug 736766 has been marked as a duplicate of this bug. ***

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