1221856 - Assertion failure: GetReadyStateEnum() == nsIDocument::READYSTATE_LOADING (Bad readyState), at dom/html/MediaDocument.cpp:205

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Linux crash report

1. https://www.lodenfrey.com/Trachten/Damen/Trachtenjacken/Meindl-Amisiella-Leinen-Trachten-Jacke.html?utm_source=criteo&utm_medium=display&utm_campaign=criteo&campaign=criteo

2. Assertion failure: GetReadyStateEnum() == nsIDocument::READYSTATE_LOADING (Bad readyState), at /builds/slave/m-cen-l64-d-000000000000000000/build/src/dom/html/MediaDocument.cpp:205

Linux, OSX, Windows 7 Debug Beta/43, Aurora/44, Nightly/45

I can reproduced using a saved version of the page on OSX 10.9 with a Nightly. YMMV.

This is a long standing problem. If you wait too long you may lose the ability to reproduce it.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: lodenfrey.tar.bz2

Comment 2

[Carsten Book [:Tomcat]]

Attachment: windbg information/stack

still reproducible with a win 7 trunk debug build based on todays m-c tip

bz: is this something for you ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

No, probably for someone familiar with our mediadocument code.

Comment 4

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

data:text/html,<iframe src='data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==' onload='setTimeout(function(){ event.target.contentDocument.open();}, 0)'>

is a minimal testcase for this. Looks like the code added in bug 775467 doesn't deal with document.open()

Comment 5

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

data:text/html,<iframe src='data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==' onload='event.target.contentDocument.open();'>

seems to work too, and giving closer to the original stack. But same issue as with previous data-url.

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #4)
> data:text/html,<iframe
> src='data:image/gif;base64,
> R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=='
> onload='setTimeout(function(){ event.target.contentDocument.open();}, 0)'>
> 
> is a minimal testcase for this. Looks like the code added in bug 775467
> doesn't deal with document.open()

Indeed. Looking at the patch for that bug and thinking what I must have been thinking, it seems to me that SetScriptGlobalObject() was a suitable opportunity in the call sequence of methods of the synthetic document to trigger the transition from loading to interactive. There doesn't seem to be any inherent reason for that transition to be coupled with the global object getting set per se.

And, indeed, it seems that I failed to take into account document.open().

smaug, do you have ideas for a non-terrible way of detecting whether we have the document.open() case from within SetScriptGlobalObject()? As the comments say, if our readyState code wasn't so bogus to begin with, we could look at readyState itself there instead of asserting on it...

Comment 7

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

I'm not too familiar with readyState being bogus, usually assertions like in this bug have shown up to be bugs in other code.
So I wonder if it was ok to just check readyState here?

But another option is to check in nsGlobalWindow::SetNewDocument whether a new inner window is actually created (and not just reusing some old one, or reusing inner window from bfcached thing), and add a flag to SetScriptGlobalObject. Hmm, though, if a document has ever had a scriptglobal, 
mScopeObject should be a WeakPtr to that global. So I guess you could 
do_QueryReferent mScopeObject and compare to the aScriptGlobalObject in ::SetScriptGlobalObject.
If it is the same, we're coming out from bfcache or something. If do_QueryReferent'ed mScopeObject is null and aScriptGlobalObject is not, we're setting the initial global, and if
do_QueryReferent'ed mScopeObject isn't the same as aScriptGlobalObject but both are non-null, 
we should be dealing with document.open case.

Comment 8

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

Henri, want to write a patch?

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Patch

Seems complicated. Let's just do what the original comment suggested we could do if we trusted our readyState code to be less bogus.

Comment 10

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=371ce7f43798

Comment 11

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be5a90b60083

Comment 12

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

Comment on attachment 8743331 [details] [diff] [review]
Patch

Took a bit time to remember this stuff, and figure out that the comment isn't just about bfcache, but that with document.open we enter this code with
nsIDocument::READYSTATE_COMPLETE and then in nsHTMLDocument::Open explicitly call SetReadyStateInternal(nsIDocument::READYSTATE_LOADING);

Could you mention document.open/write case in the comment too.

Comment 13

[Henri Sivonen (:hsivonen)]

Surely we can't document.open a MediaDocument?

Comment 14

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59968877b71bc3c8ffc42befbb02c49bf258c0bf
Bug 1221856 - Infer b/f cache restoration of MediaDocument from readyState. r=smaug.

Comment 15

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

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> Surely we can't document.open a MediaDocument?

Of course we can.
That is what my testcase is doing, see comment 5.
And, "class MediaDocument : public nsHTMLDocument"

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/59968877b71b

Comment 17

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3093f48fd366