Closed Bug 606942 Opened 9 years ago Closed 9 years ago

Still notifying SMIL animation controller even though all windows are closed

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

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

People

(Reporter: mstange, Assigned: dholbert)

References

Details

(Whiteboard: [sg:low spoof])

Attachments

(1 file, 1 obsolete file)

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
> | | | ...
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ah, ok -- this may be an SVG-as-image bug then (apparently not pausing animated images correctly when their only owner goes into bfcache).
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.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch possible fix (obsolete) — Splinter Review
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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.
Attachment #486366 - Attachment is obsolete: true
Attached patch fix v2Splinter Review
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)
Attachment #486466 - Flags: review?(roc)
Whiteboard: [sg:low spoof][needs landing]
Landed: http://hg.mozilla.org/mozilla-central/rev/0dd06a4a870c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:low spoof][needs landing] → [sg:low spoof]
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.