From af4862ce32b96eb59cb60425d5ee1340efc27d0b Mon Sep 17 00:00:00 2001 From: Eric Williamson Date: Thu, 1 Aug 2019 21:55:45 -0700 Subject: [PATCH] Revert D16555673: [RN][TurboModule] Move ownership of TurboModule jni ref to JavaTurboModule Differential Revision: D16555673 Original commit changeset: 2778fc5a372c fbshipit-source-id: fac3a6ea185acaa750f58e19d24c194668749636 --- .../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, 44 insertions(+), 43 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 2f349c1b3c1..e5106e639d5 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 delegate + jni::alias_ref tmmDelegate ): javaPart_(jni::make_global(jThis)), runtime_(rt), jsCallInvoker_(jsCallInvoker), - delegate_(jni::make_global(delegate)) + turboModuleManagerDelegate_(jni::make_global(tmmDelegate)) {} jni::local_ref TurboModuleManager::initHybrid( jni::alias_ref jThis, jlong jsContext, jni::alias_ref jsCallInvokerHolder, - jni::alias_ref delegate + jni::alias_ref tmmDelegate ) { auto jsCallInvoker = jsCallInvokerHolder->cthis()->getJSCallInvoker(); - return makeCxxInstance(jThis, (jsi::Runtime *) jsContext, jsCallInvoker, delegate); + return makeCxxInstance(jThis, (jsi::Runtime *) jsContext, jsCallInvoker, tmmDelegate); } void TurboModuleManager::registerNatives() { @@ -61,26 +61,23 @@ void TurboModuleManager::installJSIBindings() { return turboModuleLookup->second; } - auto cxxModule = delegate_->cthis()->getTurboModule(name, jsCallInvoker_); + auto cxxModule = turboModuleManagerDelegate_->cthis()->getTurboModule(name, jsCallInvoker_); if (cxxModule) { turboModuleCache_.insert({name, cxxModule}); return cxxModule; } - static auto getLegacyCxxModule = delegate_->getClass()->getMethod(const std::string&)>("getLegacyCxxModule"); - auto legacyCxxModule = getLegacyCxxModule(delegate_.get(), name); - + auto legacyCxxModule = getLegacyCxxJavaModule(name); if (legacyCxxModule) { auto turboModule = std::make_shared(legacyCxxModule->cthis()->getModule(), jsCallInvoker_); turboModuleCache_.insert({name, turboModule}); return turboModule; } - static auto getJavaModule = javaClassStatic()->getMethod(const std::string&)>("getJavaModule"); - auto moduleInstance = getJavaModule(javaPart_.get(), name); + auto moduleInstance = getJavaModule(name); if (moduleInstance) { - auto turboModule = delegate_->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker_); + auto turboModule = turboModuleManagerDelegate_->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker_); turboModuleCache_.insert({name, turboModule}); return turboModule; } @@ -90,5 +87,19 @@ 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 f72d8d4c955..269b2da367c 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 delegate + jni::alias_ref tmmDelegate ); static void registerNatives(); private: @@ -36,7 +36,7 @@ private: jni::global_ref javaPart_; jsi::Runtime* runtime_; std::shared_ptr jsCallInvoker_; - jni::global_ref delegate_; + jni::global_ref turboModuleManagerDelegate_; /** * TODO(T48018690): @@ -46,12 +46,14 @@ 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 delegate + jni::alias_ref tmmDelegate ); }; 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 1732e82202d..fe07922c99b 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::alias_ref turboModule, std::shared_ptr jsInvoker) = 0; + virtual std::shared_ptr getTurboModule(std::string name, jni::global_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 86a01ef0729..dd416b0de95 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.cpp @@ -25,11 +25,8 @@ namespace facebook { namespace react { -JavaTurboModule::JavaTurboModule( - const std::string &name, - jni::alias_ref instance, - std::shared_ptr jsInvoker) - : TurboModule(name, jsInvoker), instance_(jni::make_global(instance)) {} +JavaTurboModule::JavaTurboModule(const std::string &name, jni::global_ref instance, std::shared_ptr jsInvoker) + : TurboModule(name, jsInvoker), instance_(instance) {} jni::local_ref createJavaCallbackFromJSIFunction( jsi::Function &function, @@ -325,20 +322,16 @@ 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 9e3648bcfcf..891b67fe8a0 100644 --- a/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h +++ b/ReactCommon/turbomodule/core/platform/android/JavaTurboModule.h @@ -17,16 +17,12 @@ 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::alias_ref instance, - std::shared_ptr jsInvoker); +public: + JavaTurboModule(const std::string &name, jni::global_ref instance, std::shared_ptr jsInvoker); jsi::Value invokeJavaMethod( jsi::Runtime &runtime, TurboModuleMethodValueKind valueKind, @@ -34,8 +30,7 @@ class JSI_EXPORT JavaTurboModule : public TurboModule { const std::string &methodSignature, const jsi::Value *args, size_t count); - - private: +private: jni::global_ref instance_; jclass findClass(JNIEnv *env) const; };