Skip to content

Commit 0443da0

Browse files
committed
fix: Memory mismanagement with UI scheduled callbacks (#6900)
The pattern we used with scheduling callbacks on the UI thread - capturing `this` as a reference - is prone to memory issues. Given such code: ```c++ void WorkletsModuleProxy::scheduleOnUI( jsi::Runtime &rt, const jsi::Value &worklet) { auto shareableWorklet = extractShareableOrThrow<ShareableWorklet>( rt, worklet, "[Worklets] Only worklets can be scheduled to run on UI."); uiScheduler_->scheduleOnUI( [this, shareableWorklet] { this->uiWorkletRuntime_->runGuarded(shareableWorklet); }); } ``` We are likely to run into accessing invalidated memory during a reload. This is due to fact that `WorkletsModuleProxy` is managed by some object held by the instance of React Native. Let's look at the following scenario. 1. `WorkletsModuleProxy` is created on the JS thread and held by the `WorkletsModule` Native Module. 2. `WorkletsModuleProxy::scheduleOnUI` is invoked on the JS thread. The callback is scheduled to be executed on the UI thread. 3. Application's reload gets triggered. A tear down of React Native is starting on the JS thread. 4. `WorkletsModule` gets destroyed. Therefore, `WorkletsModuleProxy` is released and also destroyed. 5. The callback is finally executed on the UI thread by the scheduler. However, `this` has been invalidated. The App crashes. Keep in mind that this isn't exclusive to thread jumps exclusively. Calling `scheduleOnUI` on the UI thread could still result in the callback executing after the memory has been invalidated. `WorkletsModuleProxy` is only an example here, the problem could manifest in all the places where we pass lambdas that capture `this` by reference. To fix this I refactored the code so everytime we pass `this` to a scheduled callback, it would be done via a _weak pointer_ which would lock the object and prevent it from being destroyed while the callback is being executed on the UI thread. Perhaps some bits of code don't need this safety measure due to a heuristic existing that guarantees that respective memory won't be invalidated before the callback gets executed. However, I found it extremely challenging and unreliable to come up with these heuristics, as they could possibly break at any future change of the code. - ReanimatedCommitHook - LayoutAnimationProxy - ReanimatedModuleProxy - WorkletsModuleProxy - WorkletRuntime - NativeProxy Reloading the app no longer causes a crash on a scheduled UI callback.
1 parent b229c2c commit 0443da0

File tree

13 files changed

+304
-172
lines changed

13 files changed

+304
-172
lines changed

packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedCommitHook.cpp

+5-3
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,16 @@ void ReanimatedCommitHook::maybeInitializeLayoutAnimations(
3434
// when a new surfaceId is observed we call setMountingOverrideDelegate
3535
// for all yet unseen surfaces
3636
uiManager_->getShadowTreeRegistry().enumerate(
37-
[this](const ShadowTree &shadowTree, bool &stop) {
38-
if (shadowTree.getSurfaceId() <= currentMaxSurfaceId_) {
37+
[strongThis = shared_from_this()](
38+
const ShadowTree &shadowTree, bool &stop) {
39+
// Executed synchronously.
40+
if (shadowTree.getSurfaceId() <= strongThis->currentMaxSurfaceId_) {
3941
// the set function actually adds our delegate to a list, so we
4042
// shouldn't invoke it twice for the same surface
4143
return;
4244
}
4345
shadowTree.getMountingCoordinator()->setMountingOverrideDelegate(
44-
layoutAnimationsProxy_);
46+
strongThis->layoutAnimationsProxy_);
4547
});
4648
currentMaxSurfaceId_ = surfaceId;
4749
}

packages/react-native-reanimated/Common/cpp/reanimated/Fabric/ReanimatedCommitHook.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ using namespace facebook::react;
1212

1313
namespace reanimated {
1414

15-
class ReanimatedCommitHook : public UIManagerCommitHook {
15+
class ReanimatedCommitHook
16+
: public UIManagerCommitHook,
17+
public std::enable_shared_from_this<ReanimatedCommitHook> {
1618
public:
1719
ReanimatedCommitHook(
1820
const std::shared_ptr<PropsRegistry> &propsRegistry,

packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy.cpp

+93-61
Original file line numberDiff line numberDiff line change
@@ -650,19 +650,25 @@ void LayoutAnimationsProxy::startEnteringAnimation(
650650
static_cast<const ViewProps &>(*mutation.newChildShadowView.props);
651651
auto opacity = viewProps.opacity;
652652

653-
uiScheduler_->scheduleOnUI([finalView,
653+
uiScheduler_->scheduleOnUI([weakThis = weak_from_this(),
654+
finalView,
654655
current,
655656
#if REACT_NATIVE_MINOR_VERSION < 78
656657
parent,
657-
#endif // REACT_NATIVE_MINOR_VERSION < 78
658+
#endif // RE
658659
mutation,
659660
opacity,
660-
this,
661661
tag]() {
662+
auto strongThis = weakThis.lock();
663+
if (!strongThis) {
664+
return;
665+
}
666+
662667
Rect window{};
663668
{
669+
auto &mutex = strongThis->mutex;
664670
auto lock = std::unique_lock<std::recursive_mutex>(mutex);
665-
layoutAnimations_.insert_or_assign(
671+
strongThis->layoutAnimations_.insert_or_assign(
666672
tag, LayoutAnimation {
667673
finalView, current,
668674
#if REACT_NATIVE_MINOR_VERSION >= 78
@@ -672,21 +678,23 @@ void LayoutAnimationsProxy::startEnteringAnimation(
672678
#endif // REACT_NATIVE_MINOR_VERSION >= 78
673679
opacity
674680
});
675-
window = surfaceManager.getWindow(mutation.newChildShadowView.surfaceId);
681+
window = strongThis->surfaceManager.getWindow(
682+
mutation.newChildShadowView.surfaceId);
676683
}
677684

678685
Snapshot values(mutation.newChildShadowView, window);
679-
jsi::Object yogaValues(uiRuntime_);
680-
yogaValues.setProperty(uiRuntime_, "targetOriginX", values.x);
681-
yogaValues.setProperty(uiRuntime_, "targetGlobalOriginX", values.x);
682-
yogaValues.setProperty(uiRuntime_, "targetOriginY", values.y);
683-
yogaValues.setProperty(uiRuntime_, "targetGlobalOriginY", values.y);
684-
yogaValues.setProperty(uiRuntime_, "targetWidth", values.width);
685-
yogaValues.setProperty(uiRuntime_, "targetHeight", values.height);
686-
yogaValues.setProperty(uiRuntime_, "windowWidth", values.windowWidth);
687-
yogaValues.setProperty(uiRuntime_, "windowHeight", values.windowHeight);
688-
layoutAnimationsManager_->startLayoutAnimation(
689-
uiRuntime_, tag, LayoutAnimationType::ENTERING, yogaValues);
686+
auto &uiRuntime = strongThis->uiRuntime_;
687+
jsi::Object yogaValues(uiRuntime);
688+
yogaValues.setProperty(uiRuntime, "targetOriginX", values.x);
689+
yogaValues.setProperty(uiRuntime, "targetGlobalOriginX", values.x);
690+
yogaValues.setProperty(uiRuntime, "targetOriginY", values.y);
691+
yogaValues.setProperty(uiRuntime, "targetGlobalOriginY", values.y);
692+
yogaValues.setProperty(uiRuntime, "targetWidth", values.width);
693+
yogaValues.setProperty(uiRuntime, "targetHeight", values.height);
694+
yogaValues.setProperty(uiRuntime, "windowWidth", values.windowWidth);
695+
yogaValues.setProperty(uiRuntime, "windowHeight", values.windowHeight);
696+
strongThis->layoutAnimationsManager_->startLayoutAnimation(
697+
uiRuntime, tag, LayoutAnimationType::ENTERING, yogaValues);
690698
});
691699
}
692700

@@ -698,30 +706,38 @@ void LayoutAnimationsProxy::startExitingAnimation(
698706
#endif
699707
auto surfaceId = mutation.oldChildShadowView.surfaceId;
700708

701-
uiScheduler_->scheduleOnUI([this, tag, mutation, surfaceId]() {
702-
auto oldView = mutation.oldChildShadowView;
703-
Rect window{};
704-
{
705-
auto lock = std::unique_lock<std::recursive_mutex>(mutex);
706-
createLayoutAnimation(mutation, oldView, surfaceId, tag);
707-
window = surfaceManager.getWindow(surfaceId);
708-
}
709+
uiScheduler_->scheduleOnUI(
710+
[weakThis = weak_from_this(), tag, mutation, surfaceId]() {
711+
auto strongThis = weakThis.lock();
712+
if (!strongThis) {
713+
return;
714+
}
709715

710-
Snapshot values(oldView, window);
711-
712-
jsi::Object yogaValues(uiRuntime_);
713-
yogaValues.setProperty(uiRuntime_, "currentOriginX", values.x);
714-
yogaValues.setProperty(uiRuntime_, "currentGlobalOriginX", values.x);
715-
yogaValues.setProperty(uiRuntime_, "currentOriginY", values.y);
716-
yogaValues.setProperty(uiRuntime_, "currentGlobalOriginY", values.y);
717-
yogaValues.setProperty(uiRuntime_, "currentWidth", values.width);
718-
yogaValues.setProperty(uiRuntime_, "currentHeight", values.height);
719-
yogaValues.setProperty(uiRuntime_, "windowWidth", values.windowWidth);
720-
yogaValues.setProperty(uiRuntime_, "windowHeight", values.windowHeight);
721-
layoutAnimationsManager_->startLayoutAnimation(
722-
uiRuntime_, tag, LayoutAnimationType::EXITING, yogaValues);
723-
layoutAnimationsManager_->clearLayoutAnimationConfig(tag);
724-
});
716+
auto oldView = mutation.oldChildShadowView;
717+
Rect window{};
718+
{
719+
auto &mutex = strongThis->mutex;
720+
auto lock = std::unique_lock<std::recursive_mutex>(mutex);
721+
strongThis->createLayoutAnimation(mutation, oldView, surfaceId, tag);
722+
window = strongThis->surfaceManager.getWindow(surfaceId);
723+
}
724+
725+
Snapshot values(oldView, window);
726+
727+
auto &uiRuntime = strongThis->uiRuntime_;
728+
jsi::Object yogaValues(uiRuntime);
729+
yogaValues.setProperty(uiRuntime, "currentOriginX", values.x);
730+
yogaValues.setProperty(uiRuntime, "currentGlobalOriginX", values.x);
731+
yogaValues.setProperty(uiRuntime, "currentOriginY", values.y);
732+
yogaValues.setProperty(uiRuntime, "currentGlobalOriginY", values.y);
733+
yogaValues.setProperty(uiRuntime, "currentWidth", values.width);
734+
yogaValues.setProperty(uiRuntime, "currentHeight", values.height);
735+
yogaValues.setProperty(uiRuntime, "windowWidth", values.windowWidth);
736+
yogaValues.setProperty(uiRuntime, "windowHeight", values.windowHeight);
737+
strongThis->layoutAnimationsManager_->startLayoutAnimation(
738+
uiRuntime, tag, LayoutAnimationType::EXITING, yogaValues);
739+
strongThis->layoutAnimationsManager_->clearLayoutAnimationConfig(tag);
740+
});
725741
}
726742

727743
void LayoutAnimationsProxy::startLayoutAnimation(
@@ -732,36 +748,46 @@ void LayoutAnimationsProxy::startLayoutAnimation(
732748
#endif
733749
auto surfaceId = mutation.oldChildShadowView.surfaceId;
734750

735-
uiScheduler_->scheduleOnUI([this, mutation, surfaceId, tag]() {
751+
uiScheduler_->scheduleOnUI([weakThis = weak_from_this(),
752+
mutation,
753+
surfaceId,
754+
tag]() {
755+
auto strongThis = weakThis.lock();
756+
if (!strongThis) {
757+
return;
758+
}
759+
736760
auto oldView = mutation.oldChildShadowView;
737761
Rect window{};
738762
{
763+
auto &mutex = strongThis->mutex;
739764
auto lock = std::unique_lock<std::recursive_mutex>(mutex);
740-
createLayoutAnimation(mutation, oldView, surfaceId, tag);
741-
window = surfaceManager.getWindow(surfaceId);
765+
strongThis->createLayoutAnimation(mutation, oldView, surfaceId, tag);
766+
window = strongThis->surfaceManager.getWindow(surfaceId);
742767
}
743768

744769
Snapshot currentValues(oldView, window);
745770
Snapshot targetValues(mutation.newChildShadowView, window);
746771

747-
jsi::Object yogaValues(uiRuntime_);
748-
yogaValues.setProperty(uiRuntime_, "currentOriginX", currentValues.x);
749-
yogaValues.setProperty(uiRuntime_, "currentGlobalOriginX", currentValues.x);
750-
yogaValues.setProperty(uiRuntime_, "currentOriginY", currentValues.y);
751-
yogaValues.setProperty(uiRuntime_, "currentGlobalOriginY", currentValues.y);
752-
yogaValues.setProperty(uiRuntime_, "currentWidth", currentValues.width);
753-
yogaValues.setProperty(uiRuntime_, "currentHeight", currentValues.height);
754-
yogaValues.setProperty(uiRuntime_, "targetOriginX", targetValues.x);
755-
yogaValues.setProperty(uiRuntime_, "targetGlobalOriginX", targetValues.x);
756-
yogaValues.setProperty(uiRuntime_, "targetOriginY", targetValues.y);
757-
yogaValues.setProperty(uiRuntime_, "targetGlobalOriginY", targetValues.y);
758-
yogaValues.setProperty(uiRuntime_, "targetWidth", targetValues.width);
759-
yogaValues.setProperty(uiRuntime_, "targetHeight", targetValues.height);
760-
yogaValues.setProperty(uiRuntime_, "windowWidth", targetValues.windowWidth);
772+
auto &uiRuntime = strongThis->uiRuntime_;
773+
jsi::Object yogaValues(uiRuntime);
774+
yogaValues.setProperty(uiRuntime, "currentOriginX", currentValues.x);
775+
yogaValues.setProperty(uiRuntime, "currentGlobalOriginX", currentValues.x);
776+
yogaValues.setProperty(uiRuntime, "currentOriginY", currentValues.y);
777+
yogaValues.setProperty(uiRuntime, "currentGlobalOriginY", currentValues.y);
778+
yogaValues.setProperty(uiRuntime, "currentWidth", currentValues.width);
779+
yogaValues.setProperty(uiRuntime, "currentHeight", currentValues.height);
780+
yogaValues.setProperty(uiRuntime, "targetOriginX", targetValues.x);
781+
yogaValues.setProperty(uiRuntime, "targetGlobalOriginX", targetValues.x);
782+
yogaValues.setProperty(uiRuntime, "targetOriginY", targetValues.y);
783+
yogaValues.setProperty(uiRuntime, "targetGlobalOriginY", targetValues.y);
784+
yogaValues.setProperty(uiRuntime, "targetWidth", targetValues.width);
785+
yogaValues.setProperty(uiRuntime, "targetHeight", targetValues.height);
786+
yogaValues.setProperty(uiRuntime, "windowWidth", targetValues.windowWidth);
761787
yogaValues.setProperty(
762-
uiRuntime_, "windowHeight", targetValues.windowHeight);
763-
layoutAnimationsManager_->startLayoutAnimation(
764-
uiRuntime_, tag, LayoutAnimationType::LAYOUT, yogaValues);
788+
uiRuntime, "windowHeight", targetValues.windowHeight);
789+
strongThis->layoutAnimationsManager_->startLayoutAnimation(
790+
uiRuntime, tag, LayoutAnimationType::LAYOUT, yogaValues);
765791
});
766792
}
767793

@@ -777,8 +803,14 @@ void LayoutAnimationsProxy::maybeCancelAnimation(const int tag) const {
777803
return;
778804
}
779805
layoutAnimations_.erase(tag);
780-
uiScheduler_->scheduleOnUI([this, tag]() {
781-
layoutAnimationsManager_->cancelLayoutAnimation(uiRuntime_, tag);
806+
uiScheduler_->scheduleOnUI([weakThis = weak_from_this(), tag]() {
807+
auto strongThis = weakThis.lock();
808+
if (!strongThis) {
809+
return;
810+
}
811+
812+
auto &uiRuntime = strongThis->uiRuntime_;
813+
strongThis->layoutAnimationsManager_->cancelLayoutAnimation(uiRuntime, tag);
782814
});
783815
}
784816

packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,9 @@ struct LayoutAnimation {
3535
LayoutAnimation &operator=(const LayoutAnimation &other) = default;
3636
};
3737

38-
struct LayoutAnimationsProxy : public MountingOverrideDelegate {
38+
struct LayoutAnimationsProxy
39+
: public MountingOverrideDelegate,
40+
public std::enable_shared_from_this<LayoutAnimationsProxy> {
3941
mutable std::unordered_map<Tag, std::shared_ptr<Node>> nodeForTag_;
4042
mutable std::unordered_map<Tag, LayoutAnimation> layoutAnimations_;
4143
mutable std::recursive_mutex mutex;

0 commit comments

Comments
 (0)