Closed Bug 793580 Opened 7 years ago Closed 7 years ago

mochitest-other and xpcshell rather thoroughly broken on win64 by nsresult-enum

Categories

(Core :: XPCOM, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: philor, Assigned: standard8)

References

Details

Attachments

(1 file)

You've never seen it before, because we only run it on mozilla-central, and run it hidden there, but after a bunch of retriggers for skipped runs and runs ruined by our inability to rm -rf on win64, we were green in https://tbpl.mozilla.org/?noignore=1&rev=3c68fdd4f77a&jobname=WINNT%206.1%20x64, and by https://tbpl.mozilla.org/?noignore=1&rev=d1b23fd87ba2&jobname=WINNT%206.1%20x64 we had 50 failures in mochitest-chrome, 24 failures in mochitest-browser-chrome, 3 failures in mochitest-a11y, and 9 failures in xpcshell, which seem to be permafails.

See https://tbpl.mozilla.org/php/getParsedLog.php?id=15421759&tree=Firefox for mochitest-other, and https://tbpl.mozilla.org/php/getParsedLog.php?id=15422100&tree=Firefox for xpcshell.
We've seen this on the Thunderbird builders as well. More specifically, is that the error code doesn't seem to be translated very well.

For example in our test_smtpProtocols.js we're calling a function in a js component (i.e. via xpcom) that does "throw Components.results.NS_ERROR_NOT_IMPLEMENTED;". When we get that back and catch it, the error value in the catch is '2147483648' aka 0x80000000 aka -1. In that same try/catch place, Components.results.NS_ERROR_NOT_IMPLEMENTED is correctly recognised as 2147500036, aka 0x80004004.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15536370&tree=Thunderbird-Trunk#error1

TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/mailnews/compose/test/unit/test_smtpProtocols.js | 2147483648 == 2147500033

This failure, from the same log is another good example but entirely in m-c land:

TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_mozIAsyncLivemarks.js | 2147483648 == 2147942487

From a quick look at the rest of the failures, I think that this is the route cause of all the xpcshell failures. 


My wild guess here is that http://hg.mozilla.org/mozilla-central/annotate/ca4af4af5334/js/xpconnect/src/XPCComponents.cpp#l1882 seems to be handling exception results as 32 bit signed ints. Changing this would be in similar vain to the change on bug 662750, but I'm not sure why this would affect windows-only 64 bit, nor do I have facilities to test it (especially as try server doesn't run 64 bit tests :-( ).
Can you please CC me on bug 662750?
FWIW, I cannot reproduce any of these failures in my local debug x64 build.  I'm currently doing an optimized build to see if that helps.
I can reproduce it in my opt build.  I looked around the code that I modified for the Components.results fix in bug 777292 but did not see an obvious breakage, and comment 1 isn't really obvious to understand to me.

Makoto, are you still interested in fixing up Win64?  I may not have enough time to pursue this further for now.
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> Can you please CC me on bug 662750?

Urgh, sorry I meant attachment 662750 [details] [diff] [review].
Attached patch Speculative fixSplinter Review
Ehsan, I'm just speculating, but as you can reproduce, how about giving this a quick try?
Attachment #665123 - Flags: feedback?(ehsan)
Duplicate of this bug: 794734
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> I can reproduce it in my opt build.  I looked around the code that I
> modified for the Components.results fix in bug 777292 but did not see an
> obvious breakage, and comment 1 isn't really obvious to understand to me.
> 
> Makoto, are you still interested in fixing up Win64?  I may not have enough
> time to pursue this further for now.

Microsoft compiler's behavior for enum-type is different of gcc.  So we sometimes need underlying type to enum on MSVC.

There is 2 workarounds.

- use Mark's code
- Add underlying type "enum nsresult : uint32_t {" in nsError.h
Mark, could you merge https://hg.mozilla.org/try/rev/81cc1d5922a6 to your fix?

xpconnect uses nsresult as uint32_t, so we should add underlying type to enum as prevention, too.
(In reply to Makoto Kato from comment #8)
> Microsoft compiler's behavior for enum-type is different of gcc.  So we
> sometimes need underlying type to enum on MSVC.
> 
> There is 2 workarounds.
> 
> - use Mark's code
> - Add underlying type "enum nsresult : uint32_t {" in nsError.h

The latter will happen anyway in bug nsresult-enum-class (bug 779473), if no one else does it before then.  So a temporary hack like the one comment 9 links to isn't as terrible as it could be.
Comment on attachment 665123 [details] [diff] [review]
Speculative fix

This code is never examined in the failing tests that I'm looking at at least, but it makes sense regardless.
Attachment #665123 - Flags: feedback?(ehsan) → review+
(In reply to Makoto Kato from comment #9)
> Mark, could you merge https://hg.mozilla.org/try/rev/81cc1d5922a6 to your
> fix?

r=me on this patch too!
Bah, if we'd only used : uint32_t on all MSVC builds (both 32 and 64 bit) then we wouldn't have had this bug or bug 777292 etc...
(In reply to comment #14)
> Bah, if we'd only used : uint32_t on all MSVC builds (both 32 and 64 bit) then
> we wouldn't have had this bug or bug 777292 etc...

Bug 777292 is the thing which converted nsresult into an enum! ;-)

FWIW I can probably be convinced to do this for all MSVC builds.  Please file another bug and CC me on it.
https://hg.mozilla.org/mozilla-central/rev/8e8915a37f5a
https://hg.mozilla.org/mozilla-central/rev/3aab17dffac7
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Ehsan Akhgari from comment #15)
> Bug 777292 is the thing which converted nsresult into an enum! ;-)
Sorry, I was thinking of the Components.results fix, but overlooked the fact that it hadn't been filed as a separate bug (slaps wrist)...
Excellent, that fixed all of Win64's xpcshell failures we were seeing on Thunderbird.
(In reply to comment #18)
> Excellent, that fixed all of Win64's xpcshell failures we were seeing on
> Thunderbird.

\o/
(In reply to Ryan VanderMeulen from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/3aab17dffac7

This added a C++ style comment to nsError.h, which makes us spam this build warning for .c files:
../../dist/include/nsError.h:120:3: warning: C++ style comments are not allowed in ISO C90 [enabled by default]

I pushed a followup to convert that comment to C-style:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0394fcdfec
(In reply to comment #20)
> (In reply to Ryan VanderMeulen from comment #16)
> > https://hg.mozilla.org/mozilla-central/rev/3aab17dffac7
> 
> This added a C++ style comment to nsError.h, which makes us spam this build
> warning for .c files:
> ../../dist/include/nsError.h:120:3: warning: C++ style comments are not allowed
> in ISO C90 [enabled by default]
> 
> I pushed a followup to convert that comment to C-style:
>  https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0394fcdfec

Thanks for doing that!
Assignee: nobody → mbanner
Depends on: 910518
No longer depends on: 910518
You need to log in before you can comment on or make changes to this bug.