From 5b1f4c1be494815f5465dc2003e6cbb5afe66e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20=C5=BBelawski?= Date: Mon, 23 Dec 2024 22:47:39 +0100 Subject: [PATCH 1/5] refactor: explicit invalidations for native and cpp --- .../NativeModules/ReanimatedModuleProxy.cpp | 6 ++++++ .../reanimated/NativeModules/ReanimatedModuleProxy.h | 4 +++- .../cpp/worklets/Tools/SingleInstanceChecker.h | 4 +--- .../java/com/swmansion/reanimated/NativeProxy.java | 6 ++++++ .../src/main/cpp/reanimated/android/NativeProxy.cpp | 9 ++++++++- .../src/main/cpp/reanimated/android/NativeProxy.h | 2 ++ .../src/main/cpp/worklets/android/WorkletsModule.cpp | 8 +++++++- .../src/main/cpp/worklets/android/WorkletsModule.h | 12 +++++++----- .../java/com/swmansion/reanimated/NodesManager.java | 1 + .../java/com/swmansion/worklets/WorkletsModule.java | 6 ++++++ .../java/com/swmansion/reanimated/NativeProxy.java | 6 ++++++ .../apple/worklets/apple/WorkletsModule.mm | 9 +++++++++ 12 files changed, 62 insertions(+), 11 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 b159b070d6c..7f121f03674 100644 --- a/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp +++ b/packages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp @@ -844,4 +844,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 dd3b6661999..a1da65df3a2 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 3be7ce80fec..99184667b9b 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_ < 1, "[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 1701b938ca4..7921dde2b00 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 e8deed16428..c240d04e1ea 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,10 @@ void NativeProxy::setupLayoutAnimations() { }); } +void NativeProxy::invalidateCpp() { + workletsModuleProxy_.reset(); + 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 09a8e37e926..af8bdbccfbd 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 bd1b3ecd897..afbedf5b334 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 @@ -61,8 +61,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 094287271fc..5a8513ff5e0 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 @@ -46,11 +46,6 @@ 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, @@ -59,6 +54,13 @@ class WorkletsModule : public jni::HybridClass { const std::shared_ptr &jsCallInvoker, const std::shared_ptr &jsScheduler, const std::shared_ptr &uiScheduler); + + void invalidateCpp(); + + friend HybridBase; + jni::global_ref javaPart_; + 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 58cdc88924d..8cb9a929e6f 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 35cf7d07829..765888c38bc 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,12 @@ 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 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 7f7ebada7cd..a4330032a00 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 eed668545c4..a09966065da 100644 --- a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm +++ b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm @@ -52,4 +52,13 @@ @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 From 75bc3a50e5fbe98736d054e0697ec74b1592302b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20=C5=BBelawski?= Date: Tue, 24 Dec 2024 14:14:15 +0100 Subject: [PATCH 2/5] refactor: remove javaPart from WorkletsModule --- .../src/main/cpp/worklets/android/WorkletsModule.cpp | 7 ++----- .../android/src/main/cpp/worklets/android/WorkletsModule.h | 4 +--- .../main/java/com/swmansion/worklets/WorkletsModule.java | 2 +- 3 files changed, 4 insertions(+), 9 deletions(-) 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 afbedf5b334..fa8646fd9d5 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, 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 5a8513ff5e0..1cd7bf23b5a 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, @@ -47,7 +47,6 @@ class WorkletsModule : public jni::HybridClass { private: explicit WorkletsModule( - jni::alias_ref jThis, jsi::Runtime *rnRuntime, const std::string &valueUnpackerCode, jni::alias_ref messageQueueThread, @@ -58,7 +57,6 @@ class WorkletsModule : public jni::HybridClass { void invalidateCpp(); friend HybridBase; - jni::global_ref javaPart_; jsi::Runtime *rnRuntime_; std::shared_ptr workletsModuleProxy_; }; 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 765888c38bc..6f58f5ad70f 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 @@ -71,7 +71,7 @@ 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 destruction. + // to invalidated memory and will crash on its destruction. invalidateCpp(); mAndroidUIScheduler.deactivate(); } From 497b4e8953324510207ce7f362b038ef7201ab72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20=C5=BBelawski?= Date: Thu, 9 Jan 2025 12:22:21 +0100 Subject: [PATCH 3/5] chore: review changes --- .../Common/cpp/worklets/Tools/SingleInstanceChecker.h | 2 +- .../src/main/java/com/swmansion/worklets/WorkletsModule.java | 1 + .../apple/worklets/apple/WorkletsModule.mm | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) 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 99184667b9b..9c8c0eb23cb 100644 --- a/packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h +++ b/packages/react-native-reanimated/Common/cpp/worklets/Tools/SingleInstanceChecker.h @@ -53,7 +53,7 @@ SingleInstanceChecker::SingleInstanceChecker() { __cxxabiv1::__cxa_demangle(typeid(T).name(), nullptr, nullptr, &status); 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/main/java/com/swmansion/worklets/WorkletsModule.java b/packages/react-native-reanimated/android/src/main/java/com/swmansion/worklets/WorkletsModule.java index 6f58f5ad70f..5690e85aff8 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 @@ -73,6 +73,7 @@ public void invalidate() { // 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(); } diff --git a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm index a09966065da..dfa40970a12 100644 --- a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm +++ b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm @@ -58,6 +58,7 @@ - (void)invalidate // 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]; } From d1cea151ca055803168fc7caa347859cf0d71c9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20=C5=BBelawski?= Date: Thu, 9 Jan 2025 14:04:46 +0100 Subject: [PATCH 4/5] chore: fix formatting --- .../src/main/java/com/swmansion/worklets/WorkletsModule.java | 2 +- .../apple/worklets/apple/WorkletsModule.mm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 5690e85aff8..d5249f8050b 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 @@ -73,7 +73,7 @@ public void invalidate() { // 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(); } diff --git a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm index dfa40970a12..9dacb4b6f00 100644 --- a/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm +++ b/packages/react-native-reanimated/apple/worklets/apple/WorkletsModule.mm @@ -58,7 +58,7 @@ - (void)invalidate // 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]; } From c0aa521cc0b20e12f6a288d9dcefacab381e3be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20=C5=BBelawski?= Date: Fri, 10 Jan 2025 10:32:40 +0100 Subject: [PATCH 5/5] chore: add guard for invalidate call --- .../android/src/main/cpp/reanimated/android/NativeProxy.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 c240d04e1ea..5e608618072 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 @@ -622,7 +622,9 @@ void NativeProxy::setupLayoutAnimations() { void NativeProxy::invalidateCpp() { workletsModuleProxy_.reset(); - reanimatedModuleProxy_->invalidate(); + if (reanimatedModuleProxy_ != nullptr) { + reanimatedModuleProxy_->invalidate(); + } reanimatedModuleProxy_.reset(); }