destroy callbacks even if they aren't called, when java object is des…

…troyed Summary: JSI callbacks are only destroyed if the callback is called. If the callback is never called, we're potentially leaking a lot of callbacks. To mitigate this, we add a wrapper object that is owned by the std::function. Whenever the std::function is destroyed, the wrapper is destroyed and it deallocates the callback as well. Changelog: [Internal] Reviewed By: RSNara Differential Revision: D27436402 fbshipit-source-id: d153640d5d7988c7fadaf2cb332ec00dadd0689a
mysport12 · Apr 1, 2021 · 3d1afbb · 3d1afbb
1 parent 0901830
commit 3d1afbb
Show file tree

Hide file tree

Showing 7 changed files with 95 additions and 32 deletions.
diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java
@@ -23,6 +23,13 @@ public class ReactFeatureFlags {
    */
   public static volatile boolean useTurboModules = false;
 
+  /**
+   * Should application use the new TM callback manager in Cxx? This is assumed to be a sane
+   * default, but it's new. We will delete once (1) we know it's safe to ship and (2) we have
+   * quantified impact.
+   */
+  public static volatile boolean useTurboModulesRAIICallbackManager = false;
+
   /** Should we dispatch TurboModule methods with promise returns to the NativeModules thread? */
   public static volatile boolean enableTurboModulePromiseAsyncDispatch = false;
 

diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java
@@ -16,6 +16,7 @@
 import com.facebook.react.bridge.CxxModuleWrapper;
 import com.facebook.react.bridge.JSIModule;
 import com.facebook.react.bridge.RuntimeExecutor;
+import com.facebook.react.config.ReactFeatureFlags;
 import com.facebook.react.turbomodule.core.interfaces.CallInvokerHolder;
 import com.facebook.react.turbomodule.core.interfaces.TurboModule;
 import com.facebook.react.turbomodule.core.interfaces.TurboModuleRegistry;
@@ -58,7 +59,8 @@ public TurboModuleManager(
             runtimeExecutor,
             (CallInvokerHolderImpl) jsCallInvokerHolder,
             (CallInvokerHolderImpl) nativeCallInvokerHolder,
-            delegate);
+            delegate,
+            ReactFeatureFlags.useTurboModulesRAIICallbackManager);
     installJSIBindings();
 
     mEagerInitModuleNames =
@@ -290,7 +292,8 @@ private native HybridData initHybrid(
       RuntimeExecutor runtimeExecutor,
       CallInvokerHolderImpl jsCallInvokerHolder,
       CallInvokerHolderImpl nativeCallInvokerHolder,
-      TurboModuleManagerDelegate tmmDelegate);
+      TurboModuleManagerDelegate tmmDelegate,
+      boolean useTurboModulesRAIICallbackManager);
 
   private native void installJSIBindings();
 

diff --git a/.../src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/.../src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp
@@ -39,10 +39,15 @@ jni::local_ref<TurboModuleManager::jhybriddata> TurboModuleManager::initHybrid(
     jni::alias_ref<JRuntimeExecutor::javaobject> runtimeExecutor,
     jni::alias_ref<CallInvokerHolder::javaobject> jsCallInvokerHolder,
     jni::alias_ref<CallInvokerHolder::javaobject> nativeCallInvokerHolder,
-    jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate) {
+    jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate,
+    bool useTurboModulesRAIICallbackManager) {
   auto jsCallInvoker = jsCallInvokerHolder->cthis()->getCallInvoker();
   auto nativeCallInvoker = nativeCallInvokerHolder->cthis()->getCallInvoker();
 
+  if (useTurboModulesRAIICallbackManager) {
+    JavaTurboModule::enableUseTurboModulesRAIICallbackManager(true);
+  }
+
   return makeCxxInstance(
       jThis,
       runtimeExecutor->cthis()->get(),

diff --git a/...id/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h b/...id/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h
@@ -32,7 +32,8 @@ class TurboModuleManager : public jni::HybridClass<TurboModuleManager> {
       jni::alias_ref<JRuntimeExecutor::javaobject> runtimeExecutor,
       jni::alias_ref<CallInvokerHolder::javaobject> jsCallInvokerHolder,
       jni::alias_ref<CallInvokerHolder::javaobject> nativeCallInvokerHolder,
-      jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate);
+      jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate,
+      bool useTurboModulesRAIICallbackManager);
   static void registerNatives();
 
  private:

diff --git a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleUtils.h
@@ -84,5 +84,23 @@ class CallbackWrapper : public LongLivedObject {
   }
 };
 
+class RAIICallbackWrapperDestroyer {
+ public:
+  RAIICallbackWrapperDestroyer(std::weak_ptr<CallbackWrapper> callbackWrapper)
+      : callbackWrapper_(callbackWrapper) {}
+
+  ~RAIICallbackWrapperDestroyer() {
+    auto strongWrapper = callbackWrapper_.lock();
+    if (!strongWrapper) {
+      return;
+    }
+
+    strongWrapper->destroy();
+  }
+
+ private:
+  std::weak_ptr<CallbackWrapper> callbackWrapper_;
+};
+
 } // namespace react
 } // namespace facebook
diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp
@@ -52,6 +52,11 @@ JavaTurboModule::~JavaTurboModule() {
   });
 }
 
+bool JavaTurboModule::useTurboModulesRAIICallbackManager_ = false;
+void JavaTurboModule::enableUseTurboModulesRAIICallbackManager(bool enable) {
+  JavaTurboModule::useTurboModulesRAIICallbackManager_ = enable;
+}
+
 namespace {
 jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
     jsi::Function &&function,
@@ -60,9 +65,20 @@ jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
   auto weakWrapper =
       react::CallbackWrapper::createWeak(std::move(function), rt, jsInvoker);
 
+  // This needs to be a shared_ptr because:
+  // 1. It cannot be unique_ptr. std::function is copyable but unique_ptr is
+  // not.
+  // 2. It cannot be weak_ptr since we need this object to live on.
+  // 3. It cannot be a value, because that would be deleted as soon as this
+  // function returns.
+  auto callbackWrapperOwner =
+      (JavaTurboModule::useTurboModulesRAIICallbackManager_
+           ? std::make_shared<RAIICallbackWrapperDestroyer>(weakWrapper)
+           : nullptr);
+
   std::function<void(folly::dynamic)> fn =
-      [weakWrapper,
-       wrapperWasCalled = false](folly::dynamic responses) mutable {
+      [weakWrapper, callbackWrapperOwner, wrapperWasCalled = false](
+          folly::dynamic responses) mutable {
         if (wrapperWasCalled) {
           throw std::runtime_error(
               "callback 2 arg cannot be called more than once");
@@ -73,35 +89,41 @@ jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
           return;
         }
 
-        strongWrapper->jsInvoker().invokeAsync([weakWrapper, responses]() {
-          auto strongWrapper2 = weakWrapper.lock();
-          if (!strongWrapper2) {
-            return;
-          }
-
-          // TODO (T43155926) valueFromDynamic already returns a Value array.
-          // Don't iterate again
-          jsi::Value args =
-              jsi::valueFromDynamic(strongWrapper2->runtime(), responses);
-          auto argsArray = args.getObject(strongWrapper2->runtime())
-                               .asArray(strongWrapper2->runtime());
-          std::vector<jsi::Value> result;
-          for (size_t i = 0; i < argsArray.size(strongWrapper2->runtime());
-               i++) {
-            result.emplace_back(
-                strongWrapper2->runtime(),
-                argsArray.getValueAtIndex(strongWrapper2->runtime(), i));
-          }
-          strongWrapper2->callback().call(
-              strongWrapper2->runtime(),
-              (const jsi::Value *)result.data(),
-              result.size());
-
-          strongWrapper2->destroy();
-        });
+        strongWrapper->jsInvoker().invokeAsync(
+            [weakWrapper, callbackWrapperOwner, responses]() mutable {
+              auto strongWrapper2 = weakWrapper.lock();
+              if (!strongWrapper2) {
+                return;
+              }
+
+              // TODO (T43155926) valueFromDynamic already returns a Value
+              // array. Don't iterate again
+              jsi::Value args =
+                  jsi::valueFromDynamic(strongWrapper2->runtime(), responses);
+              auto argsArray = args.getObject(strongWrapper2->runtime())
+                                   .asArray(strongWrapper2->runtime());
+              std::vector<jsi::Value> result;
+              for (size_t i = 0; i < argsArray.size(strongWrapper2->runtime());
+                   i++) {
+                result.emplace_back(
+                    strongWrapper2->runtime(),
+                    argsArray.getValueAtIndex(strongWrapper2->runtime(), i));
+              }
+              strongWrapper2->callback().call(
+                  strongWrapper2->runtime(),
+                  (const jsi::Value *)result.data(),
+                  result.size());
+
+              if (JavaTurboModule::useTurboModulesRAIICallbackManager_) {
+                callbackWrapperOwner.reset();
+              } else {
+                strongWrapper2->destroy();
+              }
+            });
 
         wrapperWasCalled = true;
       };
+
   return JCxxCallbackImpl::newObjectCxxArgs(fn);
 }
 

diff --git a/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h b/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.h
@@ -51,6 +51,13 @@ class JSI_EXPORT JavaTurboModule : public TurboModule {
       const jsi::Value *args,
       size_t argCount);
 
+  static void enableUseTurboModulesRAIICallbackManager(bool enable);
+
+  /**
+   * Experiments
+   */
+  static bool useTurboModulesRAIICallbackManager_;
+
  private:
   jni::global_ref<JTurboModule> instance_;
   std::shared_ptr<CallInvoker> nativeInvoker_;