606942 - Still notifying SMIL animation controller even though all windows are closed

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Markus Stange [:mstange]]

This is happening in the same Firefox session in which I've experienced bug 606932. I've closed all windows, but when I profile the Firefox process I still see the refresh driver call nsSMILAnimationController::WillRefresh 60 times a second.

The profile looks like this (focused on the 5% taken up by WillRefresh):

> + 100.0%, nsSMILAnimationController::WillRefresh(mozilla::TimeStamp), XUL
> | + 100.0%, nsSMILTimeContainer::Sample(), XUL
> | | + 100.0%, nsSMILAnimationController::DoSample(int), XUL
> | | | + 95.7%, PL_DHashTableEnumerate, XUL
> | | | | + 85.2%, DoComposeAttribute(nsSMILCompositor*, void*), XUL
> | | | | | + 85.2%, nsSMILCompositor::ComposeAttribute(), XUL
> | | | | | | + 29.6%, mozilla::SVGMotionSMILAttr::SetAnimValue(nsSMILValue const&), XUL
> | | | | | | | + 27.0%, nsSVGGFrame::AttributeChanged(int, nsIAtom*, int), XUL
> | | | | | | | | + 26.5%, nsSVGUtils::NotifyChildrenOfSVGChange(nsIFrame*, unsigned int), XUL
> | | | | | | | | | + 22.6%, nsSVGUtils::UpdateGraphic(nsISVGChildFrame*), XUL
> | | | | | | | | | | - 14.8%, nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion(nsIFrame*), XUL
> | | | | | | | | | | - 4.8%, nsSVGRenderingObserverList::InvalidateAll(), XUL
> | | | | | | | | | | - 2.6%, nsSVGEffects::InvalidateRenderingObservers(nsIFrame*), XUL
> | | | | | | | | | - 3.0%, nsSVGUtils::NotifyChildrenOfSVGChange(nsIFrame*, unsigned int), XUL
> | | | | | | | | | ...
> | | | | | | | | ...
> | | | | | | | - 1.7%, mozilla::SVGMotionSMILType::CreateMatrix(nsSMILValue const&), XUL
> | | | | | | | …
> | | | | | | - 24.8%, nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&, nsSMILValue&), XUL
> | | | | | | - 11.7%, nsSVGTransformSMILAttr::SetAnimValue(nsSMILValue const&), XUL
> | | | | | | - 9.6%, nsSVGLength2::SMILLength::SetAnimValue(nsSMILValue const&), XUL
> | | | | | | ...
> | | | | - 7.4%, nsSMILAnimationController::SampleAnimation(nsPtrHashKey<nsISMILAnimationElement>*, void*), XUL
> | | | | - 1.7%, RemoveCompositorFromTable(nsSMILCompositor*, void*), XUL
> | | | | ...
> | | | - 3.0%, nsDocument::FlushPendingNotifications(mozFlushType), XUL
> | | | - 0.4%, nsSMILAnimationController::DoMilestoneSamples(), XUL
> | | | ...

Comment 1

[Daniel Holbert [:dholbert]]

I think Brian's going to look into bug 606932, and that may or may not reveal more about what's going on here.

In the meantime: Markus, if you end up finding (semi-)reliable steps to reproduce or a testcase for this, that'd definitely be helpful in figuring out what's going on.

Comment 2

[Markus Stange [:mstange]]

Actually, reproducing this seems to be fairly easy:
1. Open planet.mozilla.org in a tab and wait for it to load.
2. Middle-click the link to https://wiki.mozilla.org/WeeklyUpdates/2010-10-25 to open it in a new tab and wait for it to load.
3. Close the Planet Mozilla tab.
4. Close the Mozilla Wiki tab.
5. Wait until you see the Mozilla Wiki tab GCed:
--DOMWINDOW == 13 (0x12ea40e40) [serial = 17] [outer = 0x0] [url = https://wiki.mozilla.org/WeeklyUpdates/2010-10-25]
6. Notice that the PMO document hasn't been GCed yet.
7. Using a debugger to break at nsSMILAnimationController::DoMilestoneSamples, notice that http://people.mozilla.org/~dholbert/demos/svgAsImg/fish.svg is still animating somewhere.

Maybe the real problem is that PMO document isn't cleaned up properly and this isn't related to SMIL after all.

Comment 3

[Daniel Holbert [:dholbert]]

Ah, ok -- this may be an SVG-as-image bug then (apparently not pausing animated images correctly when their only owner goes into bfcache).

Comment 4

[Daniel Holbert [:dholbert]]

Yeah, this is broken.  Currently, when we lose our last image consumer (i.e. when you close Planet), we effectively call "svgElem.pauseAnimations()" in the helper SVG document.  However, that just pauses our timeline -- it doesn't stop us from sampling.

In actuality, we should be treating "last-image-consumer-went-away" / "we-just-got-an-image-consumer-again" events as if they were PageHide / PageShow event for the helper document.  That triggers nsSMILAnimationController::OnPage[Hide|Show], which (un)registers with the refresh driver and gets sampling [dis|en]abled.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: possible fix

This patch just switches to using OnPageHide/OnPageShow, as suggested in comment 4.

I'm not yet convinced that this does the right thing e.g. when animations are disabled entirely -- I'm testing that currently.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 486366 [details] [diff] [review]
possible fix

I didn't immediately find anything where directly calling nsIDocument::OnPage[Hide|Show] breaks things, but whether or not there are side effects (which I suspect there would be), I think it's more heavy-handed than what we actually want.  (since this code gets invoked when we're asked to pause animations in our image -- not only when the image is "hidden")

What we want is:
 - Pause SMIL animations in our helper-document
 - Tell any images for which our helper-document is an observer that they're also free to suspend.

Patches to do that coming up.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v2

This patch:
 - Moves the "SetImagesNeedAnimating" from being a protected nsDocument method to a public nsIDocument method.  (to let us signal that images **inside of** our SVG-as-image helper-document can be suspended)
 - Changes SVGDocumentWrapper::StartAnimation() and SVGDocumentWrapper::StopAnimation() to call the method above, and to directly pause/resume our nsSMILAnimationController so that we'll stop sampling when no documents are actively using our image. (or when image animation has been otherwise disabled)

Comment 8

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/0dd06a4a870c