Enable baseline jit in xpcshell

RESOLVED FIXED in mozilla29

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla29
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Ion has the bug 931861 problem, but baseline should be ok and helps some scripts a lot already.
Created attachment 8363684 [details] [diff] [review]
Enable baseline jit in xpcshell.
Attachment #8363684 - Flags: review?(bobbyholley+bmo)
Whiteboard: [need review]
Attachment #8363684 - Flags: review?(bobbyholley+bmo) → review+
Looping Eddy so that he knows this is happening.
(Note - this probably warrants a try push if you haven't done one already)
FYI the xpcshell tests have, for quite some time, been passing `-m -n` to the xpcshell invocation, which *used to* enable the JIT, but hasn't since bug 857845.  So there has been at least some testing in this mode, historically. CC gps.
Used to enable a totally different jit. :(

I pushed https://tbpl.mozilla.org/?tree=Try&rev=31425d24b979
https://hg.mozilla.org/integration/mozilla-inbound/rev/c75f13d4f160
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla29
Backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/15a9c1e49bab (along with the patches from bug 958576) because one of them seems to have introduced a new permanent failure only on OSX 10.8 m-oth: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=33415277&tree=Mozilla-Inbound
Yeah, this time the try runs confirm it...

I have no idea why this change causes this orange only on debug 10.8, or even _how_ it manages to cause it.  The failure is during dom/tests/mochitest/chrome/test_activation.xul and it looks like this:

  Assertion failure: aPrincipal, at ../../../content/base/src/nsXMLHttpRequest.h:237

with a stack like so:

00:14:20     INFO -   0  XUL!nsXMLHttpRequest::Construct(nsIPrincipal*, nsIGlobalObject*, nsIURI*) [nsXMLHttpRequest.h:143dbce829fe : 237 + 0x0]
00:14:20     INFO -   1  XUL!nsXMLHttpRequest::Constructor(mozilla::dom::GlobalObject 00:14:20     INFO -   2  XUL!mozilla::dom::XMLHttpRequestBinding::_constructor [XMLHttpRequestBinding.cpp:143dbce829fe : 1518 + 0x4]

So basically script did |new XMLHttpRequest|, we QIed the global to nsIScriptObjectPrincipal, but then calling GetPrincipal() on that returned null.

It doesn't happen every time, but pretty close.

All of the above is happening in browser, not in xpcshell.  It's also happening under the interpreter...  I'll attach a full stack, for posterity.

So here's the interesting thing.  In a passing test run with this patch, I get this output in the log:

19:45:59     INFO -  5844 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | Should be instance of HTMLLabelElement
19:45:59     INFO -  5847 INFO TEST-END | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | finished in 258ms
19:45:59     INFO -  5848 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | report should have been submitted successfully
19:45:59     INFO -  5849 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | Checking correct topic
19:45:59     INFO -  5850 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | Subject should be a property bag
19:45:59     INFO -  5851 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | Should have a server crash ID
19:45:59     INFO -  5852 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOM_element_instanceof.xul | Server crash ID should not be an empty string
19:45:59     INFO -  [1176] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../content/base/src/nsXMLHttpRequest.cpp, line 1601
19:45:59     INFO -  System JS : ERROR chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul:64 - NS_ERROR_FAILURE:
19:45:59     INFO -  5853 INFO TEST-START | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul

and then it's all fine.  In a failing run, I get:

00:14:06     INFO -  5848 INFO TEST-START | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul
00:14:07     INFO -  5849 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | report should have been submitted successfully
00:14:07     INFO -  5850 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | Checking correct topic
00:14:07     INFO -  5851 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | Subject should be a property bag
00:14:07     INFO -  5852 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | Should have a server crash ID
00:14:07     INFO -  5853 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_activation.xul | Server crash ID should not be an empty string
00:14:07     INFO -  [1185] WARNING: No outer window available!: file ../../../dom/base/nsGlobalWindow.cpp, line 11078

and then the crash.

We only have two tests that have those "crash ID" strings in them: test_crash_submit.xul and test_hang_submit.xul.  And if you look at the "passing" run there you see that it's test_hang_submit that's crapping over later tests, and in particular still running once its window is already gone.  All that matters is _how_ gone it is.  If it's still there enough to create the XHR object, we bail when we try to do GetContextForEventHandlers().  But if it's really gone we seem to hit that fatal assert.

So looking at test_hang_submit, in a log without this patch at all:

02:41:52     INFO -  5722 INFO TEST-START | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul
02:41:53     INFO -  5723 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul | Can't test plugin crash notification on OS X 10.7 or 10.8, see bug 705047
02:41:54     INFO -  5726 INFO TEST-END | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul | finished in 2143ms
02:41:54     INFO -  5727 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul | Plugin crashed notification received
02:41:54     INFO -  5728 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_hang_submit.xul | event is correct type

but then no spurious "report should have been submitted successfully" messages on later tests...  In a passing log with this patch, I see the same messages (including the test-pass bits after test-end), but then the test keeps running, apparently.
Target Milestone: mozilla29 → ---
So the test_hang_submit test does this:

27 if (isOSXLion || isOSXMtnLion) {
28   todo(false, "Can't test plugin crash notification on OS X 10.7 or 10.8, see bug 705047");
29   SimpleTest.finish();
30 }

but then keeps running the test.  Is there a reason for that?
Flags: needinfo?(ted)
Flags: needinfo?(georg.fritzsche)
Also, my best guess at this point is that xpcshell baseline compiler just changes timing for the SJS here somehow or something, such that we get a response when we're not actually expecting one.  But maybe it also makes toolkit/crashreporter/test/browser/crashreport.sjs run incorrectly....  Hard to tell so far.

Comment 13

4 years ago
That's just a bug, there should be a return() there. Although given my reading of bug 705047, those tests should be re-enabled now that the crashreporter is working again. Maybe that entire hunk should go?
Right, if that test runs fine apart from the todo we should just remove that part.
Flags: needinfo?(georg.fritzsche)
Yeah, let's try just enabling the test and see if that makes this go away. Sounds like there might be a real issue with lifetimes of the XHR etc, but maybe that's not critical to fix?
Flags: needinfo?(ted)
I did some try pushes to that effect, but it still worries me that enabling the JIT on xpcshell changed behavior here...  Why were we not getting a response at all before but are with the JIT on?
Ick. That test disablement is pretty broken. It's racing against the onload="runTests()" call.
bz: I wonder if the brokenness has to do with the fact that the test is doing a sync XHR?
http://hg.mozilla.org/mozilla-central/annotate/bfe4ed6d47ce/dom/plugins/test/mochitest/test_hang_submit.xul#l65
Doesn't seem like that should matter.  Again, what we're changing here is the xpcshell behavior.

So what I see is that without the xpcshell patch we never get past the early return in the "observe: function(subject, topic, data)" function in the test, but with the patch we do.  Assuming that function is even called without the patch...  I'm going to do a try push to see if it is.
OK, a bit more data:

1)  Just removing the broken disabling of this test seems to be green: https://tbpl.mozilla.org/?tree=Try&rev=00a71e2d9263

2)  Doing this patch on top of the removal seems to be green: https://tbpl.mozilla.org/?tree=Try&rev=e66fce02545f

3)  Without this patch, on the test with the broken disabling, the observer gets topic 'crash-report-status' and data 'submitting', then nothing.

4)  With this patch, on the test with the broken disabling, the observer gets that, and then topic 'crash-report-status' and data 'success', after which it runs those extra steps.

I have no idea why JIT for xpcshell affects what the observer gets, but I'm tempted to just remove the broken disabling and move on.
Created attachment 8365421 [details] [diff] [review]
part 1.  Remove a broken attempt to disable test_hang_submit.xul.
Attachment #8365421 - Flags: review?(ted)
Attachment #8365421 - Flags: review?(ted) → review+
Target Milestone: --- → mozilla29

Updated

4 years ago
Blocks: 933885
You need to log in before you can comment on or make changes to this bug.