962605 - Enable baseline jit in xpcshell

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Ion has the bug 931861 problem, but baseline should be ok and helps some scripts a lot already.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Enable baseline jit in xpcshell.

Comment 2

[Bobby Holley (:bholley)]

Looping Eddy so that he knows this is happening.

Comment 3

[Bobby Holley (:bholley)]

(Note - this probably warrants a try push if you haven't done one already)

Comment 4

[Zack Weinberg (:zwol)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Used to enable a totally different jit. :(

I pushed https://tbpl.mozilla.org/?tree=Try&rev=31425d24b979

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c75f13d4f160

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=ddeab064b5bf is totally green, so:

https://hg.mozilla.org/integration/mozilla-inbound/rev/26984019ff59

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspicion of causing OSX 10.8 mochitest-other crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0df37c18e54

https://tbpl.mozilla.org/php/getParsedLog.php?id=33467743&tree=Mozilla-Inbound

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

[Benjamin Smedberg]

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?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Right, if that test runs fine apart from the todo we should just remove that part.

Comment 15

[(not currently active) Ted Mielczarek]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

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?

Comment 17

[(not currently active) Ted Mielczarek]

Ick. That test disablement is pretty broken. It's racing against the onload="runTests()" call.

Comment 18

[(not currently active) Ted Mielczarek]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove a broken attempt to disable test_hang_submit.xul.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Oh, for #3 and #4 the try runs are https://tbpl.mozilla.org/?tree=Try&rev=112b3785188b and https://tbpl.mozilla.org/?tree=Try&rev=141af1f166a3 respectively.

Comment 23

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd790518733
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c074cbfa54c6

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fdd790518733
https://hg.mozilla.org/mozilla-central/rev/c074cbfa54c6