914521 - Crash [@ nsDocShell::InternalLoad] doing sync XHR while navigating a window that is also closing

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack+

On the ~third time through the loop, it crashes due to a null mScriptGlobal.

Nightly: bp-2ed9927e-43b4-433b-bada-558192130910

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/6066b9d92032
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130522 Firefox/24.0 ID:20130522071950
Bad:
http://hg.mozilla.org/mozilla-central/rev/00b264c7cced
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130522 Firefox/24.0 ID:20130522092949
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6066b9d92032&tochange=00b264c7cced

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8d56511bfcec
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130521 Firefox/24.0 ID:20130521104616
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/75abbd7e7e24
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130521 Firefox/24.0 ID:20130521104716
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d56511bfcec&tochange=75abbd7e7e24

Triggered by Bug 841312

Comment 3

[Boris Zbarsky [:bzbarsky]]

Bobby, can you take a look?

Comment 4

[bhavana bajaj [:bajaj]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Bobby, can you take a look?

I can track it given it has reproducible test case and is a recent regression.But unless absolutely critical i don't think we can do anything for this in Fx24 as we have shipped our final beta and gone-to-build with RC.

Comment 5

[Bobby Holley (:bholley)]

So basically, docshell does a slightly more complicated version of the following [1]:

if (mScriptGlobal) {
  mScriptGlobal->DispatchSyncPopState();
  mScriptGlobal->DispatchAsyncHashChange();
}

However, the synchronous popstate event triggers a handler that opens a sync XHR, causing us to spin the event loop. The inner event loop triggers a window.close(), which is dispatched to the event loop. But since it's nested, we end up in ReallyCloseWindow() before returning from DispatchSyncPopState(). ReallCloseWindow() tears down the docshell, which calls nsDocShell::Destroy(), which nulls out mScriptGlobal. Pretty textbook event loop spinning bug.

We could save a ref to mScriptGlobal in a local variable, or null-check it. Both of those seem like wack-a-mole though. And I can't immediately think of anything all that much better. We could defer the window destruction until after the nested event loop finishes (which is what we used to do with termination functions), but that's probably observable from script, and doesn't really seem correct.

Any ideas Boris?

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9217

Comment 6

[Boris Zbarsky [:bzbarsky]]

Per spec, sync XHR should only spin very small parts of the event loop...

In the short term, I think we should just use a stack var.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Hold a stack reference to mScriptGlobal when dispatching sync events. v1

Note also MMAdeathGrip earlier in the function.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 803477 [details] [diff] [review]
Hold a stack reference to mScriptGlobal when dispatching sync events. v1

r=me

Comment 9

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8d1a234d242e

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Ed Morley [:emorley]]

Unfortunately the crashtest here caused buildbot job timeouts on Android (presumably crashes on the device), so had to be backed out. eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27753968&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27753848&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/749739c77f73

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Per spec, sync XHR should only spin very small parts of the event loop...
Note, showModalDialog tends to work as a spec-compliant spin-event-loop-at-odd-times.

Comment 14

[Bobby Holley (:bholley)]

B2G and Android presumably don't like the window.open in the crashtest, which makes sense. Disabling the test for those platforms and repushing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/742d5330f0c3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/742d5330f0c3

Comment 16

[Nick Cameron [:nrc]]

So, it seems that the test here makes Cipc timeout (https://tbpl.mozilla.org/?tree=Try&rev=7e1feb223053). This wasn't noticed because Cipc tests have not been running correctly for the past four weeks or so (bug 917576). Would someone be able to look into this please?

Comment 17

[Bobby Holley (:bholley)]

It's probably just an issue with opening windows in that configuration. Just disable the test (like we do for Android and B2G) for Cipc, however you do that.

Comment 18

[Alex Keybl [:akeybl]]

Needs an uplift request for FF25

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 803477 [details] [diff] [review]
Hold a stack reference to mScriptGlobal when dispatching sync events. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 841312
User impact if declined: crashes
Testing completed (on m-c, etc.): baked on m-c and m-a
Risk to taking this patch (and alternatives if risky): extremely low risk 
String or IDL/UUID changes made by this patch: None

Comment 20

[Bobby Holley (:bholley)]

Note that there were 2 followups to disable the crashtest in various configurations (b2g, android, and Cipc). Keep that in mind when uplifting.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

The Android/B2G disabling came along for the ride when this was re-landed. I folded in the Cipc disabling from bug 917576, though.

https://hg.mozilla.org/releases/mozilla-beta/rev/5d92f187ec81