Closed Bug 1343589 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Animation, defect)

42 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: truber, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(12 files)

330 bytes, text/html
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
Attached file 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
Flags: in-testsuite?
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.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
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
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 on attachment 8851863 [details]
Bug 1343589 - Move tests for reversing an animation to the timing-model folder

https://reviewboard.mozilla.org/r/124084/#review126584
Attachment #8851863 - Flags: review?(hikezoe) → review+
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
Attachment #8851864 - Flags: review?(hikezoe) → review+
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?
Attachment #8851865 - Flags: review?(hikezoe) → review+
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. :-)
Attachment #8851866 - Flags: review?(hikezoe) → review+
Comment on attachment 8851867 [details]
Bug 1343589 - Rename animation observer test files

https://reviewboard.mozilla.org/r/124092/#review126594
Attachment #8851867 - Flags: review?(hikezoe) → review+
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
Attachment #8851868 - Flags: review?(hikezoe) → review+
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
Attachment #8851869 - Flags: review?(hikezoe) → review+
Comment on attachment 8851870 [details]
Bug 1343589 - Add a crashtest for when reverse() throws

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

Nice!
Attachment #8851870 - Flags: review?(hikezoe) → review+
(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.
Somehow I knew MozReview would get that wrong. And I want to put a facepalm emoji here but Bugzilla doesn't support emoji :/
Attachment #8851867 - Flags: review+ → review?(hikezoe)
Attachment #8851868 - Flags: review+ → review?(hikezoe)
Attachment #8851869 - Flags: review+ → review?(hikezoe)
Hang on, MozReview is about to have another fit as I go to update one of the commit messages here...
Attachment #8851866 - Flags: review+ → review?(hikezoe)
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..
Attachment #8851893 - Flags: review?(hikezoe) → review+
Comment on attachment 8851894 [details]
Bug 1343589 - Add tests that the playback rate is updated silently

https://reviewboard.mozilla.org/r/124130/#review126622
Attachment #8851894 - Flags: review?(hikezoe) → review+
Comment on attachment 8851895 [details]
Bug 1343589 - Add a test that reverse() updates animations on the compositor

https://reviewboard.mozilla.org/r/124132/#review126624
Attachment #8851895 - Flags: review?(hikezoe) → review+
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.
Attachment #8851868 - Flags: review?(hikezoe) → review+
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
Attachment #8851869 - Flags: review?(hikezoe) → review+
Comment on attachment 8851867 [details]
Bug 1343589 - Rename animation observer test files

https://reviewboard.mozilla.org/r/124092/#review126630
Attachment #8851867 - Flags: review?(hikezoe) → review+
Comment on attachment 8851865 [details]
Bug 1343589 - Restore the playbackRate when reverse() throws.

https://reviewboard.mozilla.org/r/124088/#review126632
Comment on attachment 8851865 [details]
Bug 1343589 - Restore the playbackRate when reverse() throws.

https://reviewboard.mozilla.org/r/124088/#review126634
Thanks!
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?
Attachment #8851866 - Flags: review?(hikezoe) → review+
(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...
Pushed by bbirtles@mozilla.com:
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
How far back does this issue go?
Flags: needinfo?(bbirtles)
Flags: in-testsuite?
Flags: in-testsuite+
It's a (spec) problem with reverse() which we implemented in bug 1150808, i.e. Firefox 42.
Flags: needinfo?(bbirtles)
Can this ride the trains or is it something we need to be tracking for possible backport?
Flags: needinfo?(bbirtles)
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.
Flags: needinfo?(bbirtles)
You need to log in before you can comment on or make changes to this bug.