608295 - "ABORT: Clearing window pointer while animations are unpaused" with document.write

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase.
2. Quit Firefox or navigate to another page.

Result:
###!!! ABORT: Clearing window pointer while animations are unpaused: 'aScriptGlobalObject || !mAnimationController || mAnimationController->IsPausedByType( nsSMILTimeContainer::PAUSE_PAGEHIDE | nsSMILTimeContainer::PAUSE_BEGIN)', file /builds/slave/mozilla-central-macosx-debug/build/content/base/src/nsDocument.cpp, line 3740

Security-sensitive for now because the message sounds scary.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Holbert [:dholbert]]

This isn't actually security sensitive -- this failure means that we could have a SMIL sample that crashes on a null-pointer-deref when trying to query style data (if the right kinds of animations are running).

Comment 3

[Daniel Holbert [:dholbert]]

(but thanks for the caution & for filing)

Taking, since i added the abort_if_false that's failing here.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: exploratory testcase

So this testcase shows a little of what's going on here.  It's got two iframes, both of which get their contentDocument written at page-load-time.

The first iframe "fa" gets these contents:
> before script<script src="...do some stuff..."></script>AFTER SCRIPT

The second iframe "fb" gets these contents:
> before script<script></script>AFTER SCRIPT

The odd thing is -- in the first iframe, "AFTER SCRIPT" never visibly makes it into the document for some reason.  (The script runs, though -- verifiable because it adds text at the end of the page).

This behavior into play in the original testcase, but with <svg> in place of "AFTER SCRIPT".  The <svg> node doesn't get a BindToTree call when the document is loaded.

HOWEVER -- it DOES get a BindToTree call when the document is **torn down**, inside of a call to nsHtml5TreeOpExecutor::FlushDocumentWrite (which is in turn a few levels down from nsDocShell::Destroy's "Stop" call).  That BindToTree call creates & unpauses our animation controller.  Once we pop back up to nsDocShell::Destroy, we end up hitting this bug's ABORT_IF_FALSE from another function call there (mContentViewer->Close()).

So, to sum up -- the basic problem here is that we're parsing & creating content **within** nsDocShell::Destroy, **after** we've fired our "page hide" notification (which normally would pause the SMIL animation controller and prevent this ABORT_IF_FALSE from tripping).

Comment 5

[Jesse Ruderman]

ok, *now* i'm scared.

Comment 6

[Daniel Holbert [:dholbert]]

In particular, we're lazily creating our SMIL Animation Controller at the first time we get a <svg> node bound, and in this testcase, that happens after we've run nsDocument::OnPageHide.

Normally we pause our SMIL animation controller in nsDocument::OnPageHide, but we can't if it's not created yet.

The simplest fix here is probably this: in nsDocument::GetAnimationController, where the controller is lazily created, we can just check whether the document's mHidden flag is set (i.e. check if OnPageHide has been run) -- and if so, we should pause the just-created animation controller in the same way we do in OnPageHide.

Comment 7

[Daniel Holbert [:dholbert]]

Note that in Firefox 3.6, "exploratory testcase" renders just fine. (it shows AFTER SCRIPT in both iframes)  So does Opera.  So "exploratory testcase" reveals the real underlying bug here -- we're apparently postponing some HTML parsing for longer than we should be (and not flushing it until docshell-destroy-time)

I'll file a new bug on that, and I'll post the band-aid patch suggested in Comment 6 for the ABORT_IF_FALSE failure here in the meantime.

Comment 8

[Daniel Holbert [:dholbert]]

Re-hiding just to be safe, since creating content within nsDocShell::Destroy seems a little scary, and I don't know what the security implications of it are.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> I'll file a new bug on that

Spun off Bug 608373 for "exploratory testcase".

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: bandaid fix v1

Here's a band-aid fix as described at the end of Comment 6.  (I meant "mIsShowing" instead of "mHidden" in that comment -- it's correct in the patch.)

Note that fixing Bug 608373 would be sufficient to fix this bug; however, that bug scares me, and I don't know exactly what needs fixing there.

I propose that we get this 3-line band-aid & crashtest landed, for now, and then when Bug 608373 is fixed, we can revert the band-aid but leave the crashtest in.  (If Bug 608373 ends up being fixed before tree rules allow this band-aid to land, then all the better.)

Comment 11

[Daniel Holbert [:dholbert]]

One clarification on the fix -- you can't quite tell from the context, but the new code is for the case where we don't have an animation controller yet, and so we're lazily creating one.  (Otherwise -- if we already have a controller -- we return it at the very beginning of that function.)

Comment 12

[Daniel Holbert [:dholbert]]

Requesting blocking -- this is an abort in debug builds that could be turned into a (null) crash in opt builds.

Comment 13

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/04311a70fa33

Removed [sg:critical?] notation for this particular abort & its fix here, per comment 1.

Bug 608373, which spun off of this bug, may or may not be [sg:critical], per comment 8 / 9.