From cd09677feb02284dd31174a32f209c7426b5cb5f Mon Sep 17 00:00:00 2001 From: Bartlomiej Bloniarz Date: Fri, 19 Jun 2026 04:53:40 -0700 Subject: [PATCH] Fix render callback start/stop race in C++ NativeAnimated Summary: 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 Differential Revision: D109009181 --- .../animated/NativeAnimatedNodesManager.cpp | 101 +++++++++++------- .../animated/NativeAnimatedNodesManager.h | 5 +- 2 files changed, 66 insertions(+), 40 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp b/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp index 363c49a0cba3..6d57b522ed1d 100644 --- a/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.cpp +++ b/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 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; + { + std::lock_guard 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); } } } diff --git a/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.h b/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.h index 26a8209a61d2..63c7bb3e82c3 100644 --- a/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.h +++ b/packages/react-native/ReactCommon/react/renderer/animated/NativeAnimatedNodesManager.h @@ -298,7 +298,10 @@ class NativeAnimatedNodesManager : public std::enable_shared_from_this animationBackendCallbackId_; friend class ColorAnimatedNode; friend class AnimationDriver;