Move ownership of TurboModule jni ref to JavaTurboModule

Summary: When you create a TurboModule from the JS side, we instantiate its Java class and simply make this `javaobject` a `jni::global_ref` in C++. But the reason why we need to make this a global ref is because `JavaTurboModule` needs it to be a global reference for method calls. Making this a `jni::global_ref` from the perspective to TurboModuleManager doesn't really make any sense. So, this diff refactors that bit of code.

Differential Revision: D16622133

fbshipit-source-id: 6a5c20bb405b945c06378a3423d5e7eb38ef244c

ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp

@@ -24,23 +24,23 @@ TurboModuleManager::TurboModuleManager(
   jni::alias_ref<TurboModuleManager::javaobject> jThis,
   jsi::Runtime* rt,
   std::shared_ptr<JSCallInvoker> jsCallInvoker,
-  jni::alias_ref<TurboModuleManagerDelegate::javaobject> tmmDelegate
+  jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate
 ):
   javaPart_(jni::make_global(jThis)),
   runtime_(rt),
   jsCallInvoker_(jsCallInvoker),
-  turboModuleManagerDelegate_(jni::make_global(tmmDelegate))
+  delegate_(jni::make_global(delegate))
   {}
 
 jni::local_ref<TurboModuleManager::jhybriddata> TurboModuleManager::initHybrid(
   jni::alias_ref<jhybridobject> jThis,
   jlong jsContext,
   jni::alias_ref<JSCallInvokerHolder::javaobject> jsCallInvokerHolder,
-  jni::alias_ref<TurboModuleManagerDelegate::javaobject> tmmDelegate
+  jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate
 ) {
   auto jsCallInvoker = jsCallInvokerHolder->cthis()->getJSCallInvoker();
 
-  return makeCxxInstance(jThis, (jsi::Runtime *) jsContext, jsCallInvoker, tmmDelegate);
+  return makeCxxInstance(jThis, (jsi::Runtime *) jsContext, jsCallInvoker, delegate);
 }
 
 void TurboModuleManager::registerNatives() {
@@ -61,23 +61,26 @@ void TurboModuleManager::installJSIBindings() {
           return turboModuleLookup->second;
         }
 
-        auto cxxModule = turboModuleManagerDelegate_->cthis()->getTurboModule(name, jsCallInvoker_);
+        auto cxxModule = delegate_->cthis()->getTurboModule(name, jsCallInvoker_);
         if (cxxModule) {
           turboModuleCache_.insert({name, cxxModule});
           return cxxModule;
         }
 
-        auto legacyCxxModule = getLegacyCxxJavaModule(name);
+        static auto getLegacyCxxModule = delegate_->getClass()->getMethod<jni::alias_ref<CxxModuleWrapper::javaobject>(const std::string&)>("getLegacyCxxModule");
+        auto legacyCxxModule = getLegacyCxxModule(delegate_.get(), name);
+
         if (legacyCxxModule) {
           auto turboModule = std::make_shared<react::TurboCxxModule>(legacyCxxModule->cthis()->getModule(), jsCallInvoker_);
           turboModuleCache_.insert({name, turboModule});
           return turboModule;
         }
 
-        auto moduleInstance = getJavaModule(name);
+        static auto getJavaModule = javaClassStatic()->getMethod<jni::alias_ref<JTurboModule>(const std::string&)>("getJavaModule");
+        auto moduleInstance = getJavaModule(javaPart_.get(), name);
 
         if (moduleInstance) {
-          auto turboModule = turboModuleManagerDelegate_->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker_);
+          auto turboModule = delegate_->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker_);
           turboModuleCache_.insert({name, turboModule});
           return turboModule;
         }
@@ -87,19 +90,5 @@ void TurboModuleManager::installJSIBindings() {
   );
 }
 
-jni::global_ref<JTurboModule> TurboModuleManager::getJavaModule(std::string name) {
-  static auto method = javaClassStatic()->getMethod<jni::alias_ref<JTurboModule>(const std::string&)>("getJavaModule");
-
-  auto module = jni::make_global(method(javaPart_.get(), name));
-
-  return module;
-}
-
-jni::global_ref<CxxModuleWrapper::javaobject> TurboModuleManager::getLegacyCxxJavaModule(std::string name) {
-  static auto method = turboModuleManagerDelegate_->getClass()->getMethod<jni::alias_ref<CxxModuleWrapper::javaobject>(const std::string&)>("getLegacyCxxModule");
-  auto module = jni::make_global(method(turboModuleManagerDelegate_.get(), name));
-  return module;
-}
-
 } // namespace react
 } // namespace facebook

ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h

@@ -28,7 +28,7 @@ public:
     jni::alias_ref<jhybridobject> jThis,
     jlong jsContext,
     jni::alias_ref<JSCallInvokerHolder::javaobject> jsCallInvokerHolder,
-    jni::alias_ref<TurboModuleManagerDelegate::javaobject> tmmDelegate
+    jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate
   );
   static void registerNatives();
 private:
@@ -36,7 +36,7 @@ private:
   jni::global_ref<TurboModuleManager::javaobject> javaPart_;
   jsi::Runtime* runtime_;
   std::shared_ptr<JSCallInvoker> jsCallInvoker_;
-  jni::global_ref<TurboModuleManagerDelegate::javaobject> turboModuleManagerDelegate_;
+  jni::global_ref<TurboModuleManagerDelegate::javaobject> delegate_;
 
   /**
    * TODO(T48018690):
@@ -46,14 +46,12 @@ private:
    */
   std::unordered_map<std::string, std::shared_ptr<react::TurboModule>> turboModuleCache_;
 
-  jni::global_ref<JTurboModule> getJavaModule(std::string name);
-  jni::global_ref<CxxModuleWrapper::javaobject> getLegacyCxxJavaModule(std::string name);
   void installJSIBindings();
   explicit TurboModuleManager(
     jni::alias_ref<TurboModuleManager::jhybridobject> jThis,
     jsi::Runtime *rt,
     std::shared_ptr<JSCallInvoker> jsCallInvoker,
-    jni::alias_ref<TurboModuleManagerDelegate::javaobject> tmmDelegate
+    jni::alias_ref<TurboModuleManagerDelegate::javaobject> delegate
   );
 };
 

ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManagerDelegate.h

@@ -20,7 +20,7 @@ class TurboModuleManagerDelegate : public jni::HybridClass<TurboModuleManagerDel
 public:
   static auto constexpr kJavaDescriptor = "Lcom/facebook/react/turbomodule/core/TurboModuleManagerDelegate;";
 
-  virtual std::shared_ptr<TurboModule> getTurboModule(std::string name, jni::global_ref<JTurboModule> turboModule, std::shared_ptr<JSCallInvoker> jsInvoker) = 0;
+  virtual std::shared_ptr<TurboModule> getTurboModule(std::string name, jni::alias_ref<JTurboModule> turboModule, std::shared_ptr<JSCallInvoker> jsInvoker) = 0;
   virtual std::shared_ptr<TurboModule> getTurboModule(std::string name, std::shared_ptr<JSCallInvoker> jsInvoker) = 0;
 
 private:

ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp

@@ -25,8 +25,11 @@
 namespace facebook {
 namespace react {
 
-JavaTurboModule::JavaTurboModule(const std::string &name, jni::global_ref<JTurboModule> instance, std::shared_ptr<JSCallInvoker> jsInvoker)
-  : TurboModule(name, jsInvoker), instance_(instance) {}
+JavaTurboModule::JavaTurboModule(
+    const std::string &name,
+    jni::alias_ref<JTurboModule> instance,
+    std::shared_ptr<JSCallInvoker> jsInvoker)
+    : TurboModule(name, jsInvoker), instance_(jni::make_global(instance)) {}
 
 jni::local_ref<JCxxCallbackImpl::JavaPart> createJavaCallbackFromJSIFunction(
     jsi::Function &function,
@@ -322,16 +325,20 @@ std::vector<jvalue> convertJSIArgsToJNIArgs(
 }
 
 jsi::Value convertFromJMapToValue(JNIEnv *env, jsi::Runtime &rt, jobject arg) {
-    // We currently use Java Argument.makeNativeMap() method to do this conversion
-    // This could also be done purely in C++, but iterative over map methods
-    // but those may end up calling reflection methods anyway
-    // TODO (axe) Investigate the best way to convert Java Map to Value
-    jclass jArguments = env->FindClass("com/facebook/react/bridge/Arguments");
-    static jmethodID jMakeNativeMap = env->GetStaticMethodID(jArguments, "makeNativeMap", "(Ljava/util/Map;)Lcom/facebook/react/bridge/WritableNativeMap;");
-    auto constants = (jobject) env->CallStaticObjectMethod(jArguments, jMakeNativeMap, arg);
-    auto jResult = jni::adopt_local(constants);
-    auto result = jni::static_ref_cast<NativeMap::jhybridobject>(jResult);
-    return jsi::valueFromDynamic(rt, result->cthis()->consume());
+  // We currently use Java Argument.makeNativeMap() method to do this conversion
+  // This could also be done purely in C++, but iterative over map methods
+  // but those may end up calling reflection methods anyway
+  // TODO (axe) Investigate the best way to convert Java Map to Value
+  jclass jArguments = env->FindClass("com/facebook/react/bridge/Arguments");
+  static jmethodID jMakeNativeMap = env->GetStaticMethodID(
+      jArguments,
+      "makeNativeMap",
+      "(Ljava/util/Map;)Lcom/facebook/react/bridge/WritableNativeMap;");
+  auto constants =
+      (jobject)env->CallStaticObjectMethod(jArguments, jMakeNativeMap, arg);
+  auto jResult = jni::adopt_local(constants);
+  auto result = jni::static_ref_cast<NativeMap::jhybridobject>(jResult);
+  return jsi::valueFromDynamic(rt, result->cthis()->consume());
 }
 
 jsi::Value JavaTurboModule::invokeJavaMethod(

ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h

@@ -17,12 +17,16 @@ namespace facebook {
 namespace react {
 
 struct JTurboModule : jni::JavaClass<JTurboModule> {
-  static auto constexpr kJavaDescriptor = "Lcom/facebook/react/turbomodule/core/interfaces/TurboModule;";
+  static auto constexpr kJavaDescriptor =
+      "Lcom/facebook/react/turbomodule/core/interfaces/TurboModule;";
 };
 
 class JSI_EXPORT JavaTurboModule : public TurboModule {
-public:
-  JavaTurboModule(const std::string &name, jni::global_ref<JTurboModule> instance, std::shared_ptr<JSCallInvoker> jsInvoker);
+ public:
+  JavaTurboModule(
+      const std::string &name,
+      jni::alias_ref<JTurboModule> instance,
+      std::shared_ptr<JSCallInvoker> jsInvoker);
   jsi::Value invokeJavaMethod(
       jsi::Runtime &runtime,
       TurboModuleMethodValueKind valueKind,
@@ -30,7 +34,8 @@ public:
       const std::string &methodSignature,
       const jsi::Value *args,
       size_t count);
-private:
+
+ private:
   jni::global_ref<JTurboModule> instance_;
   jclass findClass(JNIEnv *env) const;
 };