1391211 - Assertion failure: aScriptGlobalObject || !mAnimationController || mAnimationController->IsPausedByType( nsSMILTimeContainer::PAUSE_PAGEHIDE | nsSMILTimeContainer::PAUSE_BEGIN) (Clearing window pointer while animations are unpaused),

Status: NEW

(Core :: Layout, defect, P3)

Description

[Carsten Book [:Tomcat]]

Assertion failure: aScriptGlobalObject || !mAnimationController || mAnimationController->IsPausedByType( nsSMILTimeContainer::PAUSE_PAGEHIDE | nsSMILTimeContainer::PAUSE_BEGIN) (Clearing window pointer while animations are unpaused), at z:/build/build/src/dom/base/nsDocument.cpp:4816

found by bughunter and reproduced on latest nightly from taskcluster build from https://hg.mozilla.org/mozilla-central/rev/932388b8c22c9775264e543697ce918415db9e23

Steps to reproduce:
-> Load http://forum.kerbalspaceprogram.com/index.php?/topic/164055-how-to-landing-leg-a-base-autostrut-explosion-conundrum/#comment-3142152
--> Assertion failure

Comment 1

[Bob Clary [:bc] (inactive)]

Also http://ventas.globussistemas.net/Form/FrmPuntoVenta.aspx?TipMovi=V#no-back-button on Beta 56 and Nightly 57

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Attached reduced testcase.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I can't reproduce the assertions on the live site from #c0 (even with builds from around the time it was filed), but I can with the attached testcase.

For the attached testcase:
INFO: Last good revision: 9c8803d97c7dc4d536d7f218f3052f80de94ccb2
INFO: First bad revision: 956306ea34f36514350ce6f03c114eedbabe934f
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9c8803d97c7dc4d536d7f218f3052f80de94ccb2&tochange=956306ea34f36514350ce6f03c114eedbabe934f

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> I can't reproduce the assertions on the live site from #c0 (even with builds
> from around the time it was filed), but I can with the attached testcase.
> 
> For the attached testcase:
> INFO: Last good revision: 9c8803d97c7dc4d536d7f218f3052f80de94ccb2
> INFO: First bad revision: 956306ea34f36514350ce6f03c114eedbabe934f
> INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=9c8803d97c7dc4d536d7f218f3052f80de94ccb2&tochange=9563
> 06ea34f36514350ce6f03c114eedbabe934f

This problem is not a particular problem with window.open, but rather a problem with nested event loops in general.

The summary of what's going on here is as follows.

            o1 = document.createElement('frame')
            document.documentElement.appendChild(o1)
            o1.contentDocument.addEventListener('visibilitychange', function () { window.open('', '', '4') }, false)

// At this point we have an iframe in the document with the about:blank document displayed,
// and a listener for the visibilitychange event registered on that document.

            o1.src = 'data:text/html,<svg>'

// Setting the `src` attribute doesn't perform the load synchronously, but rather queues up
// some work which should perform the load later.

            document.documentElement.appendChild(o1)

// re-appending the document to the body causes the nsFrameLoader to be torn down, 
// as before we can insert it at its new location, we first have to unmount it from 
// its original location. What happens is effectively the following:
 * We notify the current document that it is being hidden, as it's about to be unloaded.
 * The document detects that its being hidden, and tries to clean up its animation controller.
 * The document synchronously dispatches the visibilitychange event to content, this allows JS to run. 
 * JS runs window.open, which causes a nested event loop to spin (since bug 1343728). 
   This would happen with alert() or sync XHR before then. We could also probably simulate 
   this with any action which synchronously loads a new document in the iframe.
 * While the nested event loop is spinning, the asynchronous event to load the svg document 
   into the iframe is fired, which sets up a new document inside of the iframe with an animation controller.
 * The document finishes clearing its visibility, and proceeds to try to tear down the docshell.
 * This assertion fails, because the current document in the docshell wasn't told that it has been hidden.

My immediate thought is to, in nsDocumentViewer::PageHide, loop calling mDocument->OnPageHide on its document until its document stops changing. This could theoretically allow for infinite loops however, as we constantly replace the document in the visibilitychange event.

ni? olli to see if he has a better idea.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

We could also just move disabling the animation controller to happen after we notify content, which would fix this particular crash.

Comment 6

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

(In reply to Nika Layzell [:mystor] from comment #4)
> My immediate thought is to, in nsDocumentViewer::PageHide, loop calling
> mDocument->OnPageHide on its document until its document stops changing.

I don't understand what this means.
Don't we change nsDocumentViewer here all the time, not just the document of it.?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I'll do my best to reconstruct the path of logic which got me to writing that post:

1. When calling AppendElement to move the iframe around, we first destroy the old iframe's docshell, which thus calls nsDocShell::Destroy (http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/docshell/base/nsDocShell.cpp#5892)

2. The crash happens here when we call mContentViewer->Destroy() in this function, as mContentViewer->Destroy calls nsDocument->Destroy which calls nsDocument->SetScriptGlobalObject, which asserts that its animation controller has been cleaned up. (http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/docshell/base/nsDocShell.cpp#5967 .. http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/base/nsDocument.cpp#4991-4995)

3. This value is supposed to be cleaned up by the call to FirePageHideNotification(true) in nsDocShell::Destroy, as the animation context is cleaned up by hiding the document (http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/docshell/base/nsDocShell.cpp#5923)

4. The specific problem here is that mAnimationController is cleaned up well before we update the visibility state, which fires an event to synchronously run javascript, so javscript gets the chance to replace the document inside the nsDocShell before the docshell goes away with a new document which has an animation controller (the svg documeent), and the assertion fires: (http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/base/nsDocument.cpp#9503-9505,9541)

I'm not sure my original idea was very good but from what I can tell:
a) We probably can't guarantee a document has received a PageHide before its docshell is cleared, as in pagehide we can replace the document and the new one won't get pagehide, so.
b) We should probably change the assertion and modify the code around it to make sure we notify the animation controller and clean it up ourselves before we need it to be gone.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add a test for the failing assertion

I tried the simplest fix I could think of, and it didn't work (just moving when we call mAnimationController->OnPageHide()). I did make a crashtest for this bug, so I figured I'd just post the patch so whoever actually fixes it (perhaps me if I figure out what the best solution is) doesn't have to do that :-).

MozReview-Commit-ID: 1p67dhny7Vu

Comment 9

[Intermittent Failures Robot]

1 failures in 3186 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-esr91: 1

Platform and build breakdown:

• windows10-32-2004-qr: 1
  • debug: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1391211&startday=2021-10-18&endday=2021-10-24&tree=all