536834 - The document for an External Resources can have its window pointer cleared, without us pausing animations

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug on the situation described in bug 535004 comment 13.

That bug's patch initially added a NS_ABORT_IF_FALSE to verify that we never clear a document's window pointer while its animations are playing.  However, it turns out we *do* do that, when we've got External Resources in play.

Quoting bug 535004 comment 13:
> So I hit this NS_ABORT_IF_FALSE if I run reftests on
> layout/reftests/reftest-sanity/reftest.list.  In particular, if I remove
> everything in that file except for the "filter-1.xhtml" or "filter-2.xhtml"
> line, then I hit this ABORT_IF_FALSE on shutdown.

I think we probably need to call "OnPageHide()" on the external resource's document at some point. (We never do right now.)  Or, we need to be also pausing the animation controller in one of the other nsDocument cleanup methods.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch that adds the failing NS_ABORT_IF_FALSE

Comment 2

[Boris Zbarsky [:bzbarsky]]

We could probably just call OnPageHide and OnPageShow on the external resource documents in the document's own OnPageHide and OnPageShow.  See nsIDocument::EnumerateExternalResources.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch does what bz suggested above -- i.e. it makes us call OnPageShow/Hide on our external resource docs, within nsDocument::OnPageShow/Hide.

This fixes the NS_ABORT_IF_FALSE failure -- with this patch, I've verified that I can successfully get through a reftest run in a debug build.

Comment 4

[Daniel Holbert [:dholbert]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/2a9219de5fbc

Marking in-testsuite+, because we already have reftests that will trigger this problem if it regresses, by failing the added NS_ABORT_IF_FASE. (e.g. filter-1.xhtml, as noted in comment 0)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Passing aDispatchStartTarget to the resource documents looks wrong to me.  I'd think we always want to pass null, no matter what aDispatchStartTarget is.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: followup: pass nsnull as our |aDispatchStartTarget| argument

ah, thanks for that! I think this should fix it... Let me know if this looks ok.

Comment 7

[Daniel Holbert [:dholbert]]

I confirmed that we still get through reftests in the |layout/reftests/reftest-sanity| directory without failing this bug's NS_ABORT_IF_FALSE (or anything else bad happening), with the followup applied.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 420274 [details] [diff] [review]
followup: pass nsnull as our |aDispatchStartTarget| argument

Looks good to me.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks! pushed followup: http://hg.mozilla.org/mozilla-central/rev/d21bc9fd9a8d

Comment 10

[Daniel Holbert [:dholbert]]

Hmm -- so, after this bug landed, bug 537157 removed the requirement that our document have a window.  As a result, the NS_ABORT_IF_FALSE added here (and described in comment 0) is no longer important. It's not a problem if our document's window pointer is cleared during animations, because we don't need it anyway.

So, I tend to think this bug's changes should be backed out, unless there's some other reason we'd want documents to notify their external resources of pageshow/pagehide events.

Thoughts?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, we still want to pause animations in the external resource doc when it goes into bfcache, right?

Comment 12

[Daniel Holbert [:dholbert]]

Mm, good point.  I think I just want to remove the ABORT_IF_FALSE, then.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: followup 2: remove no-longer-necessary NS_ABORT_IF_FALSE

This patch removes the NS_ABORT_IF_FALSE that was added in fix v1, per comment 10 & comment 12.

My original motivation in adding this chunk was to make it an "early catch" for nulled-out window pointers that were likely to cause an abort in the subsequent animation sample.  As noted in comment 10, though, animation samples don't need a window pointer now.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 420435 [details] [diff] [review]
followup 2: remove no-longer-necessary NS_ABORT_IF_FALSE

Would it make sense to still assert if the document is torn down with an active animation controller?

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 420435 [details] [diff] [review]
followup 2: remove no-longer-necessary NS_ABORT_IF_FALSE

Talked to bz in IRC a bit, and I think we do indeed want to keep this NS_ABORT_IF_FALSE after all.

Previously, I wasn't sure that a call to SetScriptGlobalObject(nsnull) always meant "we're tearing down our document", but from talking to bz and searching MXR for SetScriptGlobalObject(nsnull), it looks like that's the case.

So, given that, this NS_ABORT_IF_FALSE just asserts that when we're tearing down a document, its animations should already be paused (probably via nsSMILAnimationController::OnPageHide).  And that's a good thing to assert.

However, we do need to guard the NS_ABORT_IF_FALSE with #ifdef MOZ_SMIL, since it refers to mAnimationController which doesn't exist if SMIL is disabled at build time. (thanks to crowder for spotting that)

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> However, we do need to guard the NS_ABORT_IF_FALSE with #ifdef MOZ_SMIL

pushed: http://hg.mozilla.org/mozilla-central/rev/586f48e28a58