1343589 - Assertion failure: isRelevant == IsCurrent() || IsInEffect() (Out of date Animation::IsRelevant value)

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes an assertion failure in mozilla-central rev 34c6c2f302e7.

Assertion failure: isRelevant == IsCurrent() || IsInEffect() (Out of date Animation::IsRelevant value), at /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:977
#01: mozilla::dom::KeyframeEffect::SetTarget at dom/animation/KeyframeEffect.cpp:113
#02: mozilla::dom::KeyframeEffectBinding::set_target at obj-firefox/dom/bindings/KeyframeEffectBinding.cpp:1321
#03: mozilla::dom::GenericBindingSetter at dom/bindings/BindingUtils.cpp:2921
#04: js::CallJSNative at js/src/jscntxtinlines.h:282

Comment 1

[Brian Birtles (:birtles)]

That looks like a genuine bug. I guess we should be calling mAnimation->NotifyEffectTimingUpdated() first. By failing to do this, we might also be failing to add ourselves to the effect set.

Comment 2

[Brian Birtles (:birtles)]

In the test we have the following arrangement:

> let a = document.documentElement.animate([{}], {duration: 100, iterations: Number.POSITIVE_INFINITY});

That is, we create an animation whose target effect end is positive infinity.

> a.startTime = 2000;

Set the animation to start in the future.

> try{ a.reverse(); }catch(e){}

Try to reverse the animation.

Now, according to [1], the last step is to "Run the steps to play an animation for animation with the auto-rewind flag set to true."

In the procedure to plan an animation[2] we have the following block:

  If animation playback rate < 0, the auto-rewind flag is true and either animation’s:
    current time is unresolved, or
    current time ≤ zero, or
    current time > target effect end,
      If target effect end is positive infinity, throw an InvalidStateError and abort these steps.
      Otherwise, set animation’s hold time to target effect end.

In this case, current time is less than zero since we set the startTime so that it starts in the future (this is also why this test case sometimes fails to reproduce in debug builds).

The target effect end is positive infinity so we throw an exception. However, at this point we have reversed the playback direction so we are effectively finished. When we threw the exception, though, we failed to update the local timing state so we still think we're relevant despite being finished.

Then when we hit the next line:

>   a.effect.target = document.createElement("span");

We go to call UpdateTargetRegistration where we compare the cached relevant state on the animation with our local calculations, notice the discrepancy and freak out.

So the problem is basically failing to update local state when throwing an exception. Normally in PlayNoUpdate() that's safe to do because the point where we throw the exception we know we haven't changed any state so we can simply return early. When we're calling this from Reverse() however, we have updated the playback rate so we need to make sure to updating timing.

[1] https://w3c.github.io/web-animations/#reversing-an-animation-section
[2] https://w3c.github.io/web-animations/#play-an-animation

Comment 3

[Brian Birtles (:birtles)]

Hmm, actually, if reverse() throws, the state should remain unaffected (i.e. we should revert the change to the playback rate). This needs to be specced too.

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Move tests for reversing an animation to the timing-model folder

We are gradually line the tests up better with sections in the spec. Given that
these tests are concerned with testing the "reverse an animation" procedure in
the spec (and not the API per se) they should be moved to an appropriate
subdirectory of timing-model.

We also update the test titles to make it clear that these tests are really
covering the timing model, not the API.

Review commit: https://reviewboard.mozilla.org/r/124084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124084/

Comment 5

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add a test that the playback rate is unaffected when an exception is thrown

The spec has been updated to clarify that this is the expected behavior:

  https://github.com/w3c/web-animations/commit/c80c9984cac1091c428c7d3203479698e0d745c1

Review commit: https://reviewboard.mozilla.org/r/124086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124086/

Comment 6

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Restore the playbackRate when reverse() throws.

Review commit: https://reviewboard.mozilla.org/r/124088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124088/

Comment 7

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add animation mutation observer test for reverse() when it throws an exception

The main purpose of this patch is to add a test that when reverse throws an
exception we don't report anything to animation mutation observers. I have
verified that this added test fails without the early return added in the
previous patch in this series.

However, while adding this test I noticed that we don't have any animation
mutation observer tests for reverse() or setting the playbackRate so I added
them as well.

Review commit: https://reviewboard.mozilla.org/r/124090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124090/

Comment 8

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Rename animation observer test files

I have verified that this test fails without the fix from the previous patch.

Review commit: https://reviewboard.mozilla.org/r/124092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124092/

Comment 9

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add comments to test_animation_observers_[a]sync.html to describe how they differ

We already pass this test but it seemed like a useful test to add since I was
unsure if the early return we have in Animation::Reverse() is valid or not.

Review commit: https://reviewboard.mozilla.org/r/124094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124094/

Comment 10

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Move setupSynchronousObserver from testcommon.js to test_animation_observers_sync.html

The spec[1] says:

  Silently set the animation playback rate of animation to -animation playback
  rate.

  This must be done silently or else we may end up resolving the current
  ready promise when we do the compensatory seek despite the fact that we are
  most likely not exiting the pending play state.

This patch add tests that we don't exit the pending play state when calling
reverse() or resolve the ready promise.

[1] https://w3c.github.io/web-animations/#reverse-an-animation

Review commit: https://reviewboard.mozilla.org/r/124096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124096/

Comment 11

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add a crashtest for when reverse() throws

Review commit: https://reviewboard.mozilla.org/r/124098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124098/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851863 [details]
Bug 1343589 - Move tests for reversing an animation to the timing-model folder

https://reviewboard.mozilla.org/r/124084/#review126584

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851864 [details]
Bug 1343589 - Add a test that the playback rate is unaffected when an exception is thrown

https://reviewboard.mozilla.org/r/124086/#review126588

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851865 [details]
Bug 1343589 - Restore the playbackRate when reverse() throws.

https://reviewboard.mozilla.org/r/124088/#review126590

This change noticed me that we call PostUpdate (i.e. syncing layer) even if Play() throws an exception (also Pause() case).
It would be nice to avoid calling PostUpdate() in a follow-up bug?

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851866 [details]
Bug 1343589 - Add animation mutation observer test for reverse() when it throws an exception

https://reviewboard.mozilla.org/r/124090/#review126592

Nice!

I think all test cases here can be synchronously run. If so, we should move them to test_observers_for_sync_api.html.
we have normal reverse() case already there. :-)

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851867 [details]
Bug 1343589 - Rename animation observer test files

https://reviewboard.mozilla.org/r/124092/#review126594

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851868 [details]
Bug 1343589 - Add comments to test_animation_observers_[a]sync.html to describe how they differ

https://reviewboard.mozilla.org/r/124094/#review126596

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851869 [details]
Bug 1343589 - Move setupSynchronousObserver from testcommon.js to test_animation_observers_sync.html

https://reviewboard.mozilla.org/r/124096/#review126604

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851870 [details]
Bug 1343589 - Add a crashtest for when reverse() throws

https://reviewboard.mozilla.org/r/124098/#review126606

Nice!

Comment 20

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> Comment on attachment 8851866 [details]
> Bug 1343589 - Add animation mutation observer tests for changing
> playbackRate and calling reverse()
> 
> https://reviewboard.mozilla.org/r/124090/#review126592
> 
> Nice!
> 
> I think all test cases here can be synchronously run. If so, we should move
> them to test_observers_for_sync_api.html.
> we have normal reverse() case already there. :-)

Ok, I didn't realize we had split the tests like that. I think it's pretty confusing at the moment, so if it's ok, I'd like to:

* Rename test_observers_for_sync_api.html to test_observers_sync.html
* Rename test_observers.html to test_observers_async.html
* Add a comment to the top of each file explaining the difference and mentioning the counterpart file
* Move setupSynchronousObserver from testcommon.js to test_observers_sync.html

Then I'll rework the mutation observer tests in this bug to fit in.

Comment 21

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add a test for reversing with an infinite target effect end and playback rate of zero

We already pass this test but it seemed like a useful test to add since I was
unsure if the early return we have in Animation::Reverse() is valid or not.

Review commit: https://reviewboard.mozilla.org/r/124128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124128/

Comment 22

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add tests that the playback rate is updated silently

The spec[1] says:

  Silently set the animation playback rate of animation to -animation playback
  rate.

  This must be done silently or else we may end up resolving the current
  ready promise when we do the compensatory seek despite the fact that we are
  most likely not exiting the pending play state.

This patch add tests that we don't exit the pending play state when calling
reverse() or resolve the ready promise.

[1] https://w3c.github.io/web-animations/#reverse-an-animation

Review commit: https://reviewboard.mozilla.org/r/124130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124130/

Comment 23

[Brian Birtles (:birtles)]

Attachment: Bug 1343589 - Add a test that reverse() updates animations on the compositor

Review commit: https://reviewboard.mozilla.org/r/124132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124132/

Comment 24

[Brian Birtles (:birtles)]

Comment on attachment 8851863 [details]
Bug 1343589 - Move tests for reversing an animation to the timing-model folder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124084/diff/1-2/

Comment 25

[Brian Birtles (:birtles)]

Comment on attachment 8851864 [details]
Bug 1343589 - Add a test that the playback rate is unaffected when an exception is thrown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124086/diff/1-2/

Comment 26

[Brian Birtles (:birtles)]

Comment on attachment 8851865 [details]
Bug 1343589 - Restore the playbackRate when reverse() throws.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124088/diff/1-2/

Comment 27

[Brian Birtles (:birtles)]

Comment on attachment 8851867 [details]
Bug 1343589 - Rename animation observer test files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124092/diff/1-2/

Comment 28

[Brian Birtles (:birtles)]

Comment on attachment 8851868 [details]
Bug 1343589 - Add comments to test_animation_observers_[a]sync.html to describe how they differ

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124094/diff/1-2/

Comment 29

[Brian Birtles (:birtles)]

Comment on attachment 8851869 [details]
Bug 1343589 - Move setupSynchronousObserver from testcommon.js to test_animation_observers_sync.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124096/diff/1-2/

Comment 30

[Brian Birtles (:birtles)]

Comment on attachment 8851866 [details]
Bug 1343589 - Add animation mutation observer test for reverse() when it throws an exception

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124090/diff/1-2/

Comment 31

[Brian Birtles (:birtles)]

Comment on attachment 8851870 [details]
Bug 1343589 - Add a crashtest for when reverse() throws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124098/diff/1-2/

Comment 32

[Brian Birtles (:birtles)]

Somehow I knew MozReview would get that wrong. And I want to put a facepalm emoji here but Bugzilla doesn't support emoji :/

Comment 33

[Brian Birtles (:birtles)]

Hang on, MozReview is about to have another fit as I go to update one of the commit messages here...

Comment 34

[Brian Birtles (:birtles)]

Comment on attachment 8851869 [details]
Bug 1343589 - Move setupSynchronousObserver from testcommon.js to test_animation_observers_sync.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124096/diff/2-3/

Comment 35

[Brian Birtles (:birtles)]

Comment on attachment 8851866 [details]
Bug 1343589 - Add animation mutation observer test for reverse() when it throws an exception

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124090/diff/2-3/

Comment 36

[Brian Birtles (:birtles)]

Comment on attachment 8851870 [details]
Bug 1343589 - Add a crashtest for when reverse() throws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124098/diff/2-3/

Comment 37

[Brian Birtles (:birtles)]

Comment on attachment 8851893 [details]
Bug 1343589 - Add a test for reversing with an infinite target effect end and playback rate of zero

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124128/diff/1-2/

Comment 38

[Brian Birtles (:birtles)]

Comment on attachment 8851894 [details]
Bug 1343589 - Add tests that the playback rate is updated silently

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124130/diff/1-2/

Comment 39

[Brian Birtles (:birtles)]

Comment on attachment 8851895 [details]
Bug 1343589 - Add a test that reverse() updates animations on the compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124132/diff/1-2/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851893 [details]
Bug 1343589 - Add a test for reversing with an infinite target effect end and playback rate of zero

https://reviewboard.mozilla.org/r/124128/#review126620

MozReview can't track correct patch series..

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851894 [details]
Bug 1343589 - Add tests that the playback rate is updated silently

https://reviewboard.mozilla.org/r/124130/#review126622

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851895 [details]
Bug 1343589 - Add a test that reverse() updates animations on the compositor

https://reviewboard.mozilla.org/r/124132/#review126624

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851868 [details]
Bug 1343589 - Add comments to test_animation_observers_[a]sync.html to describe how they differ

https://reviewboard.mozilla.org/r/124094/#review126626

Thanks! This is really nice.
I am suspecting there must remain test cases that we can write as synchronous test in test_animation_observers_async.html (actually I did give up rewriting it before) though.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851869 [details]
Bug 1343589 - Move setupSynchronousObserver from testcommon.js to test_animation_observers_sync.html

https://reviewboard.mozilla.org/r/124096/#review126628

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851867 [details]
Bug 1343589 - Rename animation observer test files

https://reviewboard.mozilla.org/r/124092/#review126630

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851865 [details]
Bug 1343589 - Restore the playbackRate when reverse() throws.

https://reviewboard.mozilla.org/r/124088/#review126632

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851865 [details]
Bug 1343589 - Restore the playbackRate when reverse() throws.

https://reviewboard.mozilla.org/r/124088/#review126634

Comment 48

[Brian Birtles (:birtles)]

Thanks!

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851866 [details]
Bug 1343589 - Add animation mutation observer test for reverse() when it throws an exception

https://reviewboard.mozilla.org/r/124090/#review126636

::: dom/animation/test/chrome/test_animation_observers_sync.html:771
(Diff revision 3)
> +    try {
> +      animations[0].reverse();
> +    } catch(e) {
> +      threw = true;
> +    }
> +    assert_true(threw,

Nit: We can use assert_throws() instead?

Comment 50

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> Comment on attachment 8851865 [details]
> Bug 1343589 - Restore the playbackRate when reverse() throws.
> 
> https://reviewboard.mozilla.org/r/124088/#review126632

(In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> Comment on attachment 8851865 [details]
> Bug 1343589 - Restore the playbackRate when reverse() throws.
> 
> https://reviewboard.mozilla.org/r/124088/#review126634

Sorry. I did stamp r+ again and again...

Comment 51

[Brian Birtles (:birtles)]

Comment on attachment 8851866 [details]
Bug 1343589 - Add animation mutation observer test for reverse() when it throws an exception

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124090/diff/3-4/

Comment 52

[Brian Birtles (:birtles)]

Comment on attachment 8851870 [details]
Bug 1343589 - Add a crashtest for when reverse() throws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124098/diff/3-4/

Comment 53

[Brian Birtles (:birtles)]

Comment on attachment 8851893 [details]
Bug 1343589 - Add a test for reversing with an infinite target effect end and playback rate of zero

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124128/diff/2-3/

Comment 54

[Brian Birtles (:birtles)]

Comment on attachment 8851894 [details]
Bug 1343589 - Add tests that the playback rate is updated silently

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124130/diff/2-3/

Comment 55

[Brian Birtles (:birtles)]

Comment on attachment 8851895 [details]
Bug 1343589 - Add a test that reverse() updates animations on the compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124132/diff/2-3/

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/758adbef7005
Move tests for reversing an animation to the timing-model folder r=hiro
https://hg.mozilla.org/integration/autoland/rev/40cef2d0caa9
Add a test that the playback rate is unaffected when an exception is thrown r=hiro
https://hg.mozilla.org/integration/autoland/rev/f49062ce46dd
Restore the playbackRate when reverse() throws. r=hiro
https://hg.mozilla.org/integration/autoland/rev/5f321e0d488a
Rename animation observer test files r=hiro
https://hg.mozilla.org/integration/autoland/rev/57c53ff1d982
Add comments to test_animation_observers_[a]sync.html to describe how they differ r=hiro
https://hg.mozilla.org/integration/autoland/rev/04bb9a2a3edd
Move setupSynchronousObserver from testcommon.js to test_animation_observers_sync.html r=hiro
https://hg.mozilla.org/integration/autoland/rev/9ae2c5b72a57
Add animation mutation observer test for reverse() when it throws an exception r=hiro
https://hg.mozilla.org/integration/autoland/rev/198b57c1d07b
Add a crashtest for when reverse() throws r=hiro
https://hg.mozilla.org/integration/autoland/rev/8964f29f8d0e
Add a test for reversing with an infinite target effect end and playback rate of zero r=hiro
https://hg.mozilla.org/integration/autoland/rev/d0ec0bc4f7aa
Add tests that the playback rate is updated silently r=hiro
https://hg.mozilla.org/integration/autoland/rev/80826659caa9
Add a test that reverse() updates animations on the compositor r=hiro

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/758adbef7005
https://hg.mozilla.org/mozilla-central/rev/40cef2d0caa9
https://hg.mozilla.org/mozilla-central/rev/f49062ce46dd
https://hg.mozilla.org/mozilla-central/rev/5f321e0d488a
https://hg.mozilla.org/mozilla-central/rev/57c53ff1d982
https://hg.mozilla.org/mozilla-central/rev/04bb9a2a3edd
https://hg.mozilla.org/mozilla-central/rev/9ae2c5b72a57
https://hg.mozilla.org/mozilla-central/rev/198b57c1d07b
https://hg.mozilla.org/mozilla-central/rev/8964f29f8d0e
https://hg.mozilla.org/mozilla-central/rev/d0ec0bc4f7aa
https://hg.mozilla.org/mozilla-central/rev/80826659caa9

Comment 58

[Ryan VanderMeulen [:RyanVM]]

How far back does this issue go?

Comment 59

[Brian Birtles (:birtles)]

It's a (spec) problem with reverse() which we implemented in bug 1150808, i.e. Firefox 42.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Can this ride the trains or is it something we need to be tracking for possible backport?

Comment 61

[Brian Birtles (:birtles)]

I think this is fine to ride the trains. While it's a Web-facing change, it's definitely an edge case and this API doesn't have terribly wide adoption yet.