fix(editor): fix AnimationController scheduling race conds (#11678)

packages/excalidraw/renderer/animation.ts

@@ -5,39 +5,60 @@ export type Animation<R extends object> = (params: {
   state?: R;
 }) => R | null | undefined;
 
+type AnimationRecord = {
+  animation: Animation<any>;
+  lastTime: number;
+  state: any;
+};
+
 export class AnimationController {
   private static scheduledFrame:
     | { id: ReturnType<typeof requestAnimationFrame>; type: "raf" }
     | { id: ReturnType<typeof setTimeout>; type: "timeout" }
     | null = null;
-  private static animations = new Map<
-    string,
-    {
-      animation: Animation<any>;
-      lastTime: number;
-      state: any;
-    }
-  >();
+  private static animations = new Map<string, AnimationRecord>();
 
   static start<R extends object>(key: string, animation: Animation<R>) {
     if (AnimationController.animations.has(key)) {
       return;
     }
 
-    const initialState = animation({
-      deltaTime: 0,
+    const record: AnimationRecord = {
+      animation,
+      lastTime: 0,
       state: undefined,
-    });
+    };
+    AnimationController.animations.set(key, record);
 
-    if (initialState) {
-      AnimationController.animations.set(key, {
-        animation,
-        lastTime: 0,
-        state: initialState,
+    let initialState: R | null | undefined;
+    try {
+      initialState = animation({
+        deltaTime: 0,
+        state: undefined,
       });
-
-      AnimationController.scheduleNextFrame();
+    } catch (error) {
+      if (AnimationController.animations.get(key) === record) {
+        AnimationController.animations.delete(key);
+        AnimationController.cancelScheduledFrameIfIdle();
+      }
+      throw error;
     }
+
+    // The initial callback may synchronously cancel this animation or replace
+    // it with another animation under the same key. Never resurrect or
+    // overwrite it after control returns.
+    if (AnimationController.animations.get(key) !== record) {
+      return;
+    }
+
+    if (!initialState) {
+      AnimationController.animations.delete(key);
+      AnimationController.cancelScheduledFrameIfIdle();
+      return;
+    }
+
+    record.state = initialState;
+    AnimationController.scheduleNextFrame();
   }
 
   private static scheduleNextFrame() {
@@ -85,7 +106,15 @@ export class AnimationController {
     AnimationController.scheduledFrame = null;
 
     if (AnimationController.animations.size > 0) {
-      for (const [key, animation] of AnimationController.animations) {
+      // A callback may synchronously add, cancel, or replace animations. Work
+      // from the frame's starting set so newly started animations begin on the
+      // next frame and every record runs at most once per tick.
+      const animations = [...AnimationController.animations];
+      for (const [key, animation] of animations) {
+        if (AnimationController.animations.get(key) !== animation) {
+          continue;
+        }
+
         const now = performance.now();
         const deltaTime =
           animation.lastTime === 0 ? 0 : now - animation.lastTime;
@@ -95,6 +124,12 @@ export class AnimationController {
           state: animation.state,
         });
 
+        // The callback may have cancelled or replaced itself. Only the record
+        // that was invoked is allowed to update or remove its registration.
+        if (AnimationController.animations.get(key) !== animation) {
+          continue;
+        }
+
         if (!state) {
           AnimationController.animations.delete(key);
 

packages/excalidraw/tests/animation.test.ts

@@ -70,4 +70,98 @@ describe("AnimationController", () => {
     expect(secondFrames).toBe(1);
     expect(vi.getTimerCount()).toBe(0);
   });
+
+  it("does not resurrect an animation cancelled during its initial callback", () => {
+    let frames = 0;
+
+    AnimationController.start(FIRST_KEY, () => {
+      frames++;
+      AnimationController.cancel(FIRST_KEY);
+      return { keep: true };
+    });
+
+    expect(frames).toBe(1);
+    expect(AnimationController.running(FIRST_KEY)).toBe(false);
+    expect(vi.getTimerCount()).toBe(0);
+  });
+
+  it("cleans up the registration when the initial callback throws", () => {
+    expect(() =>
+      AnimationController.start(FIRST_KEY, () => {
+        throw new Error("initial frame failed");
+      }),
+    ).toThrow("initial frame failed");
+
+    expect(AnimationController.running(FIRST_KEY)).toBe(false);
+    expect(vi.getTimerCount()).toBe(0);
+  });
+
+  it("preserves a same-key replacement started during the initial callback", async () => {
+    let originalFrames = 0;
+    let replacementFrames = 0;
+
+    const replacement = ({ state }: { state?: { keep: true } }) => {
+      replacementFrames++;
+      return state ? null : { keep: true as const };
+    };
+
+    AnimationController.start(FIRST_KEY, () => {
+      originalFrames++;
+      AnimationController.cancel(FIRST_KEY);
+      AnimationController.start(FIRST_KEY, replacement);
+      return { keep: true };
+    });
+
+    expect(originalFrames).toBe(1);
+    expect(replacementFrames).toBe(1);
+    expect(AnimationController.running(FIRST_KEY)).toBe(true);
+
+    await vi.runOnlyPendingTimersAsync();
+
+    expect(originalFrames).toBe(1);
+    expect(replacementFrames).toBe(2);
+    expect(AnimationController.running(FIRST_KEY)).toBe(false);
+  });
+
+  it("does not let a completed callback delete its same-key replacement", async () => {
+    // tests for this unwanted case:
+    //
+    // 1. The original animation’s scheduled callback begins.
+    // 2. Before it returns, application code cancels it and starts
+    //    a replacement using the same key.
+    // 3. The original callback returns null.
+    // 4. Previously, tick() then called delete(key), accidentally deleting
+    //    the replacement.
+
+    let originalFrames = 0;
+    let replacementFrames = 0;
+
+    const replacement = ({ state }: { state?: { keep: true } }) => {
+      replacementFrames++;
+      return state ? null : { keep: true as const };
+    };
+
+    AnimationController.start(FIRST_KEY, ({ state }) => {
+      if (!state) {
+        return { keep: true };
+      }
+
+      originalFrames++;
+      AnimationController.cancel(FIRST_KEY);
+      AnimationController.start(FIRST_KEY, replacement);
+      return null;
+    });
+
+    await vi.runOnlyPendingTimersAsync();
+
+    expect(originalFrames).toBe(1);
+    expect(replacementFrames).toBe(1);
+    expect(AnimationController.running(FIRST_KEY)).toBe(true);
+
+    await vi.runOnlyPendingTimersAsync();
+
+    expect(originalFrames).toBe(1);
+    expect(replacementFrames).toBe(2);
+    expect(AnimationController.running(FIRST_KEY)).toBe(false);
+  });
 });