Closed Bug 608295 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jruderman, Assigned: dholbert)

References

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file 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.
Attached file stack trace
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).
Group: core-security
(but thanks for the caution & for filing)

Taking, since i added the abort_if_false that's failing here.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attached file 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).
ok, *now* i'm scared.
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.
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.
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.
Group: core-security
(In reply to comment #7)
> I'll file a new bug on that

Spun off Bug 608373 for "exploratory testcase".
OS: Mac OS X → All
Hardware: x86 → All
Attached patch bandaid fix v1Splinter Review
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.)
Attachment #487018 - Flags: review?(roc)
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.)
Requesting blocking -- this is an abort in debug builds that could be turned into a (null) crash in opt builds.
blocking2.0: --- → ?
Depends on: 608373
Whiteboard: [needs blocking status][needs landing]
Whiteboard: [needs blocking status][needs landing] → [needs landing]
Whiteboard: [needs landing] → [sg:critical?][needs landing]
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing]
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
Depends on: 610419
Group: core-security
You need to log in before you can comment on or make changes to this bug.