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

RESOLVED FIXED in mozilla2.0b8

Status

()

--
critical
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla2.0b8
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
Created attachment 486934 [details]
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.
(Reporter)

Comment 1

8 years ago
Created attachment 486935 [details]
stack trace
(Assignee)

Comment 2

8 years ago
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
(Assignee)

Comment 3

8 years ago
(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
(Assignee)

Comment 4

8 years ago
Created attachment 486997 [details]
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).
(Reporter)

Comment 5

8 years ago
ok, *now* i'm scared.
(Assignee)

Comment 6

8 years ago
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.
(Assignee)

Comment 7

8 years ago
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.
(Assignee)

Comment 8

8 years ago
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
(Assignee)

Comment 9

8 years ago
(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
(Assignee)

Comment 10

8 years ago
Created attachment 487018 [details] [diff] [review]
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.)
Attachment #487018 - Flags: review?(roc)
(Assignee)

Comment 11

8 years ago
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.)
(Assignee)

Comment 12

8 years ago
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
(Assignee)

Updated

8 years ago
Whiteboard: [needs blocking status][needs landing]
(Assignee)

Updated

8 years ago
Whiteboard: [needs blocking status][needs landing] → [needs landing]
Whiteboard: [needs landing] → [sg:critical?][needs landing]
(Assignee)

Comment 13

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing]
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
(Assignee)

Updated

8 years ago
Depends on: 610419
Group: core-security
You need to log in before you can comment on or make changes to this bug.