Bug 1291413 - Fix assertion when resuming an Animation with both a start time and hold time; r=hiro
authorBrian Birtles <birtles@gmail.com>
Thu, 05 Oct 2017 10:50:38 +0900
changeset 384641 843d4559d5916e0548c4bc6cd85238fa187cf63f
parent 384640 edb7fa9379c7a4464a9624948a3da7d3b73bf902
child 384642 85b73d4c4c9cf8792096b16db0c5a711b1e0ee24
push id52705
push userbbirtles@mozilla.com
push dateThu, 05 Oct 2017 04:59:44 +0000
treeherderautoland@843d4559d591 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1291413
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
dom/animation/Animation.cpp
dom/animation/test/crashtests/1291413-1.html
dom/animation/test/crashtests/1291413-2.html
dom/animation/test/crashtests/crashtests.list
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -1183,19 +1183,19 @@ Animation::PauseNoUpdate(ErrorResult& aR
 void
 Animation::ResumeAt(const TimeDuration& aReadyTime)
 {
   // This method is only expected to be called for an animation that is
   // waiting to play. We can easily adapt it to handle other states
   // 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.
   if (mStartTime.IsNull()) {
     mStartTime = StartTimeFromReadyTime(aReadyTime);
     if (mPlaybackRate != 0) {
       mHoldTime.SetNull();
     }
new file mode 100644
--- /dev/null
+++ b/dom/animation/test/crashtests/1291413-1.html
@@ -0,0 +1,20 @@
+<!doctype html>
+<html class=reftest-wait>
+<script>
+window.onload = () => {
+  const div = document.createElement('div');
+
+  document.documentElement.appendChild(div);
+  const anim = div.animate([], 1000);
+
+  anim.ready.then(() => {
+    anim.pause();
+    anim.reverse();
+    anim.playbackRate = 0;
+    anim.ready.then(() => {
+      document.documentElement.className = '';
+    });
+  });
+};
+</script>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/animation/test/crashtests/1291413-2.html
@@ -0,0 +1,21 @@
+<!doctype html>
+<html class=reftest-wait>
+<script>
+window.onload = () => {
+  const div = document.createElement('div');
+
+  document.documentElement.appendChild(div);
+  const anim = div.animate([], 1000);
+
+  anim.ready.then(() => {
+    anim.pause();
+    anim.reverse();
+    anim.playbackRate = 0;
+    anim.playbackRate = 1;
+    anim.ready.then(() => {
+      document.documentElement.className = '';
+    });
+  });
+};
+</script>
+</html>
--- a/dom/animation/test/crashtests/crashtests.list
+++ b/dom/animation/test/crashtests/crashtests.list
@@ -5,16 +5,18 @@ pref(dom.animations-api.core.enabled,tru
 pref(dom.animations-api.core.enabled,true) load 1216842-3.html
 pref(dom.animations-api.core.enabled,true) load 1216842-4.html
 pref(dom.animations-api.core.enabled,true) load 1216842-5.html
 pref(dom.animations-api.core.enabled,true) load 1216842-6.html
 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
 pref(dom.animations-api.core.enabled,true) load 1322291-2.html
 pref(dom.animations-api.core.enabled,true) load 1323114-1.html
 pref(dom.animations-api.core.enabled,true) load 1323114-2.html
 pref(dom.animations-api.core.enabled,true) load 1324554-1.html
 pref(dom.animations-api.core.enabled,true) load 1325193-1.html