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

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: dholbert)

Tracking

({power})

Trunk
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 wontfix, firefox37 fixed)

Details

(Whiteboard: power)

Attachments

(9 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
Posted image throbber.svg
No description provided.
Reporter

Comment 1

5 years ago
Posted file throbber.html
Reporter

Comment 2

5 years ago
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
Reporter

Updated

5 years ago
Attachment #8413883 - Attachment is patch: true
Attachment #8413883 - Attachment mime type: text/x-patch → text/plain
Reporter

Updated

5 years ago
Keywords: power
Whiteboard: power
Reporter

Comment 5

5 years ago
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.
Flags: needinfo?(dholbert)
Assignee

Comment 6

5 years ago
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.
Flags: needinfo?(dholbert)
Assignee

Comment 7

5 years ago
(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

5 years ago
This is being triggered by Cleopatra.
Reporter

Comment 9

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

Comment 11

5 years ago
Ideally, we should suppress its automatic refreshing entirely, and then make VectorImage::RequestRefresh forcibly trigger a refresh-driver tick...  Perhaps via nsRefreshDriver::DoTick()?
That seems reasonable, yes.
Assignee

Updated

5 years ago
Summary: Sub SVG Document containing animateTransform triggers RefreshDriver after tab is closed → SVG-as-an-image document containing animateTransform continues to spin its RefreshDriver after tab is closed
Assignee

Updated

5 years ago
Assignee: nobody → dholbert
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee

Comment 13

5 years ago
Posted patch fix v1 (obsolete) — Splinter Review
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().
Attachment #8414736 - Flags: review?(bzbarsky)
Assignee

Comment 14

5 years ago
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.
Assignee

Comment 15

5 years ago
Posted patch fix v2Splinter Review
Attachment #8414736 - Attachment is obsolete: true
Attachment #8414736 - Flags: review?(bzbarsky)
Attachment #8414745 - Flags: review?(bzbarsky)
Assignee

Comment 16

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

Comment 18

5 years ago
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.
Assignee

Comment 19

5 years ago
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.)
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 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.
Attachment #8414745 - Flags: review?(bzbarsky) → review+
Assignee

Comment 22

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

Comment 23

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

Updated

5 years ago
Flags: in-testsuite-
Assignee

Comment 24

5 years ago
(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.
Blocks: 1004541
Assignee

Comment 25

5 years ago
(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...
https://hg.mozilla.org/mozilla-central/rev/c5aae1b3dc3f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Reporter

Comment 27

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

Comment 28

5 years ago
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.
Assignee

Comment 29

5 years ago
(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)
Flags: needinfo?(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"...
Flags: needinfo?(bzbarsky)
Reporter

Comment 31

5 years ago
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.

Updated

5 years ago
Depends on: 1008598
Assignee

Updated

5 years ago
Depends on: 1058634
Assignee

Comment 32

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

Comment 34

5 years ago
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b88f451d3ecc
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 35

5 years ago
Posted patch backout patchSplinter Review
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.
Attachment #8481516 - Flags: approval-mozilla-aurora?
Assignee

Comment 36

5 years ago
(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.)
Attachment #8481516 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
Target Milestone: mozilla32 → ---
It seems unlikely we'd backport a fix for this to Beta33 at this point? I think we should just wontfix firefox33.
Assignee

Comment 40

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

Comment 41

5 years ago
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.)
Attachment #8531731 - Flags: review?(edwin)
Assignee

Comment 42

5 years ago
(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 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]:
-----------------------------------------------------------------

:(
Attachment #8531731 - Flags: review?(edwin) → review+
Assignee

Comment 44

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/0f1a15f16c65
https://hg.mozilla.org/mozilla-central/rev/100f8bfb6de2
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.