1002632 - SVG-as-an-image document containing animateTransform continues to spin its RefreshDriver after tab is closed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: throbber.svg

Comment 1

[Benoit Girard (:BenWa)]

Attachment: throbber.html

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Diagnositic patch (Not a fix)

STR:
0) Optional: Apply Diagnostic patch
1) Open https://bug1002632.bugzilla.mozilla.org/attachment.cgi?id=8413878
2) Close the tab
3) Observe 'Refresh https://bug1002632.bugzilla.mozilla.org/attachment.cgi?id=8413877' causing CPU wake ups

After ~35 seconds the refresh driver stops

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Closing throbber.svg backtrace for nsSMILAnimationController::Pause

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Closing throbber.html backtrace for nsSMILAnimationController::Pause (after ~35 seconds)

Comment 5

[Benoit Girard (:BenWa)]

dholbert, are you or know someone who is familiar with this?

My diagnostic based on the above backtrace is: If we have an SVG open we correctly call 'nsSMILAnimationController::Pause' but if we have an SVG in an image (sub document?) then 'nsSMILAnimationController::Pause' isn't called on OnPageHide.

Comment 6

[Daniel Holbert [:dholbert]]

So what's supposed to happen here is:
 (a) Clients of the image (i.e. tabs that have it open) send ticks to their images, when their own (external) refresh drivers fire, via imgIContainer::RequestRefresh().
 (b) The document inside of the image *ignores* (or suppresses) its own refresh driver, and instead responds to these "RequestRefresh" ticks, to drive its own internal SMIL/CSS animations.

This lets animation framerate be controlled by the clients, and drop to 0 when there are no clients.

Part (a) was done in bug 666446. But unfortunately, we've never gotten around to part (b). :(  There's a TODO for it, in VectorImage::RequestRefresh, here:
http://mxr.mozilla.org/mozilla-central/source/image/src/VectorImage.cpp?rev=6c1c7e45c902#491

So the behavior you're reporting is expected, with the current implementation, and we need to actually implement part (b) to fix it.

Out of curiosity, do we have any live web content (or gaia content) that's triggering this?  I don't have a good feeling for how common SMIL-animated SVG-as-an-image actually is on the web right now.

Comment 7

[Daniel Holbert [:dholbert]]

(Should've mentioned: we did the equivalent of part (b) for animated *raster* images (GIF/PNG) in bug 666446.  That was the main goal of that bug, to address a similar power/perf issue stemming from per-image timers that we were using. The equivalent SVG-image code has remained as a TODO since then, though.)

Comment 8

[(no longer active)]

This is being triggered by Cleopatra.

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Out of curiosity, do we have any live web content (or gaia content) that's
> triggering this?  I don't have a good feeling for how common SMIL-animated
> SVG-as-an-image actually is on the web right now.

Cleopatra (the profiler UI) triggers this. I also hit this needless tick when the tab is focused but the SVG is hidden (display:none I think). Cleopatra also hit needless invalidation from a hidden static progressbar. So the platform handles the cleopatra page like a complete disaster.

This bug is particularly annoying because people will open a profile, switch away from the tab and still have activity from the tab because of this bug which will show up in their next profile they save.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Do we want to suppress the refresh driver in general for documents being used as images, or only for SMIL animation?  Because just having it not tick at all would be pretty trivial, I'd think.

Comment 11

[Daniel Holbert [:dholbert]]

Ideally, we should suppress its automatic refreshing entirely, and then make VectorImage::RequestRefresh forcibly trigger a refresh-driver tick...  Perhaps via nsRefreshDriver::DoTick()?

Comment 12

[Boris Zbarsky [:bzbarsky]]

That seems reasonable, yes.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch does wht Comment 11 suggested, & it seems to fix this, according to breakpoints I put in nsSMILAnimationController::WillRefresh, and also according to BenWa's diagnostic patch.  The image still visibly animates when it's open in a foreground tab.

I'm suppressing the refresh driver's own timer by making image documents return early from nsRefreshDriver::EnsureTimerStarted().

Comment 14

[Daniel Holbert [:dholbert]]

Actually, I guess we don't technically need the StopTimer() call in this case (in the early return from EnsureTimerStarted). I'll spin this off into a separate clause without that call.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 16

[Daniel Holbert [:dholbert]]

So the patch fixes the STR from comment 2. However, it doesn't completely fix the profiler UI --  after I've created a profile & the throbber has been displayed, I get continuous refresh driver ticks in chrome://browser/content/devtools/profiler/cleopatra/images/throbber.svg [shown by BenWa's diagnostic patch], even after the throbber is no longer visible in the UI.  This continues until I close the profiler UI.

The backtrace of these ticks -- with the patch applied -- shows that they're actually being triggered by the *outer* document's refresh driver's clock, which is what's supposed to happen, per comment 6. So things are actually mostly working as they should.

The remaining problem there (after this patch is applied) is just that the image is remaining in its host document's refresh driver's list of images-to-refresh (nsRefreshDriver::mRequestTable), even though it's not visible. This issue isn't SVG-image-specific -- we'd have the same problem if this were a raster image (i.e. the image would keep the outer document's refresh driver spinning unnecessarily, and that refresh driver would trigger animation inside of the image, albeit with no invalidation).

To fix this remaining issue, we need to get nsLayoutUtils::DeregisterImageRequest() called on the image. I suspect that clearing its "src" attribute or (more severe) dropping its node from the DOM would trigger that. It's possible that "display:none" might trigger that, too, but I'm not as sure there.

We should probably spin off a separate bug for this part, anyway, probably as a profiler bug. (since I think it's something that we'd be fixing in the profiler code)

Comment 17

[Timothy Nikkel (:tnikkel)]

However the spinner is hidden we should make it so that gecko doesn't tick the refresh driver so that web content can benefit without having to change.

Comment 18

[Daniel Holbert [:dholbert]]

Local testing shows that we do the right thing (unregister the image) when I dynamically set "display:none" on the <img>. So we're doing comment 17, at least for that form of "hidden".

However, we don't stop notifying the images (though arguably we probably should) if you instead set "visibility:hidden".

In practice, I have no idea how cleopatra hides these spinners, but it should be able to benefit from this bug's patch if it uses "display:none", I'd expect.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with "display:none")

Here's a testcase to illustrate my previous comment.

With the diagnostic patch & "fix v2" applied, this testcase (using 'display:none' to hide the image) shows that the refresh driver *stops sampling* when the image disappears.

(With only the diagnostic patch & no bugfix, the refresh driver keeps spinning even after the image disappears.)

Comment 20

[Timothy Nikkel (:tnikkel)]

Yeah, we should de-register when the frame is destroy.

Bug 987212 should cover visibility hidden, but we should find out what cleopatra does, it's likely to be used in more places in the wild.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8414745 [details] [diff] [review]
fix v2

Hmm.  So if two different documents RequestRefresh on the same image we'll tick the refresh driver twice?  That seems a bit annoying.  Is there any way to avoid that?

The rest of this look fine.

Comment 22

[Daniel Holbert [:dholbert]]

Yeah, that's a flaw in the design of bug 666446 (hooking images up to their clients' refresh drivers). It affects raster animations, too, though probably less so, since it will only trigger bonus invalidations if the raster image has a really fast frame rate (> 60 FPS).

In practice, it's only a problem when we have two foreground documents with the same animation (i.e. two windows open, or perhaps a window and an iframe), since background tabs' refresh drivers will be throttled and hence won't cause many extra ticks.  So, probably an edge case.

I think the easiest way to avoid the problem (in the current design) is to have RequestRefresh check the current time, and return early if it's within N ms from our last RequestRefresh call. (where N should probably should be based on the refresh driver's normal framerate).

Comment 23

[Daniel Holbert [:dholbert]]

I filed Bug 1003683 to cover the issue discussed in Comment 21 / Comment 22. (Patch already attached there, just awaiting review.)

I've landed the patch for this bug here:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c5aae1b3dc3f

I thought about holding off on landing this until Bug 1003683 was ready, but I opted to push since that Bug 1003683 is pretty edge-casey and already affects raster images, & I'd rather have this getting nightly testing sooner rather than later. (and Bug 1003683 should presumably be reviewed & landing soon anyway)

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> So the patch fixes the STR from comment 2. However, it doesn't completely
> fix the profiler UI --  after I've created a profile & the throbber has been
> displayed, I get continuous refresh driver ticks in
> chrome://browser/content/devtools/profiler/cleopatra/images/throbber.svg
> [shown by BenWa's diagnostic patch], even after the throbber is no longer
> visible in the UI.  This continues until I close the profiler UI.

I filed bug 1004541 on this.

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Benoit Girard (:BenWa) from comment #9)
> Cleopatra (the profiler UI) triggers this.
[...]
> This bug is particularly annoying because people will open a profile, switch
> away from the tab and still have activity from the tab

Also, FWIW: per comment 16, I still see this sort of thing (activity for the profiler UI's throbber even when it's in a background tab), with this bug's patch applied.

I suspect that's because the profiler UI is in chrome, instead of in the background-tab's content, so it's driven by the chrome's refresh driver, which doesn't throttle when it's in a background tab. Or something like that.  Bug 1004541 would limit the impact of this (by silencing the throbber when it's removed from from the profiler UI), but there may be more that we should do here as well...

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c5aae1b3dc3f

Comment 27

[Benoit Girard (:BenWa)]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> I suspect that's because the profiler UI is in chrome, instead of in the
> background-tab's content

If it's open via the 'Analyze' button it's possible, however I would of assumed the add-on API wouldn't do this. Should we file a bug for that? But if you open a profile via a file or a ?report=HASH url then it can't be.

Comment 28

[Daniel Holbert [:dholbert]]

I haven't hit the "analyze" button.

My STR are as follows, for the thing I was talking about in comment 16 & comment 25:
 - Start a build with your diagnostic patch (and with this bug's fix)
 - hit Shift + F5 to open profiler UI
 - click the clock icon twice (to start & end a profile)
 - switch tabs

I see refresh driver ticks for the profiler UI ( chrome://browser/content/devtools/profiler.xul ), at what looks like ~60fps, which means we're spinning the chrome://.../profiler.xul refresh driver at full speed in the background tab.  When the throbber appears (in the background tab's profiler UI), some of those ticks also trigger ticks in the throbber. Those are the easiest to see in the diagnostic output, because they're usually persistent (for now), due to bug 1004541.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Benoit Girard (:BenWa) from comment #27)
> however I would of
> assumed the add-on API wouldn't do this

(I assume you're talking about the add-on API because that's what the profiler uses to create its footer?)

> Should we file a bug for that?

I don't know enough about how chrome documents are managed to be sure.

bz, does it strike you as unexpected that the refresh driver for the profiler footer-bar (chrome://browser/content/devtools/profiler.xul) seems to tick at full speed, even when it's in a background tab? Or is that expected behavior for some reason? (maybe as a result of it being part of the chrome)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Where is this profiler.xul file loaded?

Normally, when we switch tabs the tabbrowser marks the docshell inside the tab (and hence all its descendants) as inactive, but if there are other docshells around they wouldn't know anything about being "in a background tab"...

Comment 31

[Benoit Girard (:BenWa)]

Ohh you're using the profiler UI from the devtools panel (profiler.xul). I noticed the bug with the profiler addon which uses (http://people.mozilla.org/~bgirard/cleopatra/). That's probably where the confusion is coming from.

Comment 32

[Daniel Holbert [:dholbert]]

This broke animations in SVG-in-opentype fonts, since those use SVG-as-an-image documents, but they don't (currently) pass RequestRefresh() signals in from the host document(s). See bug 1058634 comment 3 for more.

I think we should back this out until we've got a solution there. roc agrees that we should back it out for Firefox 33 (currently Aurora, going to Beta next week), per bug 1058634 comment 8, and I'd feel more comfortable backing out there if we backout on Nightly first.  Then, we can re-land this once we've got a RequestRefresh communication channel hooked up for SVG-in-opentype.

(In the meantime, we'll take a bit of a hit on perf/power usage -- reverting to the state described in comment 2 here.)

Comment 33

[Daniel Holbert [:dholbert]]

Try run for backout: https://tbpl.mozilla.org/?tree=Try&rev=64f2739e2ede

Comment 34

[Daniel Holbert [:dholbert]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b88f451d3ecc

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: backout patch

Requesting approval to back this out on Aurora, to fix bug 1058634.

Approval Request Comment
[Feature/regressing bug #]: This bug (which I'd like to back out) caused bug 1058634.

[User impact if declined]: Animations in "SVG in OpenType" fonts won't run. These fonts are a newish feature that we support, and probably aren't widely used, but it'd be a shame to break them. We have a plan to fix them (see bug 1058634 comment 5 and bug 1058634 comment 6), but I'm not sure how soon it'll be ready, and it's probably not backport material.

[Describe test coverage new/current, TBPL]: This patch was just an optimization (not expected to impact behavior), so this is N/A. (There's some reftest coverage for animated SVG-as-an-image, though we're sadly lacking tests that would've caught bug 1058634.)

[Risks and why]: Low risk, aside from the known perf impact when this bug is triggered (which this optimization was intended to address).  In particular, we'll return to the state described in comment 2 -- animations in SVG-as-an-image in closed tabs consume CPU for ~35 seconds after the tab is closed (until they're purged from the image cache).

[String/UUID change made/needed]: None.

Comment 36

[Daniel Holbert [:dholbert]]

(After we back out on Aurora, we'll be in a state where Firefox 32 [about to ship] is affected by bug 1058634, but Firefox 33 and beyond will not be.  And as noted in comment 32, I intend to re-land this  optimization once we've fixed things so that this doesn't cause bug 1058634.)

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Daniel Holbert [:dholbert] from comment #34)
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b88f451d3ecc

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/b88f451d3ecc

Comment 38

[Daniel Holbert [:dholbert]]

Landed backout on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/27c670206df9

Comment 39

[Ryan VanderMeulen [:RyanVM]]

It seems unlikely we'd backport a fix for this to Beta33 at this point? I think we should just wontfix firefox33.

Comment 40

[Daniel Holbert [:dholbert]]

Sounds right to me. (given that this causes bug 1058634, and the hypothetical fix for that bug would probably not be backport-to-beta material.)

Comment 41

[Daniel Holbert [:dholbert]]

Attachment: fix, part 2: exclude SVG-in-opentype from this optimization, for now (with "IsFontTableURI()" check)

Here's a followup to exclude SVG-in-opentype fonts from getting this optimization, to let us land this without breaking them as described in comment 32.

(I filed bug 1107252 on creating a way to send refresh-driver ticks from the host document to drive the animations in SVG-in-opentype glyphs -- when that's fixed, we can remove this special-case exclusion.)

Comment 42

[Daniel Holbert [:dholbert]]

(Note that if the image doc has a null URI, this followup patch makes us take the "greedily use your own internal refresh driver" path.  I was just re-thinking that & considering reposting the patch with the logic flipped for that case, but I don't think it really matters, because I don't think we'd ever actually hit a null URI here, in practice, for a doc with IsBeingUsedAsImage()=true, since: I think GetDocumentURI can only be null during setup/teardown & if we seek to a different URI, none of which should be happening while we're setting up the refresh driver for an image/font internal-doc.)

Comment 43

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8531731 [details] [diff] [review]
fix, part 2: exclude SVG-in-opentype from this optimization, for now (with "IsFontTableURI()" check)

Review of attachment 8531731 [details] [diff] [review]:
-----------------------------------------------------------------

:(

Comment 44

[Daniel Holbert [:dholbert]]

Re-landed, now with "part 2" to avoid breaking svg-in-opentype fonts:
 part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1a15f16c65
 part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/100f8bfb6de2

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f1a15f16c65
https://hg.mozilla.org/mozilla-central/rev/100f8bfb6de2