Closed Bug 972199 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(4 files, 7 obsolete files)

1.29 KB, text/html
Details
2.07 KB, patch
nrc
: review+
Details | Diff | Splinter Review
1.76 KB, patch
nrc
: review+
Details | Diff | Splinter Review
2.89 KB, patch
nrc
: review+
Details | Diff | Splinter Review
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).
Blocks: 964646
Attached patch skipYetToStartAnimations (obsolete) — Splinter Review
Patch for (2) from comment 0.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Attachment #8375356 - Flags: review?(ncameron)
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?
(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.
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).
Attachment #8375356 - Flags: review?(ncameron)
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.
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?
Flags: needinfo?(ncameron)
(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
(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.
Flags: needinfo?(ncameron)
Attachment #8377367 - Attachment is obsolete: true
Attachment #8377368 - Attachment is obsolete: true
Attachment #8377372 - Attachment is obsolete: true
Attachment #8378796 - Flags: review?(ncameron)
Attachment #8378796 - Flags: review?(ncameron) → review+
Attachment #8378797 - Flags: review?(ncameron) → review+
Attachment #8378798 - Flags: review?(ncameron) → review+
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.
Attachment #8378796 - Attachment is obsolete: true
Attachment #8385119 - Flags: review?(ncameron)
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.
Attachment #8385119 - Flags: review?(ncameron) → review+
(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
(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).
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.
You need to log in before you can comment on or make changes to this bug.