972199 - Assertion fail: "Should not run animation that hasn't started yet on the compositor" when restoring refresh driver

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case (needs SuperPowers and OMTC enabled)

There is an assertion in ElementAnimations::GetPositionInIteration that checks that we don't call this method from the compositor thread for animations that have yet to start. (We know if we are being called from the compositor thread since the aAnimation parameter is only set when called from the main thread.)

The assertion is here: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp?rev=8aacc8a67472#95

Normally this is fine since we don't add animations to a layer until they are running. However, when testing we often advance the refresh driver manually which may cause an animation to be added to a layer. When we later restore the refresh driver we can end up with an animation on a layer but a refresh driver time that is *before* the start of that animation causing the above-mentioned assertion to fail.

The attached test case does this.


This needs fixing because having to work around this makes testing OMTA hard. Also, any content with access to DOMWindowUtils can produce this situation which, in release builds, will cause us to attempt to read the null aAnimation parameter.


Some possible approaches to solving this are:

(1) Change the refresh driver so that it never goes backwards. For example, add a mAdvancedDuration member to the refresh driver and increment that when we call AdvanceTimeAndRefresh. When we restore the refresh driver we would continue to add this amount in. This way application code can assume that refresh driver times are monotonically increasing.

(2) In SampleAnimations (AsyncCompositionManager.cpp) check if the elapsed duration is before the start of the animation, and if it is, skip that animation.

(3) Detect if we have an animation on the layer that is yet-to-start and remove it.


In the future I *think* we want to allow animations on a layer that are yet-to-start. That would allow setting up an animation to run in advance. That suggests (2). It's also a smaller change than (1). I'm happy to do (1) as well or instead though if that seems desirable.

Patch forthcoming for (2).

Comment 1

[Brian Birtles (:birtles)]

Attachment: skipYetToStartAnimations

Patch for (2) from comment 0.

Comment 2

[Nick Cameron [:nrc]]

Comment on attachment 8375356 [details] [diff] [review]
skipYetToStartAnimations

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

This seems to be addressing the symptoms rather than the cause - can we check when restoring the refresh driver if we have such animations and do something with them? Or address the problem at that end in some other way?

Comment 3

[Brian Birtles (:birtles)]

(In reply to Nick Cameron [:nrc] from comment #2)
> Comment on attachment 8375356 [details] [diff] [review]
> skipYetToStartAnimations
> 
> Review of attachment 8375356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to be addressing the symptoms rather than the cause - can we
> check when restoring the refresh driver if we have such animations and do
> something with them? Or address the problem at that end in some other way?

That's similar to (3) from comment 0. We could also trigger re-generation of all animations when restoring the refresh driver I guess. Is that would you have in mind?

I think (2) is still something we want in the future but we could do it when we need it.

Comment 4

[Brian Birtles (:birtles)]

Nick and I discussed this on IRC. I will make up a patch for (1) for now since it seems like it would be nice to be able assume that times from the refresh driver are always monotonically increasing, even under test conditions.

I think we can add (2) later if needed (I have in mind to rework ElementAnimations::GetPositionInIteration when implementing Web Animations since its pretty funky at the moment).

Comment 5

[Brian Birtles (:birtles)]

I made up a patch for (1) today but hit a snag because the compositor thread doesn't use times from the refresh driver. It just calls TimeStamp::Now(). So, even if we patch the refresh driver, we still don't stop time from going backwards when it comes to sampling animations.

In order for this approach to work, I'll need to patch CompositorParent in a similar way. I'll make up a patch for that tomorrow and then we can way up if it's too complex or not.

Comment 6

[Brian Birtles (:birtles)]

Attachment: Part 1 - Make refresh driver store advanced time

Comment 7

[Brian Birtles (:birtles)]

Attachment: Part 2 - Make times in CompositorParent not go backwards

Comment 8

[Brian Birtles (:birtles)]

Attachment: Part 3 - Add a crashtest for restoring the refresh driver after an advance

Comment 9

[Brian Birtles (:birtles)]

Attachment: Part 4 - Fix comment in ElementAnimations::GetPositionInIterationmentInGetPositionInIteration

Comment 10

[Brian Birtles (:birtles)]

Attachment: Part x - Make SampleAnimations filter out animations that are yet to start

Comment 11

[Brian Birtles (:birtles)]

I implemented (1) from comment 0 in patches part 1 & 2. It turns out to be quite involved. It changes a lot of code and means you can no longer force the refresh driver backwards.

I'm somewhat more inclined to take approach (2) which I've implemented in part x.

Parts 3 & 4 are common to both approaches.

Nick, what do you think?

Comment 12

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #11)
> I implemented (1) from comment 0 in patches part 1 & 2. It turns out to be
> quite involved. It changes a lot of code and means you can no longer force
> the refresh driver backwards.

Also, looks like it *still* doesn't work sometimes... https://tbpl.mozilla.org/?tree=Try&rev=ff2de2bf847f

Comment 13

[Nick Cameron [:nrc]]

(In reply to Brian Birtles (:birtles) from comment #11)
> I implemented (1) from comment 0 in patches part 1 & 2. It turns out to be
> quite involved. It changes a lot of code and means you can no longer force
> the refresh driver backwards.
> 
> I'm somewhat more inclined to take approach (2) which I've implemented in
> part x.
> 
> Parts 3 & 4 are common to both approaches.
> 
> Nick, what do you think?

Sounds like approach 2 is the winner - I would be wary of doing something as dramatic as approach 1 unless there is a pressing need - it seems dangerous.

Comment 14

[Brian Birtles (:birtles)]

Attachment: Part 1 - Skip animations on compositor thread that have yet to start

Comment 15

[Brian Birtles (:birtles)]

Attachment: Part 2 - Add a crashtest for restoring the refresh driver after an advance

Comment 16

[Brian Birtles (:birtles)]

Attachment: Part 3 - Fix comment in ElementAnimations::GetPositionInIterationmentInGetPositionInIteration

Comment 17

[Brian Birtles (:birtles)]

Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=9717790d405f

Comment 18

[Brian Birtles (:birtles)]

Attachment: Part 1 - Skip animations on compositor thread that have yet to start, v1b

Thanks for the reviews Nick.

Can I ask you to re-review this patch?

I updated it to move the line that sets activeAnimations = true to before the line where we skip animations that have yet to start. That way we won't stop compositing during the delay phase.

Comment 19

[Nick Cameron [:nrc]]

Comment on attachment 8385119 [details] [diff] [review]
Part 1 - Skip animations on compositor thread that have yet to start, v1b

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

I think this is fine. It would be better not to do this and wake the compositor back up when the delay expires - better not to spin the compositor uselessly. However, I don't think you need to do that right now. Maybe file a bug and/or leave a note in the code.

Comment 20

[Brian Birtles (:birtles)]

(In reply to Nick Cameron [:nrc] from comment #19)
> I think this is fine. It would be better not to do this and wake the
> compositor back up when the delay expires - better not to spin the
> compositor uselessly. However, I don't think you need to do that right now.
> Maybe file a bug and/or leave a note in the code.

Add this note and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/780785bdec58
https://hg.mozilla.org/integration/mozilla-inbound/rev/81e98d0d0273
https://hg.mozilla.org/integration/mozilla-inbound/rev/34bc56f8d2b0

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/780785bdec58
https://hg.mozilla.org/mozilla-central/rev/81e98d0d0273
https://hg.mozilla.org/mozilla-central/rev/34bc56f8d2b0

Comment 22

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #11)
> > I implemented (1) from comment 0 in patches part 1 & 2. It turns out to be
> > quite involved. It changes a lot of code and means you can no longer force
> > the refresh driver backwards.
> 
> Also, looks like it *still* doesn't work sometimes...
> https://tbpl.mozilla.org/?tree=Try&rev=ff2de2bf847f

I'm pretty sure the issue here was synchronizing the amount of accumulated advance between the main thread and compositor.

I filed bug 1043078 for actually fixing this (i.e. doing (1) from comment 0).

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In order to land bug 996796, I had to fix the crashtest added in this bug (part 2) so that it doesn't turn on async-animations for all crashtests after this test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb35f913cc5

Messing with prefs manually with crashtests is really rather dangerous.  You should use the manifests in reftest/crashtest and pushPrefEnv in mochitest.

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3eb35f913cc5