Closed Bug 1221856 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bc, Assigned: hsivonen)

References

()

Details

(Keywords: assertion, reproducible)

Crash Data

Attachments

(4 files)

Attached file 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.
Attached file lodenfrey.tar.bz2
still reproducible with a win 7 trunk debug build based on todays m-c tip

bz: is this something for you ?
Flags: needinfo?(bzbarsky)
No, probably for someone familiar with our mediadocument code.
Flags: needinfo?(bzbarsky)
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()
Blocks: 775467
Flags: needinfo?(hsivonen)
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.
(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...
Flags: needinfo?(hsivonen) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Henri, want to write a patch?
Flags: needinfo?(hsivonen)
Attached patch PatchSplinter Review
Seems complicated. Let's just do what the original comment suggested we could do if we trusted our readyState code to be less bogus.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)
Attachment #8743331 - Flags: review?(bugs)
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.
Attachment #8743331 - Flags: review?(bugs) → review+
Surely we can't document.open a MediaDocument?
https://hg.mozilla.org/integration/mozilla-inbound/rev/59968877b71bc3c8ffc42befbb02c49bf258c0bf
Bug 1221856 - Infer b/f cache restoration of MediaDocument from readyState. r=smaug.
(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"
https://hg.mozilla.org/mozilla-central/rev/59968877b71b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.