From 3afa921d85893ba407c2a28bd4330474e5d67b5b Mon Sep 17 00:00:00 2001 From: Brian Birtles Date: Thu, 5 Oct 2017 10:50:38 +0900 Subject: [PATCH] Bug 1291413 - Fix assertion when resuming an Animation with both a start time and hold time; r=hiro When we set the playback rate to zero on a play-pending animation that is resuming from an aborted pause we can arrive in a state where both the start time and hold time are resolved. However, we previously added an assertion that only one of these is ever set at a time. Part of the assertion is warranted since that method contains the following code: if (mStartTime.IsNull()) { mStartTime = StartTimeFromReadyTime(aReadyTime); if (mPlaybackRate != 0) { mHoldTime.SetNull(); } } Here StartTimeFromReadyTime requires a non-null hold time. So either mStartTime or mHoldTime needs to be non-null. The requirement that only one or the other be non-null, however, is not in the spec and not necessary (as the test cases in this bug show). What this assertion does bring to light, however, is that in the case where we have *both* the start time and the hold time, we need to consider whether to use the start time as-is, or calculate it from the hold time. I have filed the following spec issue for this: https://github.com/w3c/web-animations/issues/200 MozReview-Commit-ID: CTCT7Up1E5n --HG-- extra : rebase_source : 95233f7cd2bc3c4bcc56615d8387fe54852986c1 --- dom/animation/Animation.cpp | 4 ++-- dom/animation/test/crashtests/1291413-1.html | 20 ++++++++++++++++++ dom/animation/test/crashtests/1291413-2.html | 21 +++++++++++++++++++ dom/animation/test/crashtests/crashtests.list | 2 ++ 4 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 dom/animation/test/crashtests/1291413-1.html create mode 100644 dom/animation/test/crashtests/1291413-2.html diff --git a/dom/animation/Animation.cpp b/dom/animation/Animation.cpp index 66231a99453d..eabb70495ab5 100644 --- a/dom/animation/Animation.cpp +++ b/dom/animation/Animation.cpp @@ -1188,9 +1188,9 @@ Animation::ResumeAt(const TimeDuration& aReadyTime) // but it's currently not necessary. MOZ_ASSERT(mPendingState == PendingState::PlayPending, "Expected to resume a play-pending animation"); - MOZ_ASSERT(mHoldTime.IsNull() != mStartTime.IsNull(), + MOZ_ASSERT(!mHoldTime.IsNull() || !mStartTime.IsNull(), "An animation in the play-pending state should have either a" - " resolved hold time or resolved start time (but not both)"); + " resolved hold time or resolved start time"); // If we aborted a pending pause operation we will already have a start time // we should use. In all other cases, we resolve it from the ready time. diff --git a/dom/animation/test/crashtests/1291413-1.html b/dom/animation/test/crashtests/1291413-1.html new file mode 100644 index 000000000000..691a746c6eed --- /dev/null +++ b/dom/animation/test/crashtests/1291413-1.html @@ -0,0 +1,20 @@ + + + + diff --git a/dom/animation/test/crashtests/1291413-2.html b/dom/animation/test/crashtests/1291413-2.html new file mode 100644 index 000000000000..fca3a938009c --- /dev/null +++ b/dom/animation/test/crashtests/1291413-2.html @@ -0,0 +1,21 @@ + + + + diff --git a/dom/animation/test/crashtests/crashtests.list b/dom/animation/test/crashtests/crashtests.list index 880c137fc17f..b5ec7ad3f0f4 100644 --- a/dom/animation/test/crashtests/crashtests.list +++ b/dom/animation/test/crashtests/crashtests.list @@ -10,6 +10,8 @@ pref(dom.animations-api.core.enabled,true) load 1272475-1.html pref(dom.animations-api.core.enabled,true) load 1272475-2.html pref(dom.animations-api.core.enabled,true) load 1278485-1.html pref(dom.animations-api.core.enabled,true) load 1277272-1.html +pref(dom.animations-api.core.enabled,true) load 1291413-1.html +pref(dom.animations-api.core.enabled,true) load 1291413-2.html pref(dom.animations-api.core.enabled,true) load 1304886-1.html pref(dom.animations-api.core.enabled,true) load 1322382-1.html pref(dom.animations-api.core.enabled,true) load 1322291-1.html