From 1bc31af99d374d826b8d9d54a2be1a71e01fc1f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20=C5=BBelawski?= <40713406+tjzel@users.noreply.github.com> Date: Fri, 10 Jan 2025 11:04:41 +0100 Subject: [PATCH] refactor: explicit invalidations for native and cpp (#6850) ## Summary This pull requests refactors memory management of Worklets and Reanimated. Basically, since Reanimated can obtain WorkletsModuleProxy and the Worklet Runtimes as shared pointers, it has to release them explicitly during the invalidation stage of Native Modules. Releasing them later on (e.g. via deconstructors) might lead into issues and crashes. Ideally we'd instead use some different solution here than shared pointers, but it can wait as it's not mandatory at the moment and could be a significant refactor. Fixes: - iOS crash on reload - Android crash on SingleInstanceChecker during third reload ## Test plan - [x] 0.76 iOS/Android Fabric works - [x] 0.76 iOS/Android Paper works - [x] 0.75 iOS/Android Fabric works - [x] 0.75 iOS/Android Paper works - [x] 0.74 iOS/Android Fabric works - [x] 0.74 iOS/Android Paper works --- .../NativeModules/ReanimatedModuleProxy.cpp | 6 ++++++ .../NativeModules/ReanimatedModuleProxy.h | 4 +++- .../cpp/worklets/Tools/SingleInstanceChecker.h | 4 +--- .../com/swmansion/reanimated/NativeProxy.java | 6 ++++++ .../main/cpp/reanimated/android/NativeProxy.cpp | 11 ++++++++++- .../src/main/cpp/reanimated/android/NativeProxy.h | 2 ++ .../main/cpp/worklets/android/WorkletsModule.cpp | 15 +++++++++------ .../main/cpp/worklets/android/WorkletsModule.h | 14 +++++++------- .../com/swmansion/reanimated/NodesManager.java | 1 + .../com/swmansion/worklets/WorkletsModule.java | 7 +++++++ .../com/swmansion/reanimated/NativeProxy.java | 6 ++++++ .../apple/worklets/apple/WorkletsModule.mm | 10 ++++++++++ 12 files changed, 68 insertions(+), 18 deletions(-) diff --git a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp index 8341ade9834d..ffe538bd9dcc 100644 --- a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp +++ b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp @@ -843,4 +843,10 @@ void ReanimatedModuleProxy::unsubscribeFromKeyboardEvents( unsubscribeFromKeyboardEventsFunction_(listenerId.asNumber()); } +void ReanimatedModuleProxy::invalidate() { + // Make sure to release WorkletsModuleProxy on invalidate to allow it + // to destroy its runtime during the invalidation stage. + workletsModuleProxy_.reset(); +} + } // namespace reanimated diff --git a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.h b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.h index dd3b66619995..a1da65df3a2b 100644 --- a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.h +++ b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.h @@ -42,6 +42,8 @@ class ReanimatedModuleProxy : public ReanimatedModuleProxySpec { ~ReanimatedModuleProxy(); + void invalidate(); + jsi::Value registerEventHandler( jsi::Runtime &rt, const jsi::Value &worklet, @@ -175,7 +177,7 @@ class ReanimatedModuleProxy : public ReanimatedModuleProxySpec { const bool isBridgeless_; const bool isReducedMotion_; - const std::shared_ptr workletsModuleProxy_; + std::shared_ptr workletsModuleProxy_; const std::string valueUnpackerCode_; std::unique_ptr eventHandlerRegistry_; diff --git a/packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h b/packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h index 3be7ce80fecf..9c8c0eb23cb8 100644 --- a/packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h +++ b/packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h @@ -52,10 +52,8 @@ SingleInstanceChecker::SingleInstanceChecker() { std::string className = __cxxabiv1::__cxa_demangle(typeid(T).name(), nullptr, nullptr, &status); - // Only one instance should exist, but it is possible for two instances - // to co-exist during a reload. assertWithMessage( - instanceCount_ <= 1, + instanceCount_ == 0, "[Reanimated] More than one instance of " + className + " present. This may indicate a memory leak due to a retain cycle."); diff --git a/packages/react-native-reanimated/android/src/fabric/java/com/swmansion/reanimated/NativeProxy.java b/packages/react-native-reanimated/android/src/fabric/java/com/swmansion/reanimated/NativeProxy.java index 1701b938ca4e..7921dde2b006 100644 --- a/packages/react-native-reanimated/android/src/fabric/java/com/swmansion/reanimated/NativeProxy.java +++ b/packages/react-native-reanimated/android/src/fabric/java/com/swmansion/reanimated/NativeProxy.java @@ -70,6 +70,12 @@ protected HybridData getHybridData() { return mHybridData; } + private native void invalidateCpp(); + + public void invalidate() { + invalidateCpp(); + } + public static NativeMethodsHolder createNativeMethodsHolder( LayoutAnimations ignoredLayoutAnimations) { return new NativeMethodsHolder() { diff --git a/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.cpp b/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.cpp index e8deed164287..5e608618072e 100644 --- a/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.cpp +++ b/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.cpp @@ -194,7 +194,8 @@ void NativeProxy::registerNatives() { makeNativeMethod( "isAnyHandlerWaitingForEvent", NativeProxy::isAnyHandlerWaitingForEvent), - makeNativeMethod("performOperations", NativeProxy::performOperations)}); + makeNativeMethod("performOperations", NativeProxy::performOperations), + makeNativeMethod("invalidateCpp", NativeProxy::invalidateCpp)}); } void NativeProxy::requestRender( @@ -619,4 +620,12 @@ void NativeProxy::setupLayoutAnimations() { }); } +void NativeProxy::invalidateCpp() { + workletsModuleProxy_.reset(); + if (reanimatedModuleProxy_ != nullptr) { + reanimatedModuleProxy_->invalidate(); + } + reanimatedModuleProxy_.reset(); +} + } // namespace reanimated diff --git a/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.h b/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.h index 09a8e37e9268..af8bdbccfbd8 100644 --- a/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.h +++ b/packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.h @@ -279,6 +279,8 @@ class NativeProxy : public jni::HybridClass { void commonInit(jni::alias_ref &fabricUIManager); #endif // RCT_NEW_ARCH_ENABLED + + void invalidateCpp(); }; } // namespace reanimated diff --git a/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.cpp b/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.cpp index bd1b3ecd897f..fa8646fd9d5b 100644 --- a/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.cpp +++ b/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.cpp @@ -18,15 +18,13 @@ using namespace facebook; using namespace react; WorkletsModule::WorkletsModule( - jni::alias_ref jThis, jsi::Runtime *rnRuntime, const std::string &valueUnpackerCode, jni::alias_ref messageQueueThread, const std::shared_ptr &jsCallInvoker, const std::shared_ptr &jsScheduler, const std::shared_ptr &uiScheduler) - : javaPart_(jni::make_global(jThis)), - rnRuntime_(rnRuntime), + : rnRuntime_(rnRuntime), workletsModuleProxy_(std::make_shared( *rnRuntime, valueUnpackerCode, @@ -38,7 +36,7 @@ WorkletsModule::WorkletsModule( } jni::local_ref WorkletsModule::initHybrid( - jni::alias_ref jThis, + jni::alias_ref /*jThis*/, jlong jsContext, const std::string &valueUnpackerCode, jni::alias_ref messageQueueThread, @@ -52,7 +50,6 @@ jni::local_ref WorkletsModule::initHybrid( std::make_shared(*rnRuntime, jsCallInvoker); auto uiScheduler = androidUIScheduler->cthis()->getUIScheduler(); return makeCxxInstance( - jThis, rnRuntime, valueUnpackerCode, messageQueueThread, @@ -61,8 +58,14 @@ jni::local_ref WorkletsModule::initHybrid( uiScheduler); } +void WorkletsModule::invalidateCpp() { + workletsModuleProxy_.reset(); +} + void WorkletsModule::registerNatives() { - registerHybrid({makeNativeMethod("initHybrid", WorkletsModule::initHybrid)}); + registerHybrid( + {makeNativeMethod("initHybrid", WorkletsModule::initHybrid), + makeNativeMethod("invalidateCpp", WorkletsModule::invalidateCpp)}); } } // namespace worklets diff --git a/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h b/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h index 094287271fca..1cd7bf23b5af 100644 --- a/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h +++ b/packages/react-native-reanimated/android/src/main/cpp/worklets/android/WorkletsModule.h @@ -30,7 +30,7 @@ class WorkletsModule : public jni::HybridClass { "Lcom/swmansion/worklets/WorkletsModule;"; static jni::local_ref initHybrid( - jni::alias_ref jThis, + jni::alias_ref /*jThis*/, jlong jsContext, const std::string &valueUnpackerCode, jni::alias_ref messageQueueThread, @@ -46,19 +46,19 @@ class WorkletsModule : public jni::HybridClass { } private: - friend HybridBase; - jni::global_ref javaPart_; - jsi::Runtime *rnRuntime_; - std::shared_ptr workletsModuleProxy_; - explicit WorkletsModule( - jni::alias_ref jThis, jsi::Runtime *rnRuntime, const std::string &valueUnpackerCode, jni::alias_ref messageQueueThread, const std::shared_ptr &jsCallInvoker, const std::shared_ptr &jsScheduler, const std::shared_ptr &uiScheduler); + + void invalidateCpp(); + + friend HybridBase; + jsi::Runtime *rnRuntime_; + std::shared_ptr workletsModuleProxy_; }; } // namespace worklets diff --git a/packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/NodesManager.java b/packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/NodesManager.java index 58cdc88924d7..8cb9a929e6ff 100644 --- a/packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/NodesManager.java +++ b/packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/NodesManager.java @@ -128,6 +128,7 @@ public void invalidate() { } if (mNativeProxy != null) { + mNativeProxy.invalidate(); mNativeProxy = null; } } diff --git a/packages/react-native-reanimated/android/src/main/java/com/swmansion/worklets/WorkletsModule.java b/packages/react-native-reanimated/android/src/main/java/com/swmansion/worklets/WorkletsModule.java index 35cf7d078290..d5249f8050be 100644 --- a/packages/react-native-reanimated/android/src/main/java/com/swmansion/worklets/WorkletsModule.java +++ b/packages/react-native-reanimated/android/src/main/java/com/swmansion/worklets/WorkletsModule.java @@ -69,6 +69,13 @@ public boolean installTurboModule(String valueUnpackerCode) { } public void invalidate() { + // We have to destroy extra runtimes when invalidate is called. If we clean + // it up later instead there's a chance the runtime will retain references + // to invalidated memory and will crash on its destruction. + invalidateCpp(); + mAndroidUIScheduler.deactivate(); } + + private native void invalidateCpp(); } diff --git a/packages/react-native-reanimated/android/src/paper/java/com/swmansion/reanimated/NativeProxy.java b/packages/react-native-reanimated/android/src/paper/java/com/swmansion/reanimated/NativeProxy.java index 7f7ebada7cd1..a4330032a002 100644 --- a/packages/react-native-reanimated/android/src/paper/java/com/swmansion/reanimated/NativeProxy.java +++ b/packages/react-native-reanimated/android/src/paper/java/com/swmansion/reanimated/NativeProxy.java @@ -61,6 +61,12 @@ protected HybridData getHybridData() { return mHybridData; } + private native void invalidateCpp(); + + public void invalidate() { + invalidateCpp(); + } + public static NativeMethodsHolder createNativeMethodsHolder(LayoutAnimations layoutAnimations) { WeakReference weakLayoutAnimations = new WeakReference<>(layoutAnimations); return new NativeMethodsHolder() { diff --git a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm index eed668545c4e..9dacb4b6f00f 100644 --- a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm +++ b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm @@ -52,4 +52,14 @@ @implementation WorkletsModule { return @YES; } +- (void)invalidate +{ + // We have to destroy extra runtimes when invalidate is called. If we clean + // it up later instead there's a chance the runtime will retain references + // to invalidated memory and will crash on destruction. + workletsModuleProxy_.reset(); + + [super invalidate]; +} + @end