From 88e03fa4db87b8fba90f018f0e80daf2299478b4 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Thu, 1 Aug 2019 16:31:48 -0700 Subject: [PATCH] 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. Reviewed By: mdvacca Differential Revision: D16555673 fbshipit-source-id: 2778fc5a372c41847e8296c2e22bb9a8826fcc52 --- .../jni/ReactCommon/TurboModuleManager.cpp | 33 +++++++------------ .../core/jni/ReactCommon/TurboModuleManager.h | 8 ++--- .../ReactCommon/TurboModuleManagerDelegate.h | 2 +- .../core/platform/android/JavaTurboModule.cpp | 31 ++++++++++------- .../core/platform/android/JavaTurboModule.h | 13 +++++--- 5 files changed, 43 insertions(+), 44 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp index e5106e639d5..2f349c1b3c1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp @@ -24,23 +24,23 @@ TurboModuleManager::TurboModuleManager( jni::alias_ref jThis, jsi::Runtime* rt, std::shared_ptr jsCallInvoker, - jni::alias_ref tmmDelegate + jni::alias_ref delegate ): javaPart_(jni::make_global(jThis)), runtime_(rt), jsCallInvoker_(jsCallInvoker), - turboModuleManagerDelegate_(jni::make_global(tmmDelegate)) + delegate_(jni::make_global(delegate)) {} jni::local_ref TurboModuleManager::initHybrid( jni::alias_ref jThis, jlong jsContext, jni::alias_ref jsCallInvokerHolder, - jni::alias_ref tmmDelegate + jni::alias_ref 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(const std::string&)>("getLegacyCxxModule"); + auto legacyCxxModule = getLegacyCxxModule(delegate_.get(), name); + if (legacyCxxModule) { auto turboModule = std::make_shared(legacyCxxModule->cthis()->getModule(), jsCallInvoker_); turboModuleCache_.insert({name, turboModule}); return turboModule; } - auto moduleInstance = getJavaModule(name); + static auto getJavaModule = javaClassStatic()->getMethod(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 TurboModuleManager::getJavaModule(std::string name) { - static auto method = javaClassStatic()->getMethod(const std::string&)>("getJavaModule"); - - auto module = jni::make_global(method(javaPart_.get(), name)); - - return module; -} - -jni::global_ref TurboModuleManager::getLegacyCxxJavaModule(std::string name) { - static auto method = turboModuleManagerDelegate_->getClass()->getMethod(const std::string&)>("getLegacyCxxModule"); - auto module = jni::make_global(method(turboModuleManagerDelegate_.get(), name)); - return module; -} - } // namespace react } // namespace facebook diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h index 269b2da367c..f72d8d4c955 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h @@ -28,7 +28,7 @@ public: jni::alias_ref jThis, jlong jsContext, jni::alias_ref jsCallInvokerHolder, - jni::alias_ref tmmDelegate + jni::alias_ref delegate ); static void registerNatives(); private: @@ -36,7 +36,7 @@ private: jni::global_ref javaPart_; jsi::Runtime* runtime_; std::shared_ptr jsCallInvoker_; - jni::global_ref turboModuleManagerDelegate_; + jni::global_ref delegate_; /** * TODO(T48018690): @@ -46,14 +46,12 @@ private: */ std::unordered_map> turboModuleCache_; - jni::global_ref getJavaModule(std::string name); - jni::global_ref getLegacyCxxJavaModule(std::string name); void installJSIBindings(); explicit TurboModuleManager( jni::alias_ref jThis, jsi::Runtime *rt, std::shared_ptr jsCallInvoker, - jni::alias_ref tmmDelegate + jni::alias_ref delegate ); }; diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManagerDelegate.h b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManagerDelegate.h index fe07922c99b..1732e82202d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManagerDelegate.h +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManagerDelegate.h @@ -20,7 +20,7 @@ class TurboModuleManagerDelegate : public jni::HybridClass getTurboModule(std::string name, jni::global_ref turboModule, std::shared_ptr jsInvoker) = 0; + virtual std::shared_ptr getTurboModule(std::string name, jni::alias_ref turboModule, std::shared_ptr jsInvoker) = 0; virtual std::shared_ptr getTurboModule(std::string name, std::shared_ptr jsInvoker) = 0; private: diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp index dd416b0de95..86a01ef0729 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp @@ -25,8 +25,11 @@ namespace facebook { namespace react { -JavaTurboModule::JavaTurboModule(const std::string &name, jni::global_ref instance, std::shared_ptr jsInvoker) - : TurboModule(name, jsInvoker), instance_(instance) {} +JavaTurboModule::JavaTurboModule( + const std::string &name, + jni::alias_ref instance, + std::shared_ptr jsInvoker) + : TurboModule(name, jsInvoker), instance_(jni::make_global(instance)) {} jni::local_ref createJavaCallbackFromJSIFunction( jsi::Function &function, @@ -322,16 +325,20 @@ std::vector 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(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(jResult); + return jsi::valueFromDynamic(rt, result->cthis()->consume()); } jsi::Value JavaTurboModule::invokeJavaMethod( diff --git a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h index 891b67fe8a0..9e3648bcfcf 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h @@ -17,12 +17,16 @@ namespace facebook { namespace react { struct JTurboModule : jni::JavaClass { - 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 instance, std::shared_ptr jsInvoker); + public: + JavaTurboModule( + const std::string &name, + jni::alias_ref instance, + std::shared_ptr 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 instance_; jclass findClass(JNIEnv *env) const; };