Skip to content

Commit 027e29f

Browse files
Bartlomiej Bloniarzfacebook-github-bot
authored andcommitted
Fix AnimatedPropsRegistry surface-stop resurrection race (#57376)
Summary: AnimatedPropsRegistry::update() runs on the UI thread every animation frame and created a surface's entry via operator[]. clearOnSurfaceStop() (run on the JS thread when a surface stops) erases that entry, but an in-flight animation frame landing after the stop re-created it via operator[] -- and since the surface is gone, nothing ever cleans it up again. The resurrected entry leaks its PropsSnapshot and ShadowNodeFamily for the lifetime of the registry. A surface's entry is legitimately created by getMap(), which AnimationBackendCommitHook calls on every React commit. stopSurface drains in-flight commits before unregistering the ShadowTree (ShadowTreeRegistry::remove takes the registry's unique lock, which excludes the shared-locked commit visits), so getMap() can never run for a stopped surface. That leaves update()'s operator[] as the only thing that can resurrect one. Fix: update() now only refines surfaces that already exist (find instead of operator[]) and never creates an entry; getMap() remains the sole creator. A stopped surface can no longer be resurrected, and there is no extra bookkeeping that could grow over time. Changelog: [General][Fixed] - Fix a surface-stop race in the C++ Animated shared backend that could permanently leak per-surface animated state Differential Revision: D109156094
1 parent b364490 commit 027e29f

19 files changed

Lines changed: 86 additions & 43 deletions

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ NativeAnimatedNodesManagerProvider::getOrCreate(
119119
});
120120

121121
// Register on surfaces started in the future
122-
uiManager->setOnSurfaceStartCallback(
122+
uiManager->addOnSurfaceStartCallback(
123123
[animatedMountingOverrideDelegate =
124124
std::weak_ptr<const AnimatedMountingOverrideDelegate>(
125125
animatedMountingOverrideDelegate_)](

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,11 @@ void AnimatedPropsRegistry::update(
1515
const std::unordered_map<SurfaceId, SurfaceUpdates>& surfaceUpdates) {
1616
auto lock = std::lock_guard(mutex_);
1717
for (const auto& [surfaceId, updates] : surfaceUpdates) {
18-
auto& surfaceContext = surfaceContexts_[surfaceId];
18+
auto contextIt = surfaceContexts_.find(surfaceId);
19+
if (contextIt == surfaceContexts_.end()) {
20+
continue;
21+
}
22+
auto& surfaceContext = contextIt->second;
1923
auto& pendingMap = surfaceContext.pendingMap;
2024
auto& pendingFamilies = surfaceContext.pendingFamilies;
2125

@@ -55,6 +59,11 @@ void AnimatedPropsRegistry::update(
5559
}
5660
}
5761

62+
void AnimatedPropsRegistry::initializeSurface(SurfaceId surfaceId) {
63+
auto lock = std::lock_guard(mutex_);
64+
surfaceContexts_.try_emplace(surfaceId);
65+
}
66+
5867
std::pair<
5968
std::unordered_set<std::shared_ptr<const ShadowNodeFamily>>&,
6069
SnapshotMap&>

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ using SnapshotMap = std::unordered_map<Tag, std::unique_ptr<PropsSnapshot>>;
3838
class AnimatedPropsRegistry {
3939
public:
4040
void update(const std::unordered_map<SurfaceId, SurfaceUpdates> &surfaceUpdates);
41+
void initializeSurface(SurfaceId surfaceId);
4142
void clear(SurfaceId surfaceId);
4243
void clearOnSurfaceStop(SurfaceId surfaceId);
4344
std::pair<std::unordered_set<std::shared_ptr<const ShadowNodeFamily>> &, SnapshotMap &> getMap(SurfaceId surfaceId);

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,24 @@ AnimationBackend::AnimationBackend(
5656
commitHook_(*uiManager, animatedPropsRegistry_),
5757
uiManager_(std::move(uiManager)) {
5858
react_native_assert(uiManager_.expired() == false);
59+
60+
auto weakAnimatedPropsRegistry =
61+
std::weak_ptr<AnimatedPropsRegistry>(animatedPropsRegistry_);
62+
auto initializeSurfaceContext =
63+
[weakAnimatedPropsRegistry](const ShadowTree& shadowTree) {
64+
if (auto animatedPropsRegistry = weakAnimatedPropsRegistry.lock()) {
65+
animatedPropsRegistry->initializeSurface(shadowTree.getSurfaceId());
66+
}
67+
};
68+
69+
if (auto lockedUIManager = uiManager_.lock()) {
70+
lockedUIManager->addOnSurfaceStartCallback(initializeSurfaceContext);
71+
lockedUIManager->getShadowTreeRegistry().enumerate(
72+
[&initializeSurfaceContext](
73+
const ShadowTree& shadowTree, bool& /*stop*/) {
74+
initializeSurfaceContext(shadowTree);
75+
});
76+
}
5977
}
6078

6179
void AnimationBackend::unpackMutations(

packages/react-native/ReactCommon/react/renderer/scheduler/Scheduler.cpp

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include <react/renderer/runtimescheduler/RuntimeScheduler.h>
2323
#include <react/renderer/uimanager/UIManager.h>
2424
#include <react/renderer/uimanager/UIManagerBinding.h>
25+
#include <mutex>
2526

2627
namespace facebook::react {
2728

@@ -459,9 +460,13 @@ void Scheduler::uiManagerDidPromoteReactRevision(const ShadowTree& shadowTree) {
459460
}
460461

461462
void Scheduler::uiManagerDidStartSurface(const ShadowTree& shadowTree) {
462-
std::shared_lock lock(onSurfaceStartCallbackMutex_);
463-
if (onSurfaceStartCallback_) {
464-
onSurfaceStartCallback_(shadowTree);
463+
std::vector<OnSurfaceStartCallback> callbacks;
464+
{
465+
std::shared_lock lock(onSurfaceStartCallbackMutex_);
466+
callbacks = onSurfaceStartCallbacks_;
467+
}
468+
for (const auto& callback : callbacks) {
469+
callback(shadowTree);
465470
}
466471
}
467472

@@ -491,10 +496,10 @@ void Scheduler::removeEventListener(
491496
}
492497
}
493498

494-
void Scheduler::uiManagerShouldSetOnSurfaceStartCallback(
499+
void Scheduler::uiManagerShouldAddOnSurfaceStartCallback(
495500
OnSurfaceStartCallback&& callback) {
496-
std::shared_lock lock(onSurfaceStartCallbackMutex_);
497-
onSurfaceStartCallback_ = std::move(callback);
501+
std::unique_lock lock(onSurfaceStartCallbackMutex_);
502+
onSurfaceStartCallbacks_.push_back(std::move(callback));
498503
}
499504

500505
} // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/scheduler/Scheduler.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
#include <atomic>
1111
#include <memory>
12+
#include <vector>
1213

1314
#include <ReactCommon/RuntimeExecutor.h>
1415
#include <react/performance/cdpmetrics/CdpMetricsReporter.h>
@@ -117,7 +118,7 @@ class Scheduler final : public UIManagerDelegate {
117118
void removeEventListener(const std::shared_ptr<const EventListener> &listener);
118119

119120
#pragma mark - Surface start callback
120-
void uiManagerShouldSetOnSurfaceStartCallback(OnSurfaceStartCallback &&callback) override;
121+
void uiManagerShouldAddOnSurfaceStartCallback(OnSurfaceStartCallback &&callback) override;
121122

122123
private:
123124
friend class SurfaceHandler;
@@ -159,7 +160,7 @@ class Scheduler final : public UIManagerDelegate {
159160
std::shared_ptr<ViewTransitionModule> viewTransitionModule_;
160161

161162
mutable std::shared_mutex onSurfaceStartCallbackMutex_;
162-
OnSurfaceStartCallback onSurfaceStartCallback_;
163+
std::vector<OnSurfaceStartCallback> onSurfaceStartCallbacks_;
163164
};
164165

165166
} // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -765,10 +765,10 @@ void UIManager::removeEventListener(
765765
}
766766
}
767767

768-
void UIManager::setOnSurfaceStartCallback(
768+
void UIManager::addOnSurfaceStartCallback(
769769
UIManagerDelegate::OnSurfaceStartCallback&& callback) {
770770
if (delegate_ != nullptr) {
771-
delegate_->uiManagerShouldSetOnSurfaceStartCallback(std::move(callback));
771+
delegate_->uiManagerShouldAddOnSurfaceStartCallback(std::move(callback));
772772
}
773773
}
774774

packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,8 @@ class UIManager final : public ShadowTreeDelegate {
223223

224224
void removeEventListener(const std::shared_ptr<const EventListener> &listener);
225225

226-
#pragma mark - Set on surface start callback
227-
void setOnSurfaceStartCallback(UIManagerDelegate::OnSurfaceStartCallback &&callback);
226+
#pragma mark - Add on surface start callback
227+
void addOnSurfaceStartCallback(UIManagerDelegate::OnSurfaceStartCallback &&callback);
228228

229229
private:
230230
friend class UIManagerBinding;

packages/react-native/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ class UIManagerDelegate {
9595
virtual void uiManagerDidPromoteReactRevision(const ShadowTree &shadowTree) = 0;
9696

9797
using OnSurfaceStartCallback = std::function<void(const ShadowTree &shadowTree)>;
98-
virtual void uiManagerShouldSetOnSurfaceStartCallback(OnSurfaceStartCallback &&callback) = 0;
98+
virtual void uiManagerShouldAddOnSurfaceStartCallback(OnSurfaceStartCallback &&callback) = 0;
9999

100100
// View transition bitmap snapshot capture and application.
101101
virtual void uiManagerDidCaptureViewSnapshot(Tag tag, SurfaceId surfaceId) = 0;

packages/react-native/ReactCommon/react/renderer/viewtransition/ViewTransitionModule.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ void ViewTransitionModule::initialize(
5959
});
6060

6161
// Register on surfaces started in the future
62-
uiManager_->setOnSurfaceStartCallback(
62+
uiManager_->addOnSurfaceStartCallback(
6363
[weakThis](const ShadowTree& shadowTree) {
6464
shadowTree.getMountingCoordinator()->setMountingOverrideDelegate(
6565
weakThis);

0 commit comments

Comments
 (0)