Bug 777292 (nsresult-enum)

Make nsresult an enum

RESOLVED FIXED

Status

()

Core
XPCOM
--
enhancement
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 5 bugs)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(31 attachments, 5 obsolete attachments)

14.19 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
115.39 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
66.55 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
971 bytes, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
3.02 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.35 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.77 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.78 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
7.45 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.68 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
3.12 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.00 KB, patch
roc
: review+
ayg
: checkin+
Details | Diff | Splinter Review
3.53 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.18 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
3.77 KB, patch
Details | Diff | Splinter Review
2.26 KB, patch
roc
: review+
ayg
: checkin+
Details | Diff | Splinter Review
3.73 KB, patch
Benjamin Smedberg
: review+
ayg
: checkin+
Details | Diff | Splinter Review
6.34 KB, patch
Details | Diff | Splinter Review
1.02 KB, patch
kaie
: review-
ayg
: checkin+
Details | Diff | Splinter Review
5.36 KB, patch
ayg
: checkin+
Details | Diff | Splinter Review
3.40 KB, patch
kaie
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.29 KB, patch
roc
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.43 KB, patch
Ehsan
: review+
ayg
: checkin+
Details | Diff | Splinter Review
1.52 KB, patch
cjones
: review+
Details | Diff | Splinter Review
10.23 KB, patch
Benjamin Smedberg
: review+
ayg
: checkin+
Details | Diff | Splinter Review
107.65 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
29.91 KB, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
137.81 KB, patch
Ehsan
: checkin+
Details | Diff | Splinter Review
1.45 KB, patch
bholley
: review+
Ehsan
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
Ehsan
: checkin+
Details | Diff | Splinter Review
The type errors here are ludicrous.  I'm seeing things like

  bool F();
  nsresult G()
  {
    return F();
  }

where bools are silently converted to nsresults or vice versa.  Or things defined to return pointers return NS_OK, which silently converts to a null pointer (!!).  An enum would fix this.
So some remarks on various patterns that came up as I've tried to fix this, in random order:

1) Obviously, all errors must now be centrally declared in nsError.h, not scattered everywhere around the codebase.  This strikes me as a good thing.  :)  It means that I removed some header files that formerly contained only error codes, and changed everything that included them to include nsError.h instead.  I just used sed -i for this, and didn't worry if files multiply included nsError.h as a result.

2) A lot of places do

  nsresult rv = Foo();
  rv |= Bar();
  rv |= Baz();
  NS_ENSURE_SUCCESS(rv, rv);

or such.  This was always dodgy but kind of worked, but now of course it doesn't.  I changed most such cases to be like

  nsresult rv = Foo();
  nsresult tmp = Bar();
  if (NS_FAILED(tmp)) {
    rv = tmp;
  }
  tmp = Baz();
  if (NS_FAILED(tmp)) {
    rv = tmp;
  }
  NS_ENSURE_SUCCESS(rv, rv);

but in some cases I changed things like

  nsresult rv = Foo();
  rv |= Bar();
  if (NS_SUCCEEDED(rv)) {

to

  nsresult rv1 = Foo();
  nsresult rv2 = Bar();
  if (NS_SUCCEEDED(rv1) && NS_SUCCEEDED(rv2)) {

or similar.

3) Where a function is supposed to return nsresult but something else is returned instead, like bool, I change to the equivalent nsresult if possible.  In a bunch of cases this means changing things that were equal to 0 to NS_OK.  In a few cases I changed boolean returns to NS_OK regardless, since 1 is also NS_SUCCEEDED.  This might fail if someone tests the result as a boolean instead of using NS_SUCCEEDED, but thankfully that's pretty rare.  I'd ideally like to check for this by making nsresult an enum class, but probably not right now.  The same applies if someone tries returning a small integer as an nsresult -- if I'm sure the integer was less than 0x80000000, I change it to return NS_OK instead, even though it's conceivable this could change behavior.

The same holds if a non-nsresult type is assigned to nsresult and then checked.  If I have

  rv = Foo();
  NS_ENSURE_SUCCESS(rv, rv);

I'll often just get rid of the NS_ENSURE_SUCCESS entirely, because I often know that rv will not possibly be above 0x7fffffff (e.g., it's a bool).

4) In all cases, I practice "do what I say, not what I mean".  If something clearly meant to return an error but was actually returning something equal to 0, I change it to return NS_OK so as not to change behavior.

5) netwerk/ gets special props for this gem:

"""
     * @param aStatus
     *        status code (not necessarily an error code) indicating the
     *        state of the channel (usually the state of the underlying
     *        transport).  see nsISocketTransport for socket specific status
     *        codes.
"""
http://dxr.lanedo.com/mozilla-central/netwerk/base/public/nsIProgressEventSink.idl.html#l54

That's a param to nsIProgressEventSink::OnStatus.  Most callers use it as an nsresult, but not all, so I changed the type in the IDL from nsresult to unsigned long.  This means that method gets no type-safety -- calling it with nsresult will still work (since it's not an enum class), but so will any other numeric type.

6) When an nsresult is being used where a different type is expected, and the compiler complains about it (usually it won't), generally the thing being returned doesn't make sense.  E.g., nsTableRowGroupFrame::FindLineContaining starts with

  NS_ENSURE_ARG_POINTER(aFrame);

but it returns a PRInt32, so I changed it to

  // XXX Should we return -1 here or something instead?
  NS_ENSURE_TRUE(aFrame, (PRInt32)NS_ERROR_NULL_POINTER);

because I don't know whether changing the returned value would change behavior.

7) nsSelectionIterator::IsDone returns NS_OK (== 0) if it's done and NS_ENUMERATOR_FALSE (== 1) if it's not.  Yes, the return type is nsresult.  And of course it's from an IDL, so I can't easily change the return type.  This is patently insane, but for the purposes of this patch I merely added an incredulous comment.  Likewise nsSupportsArrayEnumerator::IsDone().  This violates my normal rule that I assume nsresult returns < 0x80000000 can be changed to NS_OK (which I'm nervous about in general).

8) editor/txmgr/tests/TestTXMgr.cpp does exit(NS_ERROR_FAILURE).  I figured exit(-1) was good enough.

9) nsJPEGDecoder::WriteInternal assigns the result of setjmp() to an nsresult, then if it's nonzero, proceeds to treat it like it's an actual nsresult.  This is somewhat mind-boggling.  I preserved existing behavior with a cast and another incredulous comment.

10) A bunch of stuff I don't understand complained that IPC::ParamTraits<nsresult> didn't exist, now that it's a distinct type.  I poked around and added one to ipc/chromium/src/chrome/common/ipc_message_utils.h copied from the existing uint32 one.  Its only methods are "Write", "Read", and "Log", each one line, so I'm pretty sure my code is correct although I don't fully understand what it's doing.

11) In netwerk/base/src/nsSocketTransport2.cpp, there's a function GetXPCOMFromNSSError that does what it says.  It accomplishes this via "return NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, -1 * code);", so it doesn't use a fixed list of errors.  I fixed this one with a cast and comment too, for lack of a better option, although it means it will return things that aren't actually defined as part of the enum.

12) netwerk really gets a gold medal for nsresult abuse:

     * Although these look like XPCOM error codes and are passed in an nsresult
     * variable, they are *not* error codes.  Note that while they *do* overlap
     * with existing error codes in Necko, these status codes are confined
     * within a very limited context where no error codes may appear, so there
     * is no ambiguity.
http://dxr.lanedo.com/mozilla-central/netwerk/base/public/nsISocketTransport.idl.html#l96

Naturally, despite these codes being "confined within a very limited context where no error codes may appear", various places use them interchangeably with real nsresult, resulting in (5) above et al.  I opted to convert all relevant params to PRUint32 for the purposes of this patch, which doesn't gain type safety but doesn't lose it relative to the previous typedef.  In a few cases I wind up casting these faux nsresults to actual nsresults to stop other callers of the same functions from losing type safety.

13) Since enums cannot be forward-declared prior to C++11, nscore.h must now include nsError.h.  This could be changed if we support the new "enum nsresult : PRUint32" syntax of C++11, which can be forward-declared (behind a configure check or such).


I've converted most of the tree by now.  Shouldn't be too much longer to get a working build on localhost.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Exciting fact: the .ToInteger(), .ToDouble(), etc. methods on our string classes take PRInt32* as a param, but interpret it as nsresult*.  This means *every* caller has to be changed, since although PRInt32 and PRUint32 are silently converted back and forth, apparently PRInt32* and PRUint32* cannot (dunno why).

Comment 3

6 years ago
Reviewing the patches here would be very easier if you attach smaller patches which preferably do one thing at a time...

Comment 4

6 years ago
(In reply to :Aryeh Gregor from comment #1)
> So some remarks on various patterns that came up as I've tried to fix this,
> in random order:
> 
> 1) Obviously, all errors must now be centrally declared in nsError.h, not
> scattered everywhere around the codebase.  This strikes me as a good thing. 
> :)  It means that I removed some header files that formerly contained only
> error codes, and changed everything that included them to include nsError.h
> instead.  I just used sed -i for this, and didn't worry if files multiply
> included nsError.h as a result.

Sounds good!

> 2) A lot of places do
> 
>   nsresult rv = Foo();
>   rv |= Bar();
>   rv |= Baz();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> or such.  This was always dodgy but kind of worked, but now of course it
> doesn't.  I changed most such cases to be like
> 
>   nsresult rv = Foo();
>   nsresult tmp = Bar();
>   if (NS_FAILED(tmp)) {
>     rv = tmp;
>   }
>   tmp = Baz();
>   if (NS_FAILED(tmp)) {
>     rv = tmp;
>   }
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> but in some cases I changed things like
> 
>   nsresult rv = Foo();
>   rv |= Bar();
>   if (NS_SUCCEEDED(rv)) {
> 
> to
> 
>   nsresult rv1 = Foo();
>   nsresult rv2 = Bar();
>   if (NS_SUCCEEDED(rv1) && NS_SUCCEEDED(rv2)) {
> 
> or similar.

Makes sense.

> 3) Where a function is supposed to return nsresult but something else is
> returned instead, like bool, I change to the equivalent nsresult if
> possible.  In a bunch of cases this means changing things that were equal to
> 0 to NS_OK.  In a few cases I changed boolean returns to NS_OK regardless,
> since 1 is also NS_SUCCEEDED.  This might fail if someone tests the result
> as a boolean instead of using NS_SUCCEEDED, but thankfully that's pretty
> rare.  I'd ideally like to check for this by making nsresult an enum class,
> but probably not right now.  The same applies if someone tries returning a
> small integer as an nsresult -- if I'm sure the integer was less than
> 0x80000000, I change it to return NS_OK instead, even though it's
> conceivable this could change behavior.

This is actually not ok, I don't think.  This is relying on undefined behavior, and I wouldn't be comfortable with it.

> The same holds if a non-nsresult type is assigned to nsresult and then
> checked.  If I have
> 
>   rv = Foo();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
> I'll often just get rid of the NS_ENSURE_SUCCESS entirely, because I often
> know that rv will not possibly be above 0x7fffffff (e.g., it's a bool).

I don't understand this.  Isn't this changing the behavior of the program (not returning from the function if rv is a failure code)?

> 4) In all cases, I practice "do what I say, not what I mean".  If something
> clearly meant to return an error but was actually returning something equal
> to 0, I change it to return NS_OK so as not to change behavior.
> 
> 5) netwerk/ gets special props for this gem:
> 
> """
>      * @param aStatus
>      *        status code (not necessarily an error code) indicating the
>      *        state of the channel (usually the state of the underlying
>      *        transport).  see nsISocketTransport for socket specific status
>      *        codes.
> """
> http://dxr.lanedo.com/mozilla-central/netwerk/base/public/
> nsIProgressEventSink.idl.html#l54
> 
> That's a param to nsIProgressEventSink::OnStatus.  Most callers use it as an
> nsresult, but not all, so I changed the type in the IDL from nsresult to
> unsigned long.  This means that method gets no type-safety -- calling it
> with nsresult will still work (since it's not an enum class), but so will
> any other numeric type.
> 
> 6) When an nsresult is being used where a different type is expected, and
> the compiler complains about it (usually it won't), generally the thing
> being returned doesn't make sense.  E.g.,
> nsTableRowGroupFrame::FindLineContaining starts with
> 
>   NS_ENSURE_ARG_POINTER(aFrame);
> 
> but it returns a PRInt32, so I changed it to
> 
>   // XXX Should we return -1 here or something instead?
>   NS_ENSURE_TRUE(aFrame, (PRInt32)NS_ERROR_NULL_POINTER);
> 
> because I don't know whether changing the returned value would change
> behavior.
> 
> 7) nsSelectionIterator::IsDone returns NS_OK (== 0) if it's done and
> NS_ENUMERATOR_FALSE (== 1) if it's not.  Yes, the return type is nsresult. 
> And of course it's from an IDL, so I can't easily change the return type. 
> This is patently insane, but for the purposes of this patch I merely added
> an incredulous comment.  Likewise nsSupportsArrayEnumerator::IsDone().  This
> violates my normal rule that I assume nsresult returns < 0x80000000 can be
> changed to NS_OK (which I'm nervous about in general).
> 
> 8) editor/txmgr/tests/TestTXMgr.cpp does exit(NS_ERROR_FAILURE).  I figured
> exit(-1) was good enough.
> 
> 9) nsJPEGDecoder::WriteInternal assigns the result of setjmp() to an
> nsresult, then if it's nonzero, proceeds to treat it like it's an actual
> nsresult.  This is somewhat mind-boggling.  I preserved existing behavior
> with a cast and another incredulous comment.
> 
> 10) A bunch of stuff I don't understand complained that
> IPC::ParamTraits<nsresult> didn't exist, now that it's a distinct type.  I
> poked around and added one to
> ipc/chromium/src/chrome/common/ipc_message_utils.h copied from the existing
> uint32 one.  Its only methods are "Write", "Read", and "Log", each one line,
> so I'm pretty sure my code is correct although I don't fully understand what
> it's doing.
> 
> 11) In netwerk/base/src/nsSocketTransport2.cpp, there's a function
> GetXPCOMFromNSSError that does what it says.  It accomplishes this via
> "return NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, -1 * code);", so
> it doesn't use a fixed list of errors.  I fixed this one with a cast and
> comment too, for lack of a better option, although it means it will return
> things that aren't actually defined as part of the enum.
> 
> 12) netwerk really gets a gold medal for nsresult abuse:
> 
>      * Although these look like XPCOM error codes and are passed in an
> nsresult
>      * variable, they are *not* error codes.  Note that while they *do*
> overlap
>      * with existing error codes in Necko, these status codes are confined
>      * within a very limited context where no error codes may appear, so
> there
>      * is no ambiguity.
> http://dxr.lanedo.com/mozilla-central/netwerk/base/public/nsISocketTransport.
> idl.html#l96
> 
> Naturally, despite these codes being "confined within a very limited context
> where no error codes may appear", various places use them interchangeably
> with real nsresult, resulting in (5) above et al.  I opted to convert all
> relevant params to PRUint32 for the purposes of this patch, which doesn't
> gain type safety but doesn't lose it relative to the previous typedef.  In a
> few cases I wind up casting these faux nsresults to actual nsresults to stop
> other callers of the same functions from losing type safety.
> 
> 13) Since enums cannot be forward-declared prior to C++11, nscore.h must now
> include nsError.h.  This could be changed if we support the new "enum
> nsresult : PRUint32" syntax of C++11, which can be forward-declared (behind
> a configure check or such).
> 
> 
> I've converted most of the tree by now.  Shouldn't be too much longer to get
> a working build on localhost.

Comment 5

6 years ago
(Hit enter too soon!)

(In reply to :Aryeh Gregor from comment #1)
> 5) netwerk/ gets special props for this gem:
> 
> """
>      * @param aStatus
>      *        status code (not necessarily an error code) indicating the
>      *        state of the channel (usually the state of the underlying
>      *        transport).  see nsISocketTransport for socket specific status
>      *        codes.
> """
> http://dxr.lanedo.com/mozilla-central/netwerk/base/public/
> nsIProgressEventSink.idl.html#l54
> 
> That's a param to nsIProgressEventSink::OnStatus.  Most callers use it as an
> nsresult, but not all, so I changed the type in the IDL from nsresult to
> unsigned long.  This means that method gets no type-safety -- calling it
> with nsresult will still work (since it's not an enum class), but so will
> any other numeric type.

This seems sane, but someone from the Necko team should probably look at it.

> 6) When an nsresult is being used where a different type is expected, and
> the compiler complains about it (usually it won't), generally the thing
> being returned doesn't make sense.  E.g.,
> nsTableRowGroupFrame::FindLineContaining starts with
> 
>   NS_ENSURE_ARG_POINTER(aFrame);
> 
> but it returns a PRInt32, so I changed it to
> 
>   // XXX Should we return -1 here or something instead?
>   NS_ENSURE_TRUE(aFrame, (PRInt32)NS_ERROR_NULL_POINTER);
> 
> because I don't know whether changing the returned value would change
> behavior.

OK.  I think we should file follow-up bugs for this kind of thing though.

> 7) nsSelectionIterator::IsDone returns NS_OK (== 0) if it's done and
> NS_ENUMERATOR_FALSE (== 1) if it's not.  Yes, the return type is nsresult. 
> And of course it's from an IDL, so I can't easily change the return type. 
> This is patently insane, but for the purposes of this patch I merely added
> an incredulous comment.  Likewise nsSupportsArrayEnumerator::IsDone().

Can't you change the type in the IDL?

> This
> violates my normal rule that I assume nsresult returns < 0x80000000 can be
> changed to NS_OK (which I'm nervous about in general).

Is that assumption true?

> 8) editor/txmgr/tests/TestTXMgr.cpp does exit(NS_ERROR_FAILURE).  I figured
> exit(-1) was good enough.

Yes.

> 9) nsJPEGDecoder::WriteInternal assigns the result of setjmp() to an
> nsresult, then if it's nonzero, proceeds to treat it like it's an actual
> nsresult.  This is somewhat mind-boggling.  I preserved existing behavior
> with a cast and another incredulous comment.

Sounds good.

> 10) A bunch of stuff I don't understand complained that
> IPC::ParamTraits<nsresult> didn't exist, now that it's a distinct type.  I
> poked around and added one to
> ipc/chromium/src/chrome/common/ipc_message_utils.h copied from the existing
> uint32 one.  Its only methods are "Write", "Read", and "Log", each one line,
> so I'm pretty sure my code is correct although I don't fully understand what
> it's doing.

cjones should probably review that.

> 11) In netwerk/base/src/nsSocketTransport2.cpp, there's a function
> GetXPCOMFromNSSError that does what it says.  It accomplishes this via
> "return NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, -1 * code);", so
> it doesn't use a fixed list of errors.  I fixed this one with a cast and
> comment too, for lack of a better option, although it means it will return
> things that aren't actually defined as part of the enum.

Where is it used?  I think the list of NSS error codes is a fixed list.  Perhaps you could file a follow-up for that.

> 12) netwerk really gets a gold medal for nsresult abuse:
> 
>      * Although these look like XPCOM error codes and are passed in an
> nsresult
>      * variable, they are *not* error codes.  Note that while they *do*
> overlap
>      * with existing error codes in Necko, these status codes are confined
>      * within a very limited context where no error codes may appear, so
> there
>      * is no ambiguity.
> http://dxr.lanedo.com/mozilla-central/netwerk/base/public/nsISocketTransport.
> idl.html#l96
> 
> Naturally, despite these codes being "confined within a very limited context
> where no error codes may appear", various places use them interchangeably
> with real nsresult, resulting in (5) above et al.  I opted to convert all
> relevant params to PRUint32 for the purposes of this patch, which doesn't
> gain type safety but doesn't lose it relative to the previous typedef.  In a
> few cases I wind up casting these faux nsresults to actual nsresults to stop
> other callers of the same functions from losing type safety.

OK.  Again, a follow-up bug would be nice.

Comment 6

6 years ago
Another point to think about is what to do with the C XPCOM interface.  I don't know how much we need to keep it working, but perhaps you could preserve the existing behavior in an #ifndef __cplusplus for now?

Comment 7

6 years ago
I don't think we have a working C xpcom interface.

I'm a little worried about making this an enum because that could theoretically change the size of the type; in the past the proposals of this sort have turned nsresult into a class wrapping an int instead (with private operator/constructors for the obviously incorrect usages).

I am going on PTO until 7-August; I'm happy to delegate the decision to jlebar, but if he's not comfortable making the call then this should wait until I get back.

Comment 8

6 years ago
(In reply to comment #7)
> I don't think we have a working C xpcom interface.
> 
> I'm a little worried about making this an enum because that could theoretically
> change the size of the type;

Even if we have big enough values to require 32 bits?

Comment 9

6 years ago
It would at least be 32 bits, but might it end up as a 64-bit value on 64-bit systems?

I also happen to like the idea of being able to say result.failed() directly rather than NS_FAILED(result) in the long term, which is why I felt that a struct/class type might be better. There's a bug on this already somewhere...

Comment 10

6 years ago
(In reply to comment #9)
> It would at least be 32 bits, but might it end up as a 64-bit value on 64-bit
> systems?

Right...  I don't actually know what happens in that case.

> I also happen to like the idea of being able to say result.failed() directly
> rather than NS_FAILED(result) in the long term, which is why I felt that a
> struct/class type might be better. There's a bug on this already somewhere...

Bug 751538.  Maybe we should pursue that approach?  (We still need to take most of Aryeh's fixes in this bug anyways.)

Comment 11

6 years ago
Yes, overall I think that's better. And yes, we still need to fix the various bad cases as well as the |= thing, although I think we could probably just make |= work with a custom operator on a struct.
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Reviewing the patches here would be very easier if you attach smaller
> patches which preferably do one thing at a time...

Yeah, I'm going to do that.  Particularly because that way I'll be able to debug failures!  And some of them need review from someone other than you.

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > 3) Where a function is supposed to return nsresult but something else is
> > returned instead, like bool, I change to the equivalent nsresult if
> > possible.  In a bunch of cases this means changing things that were equal to
> > 0 to NS_OK.  In a few cases I changed boolean returns to NS_OK regardless,
> > since 1 is also NS_SUCCEEDED.  This might fail if someone tests the result
> > as a boolean instead of using NS_SUCCEEDED, but thankfully that's pretty
> > rare.  I'd ideally like to check for this by making nsresult an enum class,
> > but probably not right now.  The same applies if someone tries returning a
> > small integer as an nsresult -- if I'm sure the integer was less than
> > 0x80000000, I change it to return NS_OK instead, even though it's
> > conceivable this could change behavior.
> 
> This is actually not ok, I don't think.  This is relying on undefined
> behavior, and I wouldn't be comfortable with it.

I came to this conclusion as well.  This could cause subtle behavior changes in corner cases.  Instead, I'll audit each use like this and make sure that it doesn't change behavior, or else cast it.

> > The same holds if a non-nsresult type is assigned to nsresult and then
> > checked.  If I have
> > 
> >   rv = Foo();
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> > I'll often just get rid of the NS_ENSURE_SUCCESS entirely, because I often
> > know that rv will not possibly be above 0x7fffffff (e.g., it's a bool).
> 
> I don't understand this.  Isn't this changing the behavior of the program
> (not returning from the function if rv is a failure code)?

I meant when Foo() returns something like a boolean, or a number of bytes written, which will always be less than 0x80000000.  Then NS_ENSURE_SUCCESS(rv, rv) is a no-op, since it will always be a success code.

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> (Hit enter too soon!)

(What field did you have focused?)

> (In reply to :Aryeh Gregor from comment #1)
> > 5) netwerk/ gets special props for this gem:
> > 
> > """
> >      * @param aStatus
> >      *        status code (not necessarily an error code) indicating the
> >      *        state of the channel (usually the state of the underlying
> >      *        transport).  see nsISocketTransport for socket specific status
> >      *        codes.
> > """
> > http://dxr.lanedo.com/mozilla-central/netwerk/base/public/
> > nsIProgressEventSink.idl.html#l54
> > 
> > That's a param to nsIProgressEventSink::OnStatus.  Most callers use it as an
> > nsresult, but not all, so I changed the type in the IDL from nsresult to
> > unsigned long.  This means that method gets no type-safety -- calling it
> > with nsresult will still work (since it's not an enum class), but so will
> > any other numeric type.
> 
> This seems sane, but someone from the Necko team should probably look at it.

I think it would be better to make up a typedef here to convey intent, or (better) fix them to use actual nsresult.

> > 6) When an nsresult is being used where a different type is expected, and
> > the compiler complains about it (usually it won't), generally the thing
> > being returned doesn't make sense.  E.g.,
> > nsTableRowGroupFrame::FindLineContaining starts with
> > 
> >   NS_ENSURE_ARG_POINTER(aFrame);
> > 
> > but it returns a PRInt32, so I changed it to
> > 
> >   // XXX Should we return -1 here or something instead?
> >   NS_ENSURE_TRUE(aFrame, (PRInt32)NS_ERROR_NULL_POINTER);
> > 
> > because I don't know whether changing the returned value would change
> > behavior.
> 
> OK.  I think we should file follow-up bugs for this kind of thing though.

I'll do that when I start submitting patches.

> > 7) nsSelectionIterator::IsDone returns NS_OK (== 0) if it's done and
> > NS_ENUMERATOR_FALSE (== 1) if it's not.  Yes, the return type is nsresult. 
> > And of course it's from an IDL, so I can't easily change the return type. 
> > This is patently insane, but for the purposes of this patch I merely added
> > an incredulous comment.  Likewise nsSupportsArrayEnumerator::IsDone().
> 
> Can't you change the type in the IDL?

Well, the return type has to be nsresult unless it's changed to a non-XPCOM method.  If this interface is only exposed to our code, I'll gladly change it to a bool and invert the meaning.  (Currently IsDone() returns 0 if it's done and 1 if it's not!)  But if it's exposed to JS, or even C++ extensions, I can't do that.

> > This
> > violates my normal rule that I assume nsresult returns < 0x80000000 can be
> > changed to NS_OK (which I'm nervous about in general).
> 
> Is that assumption true?

No, probably not.  I'll rewrite to avoid it.

> > 10) A bunch of stuff I don't understand complained that
> > IPC::ParamTraits<nsresult> didn't exist, now that it's a distinct type.  I
> > poked around and added one to
> > ipc/chromium/src/chrome/common/ipc_message_utils.h copied from the existing
> > uint32 one.  Its only methods are "Write", "Read", and "Log", each one line,
> > so I'm pretty sure my code is correct although I don't fully understand what
> > it's doing.
> 
> cjones should probably review that.

Okay.  This has to land at the same time as the actual nsresult->enum change, though, because otherwise it's duplicating the existing uint32 overload.

> > 11) In netwerk/base/src/nsSocketTransport2.cpp, there's a function
> > GetXPCOMFromNSSError that does what it says.  It accomplishes this via
> > "return NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_SECURITY, -1 * code);", so
> > it doesn't use a fixed list of errors.  I fixed this one with a cast and
> > comment too, for lack of a better option, although it means it will return
> > things that aren't actually defined as part of the enum.
> 
> Where is it used?  I think the list of NSS error codes is a fixed list. 
> Perhaps you could file a follow-up for that.

Will do.

> > 12) netwerk really gets a gold medal for nsresult abuse:
> > 
> >      * Although these look like XPCOM error codes and are passed in an
> > nsresult
> >      * variable, they are *not* error codes.  Note that while they *do*
> > overlap
> >      * with existing error codes in Necko, these status codes are confined
> >      * within a very limited context where no error codes may appear, so
> > there
> >      * is no ambiguity.
> > http://dxr.lanedo.com/mozilla-central/netwerk/base/public/nsISocketTransport.
> > idl.html#l96
> > 
> > Naturally, despite these codes being "confined within a very limited context
> > where no error codes may appear", various places use them interchangeably
> > with real nsresult, resulting in (5) above et al.  I opted to convert all
> > relevant params to PRUint32 for the purposes of this patch, which doesn't
> > gain type safety but doesn't lose it relative to the previous typedef.  In a
> > few cases I wind up casting these faux nsresults to actual nsresults to stop
> > other callers of the same functions from losing type safety.
> 
> OK.  Again, a follow-up bug would be nice.

Okay.

(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #9)
> It would at least be 32 bits, but might it end up as a 64-bit value on
> 64-bit systems?

We could always make it "enum nsresult : PRUint32" on compilers that support that syntax.  (It's part of C11/C++11, but I believe VC supported it as a nonstandard extension long before.)  For compilers that don't support it, we could do a static assert to verify that it's the right size.

(Using this syntax will also allow us to forward-declare the enum.  That way nscore.h won't have to include nsError.h.)

I *think* enums in the compilers we care about are either the smallest size possible, or the size of an int.  I'd be surprised if compilers made enums 64-bit unnecessarily -- nobody's going to want that much space.

> I also happen to like the idea of being able to say result.failed() directly
> rather than NS_FAILED(result) in the long term, which is why I felt that a
> struct/class type might be better. There's a bug on this already somewhere...

Or we might even want to overload the boolean operator.  But then how do we define things like NS_OK?  Is there any way to make them compile-time constants when they have object value, short of C++11 constexpr?  Also, might compilers not optimize objects as well as enums?

Anyway, once we have *something* type-safe, it will be easy to switch to objects if we want.

(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #11)
> Yes, overall I think that's better. And yes, we still need to fix the
> various bad cases as well as the |= thing, although I think we could
> probably just make |= work with a custom operator on a struct.

This occurred to me.  The thing is, |'ing two nsresults doesn't generally return a useful nsresult.  It only guarantees that the result has the high bit set if and only if one of the |'d arguments was an error.  So if we wanted it to return a legitimate nsresult, we'd have to define something like

  nsresult& operator |=(nsresult& aLeft, const nsresult& aRight)
  {
    if (NS_FAILED(aRight)) {
      aLeft = aRight;
    }
    return aLeft;
  }

But this isn't the usual meaning of |= at all, so it would be confusing IMO.

Anyway, you can do this with enums too.  Operators can be overloaded for enums just as for classes.  The more interesting possibility, IMO, is deleting conversion operators -- you can't override conversion operators for enums, only for classes.  But we could use a C++11 enum class for this too, since it can't be converted implicitly to anything (which is what we want for nsresult).

I don't think rv.failed() is a win over failed(rv) or FAILED(rv).  Of course, ideally we should make that function type-safe, not a macro, whatever it is.
Created attachment 646517 [details] [diff] [review]
Remove conversions to nsresult that will always result in NS_SUCCEEDED

This fixes cases where functions return bool, or something else that will always be less than 0x80000000 (like number of bytes read/written by one call); and the result is then converted to nsresult and then immediately used only for NS_ENSURE_SUCCESS or similar.  In such cases, we know it will always be a success, so the existing code is equivalent to just ignoring the return value.
Attachment #646517 - Flags: review?(ehsan)
Created attachment 646518 [details] [diff] [review]
Convert incorrect conversions to nsresult and fix named constants

This is a long, long, long, boring patch.  It's meant to do two things and two things only:

1) Convert various constants equal to 0 to/from NS_OK.  E.g., change NS_OK to nsnull or 0, or vice versa.  This obviously does not affect the generated code at all.

2) Change things from PRInt32/PRUint32 to nsresult.  This will not affect the generated code, and only adds type safety, so it should also be safe.

There might be a few other fixes that crept into this patch by mistake instead of other patches, but they should be self-explanatory.  Tell me if they're not.  This one is long, but easy to check if you get over the tedium.
Attachment #646518 - Flags: review?(ehsan)
Created attachment 646520 [details] [diff] [review]
Don't use |= on nsresult

This one is also long and tedious, but necessarily a bit trickier to review than the last one.  Ms2ger, want to help out?  (Please feel free to help out with all the nontrivial patches here, in fact.)
Attachment #646520 - Flags: review?(ehsan)
Attachment #646520 - Flags: feedback?(Ms2ger)
Created attachment 646522 [details] [diff] [review]
Remove unreached return

Trivial.
Attachment #646522 - Flags: review?(ehsan)
Created attachment 646523 [details] [diff] [review]
exit(-1) instead of exit(NS_ERROR_FAILURE)
Attachment #646523 - Flags: review?(ehsan)
Depends on: 778103
Created attachment 646526 [details] [diff] [review]
Cast the result of setjmp() to nsresult where it's incorrectly used that way

I vote for this as the craziest code this conversion turned up.
Attachment #646526 - Flags: review?(ehsan)
Created attachment 646527 [details] [diff] [review]
Convert an nsresult variable to PRStatus

PRStatus is defined as:

typedef enum { PR_FAILURE = -1, PR_SUCCESS = 0 } PRStatus;
http://dxr.lanedo.com/mozilla-central/nsprpub/pr/include/prtypes.h.html#l454

so this happened to work before.
Attachment #646527 - Flags: review?(ehsan)
Created attachment 646528 [details] [diff] [review]
Add default cases to switches on nsresult

Avoids compiler warnings.
Attachment #646528 - Flags: review?(ehsan)
Created attachment 646529 [details] [diff] [review]
Fix incorrect conversions to nsresult

This is like the first patch, in that it fixes incorrect conversions to nsresult that will always result in a success code.  However, here the results of the conversions are actually returned.  As such, I manually verified that the callers of every function I'm changing only pass the result to a function like NS_FAILED, so they won't notice if I return NS_OK instead.  I might have made a mistake here -- please verify!
Attachment #646529 - Flags: review?(ehsan)
Depends on: 778104
Depends on: 778105
Depends on: 778106
Depends on: 778108
Depends on: 778109
Depends on: 778110
Created attachment 646540 [details] [diff] [review]
Annotate some incorrect conversions to nsresult

This is like the last patch, except I wasn't able to easily verify that actually changing the return value is correct, so I added a cast and comment instead.  Bugs filed on all.  These should be summarily fixable once we have nsresult converted to an enum class or class, and know that no one can be treating it as anything but nsresult.
Attachment #646540 - Flags: review?(ehsan)
Depends on: 778111
Created attachment 646542 [details] [diff] [review]
Cast NS_ENUMERATOR_FALSE to nsresult and complain in comments

Bug 778111 filed.
Attachment #646542 - Flags: review?(ehsan)
Depends on: 778113
Created attachment 646544 [details] [diff] [review]
Add casts and annoyed comments where we make up nsresult codes on the fly

Filed bug 778113.
Attachment #646544 - Flags: review?(ehsan)
Created attachment 646545 [details] [diff] [review]
Don't treat number of bytes as an nsresult

These Read()/Write() methods return the number of bytes read/written, not an nsresult.  It doesn't matter; the result was unused anyway.
Attachment #646545 - Flags: review?(ehsan)
Created attachment 646546 [details] [diff] [review]
Use assert for arg validity in nsHTMLAnonymousUtils' GetCSSFloatValue instead of NS_ENSURE_ARG_POINTER

The only caller already has a null check anyway, so it should be safe.
Attachment #646546 - Flags: review?(ehsan)
Created attachment 646551 [details] [diff] [review]
Return -1 from nsTableRowGroupFrame::FindLineContaining on null param

This previously returned NS_ERROR_NULL_POINTER, which is probably not what any callers were expecting!  I looked at the callers and am pretty sure no one passes a null aFrame anyway, though.  (I'd make it MOZ_ASSERT like the last patch, but . . . :) )
Attachment #646551 - Flags: review?(roc)
Created attachment 646552 [details] [diff] [review]
Convert some nsresult to SECStatus

SECStatus is defined as:

typedef enum _SECStatus {
    SECWouldBlock = -2,
    SECFailure = -1,
    SECSuccess = 0
} SECStatus;
http://dxr.lanedo.com/mozilla-central/security/nss/lib/util/seccomon.h.html#l91
Attachment #646552 - Flags: review?(ehsan)
Created attachment 646554 [details] [diff] [review]
Switch incorrect nsresult to nsrefcnt

This confused me for a while, until I realized that it was the return type that was wrong, not the call itself.

So this is everything needed to compile on localhost except

1) Dealing with the netwerk/ craziness of "use nsresult except with deliberately non-nsresult values".  I want to rewrite the way I approached that, so it's switched to use nsresult instead of PRUint32.

2) Actually moving all the error code definitions to nsError.h and making it an enum.

Plus, of course, anything needed to compile on platforms other than localhost, which is a popular development target but not sufficient for a cross-platform project like Mozilla.

Here's a try run for all patches submitted up to this point: <https://tbpl.mozilla.org/?tree=Try&rev=e14ebc503da0>.  There's no reason it shouldn't be green, since this doesn't include the final patch to make nsresult an enum.  But let's see.
Attachment #646554 - Flags: review?(ehsan)

Comment 30

6 years ago
Comment on attachment 646517 [details] [diff] [review]
Remove conversions to nsresult that will always result in NS_SUCCEEDED

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

r=me on the editor parts.  I think you should ask review on the other parts of the patch from module owners, since in some of these places we may actually need error checking.  Even though your patch doesn't change the behavior, but it will hide the fact that error checking might have been required in some of these places.
Attachment #646517 - Flags: review?(ehsan) → review+
Comment on attachment 646529 [details] [diff] [review]
Fix incorrect conversions to nsresult

This would also be a good patch to get extra eyes on.
Attachment #646529 - Flags: feedback?(Ms2ger)
Comment on attachment 646518 [details] [diff] [review]
Convert incorrect conversions to nsresult and fix named constants

And this.
Attachment #646518 - Flags: feedback?(Ms2ger)
CCing bz as a networking peer, and Christian Biesinger because he's listed as owner and I don't know any of the other peers.  nsISocketTransport.idl currently defines

    const unsigned long STATUS_RESOLVING      = 0x804b0003;
    const unsigned long STATUS_RESOLVED       = 0x804b000b;
    const unsigned long STATUS_CONNECTING_TO  = 0x804b0007;
    const unsigned long STATUS_CONNECTED_TO   = 0x804b0004;
    const unsigned long STATUS_SENDING_TO     = 0x804b0005;
    const unsigned long STATUS_WAITING_FOR    = 0x804b000a;
    const unsigned long STATUS_RECEIVING_FROM = 0x804b0006;

and then various things use them instead of nsresult.  This breaks when the final patch in this series is applied (making nsresult an enum).  Making everything that uses these PRUint32 instead more or less works, but it's both less informative and less type-safe.  Plus sometimes it leaks out into the wider codebase unless I cast a lot of things.

So what would you suggest?  Is this API used by JS anywhere, or only C++?  Could I move the definitions of these class constants to C++, and define them as being equal to nsresults defined centrally in the enum?  Or should I leave them as-is, but make the C++ equivalents (NS_NET_STATUS_*) into nsresults and port all C++ code to use them instead?

Updated

6 years ago
Attachment #646518 - Flags: review?(ehsan) → review+
You should probably use c++casts. Makes a lot easier to find them afterwards.
Created attachment 646561 [details] [diff] [review]
content/, js/, layout/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

See comment 13 and comment 30 for explanation.
Attachment #646561 - Flags: review?(bzbarsky)
Created attachment 646562 [details] [diff] [review]
gfx/, widget/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

See comment 13 and comment 30 for explanation.
Attachment #646562 - Flags: review?(roc)
Created attachment 646563 [details] [diff] [review]
accessible/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

See comment 13 and comment 30 for explanation.
Attachment #646563 - Flags: review?(surkov.alexander)
Created attachment 646564 [details] [diff] [review]
modules/libpref/, toolkit/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

See comment 13 and comment 30 for explanation.  Not sure if you're the right person to ask for all of these; if not, let me know.
Attachment #646564 - Flags: review?(benjamin)

Comment 39

6 years ago
Comment on attachment 646520 [details] [diff] [review]
Don't use |= on nsresult

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

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +333,5 @@
>          if (!nodeInfos.AppendObject(nodeInfo))
> +            tmp = NS_ERROR_OUT_OF_MEMORY;
> +        if (NS_FAILED(tmp)) {
> +          rv = tmp;
> +        }

Nit: this if should be nested inside the if on line 333.

@@ +350,5 @@
>              if (! pi) {
> +               tmp = NS_ERROR_OUT_OF_MEMORY;
> +               if (NS_FAILED(tmp)) {
> +                 rv = tmp;
> +               }

Nit: rv = NS_ERROR_OUT_OF_MEMORY;.
Attachment #646520 - Flags: review?(ehsan) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #39)
> ::: content/xul/document/src/nsXULPrototypeDocument.cpp
> @@ +333,5 @@
> >          if (!nodeInfos.AppendObject(nodeInfo))
> > +            tmp = NS_ERROR_OUT_OF_MEMORY;
> > +        if (NS_FAILED(tmp)) {
> > +          rv = tmp;
> > +        }
> 
> Nit: this if should be nested inside the if on line 333.

Actually, it should just be "rv = NS_ERROR_OUT_OF_MEMORY" here too.  I fixed a few of these as I saw them, but I made this patch semi-automatically, so I missed some.  Thanks!

Updated

6 years ago
Attachment #646522 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646523 - Flags: review?(ehsan) → review+

Comment 41

6 years ago
Comment on attachment 646526 [details] [diff] [review]
Cast the result of setjmp() to nsresult where it's incorrectly used that way

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

Sigh...
Attachment #646526 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646527 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646528 - Flags: review?(ehsan) → review+

Comment 42

6 years ago
Comment on attachment 646529 [details] [diff] [review]
Fix incorrect conversions to nsresult

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

Please ask the module owners to review this one.
Attachment #646529 - Flags: review?(ehsan)

Updated

6 years ago
Attachment #646540 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646542 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646544 - Flags: review?(ehsan) → review+

Comment 43

6 years ago
Comment on attachment 646545 [details] [diff] [review]
Don't treat number of bytes as an nsresult

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

This needs module owner review as well.
Attachment #646545 - Flags: review?(ehsan)

Updated

6 years ago
Attachment #646546 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646552 - Flags: review?(ehsan) → review+

Updated

6 years ago
Attachment #646554 - Flags: review?(ehsan) → review+

Comment 44

6 years ago
(BTW, I think I prefer the idea of converting nsresult into a class, since it gives us better control over what it can do.  We can then have an enum type which can be used to initialized nsresult objects.)

Comment 45

6 years ago
Jason: please see comment 33.

Comment 46

6 years ago
(In reply to comment #40)
> (In reply to Ehsan Akhgari [:ehsan] from comment #39)
> > ::: content/xul/document/src/nsXULPrototypeDocument.cpp
> > @@ +333,5 @@
> > >          if (!nodeInfos.AppendObject(nodeInfo))
> > > +            tmp = NS_ERROR_OUT_OF_MEMORY;
> > > +        if (NS_FAILED(tmp)) {
> > > +          rv = tmp;
> > > +        }
> > 
> > Nit: this if should be nested inside the if on line 333.
> 
> Actually, it should just be "rv = NS_ERROR_OUT_OF_MEMORY" here too.  I fixed a
> few of these as I saw them, but I made this patch semi-automatically, so I
> missed some.  Thanks!

Right!
Created attachment 646566 [details] [diff] [review]
content/, dom/base/, embedding/, js/: Fix incorrect conversions to nsresult

See comment 13 and comment 21 for context.
Attachment #646566 - Flags: review?(bzbarsky)
Created attachment 646568 [details] [diff] [review]
dom/plugins/: Fix incorrect conversions to nsresult

See comment 13 and comment 21 for context.
Attachment #646568 - Flags: review?(roc)
Created attachment 646569 [details] [diff] [review]
modules/libpref/, toolkit/: Fix incorrect conversions to nsresult

See comment 13 and comment 21 for context.
Attachment #646569 - Flags: review?(benjamin)
Created attachment 646571 [details] [diff] [review]
security/manager/: Fix incorrect conversions to nsresult

See comment 13 and comment 21 for context.
Attachment #646571 - Flags: review?(kaie)
(In reply to Ehsan Akhgari [:ehsan] from comment #44)
> (BTW, I think I prefer the idea of converting nsresult into a class, since
> it gives us better control over what it can do.  We can then have an enum
> type which can be used to initialized nsresult objects.)

I suggest we get the conversion to enum landed first, because its type safety is weaker, so it's easier to do.  Converting to class or enum class would require a bunch more prep work to get rid of all the places that implicitly convert *from* nsresult *to* a numeric type, since that's still legal with enums.
Attachment #646529 - Attachment is obsolete: true
Attachment #646529 - Flags: feedback?(Ms2ger)
Created attachment 646574 [details] [diff] [review]
netwerk/: Don't treat number of bytes as an nsresult

(In reply to :Aryeh Gregor from comment #25)
> These Read()/Write() methods return the number of bytes read/written, not an
> nsresult.  It doesn't matter; the result was unused anyway.
Attachment #646545 - Attachment is obsolete: true
Attachment #646574 - Flags: review?(bzbarsky)
Created attachment 646575 [details] [diff] [review]
security/manager/: Don't treat number of bytes as an nsresult

(In reply to :Aryeh Gregor from comment #25)
> These Read()/Write() methods return the number of bytes read/written, not an
> nsresult.  It doesn't matter; the result was unused anyway.
Attachment #646575 - Flags: review?(kaie)
> Is this API used by JS anywhere,

Yes.  The HUD service uses it, and lots of use in extensions (e.g. any extension that needs raw socket access, like ChatZilla).

Can you just file a separate bug on this?  Make sure to cc the active network maintainers (so jduell, mayhemer, mcmanus).  Having a discussion in this bug is pretty impossible.
Comment on attachment 646561 [details] [diff] [review]
content/, js/, layout/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

I would prefer this actually do the error checks right.  Certainly in the query processor case.  And the XPConnect case, where I believe ignoring failure is a security bug.  And in the slider frame case, where it'll happily operate on uninitialized memory iirc...

Yes, that will change behavior.  Land it in a separate patch from this bug.  ;)
Attachment #646561 - Flags: review?(bzbarsky) → review-

Comment 56

6 years ago
(In reply to comment #51)
> (In reply to Ehsan Akhgari [:ehsan] from comment #44)
> > (BTW, I think I prefer the idea of converting nsresult into a class, since
> > it gives us better control over what it can do.  We can then have an enum
> > type which can be used to initialized nsresult objects.)
> 
> I suggest we get the conversion to enum landed first, because its type safety
> is weaker, so it's easier to do.  Converting to class or enum class would
> require a bunch more prep work to get rid of all the places that implicitly
> convert *from* nsresult *to* a numeric type, since that's still legal with
> enums.

Do you have a good solution to ensure the size of the enum to be 32-bits on all platforms?
Comment on attachment 646566 [details] [diff] [review]
content/, dom/base/, embedding/, js/: Fix incorrect conversions to nsresult

The MorphSlimWrapper bit is wrong.  That really really needs to actually fail if it fails.

The script error reporting part looks kinda fishy too, but probably OK.

The rest are fine.
Attachment #646566 - Flags: review?(bzbarsky) → review-
Comment on attachment 646574 [details] [diff] [review]
netwerk/: Don't treat number of bytes as an nsresult

r=me
Attachment #646574 - Flags: review?(bzbarsky) → review+
> Do you have a good solution to ensure the size of the enum to be 32-bits on all platforms?

I think it's going to be 32 bits in C++ (*).  The C++98 spec (section 7.2.5) says:

> It is implementation-defined which integral type is used as the underlying type
> for an enumeration except that the underlying type shall not be larger than int
> unless the value of an enumerator cannot fit in an int or unsigned int.

(*) Except on ILP64 systems, of which the only one that matters, I think, is Solaris64.  Dunno if we care.

C99, on the other hand, leaves the type entirely implementation-defined.  I would guess that, for compatibility with C++, compilers would apply the C++ rule to C, so if it works on all the platforms we care about, I'd be OK putting in a static assert.
(From comment 7)
> I don't think we have a working C xpcom interface.

Oh, I missed this earlier.

> I'm a little worried about making this an enum because that could theoretically change the size of 
> the type; in the past the proposals of this sort have turned nsresult into a class wrapping an int 
> instead (with private operator/constructors for the obviously incorrect usages).

If we only care about C++, then it seems using an enum is safe iff we don't care about ILP64 systems.

Comment 61

6 years ago
(In reply to comment #60)
> > I'm a little worried about making this an enum because that could theoretically change the size of 
> > the type; in the past the proposals of this sort have turned nsresult into a class wrapping an int 
> > instead (with private operator/constructors for the obviously incorrect usages).
> 
> If we only care about C++, then it seems using an enum is safe iff we don't
> care about ILP64 systems.

Well, Solaris64 is definitely not Tier1, but I don't know how to determine whether it's OK to intentionally break them (not that we still know if we will really be breaking them...)
Comment on attachment 646568 [details] [diff] [review]
dom/plugins/: Fix incorrect conversions to nsresult

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

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +535,5 @@
>  
>    *aRetainedPeer = NULL;
>  
>    if (!pstream || !pstream->ndata)
> +    return NS_OK;

Shouldn't this be converted to an error result? NS_ERROR_NULL_POINTER perhaps?
(In reply to Justin Lebar [:jlebar] from comment #59)
> C99, on the other hand, leaves the type entirely implementation-defined.  I
> would guess that, for compatibility with C++, compilers would apply the C++
> rule to C, so if it works on all the platforms we care about, I'd be OK
> putting in a static assert.

Sounds like adding a static assert that sizeof(nsresult) == sizeof(uint32_t) is the right thing to do, and we can fix minority platform bustage if it happens.
(In reply to :Aryeh Gregor from comment #51)
> (In reply to Ehsan Akhgari [:ehsan] from comment #44)
> > (BTW, I think I prefer the idea of converting nsresult into a class, since
> > it gives us better control over what it can do.  We can then have an enum
> > type which can be used to initialized nsresult objects.)

fwiw I'd agree, .Failed() / Succeeded() sound nice, but an enum seems better than what we have.

> I suggest we get the conversion to enum landed first, because its type
> safety is weaker, so it's easier to do.  Converting to class or enum class
> would require a bunch more prep work to get rid of all the places that
> implicitly convert *from* nsresult *to* a numeric type, since that's still
> legal with enums.

well, you could just add temporary operator PRInt32 / PRUint32 <.<
Comment on attachment 646563 [details] [diff] [review]
accessible/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

> 
>-    nsresult rv = gApplicationAccessible->Init();
>-    if (NS_FAILED(rv)) {
>-      gApplicationAccessible->Shutdown();
>-      NS_RELEASE(gApplicationAccessible);
>-      return nsnull;
>-    }
>+    gApplicationAccessible->Init();

its no more broken than it was, but why not just use the bool Init() returns where NS_FAILED() was used before?
(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #9)
> It would at least be 32 bits, but might it end up as a 64-bit value on
> 64-bit systems?

I just found this in C++11, 7.2(6):

"""
For an enumeration whose underlying type is not fixed, the underlying type is an integral type that can represent all the enumerator values defined in the enumeration. If no integral type can represent all the enumerator values, the enumeration is ill-formed. It is implementation-defined which integral type is used as the underlying type except that the underlying type shall not be larger than int unless the value of an enumerator cannot fit in an int or unsigned int.
"""

So looks like we're safe.
Attachment #646517 - Flags: checkin+
Attachment #646520 - Flags: checkin+
Attachment #646522 - Flags: checkin+
Attachment #646523 - Flags: checkin+
Attachment #646526 - Flags: checkin+
Attachment #646527 - Flags: checkin+
Attachment #646528 - Flags: checkin+
Attachment #646540 - Flags: checkin+
Attachment #646542 - Flags: checkin+
Attachment #646544 - Flags: checkin+
Attachment #646546 - Flags: checkin+
Attachment #646552 - Flags: checkin+
Attachment #646554 - Flags: checkin+
Depends on: 778680
Depends on: 778681
Sorry for how crowded this bug is getting -- I'll try to move as much activity as possible to blockers.

(In reply to Boris Zbarsky (:bz) from comment #54)
> Can you just file a separate bug on this?  Make sure to cc the active
> network maintainers (so jduell, mayhemer, mcmanus).  Having a discussion in
> this bug is pretty impossible.

Bug 778680.

(In reply to Boris Zbarsky (:bz) from comment #55)
> I would prefer this actually do the error checks right.  Certainly in the
> query processor case.  And the XPConnect case, where I believe ignoring
> failure is a security bug.  And in the slider frame case, where it'll
> happily operate on uninitialized memory iirc...
> 
> Yes, that will change behavior.  Land it in a separate patch from this bug. 
> ;)

Bug 778681.

(In reply to Ehsan Akhgari [:ehsan] from comment #56)
> Do you have a good solution to ensure the size of the enum to be 32-bits on
> all platforms?

AFAICT, it already will be in practice.  Technically the spec permits IPL64 platforms to make it 64-bit, but I really doubt any of them do, so I think a static assert is fine until it's proven that some compiler exists that doesn't satisfy it.

(In reply to Boris Zbarsky (:bz) from comment #57)
> The MorphSlimWrapper bit is wrong.  That really really needs to actually
> fail if it fails.
> 
> The script error reporting part looks kinda fishy too, but probably OK.
> 
> The rest are fine.

I've lumped this under bug 778681 too.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #62)
> ::: dom/plugins/base/nsNPAPIPlugin.cpp
> @@ +535,5 @@
> >  
> >    *aRetainedPeer = NULL;
> >  
> >    if (!pstream || !pstream->ndata)
> > +    return NS_OK;
> 
> Shouldn't this be converted to an error result? NS_ERROR_NULL_POINTER
> perhaps?

I guess that would work just as well.  It looks like the only caller ignores the result anyway:

http://dxr.lanedo.com/mozilla-central/dom/plugins/ipc/BrowserStreamParent.cpp.html#l149

(In reply to Trevor Saunders (:tbsaunde) from comment #65)
> Comment on attachment 646563 [details] [diff] [review]
> accessible/: Remove conversions to nsresult that will always result in
> NS_SUCCEEDED
> 
> > 
> >-    nsresult rv = gApplicationAccessible->Init();
> >-    if (NS_FAILED(rv)) {
> >-      gApplicationAccessible->Shutdown();
> >-      NS_RELEASE(gApplicationAccessible);
> >-      return nsnull;
> >-    }
> >+    gApplicationAccessible->Init();
> 
> its no more broken than it was, but why not just use the bool Init() returns
> where NS_FAILED() was used before?

Because that changes behavior, which I want to avoid in this bug.
Created attachment 647114 [details] [diff] [review]
dom/plugins/: Fix incorrect conversions to nsresult, v2

This uses NS_ERROR_NULL_POINTER instead of NS_OK.  Shouldn't change anything, since AFAICT there's only one caller and it ignores the return value entirely.
Attachment #646568 - Attachment is obsolete: true
Attachment #646568 - Flags: review?(roc)
Attachment #647114 - Flags: review?(roc)
Created attachment 647140 [details] [diff] [review]
Fix so "Convert incorrect conversions to nsresult and fix named constants" builds on Android (folded for checkin)

I missed a couple of uses on Android, so try failed.  With this, try succeeds:

https://tbpl.mozilla.org/?tree=Try&rev=fac5182a8d75
Attachment #647140 - Flags: review?(ehsan)
Attachment #646518 - Flags: feedback?(Ms2ger)
Attachment #646520 - Flags: feedback?(Ms2ger)

Updated

6 years ago
Attachment #647140 - Flags: review?(ehsan) → review+
Attachment #646518 - Flags: checkin+
Attachment #647140 - Attachment description: Fix so "Convert incorrect conversions to nsresult and fix named constants" builds on Android → Fix so "Convert incorrect conversions to nsresult and fix named constants" builds on Android (folded for checkin)
Attachment #647140 - Flags: checkin+
Attachment #646562 - Flags: checkin+
Attachment #646574 - Flags: checkin+
Attachment #646551 - Flags: checkin+
Moving nsresult to an enum would cause one major critical bustage for anything that uses XPCOM from C++ that is not Firefox:

It prevents people from defining their own nsresult error codes.

Irrespective of any benefit that this change brings by itself, this is the sort of magnitude of change that SHOULD NOT be solely discussed in a bug that nobody sees. This has the potential to break any and all potential downstream users--from Thunderbird to people like Komodo.

I only heard about this change by lounging in #developers; I then came to this bug and saw that a lot of patches were checked in and started freaking out. Why hasn't this been discussed this on m.d.platform or m.d.planning before going ahead with implementation? Why do I see no XPCOM peers weighing in?
(In reply to :Aryeh Gregor from comment #1)
> So some remarks on various patterns that came up as I've tried to fix this,
> in random order:
> 
> 1) Obviously, all errors must now be centrally declared in nsError.h, not
> scattered everywhere around the codebase.  This strikes me as a good thing. 
> :)  It means that I removed some header files that formerly contained only
> error codes, and changed everything that included them to include nsError.h
> instead.  I just used sed -i for this, and didn't worry if files multiply
> included nsError.h as a result.

I've only just heard about this, and I'm concerned, namely:

- How will non-FF applications define their error values?
-- Thunderbird currently defines a whole load in http://mxr.mozilla.org/comm-central/source/mailnews/base/public/msgCore.h, I'm not seeing a description of how we would be able to continue that.

- Will js components still be able to throw their own custom result values that may get passed back into c++, violating the enum values?
> Why do I see no XPCOM peers weighing in?

bsmedberg has been active in this bug, and he's the owner of XPCOM.  He's also explicitly delegated his authority to others in this bug.
(In reply to Justin Lebar [:jlebar] from comment #74)
> > Why do I see no XPCOM peers weighing in?
> 
> bsmedberg has been active in this bug, and he's the owner of XPCOM.  He's
> also explicitly delegated his authority to others in this bug.

The actual correct answer to this question is "there were so many comments about the fix-broken-stuff patches that I didn't see the discussions about the actual concepts themselves," although I'm still dismayed by the lack of discussion on the impact on non-core code.

Comment 76

6 years ago
(In reply to comment #72)
> It prevents people from defining their own nsresult error codes.

That's not true.

#define MY_ERROR_CODE ((nsresult)0xdeadbeef)

> I only heard about this change by lounging in #developers; I then came to this
> bug and saw that a lot of patches were checked in and started freaking out. Why
> hasn't this been discussed this on m.d.platform or m.d.planning before going
> ahead with implementation?

Precisely because it will not break other consumers.  The patches that have been checked in so far in this bug are just fixing bad usages of nsresult which were not previously caught by the compiler.  There's no reason why anyone should freak out!

Comment 77

6 years ago
(In reply to comment #73)
> (In reply to :Aryeh Gregor from comment #1)
> > So some remarks on various patterns that came up as I've tried to fix this,
> > in random order:
> > 
> > 1) Obviously, all errors must now be centrally declared in nsError.h, not
> > scattered everywhere around the codebase.  This strikes me as a good thing. 
> > :)  It means that I removed some header files that formerly contained only
> > error codes, and changed everything that included them to include nsError.h
> > instead.  I just used sed -i for this, and didn't worry if files multiply
> > included nsError.h as a result.
> 
> I've only just heard about this, and I'm concerned, namely:
> 
> - How will non-FF applications define their error values?

See my previous comment.

> -- Thunderbird currently defines a whole load in
> http://mxr.mozilla.org/comm-central/source/mailnews/base/public/msgCore.h, I'm
> not seeing a description of how we would be able to continue that.

They will all work just fine since they use NS_ERROR_GENERATE which uses C-style tags.

> - Will js components still be able to throw their own custom result values that
> may get passed back into c++, violating the enum values?

Sure.  The enum values will only be checked for the error codes which are actually in the enum.  Everything else will be cast to the nsresult type.
(In reply to Ehsan Akhgari [:ehsan] from comment #76)
> (In reply to comment #72)
> > It prevents people from defining their own nsresult error codes.
> 
> That's not true.
> 
> #define MY_ERROR_CODE ((nsresult)0xdeadbeef)

I'm not convinced that's fully safe. From C++11, section 7.2:
An expression of arithmetic or enumeration type can be converted to an enumeration type explicitly. The value is unchanged if it is in the range of enumeration values of the enumeration type; otherwise the resulting enumeration value is unspecified.

[Note: enumeration type is distinct from the underlying type.]

In other words, this is basically unsafe unless you explicitly declare an underlying type (cf. a few paragraphs earlier).
> I'm not convinced that's fully safe. From C++11, section 7.2:
> An expression of arithmetic or enumeration type can be converted to an
> enumeration type explicitly. The value is unchanged if it is in the range of
> enumeration values of the enumeration type; otherwise the resulting
> enumeration value is unspecified.
> 
> [Note: enumeration type is distinct from the underlying type.]
> 
> In other words, this is basically unsafe unless you explicitly declare an
> underlying type (cf. a few paragraphs earlier).

Both clang and gcc put this behind -fstrict-enum (which we don't use). Not sure about msvc.

Comment 80

6 years ago
(In reply to comment #78)
> (In reply to Ehsan Akhgari [:ehsan] from comment #76)
> > (In reply to comment #72)
> > > It prevents people from defining their own nsresult error codes.
> > 
> > That's not true.
> > 
> > #define MY_ERROR_CODE ((nsresult)0xdeadbeef)
> 
> I'm not convinced that's fully safe. From C++11, section 7.2:
> An expression of arithmetic or enumeration type can be converted to an
> enumeration type explicitly. The value is unchanged if it is in the range of
> enumeration values of the enumeration type; otherwise the resulting enumeration
> value is unspecified.
> 
> [Note: enumeration type is distinct from the underlying type.]
> 
> In other words, this is basically unsafe unless you explicitly declare an
> underlying type (cf. a few paragraphs earlier).

If we guarantee the size of the enum at compile time, I think this will be fine.
> If we guarantee the size of the enum at compile time, I think this will be
> fine.

With a strict compiler (-fstrict-enum with gcc and clang), the problem is not the size, it is that the compiler can optimize based on the knowledge of the enum values. If the standard says that an enum can only hold values 0..7 based on the values its definition contains, the compiler can fold

if (x == 12)

to false.

Comment 82

6 years ago
(In reply to comment #81)
> > If we guarantee the size of the enum at compile time, I think this will be
> > fine.
> 
> With a strict compiler (-fstrict-enum with gcc and clang), the problem is not
> the size, it is that the compiler can optimize based on the knowledge of the
> enum values. If the standard says that an enum can only hold values 0..7 based
> on the values its definition contains, the compiler can fold
> 
> if (x == 12)
> 
> to false.

I see.  In that case we should probably attempt to detect -fstrict-enum in the CFLAGS and just fail those builds at configure time.
> I see.  In that case we should probably attempt to detect -fstrict-enum in
> the CFLAGS and just fail those builds at configure time.

Maybe. I remember some discussion about us not wanting to black list options (I think it was -ffast-math at the time), but I don't remember the result of the discussion.

Comment 84

6 years ago
(In reply to comment #83)
> > I see.  In that case we should probably attempt to detect -fstrict-enum in
> > the CFLAGS and just fail those builds at configure time.
> 
> Maybe. I remember some discussion about us not wanting to black list options (I
> think it was -ffast-math at the time), but I don't remember the result of the
> discussion.

Hmm, in the case of -fstrict-enum, I see no reason for us to make it possible for people to create broken binaries!  (Note that nsresult will not be the first enum in our code base which breaks strict enum rules!)
> Hmm, in the case of -fstrict-enum, I see no reason for us to make it
> possible for people to create broken binaries!  (Note that nsresult will not
> be the first enum in our code base which breaks strict enum rules!)

I guess I am OK with it, but you might want to get glandium to review it. The bug about -ffast-math was 680016. The situation is a bit different as -fstrict-enum *is* standard compliant.

Comment 86

6 years ago
(In reply to comment #85)
> > Hmm, in the case of -fstrict-enum, I see no reason for us to make it
> > possible for people to create broken binaries!  (Note that nsresult will not
> > be the first enum in our code base which breaks strict enum rules!)
> 
> I guess I am OK with it, but you might want to get glandium to review it. The
> bug about -ffast-math was 680016. The situation is a bit different as
> -fstrict-enum *is* standard compliant.

Filed bug 778992.

Comment 87

6 years ago
Comment on attachment 646563 [details] [diff] [review]
accessible/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

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

::: accessible/src/base/nsAccessNode.cpp
@@ -100,5 @@
> -    if (NS_FAILED(rv)) {
> -      gApplicationAccessible->Shutdown();
> -      NS_RELEASE(gApplicationAccessible);
> -      return nsnull;
> -    }

Init() returns boolean, so either you should convert Init() to void or do
if (!gApplicationAcc->Init()) {
  //
}
(In reply to Joshua Cranmer [:jcranmer] from comment #78)
> I'm not convinced that's fully safe. From C++11, section 7.2:
> An expression of arithmetic or enumeration type can be converted to an
> enumeration type explicitly. The value is unchanged if it is in the range of
> enumeration values of the enumeration type; otherwise the resulting
> enumeration value is unspecified.
> 
> [Note: enumeration type is distinct from the underlying type.]
> 
> In other words, this is basically unsafe unless you explicitly declare an
> underlying type (cf. a few paragraphs earlier).

See 7.2(7):

"""
For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type. Otherwise, for an enumeration where emin is the smallest enumerator and emax is the largest, the values of the enumeration are the values in the range bmin to bmax, defined as follows: Let K be 1 for a two’s complement representation and 0 for a one’s complement or sign-magnitude representation. bmax is the smallest value greater than or equal to max(|emin| − K, |emax|) and equal to 2M − 1, where M is a non-negative integer. bmin is zero if emin is non-negative and −(bmax + K) otherwise. The size of the smallest bit-field large enough to hold all the values of the enumeration type is max(M, 1) if bmin is zero and M + 1 otherwise.
"""

All the values we declare for nsresult are >= 0 and < 2^32, and the largest is strictly between 2^31 and 2^32, so per spec the enumeration's values are the same as uint32_t, i.e., [0, 2^32).  So no, this is safe per spec.

I could define nsresult as a class instead of an enum, and add an explicit constructor from PRUint32, so we could still have all the values decentralized.  That would have all the same benefits as an actual enum, plus give us more control over how it's used.  The question is whether it might hurt optimization; I know roc has said that compilers historically have not always optimized such single-member classes as well as built-in types.  But if we aren't worried about perf, I'd be fine with doing things that way and leaving all the enum definitions decentralized.

There might be other disadvantages to doing things this way, though.  For instance, switch statements would no longer work with nsresult, right?

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #81)
> With a strict compiler (-fstrict-enum with gcc and clang), the problem is
> not the size, it is that the compiler can optimize based on the knowledge of
> the enum values. If the standard says that an enum can only hold values 0..7
> based on the values its definition contains, the compiler can fold
> 
> if (x == 12)
> 
> to false.

This is invalid per spec.  Casts from arbitrary numeric types to enums are legal and the result is well-defined, as long as the number falls within the enum's range.  If people want to enable nonstandard behavior via crazy compiler options, they'll have no problem breaking things horribly.

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #85)
> I guess I am OK with it, but you might want to get glandium to review it.
> The bug about -ffast-math was 680016. The situation is a bit different as
> -fstrict-enum *is* standard compliant.

Not AFAICT.
Attachment #647114 - Flags: checkin+
Depends on: 779091
Comment on attachment 646563 [details] [diff] [review]
accessible/: Remove conversions to nsresult that will always result in NS_SUCCEEDED

(In reply to alexander :surkov from comment #87)
> Comment on attachment 646563 [details] [diff] [review]
> accessible/: Remove conversions to nsresult that will always result in
> NS_SUCCEEDED
> 
> Review of attachment 646563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessNode.cpp
> @@ -100,5 @@
> > -    if (NS_FAILED(rv)) {
> > -      gApplicationAccessible->Shutdown();
> > -      NS_RELEASE(gApplicationAccessible);
> > -      return nsnull;
> > -    }
> 
> Init() returns boolean, so either you should convert Init() to void or do
> if (!gApplicationAcc->Init()) {
>   //
> }

I moved this to bug 779091 (where I tried to convert it to void).
Attachment #646563 - Attachment is obsolete: true
Attachment #646563 - Flags: review?(surkov.alexander)
Depends on: 779122
Depends on: 779123
(In reply to Ehsan Akhgari [:ehsan] from comment #76)
> (In reply to comment #72)
> > It prevents people from defining their own nsresult error codes.
> 
> That's not true.
> 
> #define MY_ERROR_CODE ((nsresult)0xdeadbeef)

This does have one catch: gcc will complain if you use this as the case of a switch on an nsresult, with -Wswitch (which we use).  I think this is a gcc bug, though:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54140

(Notably, if we make nsresult a class instead of an enum, switch won't work anyway.)
(In reply to :Aryeh Gregor from comment #92)
> (Notably, if we make nsresult a class instead of an enum, switch won't work
> anyway.)

Actually, that's not true: switch will still work as long as the class can be implicitly converted to an integer or enumeration type.  C++ is great, isn't it?
Depends on: 779130
Created attachment 647534 [details] [diff] [review]
Define ParamTraits<nsresult>

Once nsresult is its own type, whether enum or class, it apparently needs its own ParamTraits.  This patch probably won't compile if nsresult is not yet actually its own type, so it's meant to be folded into the patch that actually redefines nsresult.

(I'm keeping further stuff in this bug only to the task of actually switching nsresult to an enum, not things that just block the switch, but this actually needs to be done as part of the switch, so I'll still use this bug for it.)
Attachment #647534 - Flags: review?(jones.chris.g)
> > if (x == 12)
> > 
> > to false.
> 
> This is invalid per spec.  Casts from arbitrary numeric types to enums are
> legal and the result is well-defined, as long as the number falls within the
> enum's range.

The point is precisely that they don't. We use values that fit the underlying type but are not in the range defined by the bits of the c++ standard you quoted. 

>  If people want to enable nonstandard behavior via crazy
> compiler options, they'll have no problem breaking things horribly.
> 
> (In reply to Rafael Ávila de Espíndola (:espindola) from comment #85)
> > I guess I am OK with it, but you might want to get glandium to review it.
> > The bug about -ffast-math was 680016. The situation is a bit different as
> > -fstrict-enum *is* standard compliant.
> 
> Not AFAICT.

I am really sure it is. All it does is tell the compiler to assume the enum is in the range you quoted.
> I am really sure it is. All it does is tell the compiler to assume the enum
> is in the range you quoted.

which may or may not be all the possible 32 bits values, but as Ehsan pointed out nsresult is not the only enum we use in cases like this, so in summary:

*) It doesn't look like we can support -fstrict-enum.
*) If we don't use, code like this is safe.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #95)
> The point is precisely that they don't. We use values that fit the
> underlying type but are not in the range defined by the bits of the c++
> standard you quoted. 

We specify the value 0, no negative value, and the largest value we specify is >= 0x80000000 and <= 0xffffffff (all failure codes are in this range).  So the bits of the C++ standard I quoted say that the range of values for the enum are from 0 to 0xffffffff, i.e., exactly a PRUint32.

In more detail: emin is 0, emax is something between 2^31 and 2^32.  Per spec, bmax is the smallest value >= max(|emin| - K, |emax|) and equal to 2^M - 1, where M is a non-negative integer.  2^31 is too small, because |emax| > 2^31.  2^32 is > |emax|, on the other hand, so bmax = 2^32 - 1.  bmin is 0.  So we're completely safe here per spec.

> I am really sure it is. All it does is tell the compiler to assume the enum
> is in the range you quoted.

Which is [0, 2^32), exactly the range of a PRUint32.
Depends on: 779442
Blocks: 779473
Comment on attachment 647534 [details] [diff] [review]
Define ParamTraits<nsresult>

I would usually recommend using an EnumSerializer for this, but I have no idea whether there are any constraints on legal values of nsresult, and I kind of doubt it.
Comment on attachment 647534 [details] [diff] [review]
Define ParamTraits<nsresult>

Er, meant to set r+.
Attachment #647534 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #99)
> I would usually recommend using an EnumSerializer for this, but I have no
> idea whether there are any constraints on legal values of nsresult, and I
> kind of doubt it.

Nope, not really.  In particular, there are parts of the code that generate nsresults on the fly that aren't part of the original enum, and it looks like we want it to stay that way (bug 778113).
Depends on: 780618
Comment on attachment 646571 [details] [diff] [review]
security/manager/: Fix incorrect conversions to nsresult

Please mention in a comment that we deliberately ignoring the failure of InsertElementAt for historic reasons.

r=kaie to a new patch with the comment added
Attachment #646571 - Flags: review?(kaie) → review-
Comment on attachment 646575 [details] [diff] [review]
security/manager/: Don't treat number of bytes as an nsresult

r=kaie
Attachment #646575 - Flags: review?(kaie) → review+
Attachment #646575 - Flags: checkin+
Attachment #646571 - Flags: checkin+

Updated

6 years ago
Attachment #646564 - Flags: review?(benjamin) → review+

Comment 106

6 years ago
Comment on attachment 646569 [details] [diff] [review]
modules/libpref/, toolkit/: Fix incorrect conversions to nsresult

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp


>   rv = mScopedXPCom->SetWindowCreator(mNativeApp);
>   NS_TIME_FUNCTION_MARK("ScopedXPCOMStartup: SetWindowCreator");
>-  NS_ENSURE_SUCCESS(rv, 1);
>+  NS_ENSURE_SUCCESS(rv, NS_OK);

Here and below, I don't see how this can be correct. Originally before the XRE_mainrun refactoring, this was the "int" return code which got passed out to main, so this should be a failure code.

>diff --git a/toolkit/xre/nsEmbedFunctions.cpp b/toolkit/xre/nsEmbedFunctions.cpp
>--- a/toolkit/xre/nsEmbedFunctions.cpp
>+++ b/toolkit/xre/nsEmbedFunctions.cpp
>@@ -282,65 +282,65 @@ XRE_InitChildProcess(int aArgc,
>   NS_ENSURE_ARG_POINTER(aArgv[0]);
> 
>   sChildProcessType = aProcess;
> 
>   // Complete 'task_t' exchange for Mac OS X. This structure has the same size
>   // regardless of architecture so we don't have any cross-arch issues here.
> #ifdef XP_MACOSX
>   if (aArgc < 1)
>-    return 1;
>+    return NS_OK;

Same thing here.
Attachment #646569 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #106)
> Here and below, I don't see how this can be correct. Originally before the
> XRE_mainrun refactoring, this was the "int" return code which got passed out
> to main, so this should be a failure code.
> 
> . . .
> 
> Same thing here.

Well, I verified that the patch doesn't change existing behavior, but it's perfectly possible that existing behavior is wrong.  I'll write a new patch that changes all these to NS_ERROR_FAILURE or such.
Attachment #646564 - Flags: checkin+
Created attachment 650008 [details] [diff] [review]
Patch v2 -- modules/libpref/, toolkit/: Fix incorrect conversions to nsresult

This changes to NS_ERROR_FAILURE instead of NS_OK, so it does change behavior.  Try: https://tbpl.mozilla.org/?tree=Try&rev=f68c61841e44
Attachment #646569 - Attachment is obsolete: true
Attachment #650008 - Flags: review?(benjamin)

Updated

6 years ago
Attachment #650008 - Flags: review?(benjamin) → review+
Comment on attachment 650008 [details] [diff] [review]
Patch v2 -- modules/libpref/, toolkit/: Fix incorrect conversions to nsresult

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c88e6a56be4

Now the only outstanding blocker in my current patchset is bug 780618, plus c-c misuse of nsresult (bug 779130).  Then I'll post the actual conversion patch for review.
Attachment #650008 - Flags: checkin+
Depends on: 782232
Created attachment 652060 [details] [diff] [review]
The final patch: Convert nsresult to enum

Bug 779130 is the last blocker, so I'll ask for your review now.  Final landing is contingent on all patches from bug 779130 being landed, successful compile of c-c on localhost, and green try run of m-c.  The change to ipc/chromium/src/chrome/common/ipc_message_utils.h was already reviewed by cjones in comment 100, together with you right after bug 782252 comment 11, so I'm only asking for review on the xpcom/ changes.

I don't like the fact that nscore.h has to include nsError.h, but enums can't be forward-declared in some of the compilers we support.  If you prefer, I could add support for forward-declaring enums to MFBT and use it here, but it would mean that some compilers would include the file and some wouldn't, which would lead to an annoying rate of unexpected try failures.  Visual Studio only supports forward-declared enums in 11, so we won't be able to use it unconditionally for a while yet.

I defined SUCCESS()/FAILURE() macros independently of NS_ERROR_GENERATE() because that casts to nsresult, so it can't be used to initialize values in the nsresult enum.  Also, I rather like the neatness and readability of just FAILURE(x) instead of NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FOO, x).

I moved NS_ERROR_GENERATE() below the enum definition because it uses the nsresult type, although of course it's a macro so it could really stay above even though nsresult is undefined.  I expect to rewrite it in a later patch to be an inline function, so it will need to be further down anyway.
Attachment #652060 - Flags: review?(ehsan)
Comment on attachment 652060 [details] [diff] [review]
The final patch: Convert nsresult to enum

>+typedef enum nsresult {
...
>+} nsresult;
Not sure what Mozilla style is, but I've seen some people write
typedef enum { ... } nsresult;
and others
typedef enum tag_nsresult { ... } nsresult;
possibly because their debuggers don't like untagged types.
Comment on attachment 652060 [details] [diff] [review]
The final patch: Convert nsresult to enum

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

::: xpcom/base/nsError.h
@@ +10,1 @@
>  

Actually, scratch this change.  The nscore.h include needs to be kept for NS_EXPORT.  This makes the include circular, but that shouldn't be harmful, I don't think.  It seems to compile for me, anyway, at least on localhost.  (I'm currently trying to get this to compile on try, and finding lots of platform-specific stuff still to fix.)
So apparently I don't know how to use Splinter -- I meant to comment on the change from including nscore.h to including prtypes.h in nsError.h.

Comment 117

6 years ago
(In reply to comment #115)
> Comment on attachment 652060 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=652060
> The final patch: Convert nsresult to enum
> 
> Review of attachment 652060 [details] [diff] [review]:
>  --> (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=777292&attachment=652060)
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsError.h
> @@ +10,1 @@
> >  
> 
> Actually, scratch this change.  The nscore.h include needs to be kept for
> NS_EXPORT.  This makes the include circular, but that shouldn't be harmful, I
> don't think.  It seems to compile for me, anyway, at least on localhost.  (I'm
> currently trying to get this to compile on try, and finding lots of
> platform-specific stuff still to fix.)

That's fine!

Comment 118

6 years ago
Comment on attachment 652060 [details] [diff] [review]
The final patch: Convert nsresult to enum

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

Looks great!

::: xpcom/base/nsError.h
@@ +130,5 @@
> + */
> +
> +/*@{*/
> +
> +typedef enum nsresult {

Nit: please name this tag_nsresult as Neil suggests.

::: xpcom/base/nsErrorAsserts.cpp
@@ +3,5 @@
> +
> +// No reason to pull in Assertions.h for every single file that includes
> +// nsError.h, so let's put this in its own .cpp file instead of in the .h.
> +MOZ_STATIC_ASSERT(sizeof(nsresult) == sizeof(PRUint32),
> +                  "nsresult must be 32 bits");

Neat!
Attachment #652060 - Flags: review?(ehsan) → review+
Depends on: 782594
Depends on: 782616
Alias: nsresult-enum
Depends on: 783523
Depends on: 783863
Depends on: 783872
So now I have a Win64-specific link error that's standing in the way of pushing this:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14807023&tree=Try#error0

"""
   Creating library xul.lib and object xul.exp
LINK : warning LNK4098: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library
xptcall.obj : error LNK2001: unresolved external symbol "public: virtual enum tag_nsresult __cdecl nsXPTCStubBase::Stub3(void)" (?Stub3@nsXPTCStubBase@@UEAA?AW4tag_nsresult@@XZ)
...
xptcall.obj : error LNK2001: unresolved external symbol "public: virtual enum tag_nsresult __cdecl nsXPTCStubBase::Stub248(void)" (?Stub248@nsXPTCStubBase@@UEAA?AW4tag_nsresult@@XZ)
xptcall.obj : error LNK2001: unresolved external symbol "public: virtual enum tag_nsresult __cdecl nsXPTCStubBase::Stub249(void)" (?Stub249@nsXPTCStubBase@@UEAA?AW4tag_nsresult@@XZ)
xul.dll : fatal error LNK1120: 247 unresolved externals
"""

So maybe I need to change the lines here:

http://dxr.mozilla.org/mozilla-central/xpcom/reflect/xptcall/src/md/win32/xptcstubs_asm_x86_64.asm.html#l87

from things like 

  ?Stub3@nsXPTCStubBase@@UEAAIXZ

to

  ?Stub3@nsXPTCStubBase@@UEAA?AW4tag_nsresult@@XZ

Is this the right approach?  If so, who should I ask for review?  (Why does this break only the Win64 assembly and not all the rest of the stuff in that dir?)

Comment 120

6 years ago
I don't really know a lot about that code, but perhaps Rafael does.
I never looked at the windows version, but if the mangling matches and all tests pass, it is probably correct.

I would blame the file and see who reviewed it last.
Depends on: 788645
Created attachment 661186 [details] [diff] [review]
Fix x86_64 assembly for mangling (to be folded with final patch)

You were the last reviewer for this file, in bug 447946, so I'll ask you.  See comment 119.  This causes the build failure to be fixed, but of course this is only used on Win64, where we have no tests, so I don't know that this does anything but build.  It's just

  s/UEAAIXZ/UEAA?AW4tag_nsresult@@XZ/

on the relevant lines.
Attachment #661186 - Flags: review?(benjamin)
So, new question: does anyone have idea what could be causing these failures?

  https://tbpl.mozilla.org/?tree=Try&rev=06d307215d45

Ed Morley says it looks like a startup hang.  It's Windows-only, I believe.  Is there any way to debug?  It looks to be caused by the final nsresult-to-enum patch, without the x86_64 mangling change.

Comment 124

6 years ago
The stack is
Thread 0
 0  ntdll.dll + 0x464f4
    eip = 0x773664f4   esp = 0x003dd8c8   ebp = 0x003dd910   ebx = 0x00000000
    esi = 0x01b212e0   edi = 0x00000000   eax = 0x00000001   ecx = 0x75e689d3
    edx = 0x00000030   efl = 0x00000202
    Found by: given as instruction pointer in context
 1  xul.dll!nsBaseAppShell::DoProcessNextNativeEvent(bool,unsigned int) [nsBaseAppShell.cpp:06d307215d45 : 139 + 0x15]
    eip = 0x6c286418   esp = 0x003dd918   ebp = 0x003dd928
    Found by: previous frame's frame pointer

So basically we're waiting for something to happen, not deadlocked at all. Does the build work locally? Have you tried to dowload and run the tbox build?

Comment 125

6 years ago
Comment on attachment 661186 [details] [diff] [review]
Fix x86_64 assembly for mangling (to be folded with final patch)

Yeah whatever ;-)

Updated

6 years ago
Attachment #661186 - Flags: review?(benjamin) → review+

Comment 126

6 years ago
Aryeh, if you don't have access to a Windows box, let me know and I'll take a look (bug you need to tell me which patches I need to apply locally to build this!)
Created attachment 662506 [details] [diff] [review]
Latest patch

I don't have easy access to a Windows machine, no.  (My wife has a Windows laptop, but she takes it with her to work, so I can't use it during working hours.)  This is the current version of the patch after all folding.  You could also try grabbing the binaries from the last try run that exhibited the problem: https://tbpl.mozilla.org/?tree=Try&rev=06d307215d45

Be aware that it's possible the patch doesn't compile on m-c at any given time, because people add a new nsresult abuse (generally an actual logic error) every week or two on average.  Or other change that breaks it, like bug 787933 breaking the IPC parts of the patch just now (which I fixed just before uploading this version).

Comment 128

6 years ago
OK, let me take a stab at building this and see if I can reproduce.

Comment 129

6 years ago
So a Windows build with this patch works fine for me.  I've pushed to try again:

https://tbpl.mozilla.org/?tree=Try&rev=8067e293041e

Comment 130

6 years ago
Hmm, I can actually reproduce this when running mochitests!

Comment 131

6 years ago
Created attachment 662750 [details] [diff] [review]
Make sure that Components.results values will be unsigned

OK, I figured out the problem.  It was the http server running in the background xpcshell which was not sending any traffic to the test runner.  This boils down to the fact that the Components.results values were interpreted as signed integers, which is bad since nsresult values should be unsigned.  This patch fixes the problem for me locally.  I'll do a new try push with it to make sure we're not missing any more tests.
Attachment #662750 - Flags: review?(bobbyholley+bmo)
Attachment #662750 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 662750 [details] [diff] [review]
Make sure that Components.results values will be unsigned

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1405,5 @@
>              if (!strcmp(name.ptr(), rv_name)) {
> +                // The double cast below is required since nsresult is an enum,
> +                // and it can be interpreted as a signed integer if we directly
> +                // cast to a double.
> +                jsval val = JS_NumberValue((double)(uint32_t)rv);

Or UINT_TO_JSVAL(), I guess

Comment 134

6 years ago
Comment on attachment 662750 [details] [diff] [review]
Make sure that Components.results values will be unsigned

https://hg.mozilla.org/integration/mozilla-inbound/rev/30ab4e1d4874

Still one more orange to debug.
Attachment #662750 - Flags: checkin+

Comment 135

6 years ago
Created attachment 663216 [details] [diff] [review]
Make sure that Components.lastResult will be unsigned

I found another occurrence of a similar problem in Components.lastResult, which was causing the last test failure.  This patch fixes it.

I'll do another try run, but unless someone has checked in some broken code since yesterday, this is ready to go as soon as Bobby reviews this patch! \o/
Attachment #663216 - Flags: review?(bobbyholley+bmo)
Comment on attachment 663216 [details] [diff] [review]
Make sure that Components.lastResult will be unsigned

nit: say "two casts" instead of "double cast", since the latter can be ambiguous!

Comment 139

6 years ago
(In reply to comment #138)
> Comment on attachment 663216 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=663216
> Make sure that Components.lastResult will be unsigned
> 
> nit: say "two casts" instead of "double cast", since the latter can be
> ambiguous!

Will do when landing!

Updated

6 years ago
Depends on: 793219
Attachment #663216 - Flags: review?(bobbyholley+bmo) → review+

Comment 140

6 years ago
Try run for 2e546f32cbe2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2e546f32cbe2
Results (out of 236 total builds):
    exception: 4
    success: 214
    warnings: 14
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-2e546f32cbe2

Comment 141

6 years ago
Comment on attachment 663216 [details] [diff] [review]
Make sure that Components.lastResult will be unsigned

https://hg.mozilla.org/mozilla-central/rev/dbcc29e9fcf6
Attachment #663216 - Flags: checkin+

Comment 143

6 years ago
I'll watch the trees and will backout if something which breaks this has landed since last night!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 793387
Depends on: 793409
Depends on: 793394

Updated

6 years ago
Depends on: 793468
Whiteboard: [Leave open]
Depends on: 793580

Updated

6 years ago
Blocks: 795238

Updated

6 years ago
Blocks: 799432

Updated

6 years ago
Blocks: 801383
No longer blocks: 801383
Depends on: 801457

Updated

6 years ago
Blocks: 802820

Updated

3 years ago
Duplicate of this bug: 751538
You need to log in before you can comment on or make changes to this bug.