Bug 2001590 - [devtools] Filter out AnimationPlayerActor instances that aren't handled by AnimationsActor. r=devtools-reviewers,bomsy.

`AnimationActor` `pauseSome`, `playSome`, `setCurrentTimes` and `setPlaybackRates`
all take an array of `AnimationPlayerActor` instances for which we'll perform
some operation on their underlying animation.
It can happen that some of those actors are not handled by the `AnimationsActor`
anymore (e.g. the animation stopped, but the client wasn't updated yet).

Performing any action on those unhandled player (pausing, setting time, …) might
actually trigger a new, unwanted, mutation, which will cause both the server
state and the client state to be out of date.

This patch filters out actors which are not into `AnimationsActor#actors` to avoid
such issue.

Differential Revision: https://phabricator.services.mozilla.com/D273562

Author: nchevobbe

devtools/server/actors/animation.js

@@ -829,11 +829,20 @@ exports.AnimationsActor = class AnimationsActor extends Actor {
    * @param {Array} actors A list of AnimationPlayerActor.
    */
   pauseSome(actors) {
-    for (const { player } of actors) {
-      this.pauseSync(player);
+    const handledActors = [];
+    for (const actor of actors) {
+      // The client could call this with actors that we no longer handle, as it might
+      // not have received the mutations event yet for removed animations.
+      // In such case, ignore the actor, as pausing the animation again might trigger a
+      // new mutation, which would cause problems here and on the client.
+      if (!this.actors.includes(actor)) {
+        continue;
+      }
+      this.pauseSync(actor.player);
+      handledActors.push(actor);
     }
 
-    return this.waitForNextFrame(actors);
+    return this.waitForNextFrame(handledActors);
   }
 
   /**
@@ -842,22 +851,39 @@ exports.AnimationsActor = class AnimationsActor extends Actor {
    * @param {Array} actors A list of AnimationPlayerActor.
    */
   playSome(actors) {
-    for (const { player } of actors) {
-      this.playSync(player);
+    const handledActors = [];
+    for (const actor of actors) {
+      // The client could call this with actors that we no longer handle, as it might
+      // not have received the mutations event yet for removed animations.
+      // In such case, ignore the actor, as playing the animation again might trigger a
+      // new mutation, which would cause problems here and on the client.
+      if (!this.actors.includes(actor)) {
+        continue;
+      }
+      this.playSync(actor.player);
+      handledActors.push(actor);
     }
 
-    return this.waitForNextFrame(actors);
+    return this.waitForNextFrame(handledActors);
   }
 
   /**
    * Set the current time of several animations at the same time.
    *
-   * @param {Array} players A list of AnimationPlayerActor.
+   * @param {Array} actors A list of AnimationPlayerActor.
    * @param {number} time The new currentTime.
    * @param {boolean} shouldPause Should the players be paused too.
    */
-  setCurrentTimes(players, time, shouldPause) {
-    for (const actor of players) {
+  setCurrentTimes(actors, time, shouldPause) {
+    const handledActors = [];
+    for (const actor of actors) {
+      // The client could call this with actors that we no longer handle, as it might
+      // not have received the mutations event yet for removed animations.
+      // In such case, ignore the actor, as setting the time might trigger a
+      // new mutation, which would cause problems here and on the client.
+      if (!this.actors.includes(actor)) {
+        continue;
+      }
       const player = actor.player;
 
       if (shouldPause) {
@@ -869,9 +895,10 @@ exports.AnimationsActor = class AnimationsActor extends Actor {
           ? time - actor.createdTime
           : actor.createdTime - time;
       player.currentTime = currentTime * Math.abs(player.playbackRate);
+      handledActors.push(actor);
     }
 
-    return this.waitForNextFrame(players);
+    return this.waitForNextFrame(handledActors);
   }
 
   /**
@@ -880,13 +907,20 @@ exports.AnimationsActor = class AnimationsActor extends Actor {
    * @param {Array} actors A list of AnimationPlayerActor.
    * @param {number} rate The new rate.
    */
-  setPlaybackRates(players, rate) {
-    return Promise.all(
-      players.map(({ player }) => {
-        player.updatePlaybackRate(rate);
-        return player.ready;
-      })
-    );
+  setPlaybackRates(actors, rate) {
+    const readyPromises = [];
+    for (const actor of actors) {
+      // The client could call this with actors that we no longer handle, as it might
+      // not have received the mutations event yet for removed animations.
+      // In such case, ignore the actor, as setting the playback rate might trigger a
+      // new mutation, which would cause problems here and on the client.
+      if (!this.actors.includes(actor)) {
+        continue;
+      }
+      actor.player.updatePlaybackRate(rate);
+      readyPromises.push(actor.player.ready);
+    }
+    return Promise.all(readyPromises);
   }
 
   /**

devtools/server/tests/browser/browser.toml

@@ -112,6 +112,8 @@ skip-if = [
 
 ["browser_animation_simple.js"]
 
+["browser_animation_unhandledActorPlayers.js"]
+
 ["browser_animation_updatedState.js"]
 
 ["browser_application_manifest.js"]

devtools/server/tests/browser/browser_animation_unhandledActorPlayers.js

@@ -0,0 +1,86 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Check that calling AnimationsActor method taking AnimationPlayerActor arrays (pauseSome,
+// playSome, setCurrentTimes, setPlaybackRates) with instances that are not handled by
+// the AnimationsActor anymore doesn't throw nor trigger unexpected animations (see Bug 2001590).
+
+add_task(async function () {
+  const { target, walker, animations } = await initAnimationsFrontForUrl(
+    `data:text/html,<meta charset=utf8>${encodeURIComponent(`
+    <style>
+      #target {
+        animation: my-anim 1s infinite alternate;
+
+        &.still {
+          animation: none;
+        }
+      }
+      @keyframes my-anim {
+        to {
+          background-color: tomato;
+        }
+      }
+    </style>
+    <div id=target>Hello</div>`)}`
+  );
+
+  info("Retrieve an animated node");
+  const node = await walker.querySelector(walker.rootNode, "#target");
+
+  const getAnimationPlayersForTargetNode = () =>
+    animations.getAnimationPlayersForNode(node);
+
+  info("Retrieve the animation player for the node");
+  const players = await getAnimationPlayersForTargetNode();
+  is(players.length, 1, "Got one animation player");
+  const animationPlayer = players[0];
+
+  info("Stop the animation on the node");
+  await node.modifyAttributes([
+    {
+      attributeName: "class",
+      newValue: "still",
+    },
+  ]);
+
+  // Wait until we're not getting the animation anymore
+  await waitFor(async () => {
+    return (await getAnimationPlayersForTargetNode()).length === 0;
+  });
+
+  info("Call methodes with outdated animationplayer front");
+  const onPause = animations.pauseSome([animationPlayer]);
+  const onPlay = animations.playSome([animationPlayer]);
+  const onCurrentTimeSet = animations.setCurrentTimes(
+    [animationPlayer],
+    1,
+    true
+  );
+  const onPlaybackRateSet = animations.setPlaybackRates([animationPlayer], 10);
+
+  await onPause;
+  ok(true, "pauseSome succeeded");
+
+  await onPlay;
+  ok(true, "playSome succedded");
+
+  await onCurrentTimeSet;
+  ok(true, "setCurrentTimes succedded");
+
+  await onPlaybackRateSet;
+  ok(true, "setPlaybackRates succedded");
+
+  // wait for a bit so we would get notified about new animations
+  await wait(500);
+  is(
+    (await getAnimationPlayersForTargetNode()).length,
+    0,
+    "No players were created after calling those methods"
+  );
+
+  await target.destroy();
+  gBrowser.removeCurrentTab();
+});