script: fix spurious animation checks to correctly invoke rAF callbacks (#35387)

After running an `rAF` callback, if no new callbacks were registered, we
send a `NoAnimationFramesCallback` to the compositor to stop ticking
animations using video refresh callbacks. This interacts badly with the
mechanism to track spurious animations frames i.e. rAF callbacks that
don't mutate the DOM. Such 'faked' rAF callbacks are triggered by
registering a oneshot timer instead of the compositor callback.

The compositor's refresh callback is never enabled back again once a
non-spurious rAF callback runs and registers a new rAF callback. If the
former callback resets the `spurious_animations_frames` counter, then when
the latter rAF callback runs, it will not schedule a OneShotTimer timer
for any rAF callback that itself registers, since the counter was reset
previously. Hence that third rAF callback that never runs as it relies
on the compsitor's refresh callback, which was disabled previously.

The current logic also doesn't actually recognize spurious animation
frames because the `spurious_animations_frames` counter is updated at
the end of the `run_the_animation_frame_callbacks`, effectively meaning
`was_faking_animation_frames` and `self.is_faking_animation_frames` will
always be the same value but the logic effectively only runs when
`(!was_faking && is_faking)` is true.

This patch fixes the logic to detect spurious animations frames by
moving logic to update the counter to be before the check for spurious
frames. It also ensures that the compositor's refesh callbacks is
re-enabled once we see a non-spurious callback.


Fixes #35386

Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>

components/script/dom/document.rs

@@ -2245,14 +2245,7 @@ impl Document {
         // If we are running 'fake' animation frames, we unconditionally
         // set up a one-shot timer for script to execute the rAF callbacks.
         if self.is_faking_animation_frames() && !self.window().throttled() {
-            warn!("Scheduling fake animation frame. Animation frames tick too fast.");
+            self.schedule_fake_animation_frame();
-            let callback = FakeRequestAnimationFrameCallback {
-                document: Trusted::new(self),
-            };
-            self.global().schedule_callback(
-                OneshotTimerCallback::FakeRequestAnimationFrame(callback),
-                Duration::from_millis(FAKE_REQUEST_ANIMATION_FRAME_DELAY),
-            );
         } else if !self.running_animation_callbacks.get() {
             // No need to send a `ChangeRunningAnimationsState` if we're running animation callbacks:
             // we're guaranteed to already be in the "animation callbacks present" state.
@@ -2276,6 +2269,17 @@ impl Document {
         }
     }
 
+    fn schedule_fake_animation_frame(&self) {
+        warn!("Scheduling fake animation frame. Animation frames tick too fast.");
+        let callback = FakeRequestAnimationFrameCallback {
+            document: Trusted::new(self),
+        };
+        self.global().schedule_callback(
+            OneshotTimerCallback::FakeRequestAnimationFrame(callback),
+            Duration::from_millis(FAKE_REQUEST_ANIMATION_FRAME_DELAY),
+        );
+    }
+
     /// <https://html.spec.whatwg.org/multipage/#run-the-animation-frame-callbacks>
     pub(crate) fn run_the_animation_frame_callbacks(&self) {
         let _realm = enter_realm(self);
@@ -2317,6 +2321,15 @@ impl Document {
             self.set_needs_paint(true);
         }
 
+        // Update the counter of spurious animation frames.
+        let spurious_frames = self.spurious_animation_frames.get();
+        if callbacks_did_not_trigger_reflow && spurious_frames < SPURIOUS_ANIMATION_FRAME_THRESHOLD
+        {
+            self.spurious_animation_frames.set(spurious_frames + 1);
+        } else {
+            self.spurious_animation_frames.set(0);
+        }
+
         // Only send the animation change state message after running any callbacks.
         // This means that if the animation callback adds a new callback for
         // the next frame (which is the common case), we won't send a NoAnimationCallbacksPresent
@@ -2326,7 +2339,9 @@ impl Document {
         // constellation to stop giving us video refresh callbacks, to save energy. (A spurious
         // animation frame is one in which the callback did not mutate the DOM—that is, an
         // animation frame that wasn't actually used for animation.)
-        if is_empty || (!was_faking_animation_frames && self.is_faking_animation_frames()) {
+        let just_crossed_spurious_animation_threshold =
+            !was_faking_animation_frames && self.is_faking_animation_frames();
+        if is_empty || just_crossed_spurious_animation_threshold {
             if is_empty {
                 // If the current animation frame list in the DOM instance is empty,
                 // we can reuse the original `Vec<T>` that we put on the stack to
@@ -2336,6 +2351,14 @@ impl Document {
                     &mut *self.animation_frame_list.borrow_mut(),
                     &mut *animation_frame_list,
                 );
+            } else if just_crossed_spurious_animation_threshold {
+                // We just realized that we need to stop requesting compositor's animation ticks
+                // due to spurious animation frames, but we still have rAF callbacks queued. Since
+                // `is_faking_animation_frames` would not have been true at the point where these
+                // new callbacks were registered, the one-shot timer will not have been setup in
+                // `request_animation_frame()`. Since we stop the compositor ticks below, we need
+                // to expliclty trigger a OneshotTimerCallback for these queued callbacks.
+                self.schedule_fake_animation_frame();
             }
             let event = ScriptMsg::ChangeRunningAnimationsState(
                 AnimationState::NoAnimationCallbacksPresent,
@@ -2343,14 +2366,13 @@ impl Document {
             self.window().send_to_constellation(event);
         }
 
-        // Update the counter of spurious animation frames.
+        // If we were previously faking animation frames, we need to re-enable video refresh
-        if callbacks_did_not_trigger_reflow {
+        // callbacks when we stop seeing spurious animation frames.
-            if self.spurious_animation_frames.get() < SPURIOUS_ANIMATION_FRAME_THRESHOLD {
+        if was_faking_animation_frames && !self.is_faking_animation_frames() && !is_empty {
-                self.spurious_animation_frames
+            self.window()
-                    .set(self.spurious_animation_frames.get() + 1)
+                .send_to_constellation(ScriptMsg::ChangeRunningAnimationsState(
-            }
+                    AnimationState::AnimationCallbacksPresent,
-        } else {
+                ));
-            self.spurious_animation_frames.set(0)
         }
     }
 

tests/wpt/meta/MANIFEST.json

@@ -729584,6 +729584,13 @@
         null,
         {}
        ]
+      ],
+      "spurious-frame-callbacks-optimization.html": [
+       "b4c582cd5edce8f92a2da0b44131a8dbdff60f11",
+       [
+        null,
+        {}
+       ]
       ]
      },
      "atob": {

tests/wpt/tests/html/webappapis/animation-frames/spurious-frame-callbacks-optimization.html

@@ -0,0 +1,41 @@
+<!DOCTYPE html>
+<head>
+  <title>Optimization of requestAnimationFrame callbacks that don't modify the DOM shouldn't break animations</title>
+  <link rel="author" title="Mukilan Thiyagarajan" href="mailto:mukilan@igalia.com">
+  <script src="/resources/testharness.js"></script>
+  <script src="/resources/testharnessreport.js"></script>
+</head>
+<body>
+    <div id="target">0</div>
+</body>
+<script>
+"use strict";
+let frame = 0;
+const draw = (t) => {
+  frame += 1;
+  if (frame < 11) {
+    // Don't mutate the DOM for 10 frames to meet the threshold for Servo's
+    // spurious frame optimization to kick in.
+    requestAnimationFrame(draw);
+  } else if (frame == 11) {
+    // Don't schedule next rAF so the compositor's tick is disabled.
+    // This is specific to Servo as the spurious frame detection at the
+    // time of this test was broken.
+    setTimeout(() => {
+      requestAnimationFrame(draw);
+    }, 10);
+  } else {
+    // Normal frames.
+    document.getElementById('target').innerText = t;
+    requestAnimationFrame(draw);
+  }
+};
+
+async_test(function(test) {
+  let target = document.getElementById('target');
+  setTimeout(test.step_func_done(() => {
+    assert_greater_than(parseInt(target.innerText), 500);
+  }), 550);
+  requestAnimationFrame(draw);
+});
+</script>