Bug 1253476 - Run microtask checkpoint for updating timing after updating all timelines; r=hiro
authorBrian Birtles <birtles@gmail.com>
Mon, 20 May 2019 05:22:03 +0000
changeset 474485 0e25f7b0790b67bcd5614384def4f7158aa3048f
parent 474484 861caf0b6714f5a119f04ba3e1e9badf44986e60
child 474486 a80ca3b22049b0a8a4cf7f8a8c1e3291d434ef3e
push id36040
push userrgurzau@mozilla.com
push dateMon, 20 May 2019 13:43:21 +0000
treeherdermozilla-central@319a369ccde4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1253476
milestone68.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 1253476 - Run microtask checkpoint for updating timing after updating all timelines; r=hiro According to the procedure to update animations and send events[1] the UA should update all timelines first and _then_ run a microtask checkpoint. As a result, when we run callbacks for the finished promise on an Animation they should see the fully up-to-date state of all animations, regardless of which timeline they are attached to. However, that is currently not the case since we run a microtask checkpoint after updating each individual timeline. This difference will become more significant later in this patch series when we introduce another step--removing replaced animations--that _also_ should happen before we run the microtask checkpoint (so that the promise callbacks always see a fully-up-to-date state). This patch makes our handling a little more in line with the spec. It's not quite the same because it's possible there may be other refresh driver observers that trigger a microtask checkpoint in between ticking the different timelines but that case is expected to be rare and fixing it would require maintaining a separate queue for timeline observers that we run after all other observers-- so it is probably not necessary to fix that case at this stage. The test added in this patch fails without the code changes in this patch. [1] https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events Differential Revision: https://phabricator.services.mozilla.com/D30319
dom/animation/DocumentTimeline.cpp
layout/base/nsRefreshDriver.cpp
testing/web-platform/tests/web-animations/timing-model/timelines/update-and-send-events.html
--- a/dom/animation/DocumentTimeline.cpp
+++ b/dom/animation/DocumentTimeline.cpp
@@ -183,23 +183,16 @@ void DocumentTimeline::MostRecentRefresh
     // of mDocument's PresShell.
     MOZ_ASSERT(GetRefreshDriver(),
                "Refresh driver should still be valid at end of WillRefresh");
     UnregisterFromRefreshDriver();
   }
 }
 
 void DocumentTimeline::WillRefresh(mozilla::TimeStamp aTime) {
-  // https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events,
-  // step2.
-  // Note that this should be done before nsAutoAnimationMutationBatch which is
-  // inside MostRecentRefreshTimeUpdated().  If PerformMicroTaskCheckpoint was
-  // called before nsAutoAnimationMutationBatch is destroyed, some mutation
-  // records might not be delivered in this checkpoint.
-  nsAutoMicroTask mt;
   MostRecentRefreshTimeUpdated();
 }
 
 void DocumentTimeline::NotifyTimerAdjusted(TimeStamp aTime) {
   MostRecentRefreshTimeUpdated();
 }
 
 void DocumentTimeline::ObserveRefreshDriver(nsRefreshDriver* aDriver) {
--- a/layout/base/nsRefreshDriver.cpp
+++ b/layout/base/nsRefreshDriver.cpp
@@ -1892,16 +1892,36 @@ void nsRefreshDriver::Tick(VsyncId aId, 
       obs->WillRefresh(aNowTime);
 
       if (!mPresContext || !mPresContext->GetPresShell()) {
         StopTimer();
         return;
       }
     }
 
+    // Any animation timelines updated above may cause animations to queue
+    // Promise resolution microtasks. We shouldn't run these, however, until we
+    // have fully updated the animation state.
+    //
+    // However, we need to be sure to run these before dispatching the
+    // corresponding animation events as required by the spec[1].
+    //
+    // [1]
+    // https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events
+    if (i == 1) {
+      nsAutoMicroTask mt;
+    }
+
+    // Check if running the microtask checkpoint caused the pres context to
+    // be destroyed.
+    if (i == 1 && (!mPresContext || !mPresContext->GetPresShell())) {
+      StopTimer();
+      return;
+    }
+
     if (i == 1) {
       // This is the FlushType::Style case.
 
       DispatchScrollEvents();
       DispatchVisualViewportScrollEvents();
       DispatchAnimationEvents();
       RunFullscreenSteps();
       RunFrameRequestCallbacks(aNowTime);
--- a/testing/web-platform/tests/web-animations/timing-model/timelines/update-and-send-events.html
+++ b/testing/web-platform/tests/web-animations/timing-model/timelines/update-and-send-events.html
@@ -218,9 +218,40 @@ promise_test(async t => {
 
   assert_array_equals(receivedEvents.map(event => event.type),
     [ 'finish', 'cancel' ],
     'Calling finish() synchronously queues a finish event when updating the ' +
     'finish state so it should appear before the cancel event');
 }, 'Playback events with the same timeline retain the order in which they are' +
    'queued');
 
+promise_test(async t => {
+  const div = createDiv(t);
+
+  // Create two animations with separate timelines
+
+  const timelineA = document.timeline;
+  const animA = div.animate(null, 100 * MS_PER_SEC);
+
+  const timelineB = new DocumentTimeline();
+  const animB = new Animation(
+    new KeyframeEffect(div, null, 100 * MS_PER_SEC),
+    timelineB
+  );
+  animB.play();
+
+  animA.currentTime = 99.9 * MS_PER_SEC;
+  animB.currentTime = 99.9 * MS_PER_SEC;
+
+  // When the next tick happens both animations should be updated, and we will
+  // notice that they are now finished. As a result their finished promise
+  // callbacks should be queued. All of that should happen before we run the
+  // next microtask checkpoint and actually run the promise callbacks and
+  // hence the calls to cancel should not stop the existing callbacks from
+  // being run.
+
+  animA.finished.then(() => { animB.cancel() });
+  animB.finished.then(() => { animA.cancel() });
+
+  await Promise.all([animA.finished, animB.finished]);
+}, 'All timelines are updated before running microtasks');
+
 </script>