Closed Bug 1209405 Opened 9 years ago Closed 7 years ago

High CPU load on Kickstarter, even without JS, CSS and images

Categories

(Core :: SVG, defect)

41 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: a3357618, Assigned: daisuke)

References

Details

(Keywords: perf, power)

Attachments

(7 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150917150946

Steps to reproduce:

Go to any Kickstarter campaign page, such as https://www.kickstarter.com/projects/899401530/fed-up-food-education-for-every-classroom/description

Check CPU load, stuck at a steady 50%.

I have a dual core CPU, so it could well be a steady 25% for a quad core.
Worth noting that only my second CPU core is going nuts, the first one stays at ~5%

I tried with and without JavaScript (blocked with NoScript 2.6.9.37), with and without CSS (disabled with ALT > Display > Page style > No style), with and without images (blocked with Adblock Plus 2.6.11), and the problem is there in all cases.

I tried with a fresh default profile and also in safe mode.

I noticed it on Firefox 41, but I don't visit Kickstarter that often. It could be a bug introduced sometime between, say, Firefox 35 and 41, but that's just a guess. That or the website was updated and now triggers a bug that has been present in Firefox even prior to 35.


Actual results:

~95% CPU load on the second CPU core out of two, ~5% on the first one.
Total 50% load.


Expected results:

Barely any CPU power used.
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
CPU is Intel Core2Duo P8400
Windows 7 64-bit clean install, almost up to date
The high CPU is caused a lot of invisible button SVG animation as follows


<button class="btn btn--green btn--block">
<span class="btn-text">
Continue
</span>
<div class="loader-dots">
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 56 16" version="1.1">
<g>
<circle r="8" cy="8" cx="8" class="loader-dot-1">
<animate values="1; .3; .3; .3;" repeatCount="indefinite" dur="1s" begin="0s" attributeType="XML" attributeName="opacity"/>
</circle>
<circle r="8" cy="8" cx="28" class="loader-dot-2">
<animate values="1; .3; .3; .3;" repeatCount="indefinite" dur="1s" begin="0.33s" attributeType="XML" attributeName="opacity"/>
</circle>
<circle r="8" cy="8" cx="48" class="loader-dot-3">
<animate values="1; .3; .3; .3;" repeatCount="indefinite" dur="1s" begin="0.66s" attributeType="XML" attributeName="opacity"/>
</circle>
</g>
</svg>
</div>

</button>
Status: UNCONFIRMED → NEW
Component: General → Layout: View Rendering
Ever confirmed: true
Keywords: power
Product: Firefox → Core
I can confirm this problem for Firefox 44 Linux 64-Bit.
Profiler:
https://cleopatra.io/#report=6d7c0a5371b55782edf2710d54db8bf08bf4c12d
Component: Layout: View Rendering → SVG
Keywords: perf
Seen on 
https://www.kickstarter.com/projects/revivalprod/overload-the-ultimate-six-degree-of-freedom-shoote

There’s also an invisible audio volume animation, hidden by default (display:none) causing this too.

<svg version="1.1" viewBox="0 0 18 17.2" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg"><g>
<polygon class="audio-indicator-bar" points="0,0 2,0 2,11.5 2,17.2 0,17.2">
<animate attributeName="points" begin="0s" calcMode="spline" dur="1.2s" keySplines="0.18 0.01 0.37 0.99;0.18 0.01 0.37 0.99;0.18 0.01 0.37 0.99" keyTimes="0; 0.3; 0.9; 1" repeatCount="indefinite" values="0,0 2,0 2,11.5 2,17.2 0,17.2;0,2.6 2,2.6 2,8.2 2,17.2 0,17.2;0,12.1 2,12.1 2,14 2,17.2 0,17.2;0,0 2,0 2,11.5 2,17.2 0,17.2"></animate>
</polygon>
 … snip … 
</g>
</svg>

Platform should be tagged as all.
OS: Windows 7 → All
Related bug 962594 for *CSS* animations in display:none elements, https://bugzilla.mozilla.org/show_bug.cgi?id=962594#c57
Brian, can we throttle SVG animations on display:none elements?
Flags: needinfo?(bbirtles)
(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #8)
> Brian, can we throttle SVG animations on display:none elements?

Not easily, I think. For CSS, we completely stop animations in display:none subtrees. That's ok because spec says animations in display:none subtrees don't run. But SVG doesn't say that (and, in most cases, SVG doesn't treat display:none specially--gradients etc. defined in display:none trees are still useable, etc.). So throttling, like you say, is probably a better idea.

We could try to use the same setup as we have for OMTA where we mark style as needing a flush but don't post a restyle? I'm not sure how much work that would be but is probably possible, at least for SVG animations that target CSS properties.

I'll see if I can find someone to look at this in Q2.
Flags: needinfo?(bbirtles)
I think this is probably easier to fix on the animation side rather than the timing side. That is, rather than trying to throttle the refresh driver, we should simply try to update the animation values lazily when we are in a display:none subtree. That's what we do for CSS animations etc.

I think the easiest way might be to exploit the mForceCompositing member in nsSMILCompositor. I think what we want to do is avoid setting mForceCompositing to true if nsSMILAnimationFunction::HasChanged() returns true (see nsSMILCompositor::GetFirstFuncToAffectSandwich) when the target element is in a display:none subtree. However, we *might* need to ensure that set call nsDocument::SetNeedStyleFlush in that case, I'm not sure.

Perhaps we need to extend mForceCompositing to a three-state enum like we do for CSS: no change, throttled update, regular update.

We'll need to tweak at least ComposeAttribute and GetFirstFuncToAffectSandwich but I think that might work.


My earlier notes (which can be ignored):

At first I thought we could deal with this at the nsISMILType layer. The difficulty here is that there are two ways we are performing animation. In comment 2 we are animating CSS properties. For that we would probably need to either:

* tweak nsSMILMappedAttribute::FlushChangesToTargetAttr to *not* call nsIPresShell::RestyleForAnimation when we're in a display:none subtree but call nsDocument::SetNeedStyleFlush instead.
* tweak the call sites of FlushChangesToTargetAttr to not end up calling it in this case.

However, in comment 6 it seems like we hit this same issue for animations of SVG attributes and profiling this seems to confirm the problem.

We have a mechanism for animated attributes to mark themselves as needing an update. AutoChangePointListNotifier calls mPointList->Element()->AnimationNeedsResample() in its destructor. This calls doc->GetAnimationController()->SetResampleNeeded() which does *two* things:

1. Calls FlagDocumentNeedsFlush()
2. Sets mResampleNeeded = true;

At first I thought we could just do (1) for the display:none subtree case (i.e. call SetNeedStyleFlush but leave mResampleNeeded alone so that if we get a call to FlushResampleRequests we can ignore it) but I think that's not quite right. Doing that would probably produce the opposite result--i.e. it will cause us to *not* do the update in the few cases where we really need to, but still do it on regular ticks.

Hence why I started looking into mForceCompositing.
Assignee: nobody → daisuke
Hi Cameron, I've been looking into this with Daisuke and I'm think we might change how animVal works when we're in display:none.

For CSS properties we can mark the style as needing a flush and only update it when getComputedStyle etc. is called. However, for our animVal interface, we don't have any code there that, e.g. checks a dirty flag and requests a sample if needed.

So if we want to stop updating animated values on elements in display:none subtrees I think we can either:

a) Add machinery to each of the animVal interfaces that checks an 'needs resample' flag and calculates the animated value on-demand when the flag is set.

or

b) Simply not update animVal values when in display:none subtrees. (We might need to add a few exceptions such as when the 'display' property itself is animated.)

I'm leaning towards (b) because in SVG2 animVal is going to become an alias for baseVal anyway. I'd also rather not add a whole lot of new code just for supporting SMIL interfaces in display:none if SMIL is on the way out.

Does that seem ok to you?
Flags: needinfo?(cam)
Also, as per comment 6, we are seeing this bug for animations of the 'points' attribute, so we can't just say, for example, "we only throttle animation of CSS properties".
(In reply to Brian Birtles (:birtles) from comment #11)
> Does that seem ok to you?

In general I am sympathetic to that plan, but will this be a problem for animating resource elements in a <defs>?  I think that's not uncommon, so if it breaks SMIL animations targetting say paint servers inside a <defs>, then we will need to go with (a) to keep that working.
Flags: needinfo?(cam)
I had a look at the test failures and observed that the reason we successfully update forwards-filling animations that use 'em' units[1] is that when we parse values that depend on context, we return a "aPreventCachingOfSandwich" value. Inside nsSMILAnimationFunction that causes us to set mValueNeedsReparsingEverySample which in turn means that nsSMILAnimationFunction::HasChanged() always returns true.

In effect that means that for forwards-filling animations that us 'em' units, we recompose the animation *every* tick. That's not great, but that's the existing behavior.

For the test that was previously failing, we need to make sure to flush pending notifications on each tick when we have one of these animations that needs to be recomposed on every sample--at least so long as we're not in a display:none subtree.

I think we can do that fairly easily because:

1. Compositors are created every tick
2. When we set them up in nsSMILAnimationController::AddAnimationToCompositorTable we call nsSMILAnimationFunction::HasChanged() on each animation function associated with the compositor. Now, HasChanged() will return true for any function that needs to be composed on every sample.
3. If HasChanged() returns true for any animation function, we call nsSMILCompositor::ToggleForceCompositing() which will set mForceCompositing to true.

So, what I *think* we could do, is when deciding if we should flush pending notifications or not, check if *either* isResampleNeeded is true, or if nsSMILCompositor::mForceCompositing is true for any of the compositors in the table.

nsSMILCompositor::mForceCompositing is not public so we'll need to add an accessor for that, maybe something like nsSMILCompositor::WillComposite() which returns mForceCompositing && !mAnimationFunctionsIsEmpty().

I think that would work, but it's a little confusing since "will composite" will return false sometimes even when we will actually compositor. To be more accurate, we should name it something like "DefinitelyNeedsToComposite".

We could probably approach this from one of a few different ways:

1. Do the above and deal with the fact that we have a slightly confusing setup and confusing names.
2. Actually expose the mValueNeedsReparsingEverySample value from nsSMILAnimationFunction (e.g. add a ValueNeedsReparsingEverySample() method);
   Query that method in nsSMILAnimationController::AddAnimationToCompositorTable and pass it back to nsSMILAnimationController::DoSample
3. Rework nsSMILCompositor::GetFirstFuncToAffectSandwich such that we can expose an nsSMILCompositor::WillComposite method that actually accurately returns whether or not we're going to compose anything;
   Make that method also cache the first function to affect the sandwich, sort the functions etc. (We might need some extra state to be able to assert that we've already done the initial setup work.)
   Use WillComposite() to control if we flush pending notifications or not and drop GetFirstFuncToAffectSandwich.

I think 3 is a bit of work and carries the risk of introducing new bugs so I'm probably leaning towards 2 at the moment, but 1 might be ok too.

[1] e.g. this test case http://codepen.io/anon/pen/VaOGyL
Review commit: https://reviewboard.mozilla.org/r/53794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53794/
Attachment #8754170 - Flags: review?(bbirtles)
Attachment #8754171 - Flags: review?(bbirtles)
Attachment #8754172 - Flags: review?(bbirtles)
Attachment #8754173 - Flags: review?(bbirtles)
Attachment #8754174 - Flags: review?(bbirtles)
Attachment #8754175 - Flags: review?(hiikezoe)
Comment on attachment 8754175 [details]
MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro

https://reviewboard.mozilla.org/r/53804/#review50496

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:13
(Diff revision 1)
> +  <svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +       xmlns:xlink="http://www.w3.org/1999/xlink">
> +    <rect width="100%" height="100%" fill="lime">
> +      <animate attributeType="XML"
> +               attributeName="fill"
> +               values="red;lime"
> +               dur="1s"
> +               repeatCount="indefinite"/>
> +    </rect>
> +  </svg>

Can we generate this SVG element dynamically?  Once this test file has other test cases, this static element will be an obstacle.

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28
(Diff revision 1)
> +</div>
> +
> +<script>
> +"use strict";
> +
> +function waitForAnimationFrames(frameCount) {

I wonder why we can't use the same function in testcommons.js here.

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:41
(Diff revision 1)
> +    }
> +    window.requestAnimationFrame(handleFrame);
> +  });
> +}
> +
> +function observeStyling(frameCount, onFrame) {

I believe we can now move this function into testcommon.js.

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:61
(Diff revision 1)
> +      resolve(stylingMarkers);
> +    });
> +  });
> +}
> +
> +function ensureElementRemoval(aElement) {

This function can be moved into testcommon.js too?

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:70
(Diff revision 1)
> +// We need to wait for all paints before running tests to avoid contaminations
> +// from styling of this document itself.
> +waitForAllPaints(function() {

I don't think we need to wait for the paints in case of SVG animations.  Our browser has SVG animations on UI running on mochitest?

Oops!  Now I noticed that ensureElementRemoval() is not used at all.  It should be removed.

And I think test test can be run with testharness.js instead of SimpleTest.js.

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:76
(Diff revision 1)
> +// from styling of this document itself.
> +waitForAllPaints(function() {
> +  add_task(function* no_restyling_if_smil_is_display_none() {
> +    var displayMarkers = yield observeStyling(5);
> +    is(displayMarkers.length, 5,
> +       "Bug 1209405: should restyle in every frame");

I don't think Bug XXX is necessary here.

::: layout/style/test/chrome/test_restyles_in_smil_animation.html:78
(Diff revision 1)
> +    document.getElementById("target").style.display = "none";
> +    getComputedStyle(document.getElementById("target")).display;
> +    var displayNoneMarkers = yield observeStyling(5);
> +    is(displayNoneMarkers.length, 0,
> +       "Bug 1209405: should never restyle if display:none");

We don't need to check after removal of display:none style?
Attachment #8754175 - Flags: review?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> Comment on attachment 8754175 [details]
> MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL
> animation. r?hiro
> 
> https://reviewboard.mozilla.org/r/53804/#review50496
> 
> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:13
> (Diff revision 1)
> > +  <svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> > +       xmlns:xlink="http://www.w3.org/1999/xlink">
> > +    <rect width="100%" height="100%" fill="lime">
> > +      <animate attributeType="XML"
> > +               attributeName="fill"
> > +               values="red;lime"
> > +               dur="1s"
> > +               repeatCount="indefinite"/>
> > +    </rect>
> > +  </svg>
> 
> Can we generate this SVG element dynamically?  Once this test file has other
> test cases, this static element will be an obstacle.
> 
> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28
> (Diff revision 1)
> > +</div>
> > +
> > +<script>
> > +"use strict";
> > +
> > +function waitForAnimationFrames(frameCount) {
> 
> I wonder why we can't use the same function in testcommons.js here.

Ah, I thought the test in dom/animation/test/.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #32)
> Comment on attachment 8754175 [details]
> MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL
> animation. r?hiro
> 
> https://reviewboard.mozilla.org/r/53804/#review50496
> 
> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:13
> (Diff revision 1)
> > +  <svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> > +       xmlns:xlink="http://www.w3.org/1999/xlink">
> > +    <rect width="100%" height="100%" fill="lime">
> > +      <animate attributeType="XML"
> > +               attributeName="fill"
> > +               values="red;lime"
> > +               dur="1s"
> > +               repeatCount="indefinite"/>
> > +    </rect>
> > +  </svg>
> 
> Can we generate this SVG element dynamically?  Once this test file has other
> test cases, this static element will be an obstacle.

Thank you very much, Hiro!
Ok, I'll do that.
Ah, also we don't need xmlns:xlink="http://www.w3.org/1999/xlink" namespace.

> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28
> (Diff revision 1)
> > +</div>
> > +
> > +<script>
> > +"use strict";
> > +
> > +function waitForAnimationFrames(frameCount) {
> 
> I wonder why we can't use the same function in testcommons.js here.

As you said, I brought the code since this test is under layout/style/test/chrome/.
Is it better to make such a testcommon.js?

> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:70
> (Diff revision 1)
> > +// We need to wait for all paints before running tests to avoid contaminations
> > +// from styling of this document itself.
> > +waitForAllPaints(function() {
> 
> I don't think we need to wait for the paints in case of SVG animations.  Our
> browser has SVG animations on UI running on mochitest?

Yes, running on mochitest.

> Oops!  Now I noticed that ensureElementRemoval() is not used at all.  It
> should be removed.

I'll use this function after revising to generate SVG element dynamically.

> And I think test test can be run with testharness.js instead of
> SimpleTest.js.

OK, change to use that.

> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:76
> (Diff revision 1)
> > +// from styling of this document itself.
> > +waitForAllPaints(function() {
> > +  add_task(function* no_restyling_if_smil_is_display_none() {
> > +    var displayMarkers = yield observeStyling(5);
> > +    is(displayMarkers.length, 5,
> > +       "Bug 1209405: should restyle in every frame");
> 
> I don't think Bug XXX is necessary here.

OK.

> ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:78
> (Diff revision 1)
> > +    document.getElementById("target").style.display = "none";
> > +    getComputedStyle(document.getElementById("target")).display;
> > +    var displayNoneMarkers = yield observeStyling(5);
> > +    is(displayNoneMarkers.length, 0,
> > +       "Bug 1209405: should never restyle if display:none");
> 
> We don't need to check after removal of display:none style?

Thanks! I'll append the test.
Comment on attachment 8754170 [details]
MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles

https://reviewboard.mozilla.org/r/53794/#review50508

r=me with hideSubtree removed from smilTestUtils.js

::: dom/smil/test/test_smilCSSFromBy.xhtml
(Diff revision 1)
> -  // Set "display:none" on everything and run the tests again
> -  SMILUtil.hideSubtree(SMILUtil.getSVGRoot(), false, false);
> -  testBundleList(gFromByBundles, new SMILTimingData(1.0, 1.0));
> -

It looks like this patch removes all references to SMILUtils.hideSubtree. If that's the case, please remove it from smilTestUtils.js.
Attachment #8754170 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #35)
> Comment on attachment 8754170 [details]
> MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use
> display:none. r?birtles
> 
> https://reviewboard.mozilla.org/r/53794/#review50508
> 
> r=me with hideSubtree removed from smilTestUtils.js
> 
> ::: dom/smil/test/test_smilCSSFromBy.xhtml
> (Diff revision 1)
> > -  // Set "display:none" on everything and run the tests again
> > -  SMILUtil.hideSubtree(SMILUtil.getSVGRoot(), false, false);
> > -  testBundleList(gFromByBundles, new SMILTimingData(1.0, 1.0));
> > -
> 
> It looks like this patch removes all references to SMILUtils.hideSubtree. If
> that's the case, please remove it from smilTestUtils.js.

Thank you very much, Brian!
Ok!
(In reply to Daisuke Akatsuka (:daisuke) from comment #34)
> > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28
> > (Diff revision 1)
> > > +</div>
> > > +
> > > +<script>
> > > +"use strict";
> > > +
> > > +function waitForAnimationFrames(frameCount) {
> > 
> > I wonder why we can't use the same function in testcommons.js here.
> 
> As you said, I brought the code since this test is under
> layout/style/test/chrome/.
> Is it better to make such a testcommon.js?

We should avoid spreading this out somehow.  Any ideas?
And now I think this test can be run without chrome privileges.

> > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:70
> > (Diff revision 1)
> > > +// We need to wait for all paints before running tests to avoid contaminations
> > > +// from styling of this document itself.
> > > +waitForAllPaints(function() {
> > 
> > I don't think we need to wait for the paints in case of SVG animations.  Our
> > browser has SVG animations on UI running on mochitest?
> 
> Yes, running on mochitest.
> 
> > Oops!  Now I noticed that ensureElementRemoval() is not used at all.  It
> > should be removed.
> 
> I'll use this function after revising to generate SVG element dynamically.
> 
> > And I think test test can be run with testharness.js instead of
> > SimpleTest.js.
> 
> OK, change to use that.

Sorry,  if the test is in layout, we don't need to use testharness.js there.
Attachment #8754171 - Flags: review?(bbirtles) → review+
Comment on attachment 8754171 [details]
MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles

https://reviewboard.mozilla.org/r/53796/#review50518

r=me with changes addressed

::: dom/smil/nsSMILCompositor.cpp:148
(Diff revision 1)
> +  bool canThrottle = mKey.mAttributeName != nsGkAtoms::display &&
> +                     styleContext && styleContext->IsInDisplayNoneSubtree();

I think this would be easier to read as:

  bool canThrottle = mKey.mAttributeName != nsGkAtoms::display &&
                     styleContext &&
                     styleContext->IsInDisplayNoneSubtree();

(I wonder if we could actually make it return true if styleContext is null, but perhaps it's safer not to for now.)
Comment on attachment 8754172 [details]
MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles

https://reviewboard.mozilla.org/r/53798/#review50510

Looks good but I'd like to have another look with the changes to AddAnimationToCompositorTable made.

::: dom/smil/nsSMILAnimationController.h:145
(Diff revision 1)
> -  static void AddAnimationToCompositorTable(
> +  // Returns true if the value that AnimationFunction handles needs to reparse every sample.
> +  static bool AddAnimationToCompositorTable(

Instead of returning true, I think it would be more clear to pass a bool reference here.

::: dom/smil/nsSMILAnimationFunction.h:252
(Diff revision 1)
>    }
>  
> +  /**
> +   * Returns true if needs to reparse every sample.
> +   */
> +  bool ValueNeedsReparsingEverySample() {

We should make this method const.

::: dom/smil/nsSMILCompositor.cpp:68
(Diff revision 1)
> -    return;
> +    // needs to update for such as following structure
> +    // layout/reftests/svg/smil/smil-transitions-interaction-3a.svg
> +    // layout/reftests/svg/smil/smil-transitions-interaction-3b.svg
> +    return true;

I think this comment should explain why we need to update style, not just point to test cases that fail.
Attachment #8754172 - Flags: review?(bbirtles)
Comment on attachment 8754173 [details]
MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles

https://reviewboard.mozilla.org/r/53800/#review50520

r=me with comments addressed

Please make sure to retrigger these tests several times on try before landing. We should make sure we're not introducing a new intermittent failure.

::: layout/reftests/svg/smil/anim-defs-fill.svg:5
(Diff revision 1)
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     class="reftest-wait">
> +
> +  <title>Test animation using 'defs' element with 'fill' attribute</title>

Nit: Test animation element in 'defs' element with 'fill' property

::: layout/reftests/svg/smil/anim-defs-fill.svg:19
(Diff revision 1)
> +             dur="100s"/>
> +  </defs>
> +
> +  <script>
> +    window.addEventListener("MozReftestInvalidate", function() {
> +      setTimeAndWaitToSnapshot(49.999, 0.001);

Let's make the wait time greater than 1 standard frame (=16ms), e.g. setTimeAndWaitToSnapshot(49.95, 0.05). That way, if setting the current time triggers a change in style that is enacted on the next tick, we won't accidentally pass.

Likewise for the other tests in this file.

::: layout/reftests/svg/smil/anim-defs-gradient-attribute.svg:5
(Diff revision 1)
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     class="reftest-wait">
> +
> +  <title>Test animation using 'defs' element with attribute of gradient</title>

Nit: Test animation of gradient attribute in 'defs' element

::: layout/reftests/svg/smil/anim-defs-gradient-property.svg:5
(Diff revision 1)
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     class="reftest-wait">
> +
> +  <title>Test animation using 'defs' element with property of gradient</title>

Nit: Test animation of gradient property in 'defs' element

::: layout/reftests/svg/smil/anim-defs-width.svg:5
(Diff revision 1)
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     class="reftest-wait">
> +
> +  <title>Test animation using 'defs' element with 'width' attribute</title>

Nit: Test animation element in 'defs' element with 'width' attribute

::: layout/reftests/svg/smil/anim-defs-width.svg:8
(Diff revision 1)
> +     class="reftest-wait">
> +
> +  <title>Test animation using 'defs' element with 'width' attribute</title>
> +  <script xlink:href="smil-util.js" type="text/javascript"/>
> +
> +  <rect id="target" fill="lime" width="100%" height="100%"/>

Can we make this so that if the animation does not run at all the test fails? e.g. see the initial width to 0%?

::: layout/reftests/svg/smil/smil-util.js:13
(Diff revision 1)
>    svg.setCurrentTime(timeInSeconds);
>    svg.removeAttribute("class");
>  }
> +
> +// Seeks to the given time and then removes the SVG document's class to trigger
> +// a reftest snapshot after wait minWaitTimeInSeconds

Nit: '... after waiting at least minWaitTimeInSeconds.'

::: layout/reftests/svg/smil/smil-util.js:19
(Diff revision 1)
> +  var takeSnapshot = function(currentTime) {
> +    if (currentTime > timeToTakeSnapshot) {
> +      svg.removeAttribute("class");
> +    } else {
> +      window.requestAnimationFrame(takeSnapshot);
> +    }
> +  };
> +  window.requestAnimationFrame(takeSnapshot);

We can write this more clearly as something like:

  requestAnimationFrame(function takeSnapshot(currentTime) {
    if (currentTime > timeToTakeSnapshot) {
      svg.removeAttribute("class");
    } else {
      requestAnimationFrame(takeSnapshot);
    }
  });
Attachment #8754173 - Flags: review?(bbirtles) → review+
Comment on attachment 8754174 [details]
MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles

https://reviewboard.mozilla.org/r/53802/#review50526

Looks good, but I should check the added test too.

::: layout/reftests/svg/smil/anim-display.svg:8
(Diff revision 1)
> +  <rect display="none" width="100%" height="100%" fill="lime">
> +    <animate attributeName="display"
> +             values="none;inline"
> +             calcMode="discrete"
> +             dur="100s"/>
> +  </rect>

Can we add a variation on this that does something like:

<g display="none" id="g">
  <rect ....>
    <animate xlink:href="#g" attributeName="display"
      ... />
  </rect>
</g>

::: layout/reftests/svg/smil/anim-display.svg:17
(Diff revision 1)
> +             dur="100s"/>
> +  </rect>
> +
> +  <script>
> +    window.addEventListener("MozReftestInvalidate", function() {
> +      setTimeAndWaitToSnapshot(49.999, 0.001);

As with the previous patch, let's wait a bit longer here, e.g. 49.9, 0.1
Attachment #8754174 - Flags: review?(bbirtles)
Comment on attachment 8754170 [details]
MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53794/diff/1-2/
Attachment #8754170 - Attachment description: MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r?birtles → MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles
Attachment #8754171 - Attachment description: MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r?birtles → MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles
Attachment #8754173 - Attachment description: MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r?birtles → MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles
Attachment #8754172 - Flags: review?(bbirtles)
Attachment #8754174 - Flags: review?(bbirtles)
Attachment #8754175 - Flags: review?(hiikezoe)
Comment on attachment 8754171 [details]
MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53796/diff/1-2/
Comment on attachment 8754172 [details]
MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53798/diff/1-2/
Comment on attachment 8754173 [details]
MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53800/diff/1-2/
Comment on attachment 8754174 [details]
MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53802/diff/1-2/
Comment on attachment 8754175 [details]
MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53804/diff/1-2/
Attachment #8754175 - Flags: review?(hiikezoe) → review+
Comment on attachment 8754175 [details]
MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro

https://reviewboard.mozilla.org/r/53804/#review50790

::: layout/style/test/test_restyles_in_smil_animation.html:28
(Diff revision 2)
> +    }
> +    window.requestAnimationFrame(handleFrame);
> +  });
> +}
> +
> +function observeStyling(frameCount, onFrame) {

nit: onFrame is not used at all.  It should be removed.

::: layout/style/test/test_restyles_in_smil_animation.html:39
(Diff revision 2)
> +
> +  docShell.recordProfileTimelineMarkers = true;
> +  docShell.popProfileTimelineMarkers();
> +
> +  return new Promise(function(resolve) {
> +    return waitForAnimationFrames(frameCount, onFrame).then(function() {

nit: waitForAnimationFrames() does not have the second argument.

::: layout/style/test/test_restyles_in_smil_animation.html:83
(Diff revision 2)
> +  return div;
> +}
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +add_task(function* no_restyling_if_smil_is_display_none() {

I think this test is for smil which is in display:none subtree.  So, smil_is_in_display_none_subtree?

::: layout/style/test/test_restyles_in_smil_animation.html:92
(Diff revision 2)
> +  var displayNoneMarkers = yield observeStyling(5);
> +  is(displayNoneMarkers.length, 0, "should never restyle if display:none");
> +
> +  div.style.display = "block";
> +  getComputedStyle(div).display;
> +  var displayNoneMarkers = yield observeStyling(5);

nit: displayNoneMarkers is re-defined here, and the second one is not for display:none actually.

::: layout/style/test/test_restyles_in_smil_animation.html:95
(Diff revision 2)
> +  div.style.display = "none";
> +  getComputedStyle(div).display;
> +  var displayNoneMarkers = yield observeStyling(5);
> +  is(displayNoneMarkers.length, 0, "should never restyle if display:none");
> +
> +  div.style.display = "block";

div.style.display = "" is a proper way, I think.

A big reason why I don't want to spread observeStyling() out is that it has currently caused an intermittent failure (bug 1244897).  I hope you help me a lot to solve that issue. ;-)
Comment on attachment 8754172 [details]
MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles

https://reviewboard.mozilla.org/r/53798/#review50798

::: dom/smil/nsSMILAnimationController.h:149
(Diff revision 2)
>                                   TimeContainerHashtable* aActiveContainers);
> +
>    static void AddAnimationToCompositorTable(
> -    mozilla::dom::SVGAnimationElement* aElement, nsSMILCompositorTable* aCompositorTable);
> +      mozilla::dom::SVGAnimationElement* aElement,
> +      nsSMILCompositorTable* aCompositorTable,
> +      bool& aFlushStyleNeeded);

Let's call this aStyleFlushNeeded.

::: dom/smil/nsSMILAnimationController.cpp:322
(Diff revision 2)
> +  bool isResampleNeeded = mResampleNeeded;
>    mResampleNeeded = false;

Why did you split this out into isResampleNeeded and isFlushStyleNeeded? Couldn't we just keep one variable, 'isStyleFlushNeeded'?

::: dom/smil/nsSMILAnimationController.cpp:383
(Diff revision 2)
> -    AddAnimationToCompositorTable(animElem, currentCompositorTable);
> +    AddAnimationToCompositorTable(animElem,
> +                                  currentCompositorTable, isFlushStyleNeeded);

isFlushStyleNeeded should go on a new line

::: dom/smil/nsSMILAnimationController.cpp:442
(Diff revision 2)
>    // STEP 5: Compose currently-animated attributes.
>    // XXXdholbert: This step traverses our animation targets in an effectively
>    // random order. For animation from/to 'inherit' values to work correctly
>    // when the inherited value is *also* being animated, we really should be
>    // traversing our animated nodes in an ancestors-first order (bug 501183)
> +  bool isUpdateStyleNeeded = false;

Let's call this mightHavePendingStyleUpdates. In fact, we could even just reset mMightHavePendingStyleUpdates and pass that, I think?

::: dom/smil/nsSMILAnimationController.cpp:640
(Diff revision 2)
>      // We've now made sure that |func|'s inactivity will be reflected as of
>      // this sample. We need to clear its HasChanged() flag so that it won't
>      // trigger this same clause in future samples (until it changes again).
>      func.ClearHasChanged();
>    }
> +  aFlushStyleNeeded = func.ValueNeedsReparsingEverySample();

This should be |= right?

::: dom/smil/nsSMILAnimationFunction.h:250
(Diff revision 2)
>    void SetWasSkipped() {
>      mWasSkippedInPrevSample = true;
>    }
>  
> +  /**
> +   * Returns true if needs to reparse every sample.

'Returns true if we need to recalculate the animation value on every sample (e.g. because it depends on context like the font-size)'

::: dom/smil/nsSMILCompositor.h:55
(Diff revision 2)
> -  // attribute
> -  void ComposeAttribute();
> +  // attribute.
> +  void ComposeAttribute(bool& aUpdateStyleNeeded);

We need to document what aUpdateStyleNeeded means (and let's call it aMightHavePendingStyleUpdates).

e.g. 'If a change is made that might produce style updates, aMightHavePendingStyleUpdates is set to true. Otherwise it is not modified.'

::: dom/smil/nsSMILCompositor.cpp:68
(Diff revision 2)
>    }
>    if (mAnimationFunctions.IsEmpty()) {
>      // No active animation functions. (We can still have a nsSMILCompositor in
>      // that case if an animation function has *just* become inactive)
>      smilAttr->ClearAnimValue();
> +    // needs to update when SMIL animation effects are removed.

'Removing the animation effect may require a style update.'
Attachment #8754172 - Flags: review?(bbirtles)
Comment on attachment 8754174 [details]
MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles

https://reviewboard.mozilla.org/r/53802/#review50800

::: layout/reftests/svg/smil/anim-display-in-g-element.svg:5
(Diff revision 2)
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     class="reftest-wait">
> +
> +  <title>Test animation that changes 'display' attribute in g element</title>

'Test animation that changes 'display' attribute on an element that is not the immediate parent'
Attachment #8754174 - Flags: review?(bbirtles) → review+
Comment on attachment 8754170 [details]
MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53794/diff/2-3/
Attachment #8754174 - Attachment description: MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r?birtles → MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles
Attachment #8754175 - Attachment description: MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r?hiro → MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro
Attachment #8754172 - Flags: review?(bbirtles)
Comment on attachment 8754171 [details]
MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53796/diff/2-3/
Comment on attachment 8754172 [details]
MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53798/diff/2-3/
Comment on attachment 8754173 [details]
MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53800/diff/2-3/
Comment on attachment 8754174 [details]
MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53802/diff/2-3/
Comment on attachment 8754175 [details]
MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53804/diff/2-3/
Comment on attachment 8754172 [details]
MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles

https://reviewboard.mozilla.org/r/53798/#review50858
Attachment #8754172 - Flags: review?(bbirtles) → review+
Comment on attachment 8754170 [details]
MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53794/diff/3-4/
Attachment #8754172 - Attachment description: MozReview Request: Bug 1209405 - Part 3: Save updating style. r?birtles → MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles
Comment on attachment 8754171 [details]
MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53796/diff/3-4/
Comment on attachment 8754172 [details]
MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53798/diff/3-4/
Comment on attachment 8754173 [details]
MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53800/diff/3-4/
Comment on attachment 8754174 [details]
MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53802/diff/3-4/
Comment on attachment 8754175 [details]
MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53804/diff/3-4/
Hi Hiro,
Could you review Part 6 patch again since the test sometime failed on Linux debug environment.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f1ba1dc9d3a&selectedJob=21154522

To solve this, I'm trying to use waitForAllPaintsFlushed instead of waitForAnimationFrames(1).
https://reviewboard.mozilla.org/r/53804/diff/3-4/
It looks ok now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f578f090e27
(In reply to Daisuke Akatsuka (:daisuke) from comment #67)
> Hi Hiro,
> Could you review Part 6 patch again since the test sometime failed on Linux
> debug environment.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6f1ba1dc9d3a&selectedJob=21154522
> 
> To solve this, I'm trying to use waitForAllPaintsFlushed instead of
> waitForAnimationFrames(1).
> https://reviewboard.mozilla.org/r/53804/diff/3-4/
> It looks ok now.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f578f090e27

I am OK with that change for now, but you should check later that where the one extra restyle for SMIL comes from or waitForPaintFlushed() is something broken.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #68)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #67)
> > Hi Hiro,
> > Could you review Part 6 patch again since the test sometime failed on Linux
> > debug environment.
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6f1ba1dc9d3a&selectedJob=21154522
> > 
> > To solve this, I'm trying to use waitForAllPaintsFlushed instead of
> > waitForAnimationFrames(1).
> > https://reviewboard.mozilla.org/r/53804/diff/3-4/
> > It looks ok now.
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f578f090e27
> 
> I am OK with that change for now, but you should check later that where the
> one extra restyle for SMIL comes from or waitForPaintFlushed() is something
> broken.

Oops. My comment was wrong.  Forget about waitForPaintFlushed().
Thanks, Hiro!
I'll request to check-in.
Keywords: checkin-needed
Status: NEW → ASSIGNED
I'm reopening this bug because it appears that the patch did not fix it. Maybe you guys fixed a different bug, like perhaps the simplified case was wrong, or only part of the problem on the actual Kickstarter website ?

Either way, the problem is exactly the same. Using Firefox 50.1 with e10s enabled, for more other report details see comment #1.


PS: For future reference, when something like that occurs, is it up to me to reopen the bug ? I'm not that acquainted to Bugzilla protocols. At least the issue initially described in this bug is not fixed, that's the certain part.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, it's not comment #1, it's the bug description.
By which I meant to say, among other things, that the bug occurs even with no CSS. (Mentioning it because this bug has been marked as blocking Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1315874
(In reply to Steph from comment #73)
> I'm reopening this bug because it appears that the patch did not fix it.
> Maybe you guys fixed a different bug, like perhaps the simplified case was
> wrong, or only part of the problem on the actual Kickstarter website ?

Right.  We seemed to fix a part of problem that causes high CPU usage.
We've been tackling remaining problem(s) in bug 1322970. Please move to that bug.
Thank you,
See Also: → 1322970
We don't reopen bugs unless the code concerned is backed out. If there are still issues here they need to be addressed in a different bug.
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.