User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0 Build ID: 20150328030209 Steps to reproduce: <style> a {transition: color 1s;} a:hover {color:#ffffff} </style> <a href="#">Test</a> Actual results: After hovering performance timeline repeats recalculating style. Expected results: Stop when transition over. Regression starts in 38. Working correct in 36/37.

Mozilla/5.0 (Windows NT 5.1; rv:39.0) Gecko/20100101 Firefox/39.0 confirming with recent Nightly on XP works with Fx 36 Mozilla/5.0 (Windows NT 5.1; rv:36.0) Gecko/20100101 Firefox/36.0 Is this a bug in layout or in the performance tool?

Hovering over multiple anchors gives a noticeable increase in CPU usage without having tool open.

[Tracking Requested - why for this release]: Possibly-serious performance regression.

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4b118b959ffd&tochange=a9dd7c71cc68 Triggered by: Bug 960465

I see this in a nightly, but it works in my tree with local changes. Need to figure out which change...

It's either patch 10 or patch 11 from bug 847287.

Apparently patch 10. I'm really not sure why, though.

Tracking for 38+. Maybe we can avoid shipping this.

So one of the changes in bug 960465 was that we retain objects for finished transitions, because the information might be needed if we're starting other transitions later. That was done in https://hg.mozilla.org/mozilla-central/rev/b2ee72589c18 I guess I thought there was something that made finished transitions not apply anymore. (I don't see that right now, but maybe there is -- and it doesn't really matter.) But I guess the problem is perhaps that we don't set mNeedsRefreshes to false on the finished transitions, or that if we do, we don't call CheckNeedsRefresh. I'll try to test this.

So (and I'm analyzing this on beta, since that's where I want to make sure to fix the bug) the thing that prevents finished transitions from applying is AnimationPlayer::ComposeStyle, which should also prevent them from having mNeedsRefreshes true, at least assuming that we call it, which we do from AnimationPlayerCollection::EnsureStyleRuleFor, which CommonAnimationManager::GetAnimationRule should do. So I guess I'll have to look to see which part of this theory is wrong...

It looks like we correctly have mNeedsRefreshes false and don't apply style; we just don't call CheckNeedsRefresh an at appropriate time.

(And CheckNeedsRefresh happens every time we remove a player, so not removing it anymore means we fail to call it.)

So I think the right fix for that is the FlushTransitions/FlushAnimations changes (at least) from https://hg.mozilla.org/mozilla-central/rev/1d5052e707c4 , which landed on mozilla-central on April 3. But now that I'm testing it on beta, I do see mNeedsRefreshes being true.

... and that's because of bug 1061364. We call GetAnimationRule twice per refresh cycle. The first time GetAnimationRule sets mNeedsRefresh to true and EnsureStyleRuleFor fixes it. The second time EnsureStyleRuleFor finds its time matches, and skips the work that fixes it.

And it's only the bug 1061364 changes that are strictly necessary to fix this bug. (Or what I'm hoping is a safer equivalent.)

Try push against beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=165f82699227

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #18) > Try push against beta: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=165f82699227 This looks fine; there are a bunch of failures in things that don't run on beta. Compare to the beta run on its parent changeset: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=a24bdacce4cc

Comment on attachment 8590456 [details] [diff] [review] patch for aurora and beta, backporting part of bug 1061364 This backports the important (here) part of bug 1061364 in what I think is a safer way than backporting the whole thing. In changes the assignment of mNeedsRefreshes = true to only happen when EnsureStyleRuleFor will actually recompute it (modulo the throttling case, which shouldn't matter since when animations are throttled we do need refreshes). This allows EnsureStyleRuleFor to be called more than once per refresh tick without forcing mNeedsRefresh to true. I'm hoping you remember what was problematic about the full bug 1061364 in order to verify that this is actually a safe subset of it.

Comment on attachment 8590456 [details] [diff] [review] patch for aurora and beta, backporting part of bug 1061364 Review of attachment 8590456 [details] [diff] [review]: ----------------------------------------------------------------- The two things I know we need to verify are: a) https://bug1110277.bugzilla.mozilla.org/attachment.cgi?id=8535092 since from bug 1061364 comment 2 that was failing at one point with the patch from bug 1061364. b) browser/components/customizableui/test/browser_989751_subviewbutton_class.js since that was also failing due to what seemed to be a transition event not firing. I was a bit afraid that something like (b) could still crop up with this patch. However, I don't *think* that's the case. With this patch we perform the early return in EnsureStyleRuleFor *more* often which should mean we are less likely to unregister from the refresh driver too soon (which was happening at the end of EnsureStyleRuleFor) and fail to dispatch events which is one of the problems we hit in bug 1117603. While I'm not 100% confident that this is a safe subset, if (a) and (b) pass (and all the other tests) then I think it's probably fine.

aurora try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f184a8a73696

And the baseline for the aurora try push on aurora is: https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=8d2d128ce671

Comment on attachment 8590456 [details] [diff] [review] patch for aurora and beta, backporting part of bug 1061364 Approval Request Comment [Feature/regressing bug #]: combination of bug 960465 and bug 1025709 [User impact if declined]: excessive CPU usage after CSS transitions complete [Describe test coverage new/current, TreeHerder]: reasonably good coverage for the affected code [Risks and why]: reasonably safe, but not guaranteed safe; the code that's being modified was trying to force the EnsureStyleRuleFor call below it to do a refresh; this patch continues making it force a refresh in the cases where it would do so, but avoids messing up the mNeedsRefreshes for the other cases [String/UUID change made/needed]: no

Comment on attachment 8590456 [details] [diff] [review] patch for aurora and beta, backporting part of bug 1061364 Approving for uplift to aurora hoping to fix performance issues.

Comment on attachment 8590456 [details] [diff] [review] patch for aurora and beta, backporting part of bug 1061364 should be in 38 beta 4.

I checked that this did in fact fix it using mozilla-aurora Linux nightlies and mozilla-beta Linux debug nightlies from April 12 and compared to April 14. Steps were: 1. turn on the devtools.timeline.enabled pref in about:config 2. Tools -> Web Developer -> Performance 3. switch to the timeline tab in the performance devtools 4. load attachment 8585239 [details] 5. start recording 6. hover over the link text, and then wait 7. move mouse off link text, and then wait Following steps 6 and 7 in the April 12 builds, there was a stream of purple (restyle) forever following the stream of green during the transition. In the April 14 builds, there was a brief purple patch that then stopped.

(Those steps appear not to be usable on mozilla-central, since devtools timeline appears to be gone.)