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.

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

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

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

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

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?

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.

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

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

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

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.

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.

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.

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

Trivial.

I vote for this as the craziest code this conversion turned up.

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.

Avoids compiler warnings.

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!

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.

Bug 778111 filed.

Filed bug 778113.

These Read()/Write() methods return the number of bytes read/written, not an nsresult. It doesn't matter; the result was unused anyway.

The only caller already has a null check anyway, so it should be safe.

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 . . . :) )

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

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.

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.

Comment on attachment 646529 [details] [diff] [review] Fix incorrect conversions to nsresult This would also be a good patch to get extra eyes on.

Comment on attachment 646518 [details] [diff] [review] Convert incorrect conversions to nsresult and fix named constants And this.

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?

You should probably use c++casts. Makes a lot easier to find them afterwards.

See comment 13 and comment 30 for explanation.

See comment 13 and comment 30 for explanation.

See comment 13 and comment 30 for explanation.

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.

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;.

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

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

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.

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.

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

Jason: please see comment 33.

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

See comment 13 and comment 21 for context.

See comment 13 and comment 21 for context.

See comment 13 and comment 21 for context.

See comment 13 and comment 21 for context.

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

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

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

> 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. ;)

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

Comment on attachment 646574 [details] [diff] [review] netwerk/: Don't treat number of bytes as an nsresult r=me

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

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

Landed some of the patches: https://hg.mozilla.org/integration/mozilla-inbound/rev/75e5b7133dfd https://hg.mozilla.org/integration/mozilla-inbound/rev/46b643235024 https://hg.mozilla.org/integration/mozilla-inbound/rev/fb231ccb1823 https://hg.mozilla.org/integration/mozilla-inbound/rev/90575bfde461 https://hg.mozilla.org/integration/mozilla-inbound/rev/b8da0e7c3dbb https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b24dee348d https://hg.mozilla.org/integration/mozilla-inbound/rev/a6842357e0bc https://hg.mozilla.org/integration/mozilla-inbound/rev/c49d2bcc6665 https://hg.mozilla.org/integration/mozilla-inbound/rev/f34b364cffcd https://hg.mozilla.org/integration/mozilla-inbound/rev/6c825468d65f https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5e0e7a0fde https://hg.mozilla.org/integration/mozilla-inbound/rev/f67184ee0f8b https://hg.mozilla.org/integration/mozilla-inbound/rev/ae22e27106b3 Green try for these patches: https://tbpl.mozilla.org/?tree=Try&rev=c783b7201c94

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.

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.

I missed a couple of uses on Android, so try failed. With this, try succeeds: https://tbpl.mozilla.org/?tree=Try&rev=fac5182a8d75

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e30103582c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/d64f195e9c43 https://hg.mozilla.org/integration/mozilla-inbound/rev/20549037ff3d https://hg.mozilla.org/integration/mozilla-inbound/rev/bc901f8f4732

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.

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

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

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

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

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

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

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

https://hg.mozilla.org/mozilla-central/rev/75e5b7133dfd https://hg.mozilla.org/mozilla-central/rev/46b643235024 https://hg.mozilla.org/mozilla-central/rev/fb231ccb1823 https://hg.mozilla.org/mozilla-central/rev/90575bfde461 https://hg.mozilla.org/mozilla-central/rev/b8da0e7c3dbb https://hg.mozilla.org/mozilla-central/rev/b0b24dee348d https://hg.mozilla.org/mozilla-central/rev/a6842357e0bc https://hg.mozilla.org/mozilla-central/rev/c49d2bcc6665 https://hg.mozilla.org/mozilla-central/rev/f34b364cffcd https://hg.mozilla.org/mozilla-central/rev/6c825468d65f https://hg.mozilla.org/mozilla-central/rev/5d5e0e7a0fde https://hg.mozilla.org/mozilla-central/rev/f67184ee0f8b https://hg.mozilla.org/mozilla-central/rev/ae22e27106b3 https://hg.mozilla.org/mozilla-central/rev/6e30103582c4 https://hg.mozilla.org/mozilla-central/rev/d64f195e9c43 https://hg.mozilla.org/mozilla-central/rev/20549037ff3d https://hg.mozilla.org/mozilla-central/rev/bc901f8f4732

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

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

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

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+.

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

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

Comment on attachment 646575 [details] [diff] [review] security/manager/: Don't treat number of bytes as an nsresult r=kaie

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb11c697f13 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e534989a794

https://hg.mozilla.org/mozilla-central/rev/3eb11c697f13 https://hg.mozilla.org/mozilla-central/rev/6e534989a794

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.

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

This changes to NS_ERROR_FAILURE instead of NS_OK, so it does change behavior. Try: https://tbpl.mozilla.org/?tree=Try&rev=f68c61841e44

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.

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.

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.

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

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

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.

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.

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.

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 on attachment 661186 [details] [diff] [review] Fix x86_64 assembly for mangling (to be folded with final patch) Yeah whatever ;-)

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

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

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

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

Hmm, I can actually reproduce this when running mochitests!

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.

Try push with the patch in comment 131: https://tbpl.mozilla.org/?tree=Try&rev=7bacf5b40b65

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

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/

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!

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

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