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

--HG--
extra : moz-landing-system : lando

dom/animation/DocumentTimeline.cpp

@@ -188,13 +188,6 @@ void DocumentTimeline::MostRecentRefreshTimeUpdated() {
 }
 
 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();
 }
 

layout/base/nsRefreshDriver.cpp

@@ -1897,6 +1897,26 @@ void nsRefreshDriver::Tick(VsyncId aId, TimeStamp aNowTime) {
       }
     }
 
+    // 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.
 

testing/web-platform/tests/web-animations/timing-model/timelines/update-and-send-events.html

@@ -223,4 +223,35 @@ promise_test(async t => {
 }, '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>