[Power] Invisible CSS animations (on 'display:none' elements) activate the refresh driver

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: rvitillo, Assigned: kats)

Tracking

(Blocks 2 bugs, {perf, power})

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2?, firefox39 fixed, b2g-v2.1 wontfix, b2g-v2.2 affected, b2g-master affected)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

Firefox is consuming an excessive amount of CPU resources while idling on Facebook compared to other browsers (see URL). The following trace seems to be the culprit:

mozilla::RefreshDriverTimer::Tick()
nsRefreshDriver::Tick(long long, mozilla::TimeStamp)
PresShell::FlushPendingNotifications(mozilla::ChangesToFlush)
mozilla::RestyleManager::ProcessPendingRestyles()
mozilla::RestyleTracker::DoProcessRestyles()
mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint)
mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, bool)
nsCSSFrameConstructor::MaybeRecreateFramesForElement(mozilla::dom::Element*)
nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*)
nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&)
nsStyleSet::FileRules(bool (*)(nsIStyleRuleProcessor*, void*), RuleProcessorData*, mozilla::dom::Element*, nsRuleWalker*)
bool EnumRulesMatching<ElementRuleProcessorData>(nsIStyleRuleProcessor*, void*)
nsCSSRuleProcessor::RulesMatching(ElementRuleProcessorData*)
RuleHash::EnumerateAllRules(mozilla::dom::Element*, ElementDependentRuleProcessorData*, NodeMatchContext&)
ContentEnumFunc(RuleValue const&, nsCSSSelector*, ElementDependentRuleProcessorData*, NodeMatchContext&, AncestorFilter*)

mozilla::RefreshDriverTimer::Tick is invoked about 60 times per second even though the page "appears" to be static (chat disabled).
with any addons?

FWIW, there are other bugs citing RestyleManager
Flags: needinfo?(rvitillo)
I used a clean profile with no addons.
Flags: needinfo?(rvitillo)
OS: Mac OS X → All
We should break in the code that schedule the timer. This should let us know what mutation is causing this.

The refresh driver is schedule when something when a new frame is (potentially) required. Sometimes we know ahead of time that we're going to produce something identical and stop.

Understanding what is causing is refresh driver to trigger in the first place is a good first step.
The culprit for the RefreshDriverTimer::Tick seems to be the css class _55ym:

<html>
    <head>
        <style>
            ._55ym{animation:rotateSpinner 1.2s linear infinite}
            @keyframes rotateSpinner{0%{transform:rotate(0deg)} 100%{transform:rotate(360deg)}}
        </style>
    </head>
    <body>
        <span class="_55ym">
        </span>
    </body>
</html>

The _55ym class is used to animate the spinning wheels on the left toolbar of Facebook. The wheels are visible only when the user clicks on one of the menu items and the content is being loaded though.
Is the refresh driver ticking even when the animation is trivially hidden?

If so that's a bug and should be fix. We can compute if something is not visible and should be able to keep the refresh driver off. Nice find.
Right, there is no visible animation but refresh driver gets scheduled anyway.
See the animation in Comment 4. Can we get someone from Layout to look at optimizing how CSS animations trigger the refresh driver? We should avoid scheduling the timer for no change animations.
Flags: needinfo?(roc)
Summary: High power consumption on Facebook while idling → [Power] Invisible CSS animations activate the refresh driver
Note that the refresh driver ticks even when the animation is trivially hidden, e.g.:

<html>
    <head>
        <style>
            ._5tqs{display: none;}
            ._55ym{animation:rotateSpinner 1.2s linear infinite}
            @keyframes rotateSpinner{0%{transform:rotate(0deg)} 100%{transform:rotate(360deg)}}
        </style>
    </head>
    <body>
        <div class="_55ym _5tqs">foo</div>
    </body>
</html>

There are two possible optimizations here: 
1) Don't tick when the animation is not visible
2) Don't tick when a visible animation is not going to produce a delta in the frame buffer (e.g. Comment 4)

For this particular bug addressing 1) should fix the issue.
Component: General → CSS Parsing and Computation
Product: Firefox → Core
Hardware: x86 → All
> 2) Don't tick when a visible animation is not going to produce a delta in the frame buffer

Animations can affect layout even without affecting what's painted, and that's observable from script.  So optimizing out animations that are not on display:none stuff without breaking compat is pretty tough.
Yeah, I suspect we want to go with something along the lines of (1) -- something like: "Don't tell the refresh driver about animations for an element IF that element's (un-animated) computed 'display' is none, and also there are currently no animations on its 'display' value."

This might want to happen in nsAnimationManager::GetElementAnimations() (e.g. maybe don't call AddElementData there), or in a related function.

We also probably need to also worry about SMIL animations, which could be another way of actually-showing something with un-animated "display:none"... Maybe as a fast pass, we could check if there are any SMIL animations targeting the element at all.
s/fast pass/quick and easy hack/
We could probably do something for animations in a display:none subtree, involving keeping the animations in the animation manager but not updating them -- especially if we can depend on the element not having a persistent style context.
One other note.  The testcase in comment 8 in fact has an observable effect: if you query the computed "transform" of the object, it should change over time as the animation progresses.  So either that needs to tick the refresh driver to update the style or we need to somehow update it when getComputedStyle happens or something...
Here's a live testcase, based on comment 8. It's got a (short) delay before the animation is applied, so you can watch (in a debugger) when we register as a refresh observer & start ticking.
(In reply to Benoit Girard (:BenWa) from comment #7)
> See the animation in Comment 4. Can we get someone from Layout to look at
> optimizing how CSS animations trigger the refresh driver?

Tagging dbaron instead of roc [to pick an assignee], since I believe style-system / animations are more his area of expertise.  (This came up in the Platform meeting today, too, as something that Power folks are looking for help on.)

[I'd be open to taking this myself, though I'm unfamiliar with nsAnimationManager.cpp, so I'd be digging a bit.]
Flags: needinfo?(roc) → needinfo?(dbaron)
Roberto you know that we do worse on power usage on this page. But do you know if other browsers optimize away the timer or are they just more efficient at returning early from their refresh timer?

If it's the later then perhaps the problem is easier to optimize. We might just have to get better at returning faster.
Flags: needinfo?(rvitillo)
(In reply to Benoit Girard (:BenWa) from comment #16)
> If it's the later then perhaps the problem is easier to optimize. We might
> just have to get better at returning faster.

I don't think that'll do it; if I recall correctly from other bugs, it's the frequent wakeups that are responsible for the power usage in cases like this (even if they're short).  They never let the CPU get into a lower power state, or something like that.
I don't think so either. But it's probably worth checking this assumption before we take on a potentially difficult bug. Unless this bug is easy, then we should do just do it.
In the Facebook case, is the offending animation actually in a display:none subtree?
See comment 8. Not a subtree in that case but it has both the animation and display none.
Robert's question was whether comment 8 is actually an accurate representation of what's going on with the original page this bug was filed on or whether it's a synthetic testcase.
(In reply to Benoit Girard (:BenWa) from comment #16)
> Roberto you know that we do worse on power usage on this page. But do you
> know if other browsers optimize away the timer or are they just more
> efficient at returning early from their refresh timer?
> 
> If it's the later then perhaps the problem is easier to optimize. We might
> just have to get better at returning faster.

Chrome optimizes away the timer. Returning early wouldn't really help though in terms of power usage (see Daniel's comment).
Flags: needinfo?(rvitillo)
(In reply to Boris Zbarsky [:bz] from comment #21)
> Robert's question was whether comment 8 is actually an accurate
> representation of what's going on with the original page this bug was filed
> on or whether it's a synthetic testcase.

Comment 8 is an accurate representation of what's causing the issue with Facebook.
It looks like they simply don't apply animations at all to display:none elements.  It's not clear to me that this is actually spec-compliant...  David?
Per conversation with bz in IRC, we have (at least) two options here:

 (a) Change the spec (and our implementation) to match the Chrome behavior, so that animations on properties in a 'display:none' subtree have no effect (except perhaps for animations targeting the 'display' property itself).

 (b) Optimize our current behavior by lazily computing animations on elements inside of a 'display:none' subtree (again, probably excluding the 'display' property itself).  Store such animations in a separate list on the animation manager, without letting them prompt the refresh driver to tick. (but if the refresh driver is already ticking for other reasons, we might as well sample them with everything else). And then -- whenever getComputedStyle() is called, flush these animations. (That's the lazy part.)  (and perhaps skip the flush if we had a refresh driver tick or a flush "recently".)
(In reply to Daniel Holbert [:dholbert] from comment #25)
> (but if the refresh driver is already ticking for other reasons, we might as
> well sample them with everything else).

Sorry to nitpick but ideally i'd be best to behave the same way regardless and not sample them at all. This way if we think an animation has no impact then it stop regardless. This make bugs where we incorrectly disable an animation obvious. Otherwise you get a bug that only reproduces if we're not painting.
(In reply to Benoit Girard (:BenWa) from comment #27)
> (In reply to Daniel Holbert [:dholbert] from comment #25)
> > (but if the refresh driver is already ticking for other reasons, we might as
> > well sample them with everything else).
> 
> Sorry to nitpick but ideally i'd be best to behave the same way regardless
> and not sample them at all.

Sure, that would work.

The reason I'm suggesting sampling them during not-requested-by-us ticks is
 (a) it should be low-cost -- as long as we're already taking the hit of ticking the refresh driver (for something else), the marginal cost of sampling these animations is minimal
 (b) it will let us keep getComputedStyle() more efficient (no need to worry about flushing these animations there, if we've had a recent tick().

> Otherwise you get a bug that only reproduces if we're not
> painting.

I don't understand what the hypothetical bug you're talking about would look like.

These animations shouldn't have any observable effect, since they're by definition in content that doesn't render.

(except via getComputedStyle, which will only have a slight observable difference -- whether it reports the animation as of the last tick (if we have other animations running; note that this is the current behavior) or whether it reports the up-to-the-millisecond animation state (if no other animations are running & hence no ticks are happening).
Blocks: 970927
Whiteboard: [possible solution in comment 25 part b]
Summary: [Power] Invisible CSS animations activate the refresh driver → [Power] Invisible CSS animations (in 'displya:none' subtree) activate the refresh driver
Summary: [Power] Invisible CSS animations (in 'displya:none' subtree) activate the refresh driver → [Power] Invisible CSS animations (in 'display:none' subtree) activate the refresh driver
wouldn't mind doing this after 960465, although I'm not sure how strong the ordering constraints actually are
Depends on: 960465
So here's an interesting question.  http://dev.w3.org/csswg/css-animations/ seems to pretty explicitly say that animations don't run on display:none elements.  David, what _is_ the spec status here?
Summary: [Power] Invisible CSS animations (in 'display:none' subtree) activate the refresh driver → [Power] Invisible CSS animations (on 'display:none' elements) activate the refresh driver
Note that the proposed solution is similar to what we do for animations that are running on the compositor thread, and can probably share code with it.
Blocks: 993259
Blocks: 1062119
Since we had the recent regression with bug 1062119, we'd really like to fix this in Gecko rather than having to find the causes in gaia. Is there anyway we could get this prioritized or what needs to be done so that we can get to this? Thanks!
Flags: needinfo?(dholbert)
Would be nice for 2.1, but that seems unlikely. Perhaps we can have this prioritized for our 2.2 release?
blocking-b2g: --- → 2.2?
I agree that we should probably prioritize this, and I also agree that this is unlikely to be fixable for 2.1 (since a patch here is likely to involve some risk, and hence would be scary to backport).

I'm told that 2.2 will be based on gecko36 or later, so I think we've got ~12 weeks, if this is targeted for 2.2.

I'll bring this up with dbaron in our 1:1 next week.
I think this is more of a "feature-b2g":2.2? rather than "blocking-b2g":2.2?
(In reply to Milan Sreckovic [:milan] from comment #35)
> I think this is more of a "feature-b2g":2.2? rather than "blocking-b2g":2.2?

right
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?
Not only is this important for battery use, on low-end devices we've seen this eat 10% of the CPU while in a fullscreen hosted app (running webrtc, which is desparate on such hardware for every cycle it can steal).
Related: we have seen 20% CPU save when disabling animations on statusbar when a fullscreen app is on top of it.
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #38)
> Related: we have seen 20% CPU save when disabling animations on statusbar
> when a fullscreen app is on top of it.

Nice. That could explain a lot of the laggish behavior I have on the Nexus S when doing network activity :)
The B2G system app is affected:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/update_manager/update_manager.css#L9

Even if FxOS is idle, a restyle is triggered constantly.

And the Gallery and Marketplace apps are probably affected too.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #40)
> The B2G system app is affected:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/
> update_manager/update_manager.css#L9
> 

Filed bug 1069997 for this one (it's actually caused by an element that can never be displayed... :/)
Michael, can you follow up this topic? Can you either + or - it? 
Thanks.
Flags: needinfo?(mtreese)

Comment 43

4 years ago
Johnny,
This looks like it may be a platform issue, can you take a look and see where it should go?

Thanks,
Michael
Flags: needinfo?(mtreese) → needinfo?(jst)
For display:none, we shouldn't be running animations on display:none subtrees at all, per recent changes to CSS animations. See bug 984850 comment 5 and 6.

But we should also be smarter about animations that are simply not visible too.
Depends on: 822092
Jet, is this filed where it belongs?
Flags: needinfo?(jst) → needinfo?(bugs)
Moved it to Core:Layout. Leaving NI:me to reassign.
Component: CSS Parsing and Computation → Layout
As a workaround, I have this in my userContent.css:

  * {
    -moz-animation: none !important;
    animation: none !important;
  }

Without the workaround, CPU usage stays high enough for my MBP to get hot and for its fans to spin up. This issue is sadly trigged by several popular sites like Twitter and Facebook.

If you want to try the workaround, but don't know what userContent.css is, see: https://ffeathers.wordpress.com/2013/03/10/how-to-override-css-stylesheets-in-firefox/
(In reply to Birunthan Mohanathas [:poiru] from comment #47)
> As a workaround, I have this in my userContent.css:
> 
>   * {
>     -moz-animation: none !important;
>     animation: none !important;
>   }

This didn't work for me.

For FF 35.0.1, I see between 33-37 "Energy Impact" in Activity Monitor on OSX 10.10.2.
For FF 38.0.1 (nightly) with e10s, I see between 4.5-5.7 "Energy Impact" with the same profile.

To test, I created a clean profile and loaded twitter and left it untouched until the energy impact settled down after a few seconds.
I believe twitter's energy problem is related to animated images, not CSS animations.  (At least it used to be.)
Duplicate of this bug: 1139918
(In reply to Not doing reviews right now from comment #30)
> So here's an interesting question.  http://dev.w3.org/csswg/css-animations/
> seems to pretty explicitly say that animations don't run on display:none
> elements.  David, what _is_ the spec status here?

If I'm understanding [1] and [2] correctly, setting display:none should stop the animation entirely. If somebody can outline where to fix this I can probably rustle up some spare cycles to write the patch.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=14785
[2] http://lists.w3.org/Archives/Public/www-style/2011Nov/0709.html
layout/style/nsAnimationManager.cpp is where we actually create the animations. We create the whole set of animations in BuildAnimations then compare it to the existing set (in CheckAnimationRule) and cancel any animations that are in the old list but not the new one. So simply updating BuildAnimations might be enough.

Regarding events, I think that will give us the behavior we want--i.e. no 'animationend' event if you set display:none part-way through. In the future we'll probably add an 'animationcancel' event for that. If you set display:block part-way through, the animation should restart which is also the behaviour that will just fall out of updating BuildAnimations.

The bigger issue is probably around invisible animations. For compositor animations, when the target is offscreen we don't create a layer and hence we can't run it on the compositor and throttle it so we keep ticking the refresh driver and use more power when the animation is invisible than when it is visible. Or at least I think that was the issue. I think dbaron had some ideas for this but I think he was also waiting to finish some other refactoring first.
(In reply to Brian Birtles (:birtles, away most of 21 Feb~6 Mar) from comment #52)
> The bigger issue is probably around invisible animations. For compositor
> animations, when the target is offscreen we don't create a layer and hence
> we can't run it on the compositor and throttle it so we keep ticking the
> refresh driver and use more power when the animation is invisible than when
> it is visible. Or at least I think that was the issue.

Correct, we have bug 1105509 about just that.
Posted patch Naive patch (obsolete) — Splinter Review
This fixes the issue I was seeing in bug 1139918, but surely there must be more to it than just this. What am I missing? Or should I do a try push and see what breaks?
Comment on attachment 8573339 [details] [diff] [review]
Naive patch

>diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
[...]
>-  BuildAnimations(aStyleContext, aElement, timeline, newPlayers);
>+  if (disp->mDisplay != NS_STYLE_DISPLAY_NONE) {
>+    BuildAnimations(aStyleContext, aElement, timeline, newPlayers);

I think you need to check whether any of the ancestor nsStyleContexts are display:none as well, too.

Also, we need to consider what happens if we *use an animation* to set an element's 'display' property to none. (I assume you can do that.)  The links in comment 51 don't really say. Seems like we'd get into a catch-22, where we stop applying animations because we're "display:none"; but the reason we're display:none is *because* of an animation... so maybe that means our animation is no longer applied & we go back to display:block again... but then we can be animated again so we reapply our animation and go back to display:none?
Flags: needinfo?(dholbert)
Posted patch Slightly less naive patch (obsolete) — Splinter Review
(In reply to Daniel Holbert [:dholbert] from comment #55)
> I think you need to check whether any of the ancestor nsStyleContexts are
> display:none as well, too.

Updated patch. I assumed we would want to cache the display:none rather than walking up the style context ancestor chain every time. I'm unsure though if the cache needs recomputing when nsStyleContext::MoveTo is called - it doesn't look like the other bits get reset when that happens.

> Also, we need to consider what happens if we *use an animation* to set an
> element's 'display' property to none. (I assume you can do that.)

Thankfully [1] doesn't say display is animatable (thereby implying it is not). Good thing, too, or you'd have to implement ease-in transitions between display:none and display:block!

[1] http://www.w3.org/TR/2012/WD-css3-transitions-20120403/#animatable-properties
Attachment #8573339 - Attachment is obsolete: true
Attachment #8573558 - Flags: feedback?(dholbert)
Comment on attachment 8573558 [details] [diff] [review]
Slightly less naive patch

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> I'm unsure though if
> the cache needs recomputing when nsStyleContext::MoveTo is called - it
> doesn't look like the other bits get reset when that happens.

Yeah -- I'd expect we'd need to check the new parent after MoveTo, though I'm not sure either. It does seem like there are some other bits (like the INLINE_DESCENDANT_OF_RUBY one) should be updated if MoveTo is called; that may be a bug.


> Thankfully [1] doesn't say display is animatable (thereby implying it is
> not). Good thing, too, or you'd have to implement ease-in transitions
> between display:none and display:block!

Ah, nice. I'd thought that maybe CSS animations allowed you to interpolate between keywords in stepwise manner, but I guess not.

However, SVG SMIL animation *does* allow you to animate "display"[1], http://www.w3.org/TR/SVG/propidx.html -- and we have that encoded into Gecko at [2].  We manage SMIL animations separately from CSS animations right now (not via this BuildAnimations call, AFAIK) -- but when we someday make all animations share a common implementation (via a single grand Web Animations backend), we'll have to worry about this problem a bit more...

[1] http://www.w3.org/TR/SVG/painting.html#DisplayProperty (see "Animatable: yes")
[2] http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILCSSProperty.cpp#186


Anyway, the patch looks OK to me (perhaps birtles or heycam should review, when you're ready for full review).  Nits below, all around a central theme:

># HG changeset patch
># User Kartikaya Gupta <kgupta@mozilla.com>
>Bug 962594 - Don't build CSS animations for elements that are display:none. r=

s/are display:none/are in a display:none subtree/

>+++ b/layout/style/nsStyleContext.h
>@@ -153,16 +153,20 @@ public:
> 
>+  // Does this style context or any of its ancestor have display:none set?
>+  bool IsDescendantOfDisplayNone() const
>+    { return !!(mBits & NS_STYLE_IS_DESCENDANT_OF_DISPLAY_NONE); }

s/ancestor/ancestors/

Also: The use of "Descendant" is slightly misleading here, since we're including the "I myself am 'display:none'" scenario, and I am *not* a "descendant" of myself. I'd suggest changing to use the term "display:none subtree", e.g. IsInDisplayNoneSubtree() and NS_STYLE_IN_DISPLAY_NONE_SUBTREE.
Attachment #8573558 - Flags: feedback?(dholbert) → feedback+
Don't forget to bump NS_STYLE_CONTEXT_TYPE_SHIFT when adding a new style context bit.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> Updated patch. I assumed we would want to cache the display:none rather than
> walking up the style context ancestor chain every time. I'm unsure though if
> the cache needs recomputing when nsStyleContext::MoveTo is called - it
> doesn't look like the other bits get reset when that happens.

We should not be calling MoveTo if the bits differ; that is handled by comparing the bits in ElementRestyler::ComputeRestyleResultFromNewContext.  Might be good to have an assertion in MoveTo, though, that the old and new parents agree on these flags bits.

While you could recompute the NS_STYLE_IN_DISPLAY_NONE_SUBTREE bits in MoveTo, it's probably not worth it, so just add another check to ComputeRestyleResultFromNewContext.
Normally we won't even construct style contexts for descendants of display:none elements -- unless somebody asks for computed style on them.  So I wonder if there's a better way that doesn't use up a bit on the style context.  Then again, now that we've switched to 64-bits there, we're probably a little less constrained.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> > Also, we need to consider what happens if we *use an animation* to set an
> > element's 'display' property to none. (I assume you can do that.)
> 
> Thankfully [1] doesn't say display is animatable (thereby implying it is
> not). Good thing, too, or you'd have to implement ease-in transitions
> between display:none and display:block!
> 
> [1]
> http://www.w3.org/TR/2012/WD-css3-transitions-20120403/#animatable-properties

That said, there's a working group resolution in https://lists.w3.org/Archives/Public/www-style/2014Oct/0290.html that changes that, since it makes all non-interpolable properties settable by animations.  (It's not clear if it's going to end up being Web-compatible.)



At some point we should also fix this for visual-only properties that are offscreen.  That might lead to a stronger fix that allows this one to be removed.
Flags: needinfo?(dbaron)
So I made all the changes in comments 57-58, including the MoveTo assertion, and pushed to try [1].

The try push shows crashes on the assert that I added. I reproduced it locally and it looks like MoveTo is getting called in a scenario where the two parents have a different value for the NS_STYLE_HAS_TEXT_DECORATION_LINES bit. Is that expected? Maybe my assertion was checking the wrong bits? See [2] for what I did.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=38ac2f818b93
[2] https://hg.mozilla.org/try/rev/6c2b17a1fc8b#l3.15
Flags: needinfo?(cam)
(And yes, that code is ugly. I will use a DebugOnly<uint64_t> mask to remove the duplication for the final version)
(In reply to David Baron [:dbaron] (UTC-8) from comment #59)
> At some point we should also fix this for visual-only properties that are
> offscreen.  That might lead to a stronger fix that allows this one to be
> removed.

Actually, ignore that.  Since this bug is also fixing things to match a statement in the spec, not building animations should be the right fix here, whereas the work for visual-only properties would be an optimization that changes mNeedsRefreshes but doesn't change whether we build animations.

That said, given that there's spec prose here, there should probably be some relevant tests in test_animations.html to test it too!


(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #60)
> So I made all the changes in comments 57-58, including the MoveTo assertion,
> and pushed to try [1].
> 
> The try push shows crashes on the assert that I added. I reproduced it
> locally and it looks like MoveTo is getting called in a scenario where the
> two parents have a different value for the
> NS_STYLE_HAS_TEXT_DECORATION_LINES bit. Is that expected? Maybe my assertion
> was checking the wrong bits? See [2] for what I did.

That may well be a regression from bug 931668.  The optimization for bug 931668 should probably get disabled if the text-decoration bit is different or if your new display-none bit is different.
(In reply to David Baron [:dbaron] (UTC-8) from comment #62)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #60)
> > So I made all the changes in comments 57-58, including the MoveTo assertion,
> > and pushed to try [1].
> > 
> > The try push shows crashes on the assert that I added. I reproduced it
> > locally and it looks like MoveTo is getting called in a scenario where the
> > two parents have a different value for the
> > NS_STYLE_HAS_TEXT_DECORATION_LINES bit. Is that expected? Maybe my assertion
> > was checking the wrong bits? See [2] for what I did.
> 
> That may well be a regression from bug 931668.  The optimization for bug
> 931668 should probably get disabled if the text-decoration bit is different
> or if your new display-none bit is different.

Though ElementRestyler::ComputeRestyleResultFromNewContext looks like it already checks the text-decoration bit.  Presumably it should also check your new bit.  That doesn't explain the assertion, though.
If I remove the NS_STYLE_HAS_TEXT_DECORATION_LINES from the assert check, there's still another call to MoveTo that's bad, resulting the Android crashtest failure on this try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea3b75d91521

I don't think I'll be able to reproduce this one locally.
Posted patch PatchSplinter Review
I decided to just check my new bit in the assert, since technically that's what my patch cares about. If we want to add other bits to the assert we can do that in follow-up patches.
Attachment #8573558 - Attachment is obsolete: true
Flags: needinfo?(cam)
Attachment #8574363 - Flags: review?(cam)
Comment on attachment 8574363 [details] [diff] [review]
Patch

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

This looks good.  I am also not sure about those assertion failures in the previous patches you pushed to try.  They look like a real problem -- can you file a bug for that (or mention it in the followup bug you file for expanding |mask| to cover more flags)?

We really need some tests for this, too, before we land it.
Attachment #8574363 - Flags: review?(cam) → review+
Posted patch Part 2 - Tests (obsolete) — Splinter Review
Tests attached. These fail without the patch and pass with the patch, tested locally on OS X (with OMTA forced on). Also pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f7f12c25de
Assignee: nobody → bugmail.mozilla
Attachment #8576681 - Flags: review?(cam)
Whiteboard: [possible solution in comment 25 part b]
See Also: → 1142531
I filed bug 1142531 for adding the asserts into MoveTo and fixing the failures, or otherwise dealing with that problem.
[Canceling stale needinfo from before this bug had an owner] (kats, thanks for taking this!)
Flags: needinfo?(bugs)
Comment on attachment 8576681 [details] [diff] [review]
Part 2 - Tests

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

This looks good, but can you also add a subtest that checks this behaviour on a child of a display:none element?
Attachment #8576681 - Flags: review?(cam)
Attachment #8576681 - Attachment is obsolete: true
Attachment #8577232 - Flags: review?(cam)
Comment on attachment 8577232 [details] [diff] [review]
Part 2 - Tests (v2)

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

Maybe use a helper function in test_animations_omta.html too to keep the files more similar?
Attachment #8577232 - Flags: review?(cam) → review+
I wanted to do that but the yields made it non-trivial to do and it was cleaner to just have two separate test functions.
https://hg.mozilla.org/mozilla-central/rev/d00224526057
https://hg.mozilla.org/mozilla-central/rev/7d034e0c62f9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
See Also: → 1182856
You need to log in before you can comment on or make changes to this bug.