Fix render callback start/stop race in C++ NativeAnimated (#57270)

Summary:
Pull Request resolved: #57270

The shared AnimationBackend path now coordinates render callback registration and publication with a small mutex-protected optional callback id. `startRenderCallbackIfNeeded` holds the lifecycle mutex across `AnimationBackend::start` and id publication because the backend registers before returning the id; `stopRenderCallbackIfNeeded` clears the published id under the same mutex before stopping that callback. This closes the window where a concurrent stop could observe no published id and leave the just-registered backend callback orphaned.

The existing non-shared platform callback path remains on the original atomic flag, preserving the avoid-calling-external-code-under-a-mutex behavior.

Changelog: [General][Fixed] - Fix a race in the C++ Animated render-callback lifecycle that could leave stale callbacks running on every frame

Reviewed By: zeyap

Differential Revision: D109009181

fbshipit-source-id: 09147d4272c98b1d298f241d8d1078697f35b979

Author: Bartlomiej Bloniarz

packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp

@@ -551,61 +551,84 @@ NativeAnimatedNodesManager::ensureEventEmitterListener() noexcept {
 }
 
 void NativeAnimatedNodesManager::startRenderCallbackIfNeeded(bool isAsync) {
-  // This method can be called from either the UI thread or JavaScript thread.
-  // It ensures `startOnRenderCallback_` is called exactly once using atomic
-  // operations. We use std::atomic_bool rather than std::mutex to avoid
-  // potential deadlocks that could occur if we called external code while
-  // holding a mutex.
-  auto isRenderCallbackStarted = isRenderCallbackStarted_.exchange(true);
-  if (isRenderCallbackStarted) {
-    // onRender callback is already started.
-    return;
-  }
-
-  if (useSharedAnimatedBackend_) {
-    if (auto animationBackend = animationBackend_.lock()) {
-      auto weak = weak_from_this();
-      animationBackendCallbackId_ = animationBackend->start(
-          [weak](AnimationTimestamp timestamp) -> AnimationMutations {
-            if (auto self = weak.lock()) {
-              return self->pullAnimationMutations(timestamp);
-            }
-            return {};
-          });
+  if (!useSharedAnimatedBackend_) {
+    // This method can be called from either the UI thread or JavaScript thread.
+    // It ensures `startOnRenderCallback_` is called exactly once using atomic
+    // operations. We use std::atomic_bool rather than std::mutex to avoid
+    // potential deadlocks that could occur if we called external code while
+    // holding a mutex.
+    auto isRenderCallbackStarted = isRenderCallbackStarted_.exchange(true);
+    if (isRenderCallbackStarted) {
+      // onRender callback is already started.
+      return;
+    }
+    if (startOnRenderCallback_) {
+      startOnRenderCallback_([this]() { onRender(); }, isAsync);
     }
-
     return;
   }
 
-  if (startOnRenderCallback_) {
-    startOnRenderCallback_([this]() { onRender(); }, isAsync);
+  {
+    auto animationBackend = animationBackend_.lock();
+    if (!animationBackend) {
+      return;
+    }
+    // AnimationBackend::start registers the callback before returning the id.
+    // Keep registration and id publication in one critical section; otherwise a
+    // concurrent stop can observe no id and leave the registered callback
+    // orphaned in the backend.
+    std::lock_guard<std::mutex> lock(animationBackendCallbackMutex_);
+    if (animationBackendCallbackId_.has_value()) {
+      return;
+    }
+    auto weak = weak_from_this();
+    auto callbackId = animationBackend->start(
+        [weak](AnimationTimestamp timestamp) -> AnimationMutations {
+          if (auto self = weak.lock()) {
+            return self->pullAnimationMutations(timestamp);
+          }
+          return {};
+        });
+    animationBackendCallbackId_ = callbackId;
   }
 }
 
 void NativeAnimatedNodesManager::stopRenderCallbackIfNeeded(
     bool isAsync) noexcept {
-  // When multiple threads reach this point, only one thread should call
-  // stopOnRenderCallback_. This synchronization is primarily needed during
-  // destruction of NativeAnimatedNodesManager. In normal operation,
-  // stopRenderCallbackIfNeeded is always called from the UI thread.
-  auto isRenderCallbackStarted = isRenderCallbackStarted_.exchange(false);
-
-  if (useSharedAnimatedBackend_) {
+  if (!useSharedAnimatedBackend_) {
+    // When multiple threads reach this point, only one thread should call
+    // stopOnRenderCallback_. This synchronization is primarily needed during
+    // destruction of NativeAnimatedNodesManager. In normal operation,
+    // stopRenderCallbackIfNeeded is always called from the UI thread.
+    auto isRenderCallbackStarted = isRenderCallbackStarted_.exchange(false);
     if (isRenderCallbackStarted) {
-      if (auto animationBackend = animationBackend_.lock()) {
-        animationBackend->stop(animationBackendCallbackId_);
+      if (stopOnRenderCallback_) {
+        stopOnRenderCallback_(isAsync);
+
+        if (frameRateListenerCallback_) {
+          frameRateListenerCallback_(false);
+        }
       }
     }
     return;
   }
 
-  if (isRenderCallbackStarted) {
-    if (stopOnRenderCallback_) {
-      stopOnRenderCallback_(isAsync);
-
-      if (frameRateListenerCallback_) {
-        frameRateListenerCallback_(false);
+  {
+    auto animationBackend = animationBackend_.lock();
+    CallbackId callbackId = 0;
+    {
+      std::lock_guard<std::mutex> lock(animationBackendCallbackMutex_);
+      if (!animationBackendCallbackId_.has_value()) {
+        return;
       }
+      // Clear the published id while holding the same mutex that protects
+      // start registration. This makes a concurrent start either see an active
+      // callback or wait until this stop owns the callback id.
+      callbackId = *animationBackendCallbackId_;
+      animationBackendCallbackId_.reset();
+    }
+    if (animationBackend) {
+      animationBackend->stop(callbackId);
     }
   }
 }

packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.h

@@ -298,7 +298,10 @@ class NativeAnimatedNodesManager : public std::enable_shared_from_this<NativeAni
   bool warnedAboutGraphTraversal_ = false;
 #endif
 
-  CallbackId animationBackendCallbackId_{0};
+  // Protects the register/publish and exchange/stop lifecycle for the shared
+  // AnimationBackend callback.
+  std::mutex animationBackendCallbackMutex_;
+  std::optional<CallbackId> animationBackendCallbackId_;
 
   friend class ColorAnimatedNode;
   friend class AnimationDriver;