Closed
Bug 793580
Opened 12 years ago
Closed 12 years ago
mochitest-other and xpcshell rather thoroughly broken on win64 by nsresult-enum
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: philor, Assigned: standard8)
References
Details
Attachments
(1 file)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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 :-( ).
Comment 2•12 years ago
|
||
Can you please CC me on bug 662750?
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
(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].
Assignee | ||
Comment 6•12 years ago
|
||
Ehsan, I'm just speculating, but as you can reproduce, how about giving this a quick try?
Attachment #665123 -
Flags: feedback?(ehsan)
Comment 8•12 years ago
|
||
(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
Comment 9•12 years ago
|
||
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.
Comment 10•12 years ago
|
||
(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 11•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
(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!
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
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...
Comment 15•12 years ago
|
||
(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.
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e8915a37f5a
https://hg.mozilla.org/mozilla-central/rev/3aab17dffac7
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 17•12 years ago
|
||
(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)...
Assignee | ||
Comment 18•12 years ago
|
||
Excellent, that fixed all of Win64's xpcshell failures we were seeing on Thunderbird.
Comment 19•12 years ago
|
||
(In reply to comment #18)
> Excellent, that fixed all of Win64's xpcshell failures we were seeing on
> Thunderbird.
\o/
Comment 20•12 years ago
|
||
(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
Comment 21•12 years ago
|
||
(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!
Reporter | ||
Comment 22•12 years ago
|
||
Updated•12 years ago
|
Assignee: nobody → mbanner
You need to log in
before you can comment on or make changes to this bug.
Description
•