Closed Bug 1117603 Opened 10 years ago Closed 9 years ago

WARNING: should already have refreshed style rule when resuming an animation

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(4 files, 2 obsolete files)

delayed-delay.html
921 bytes, text/html
Details
Don't assume style rules have been refreshed in GetAnimationRule
2.48 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Automated test case
2.17 KB, patch
Details | Diff | Splinter Review
part 2 - Don't unregister from the refresh driver unless we are also queueing events
5.84 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Attached file delayed-delay.html — 
Since bug 927349, running the following test case produces the following warning when the animation is resumed:

WARNING: should already have refreshed style rule: '!collection->mNeedsRefreshes || collection->mStyleRuleRefreshTime == mPresContext->RefreshDriver()->MostRecentRefresh()', file c:\moz\src2\layout\style\AnimationCommon.cpp, line 393
Having looked into this, what appears to be happening, is something like the following sequence (picking up at the point where that test case resumes an paused animation):

1. Script calls play()
2. The player calls collection->NotifyPlayerUpdated()
3. The collection sets mNeedsRefreshes = true and mStyleRuleRefreshTime = null
4. The collection calls manager->NotifyCollectionUpdated()
5. The animation manager calls CheckRefresh which makes it start observing the refresh driver again. However, it registers with the refresh driver for the content document.
6. Before the refresh driver for the content document gets ticked, the refresh driver for the browser chrome (chrome://browser/content/browser.xul) gets ticked.
7. That triggers a call to ProcessPendingUpdates which processes restyles and triggers nsStyleSet::RuleNodeWithReplacement, which calls CommonAnimationManager::GetAnimationRule
8. In GetAnimationRule we still have mNeedsRefreshes == true && mStyleRuleRefreshTime == null hence we hit the assertion.

Normally mStyleRuleRefreshTime will get filled in when nsAnimationManager::WillRefresh is called but in this case that won't happen until after the refresh driver for the chrome document has been ticked.

Discussing with dbaron, we agreed we should just update the style rule when needed rather than asserting that it is up-to-date.

We might be able to drop the call to set mNeedsRefreshes=true for transitions but we need to check the case brought up in bug 1061364 to see if that is possible.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Not putting this up for review just yet because I was seeing test failure on try but I'm waiting to confirm that they were the underlying changeset at fault
I made a crashtest as well but then I realised this was fixing a warning not an assertion so it will always pass.
Comment on attachment 8576457 [details] [diff] [review]
Don't assume style rules have been refreshed in GetAnimationRule

unsolicited r=dbaron, since I was thinking today that I should write a patch to do exactly the same thing, and that I was going to need it for some other cases I was about to run into in bug 847287
Attachment #8576457 - Flags: review+
Thanks for the review!

Oddly, I'm seeing test failures in dom/animation/test/css-animations/test_animation-player-starttime.html with this patch applied on Linux 64-bit and Android.

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

It's a fairly recent test (and there are changes to it sitting on inbound I believe) so I thought the underlying changeset might be at fault, so I tried just the underlying changeset on the offending platforms but it came up green:

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

Trying again with just the same subset of tests just in case:

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

Glancing at the test, I'd guess that it's timing out waiting for animationstart or animationend to fire. I suppose calling EnsureStyleRuleFor an extra time could be changing some state that means we fail to dispatch the anticipated event (or, probably more likely, dispatch it more eagerly so that when the test goes to wait the test has already fired?). Jonathan might have some ideas. If not, I'll have a closer look tomorrow.
Looks like this patch definitely triggers failures in dom/animation/test/css-animations/test_animation-player-starttime.html.
After a little investigation it seems what's happening is that the extra call to EnsureStyleRuleFor results in us unregistering from the refresh driver and hence never queuing the animationend event.

Specifically, in some contexts where we call EnsureStyleRuleFor such as nsAnimationManager::FlushAnimations, we then go on to queue events. However, in the context of GetAnimationRule we don't. We simply compose the animation style and, at the same time, notice that the animation has finished (specifically AnimationPlayer::ComposeStyle does *not* set the passed in aNeedsRefreshes flag to true) which leads us to believe we no longer need to listen to the refresh driver. Hence we never get to queue up the animationend event and the test times out.

I've wanted to refactor this arrangement for a while since it's always been a bit mysterious to me when events get queued and when they don't. Furthermore I'd like to align the handling for animations and transitions better if possible. I'm not quite sure how it should look however.

One approach is to ensure that we never unregister from the refresh driver in a context where we're not also queueing events. That should prevent us from ever hitting the end of an animation and disconnecting from the refresh driver without queueing the appropriate event. (As long as we *queue* events, we should be safe. Since even after we've unregistered from the refresh driver, the events can be dispatch later by PresShell::FlushPendingNotifications which calls DispatchEvents on the managers.)

That would suggest EnsureStyleRuleFor should not call mManager->CheckNeedsRefresh(). For EnsureStyleRuleFor, CheckNeedsRefresh() only ever causes us to *unregister* from the refresh driver so that should be ok. For other call sites we need to be sure that we still successfully *register* even if we're not queueing events.
Implement the approach outlined in comment 7. I've based this on top of the patches for bug 1125455 since I expect that might land first.
Attachment #8578462 - Flags: review?(dbaron)
Comment on attachment 8578462 [details] [diff] [review]
part 2 - Don't unregister from the refresh driver unless we are also queueing events

r=dbaron
Attachment #8578462 - Flags: review?(dbaron) → review+
sorry had to back this out for causing perma failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7833263&repo=mozilla-inbound
Nice. An OSX 10.10 only failure (which isn't run on try by default).
It turns out part 2 was at fault. It removed the call in EnsureStyleRuleFor that would cause us to either start/stop observing the refresh driver. That was deliberate since stopping to observe the refresh driver in that case can cause us to fail to dispatch some events. However, I somehow reasoned that we would only ever *stop* observing the refresh driver in that case (perhaps I misread the condition at the start of the method) so we didn't need to worry about the case where we might need to start observing the refresh driver.

In test_notificationbox.xul however we'd get a case where EnsureStyleRuleFor was actually where we noticed that we need to observe the refresh driver (I didn't dig into it to find out exactly what the parameters were that brought this about). I think on the platforms where this passed we probably had something else that kept us observing the refresh driver so we never hit this.

The fix for this is just to reinstate the call to CheckNeedsRefresh but with the new MaybeStartObservingRefreshDriver which *only* causes us to start observing the refresh driver thus avoiding the bug where we stop observing the refresh driver too soon to dispatch end events. Since this is just reinstating the code this patch was otherwise removing and is in keeping with the outline in comment 7 I don't believe it needs additional review.

Currently waiting on try to land:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ed255407e1
Attachment #8578462 - Attachment is obsolete: true
Wow, I'm having all the luck with this bug :) I have no idea how this push could affect test_ril_worker_mmi.js :(
It seems it's not just test_ril_worker_mmi.js but a bunch of files in that directory can fail so it's probably something in the xpcshell framework.

I've been trying all day to update my emulator build but it's not yet working so I can't debug this locally. Based on a couple of pushes to try (which take about 4 hours to run) I've narrowed it down to part 2.

The only real change in behavior there is in EnsureStyleRule (going from stopping or starting to observe the refresh driver to only starting; I don't expect we'd be getting calls to NotifyCollectionUpdated since that's intended to be used by the script interface). Pushing a changeset to try now that just reverts that one line (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a4124b6c367).

It's hard to imagine why *not* stopping to listen to the refresh driver would cause tests to time out. If anything, this change should mean we now succeed in dispatching events that we otherwise might not have.

For now I'll confirm if it is that line that's to blame.
the patches in this bug have some performance improvements with it:
http://alertmanager.allizom.org:8080/alerts.html?rev=c490b4f8f40e&showAll=1&testIndex=0&platIndex=0 (these are regressions seen when backed out)

Yay for perf wins!
(In reply to Brian Birtles (:birtles) from comment #21)
> For now I'll confirm if it is that line that's to blame.

Try run confirms that is *not stopping* to observe the refresh driver in EnsureStyleRule that is causing the failure.
Comparing the times on the passing/failing try runs it looks like tests take about 10% longer, on average with this change applied. I can now see something similar locally so I can debug from there. I wonder if something is waiting for an idle state and keeping the refresh driver active a moment longer is having a negative impact.
It's been a while since I looked at this bug, so as a recap:

* When we use the animations API we can end up with a situation where we clear
  mark ourselves as needing a new style rule. However, before we get a chance to
  update the style rule the refresh driver for the content document ticks,
  processes pending updates and calls GetAnimationRule which is surprised we
  haven't refreshed the style rule. This is described in comment 1.

* In part 1 we decided to just remove the assertion in GetAnimationRule and
  refresh the style rule if need be.

* That had the unfortunate side-effect that EnsureStyleRuleFor calls
  CheckNeedsRefresh which means we can end up unregistering from the refresh
  driver. However, when we call EnsureStyleRuleFor from GetAnimationRule we
  *don't* queue up events at the same time so we can end up disconnecting from
  the refresh driver before we have dispatched our end events meaning they never
  get dispatched. That was causing test_animation-player-starttime.html to fail.

* To fix the problem of unregistering without dispatching events part 2 split up
  CheckNeedsRefresh into two methods: MaybeStartObservingRefreshDriver and
  MaybeStartOrStopObservingRefreshDriver. The idea being that we should only
  call the latter if we are also dispatching events (or being destroyed).

* I incorrectly reasoned that we shouldn't even need to call CheckNeedsRefresh 
  (or its new equivalent) from GetAnimationRule. That proved to be wrong as
  comment 16 points out so I re-added a call to MaybeStartObservingRefreshDriver
  to GetAnimationRule.

* However, while writing part 2 I assumed that in our refresh driver callback
  (FlushAnimations/FlushTransitions) we were also checking if we needed to
  unregister from the refresh driver. It turns out we're not doing that and,
  indeed, part 2 only ever calls MaybeStartOrStopObservingRefreshDriver from
  ElementCollectionRemoved! Which is to say we'd never stop observing the
  refresh driver until all the animations are removed!

* I've now updated part 2 to add calls to FlushTransitions / FlushAnimations to
  MaybeStartOrStopObservingRefreshDriver so that we we actually unregister from
  the refresh driver.

In summary, these patches change the behavior in the following ways:

* We now refresh the style rule from GetAnimationRule when needed
* We never unregister from the refresh driver in EnsureStyleRuleFor
* We now unregister from the refresh driver in FlushAnimations/FlushTransitions
  when needed

This could be optimized further. For example, in FlushAnimations we'll end up
calling EnsureStyleRuleFor for each animated element and checking if we need to
start listening to the refresh driver. Then, at the end of FlushAnimations,
we'll do the same check again and either start or stop listening to the refresh
driver then. Arguably EnsureStyleRuleFor should not really be responsible for
registering/unregistering from the refresh driver but we should do that, as
appropriate, at the call sites. That's a slightly risky change which probably
deserves a separate patch/bug.

For now, I'm just requesting re-review for part 2 to cover the final point of
the first list above (adding the calls in FlushTransitions / FlushAnimations).
Attachment #8587778 - Flags: review?(dbaron)
Attachment #8581429 - Attachment is obsolete: true
(In reply to Brian Birtles (:birtles) from comment #25)
> This could be optimized further. For example, in FlushAnimations we'll end up
> calling EnsureStyleRuleFor for each animated element and checking if we need
> to start listening to the refresh driver. Then, at the end of FlushAnimations,
> we'll do the same check again and either start or stop listening to the
> refresh driver then. Arguably EnsureStyleRuleFor should not really be responsible
> for registering/unregistering from the refresh driver but we should do that, as
> appropriate, at the call sites. That's a slightly risky change which probably
> deserves a separate patch/bug.

Filed bug 1150811 for this.
https://hg.mozilla.org/mozilla-central/rev/246b8aab7c06
https://hg.mozilla.org/mozilla-central/rev/1d5052e707c4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: