From a893d0bb231189d5330a28e9256a6ff5e56e5dab Mon Sep 17 00:00:00 2001 From: Marc Horowitz Date: Wed, 5 Apr 2017 00:51:55 -0700 Subject: [PATCH] Refactor CxxModuleWrapper to support different use cases Reviewed By: javache Differential Revision: D4807680 fbshipit-source-id: 48eccfb382814a0c4082f56617e0359e61345da7 --- .../react/cxxbridge/CxxModuleWrapper.java | 65 +++---------------- .../react/cxxbridge/CxxModuleWrapperBase.java | 60 +++++++++++++++++ .../react/cxxbridge/NativeModuleRegistry.java | 4 +- ReactAndroid/src/main/jni/xreact/jni/BUCK | 2 + .../main/jni/xreact/jni/CxxModuleWrapper.cpp | 23 ++----- .../main/jni/xreact/jni/CxxModuleWrapper.h | 38 ++++------- .../jni/xreact/jni/CxxModuleWrapperBase.h | 43 ++++++++++++ .../jni/xreact/jni/CxxSharedModuleWrapper.h | 33 ++++++++++ .../jni/xreact/jni/ModuleRegistryBuilder.cpp | 4 +- .../src/main/jni/xreact/jni/OnLoad.cpp | 2 + .../src/main/jni/xreact/perftests/OnLoad.cpp | 6 +- 11 files changed, 179 insertions(+), 101 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapperBase.java create mode 100644 ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapperBase.h create mode 100644 ReactAndroid/src/main/jni/xreact/jni/CxxSharedModuleWrapper.h diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapper.java index 93f8006ce..c576290b8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapper.java @@ -2,70 +2,25 @@ package com.facebook.react.cxxbridge; -import java.util.Map; - import com.facebook.jni.HybridData; import com.facebook.proguard.annotations.DoNotStrip; -import com.facebook.react.bridge.CatalystInstance; -import com.facebook.react.bridge.ExecutorToken; -import com.facebook.react.bridge.NativeModule; -import com.facebook.react.bridge.ReadableNativeArray; import com.facebook.soloader.SoLoader; /** - * A Java Object which represents a cross-platform C++ module - * - * This module implements the NativeModule interface but will never be invoked from Java, - * instead the underlying Cxx module will be extracted by the bridge and called directly. + * This does nothing interesting, except avoid breaking existing code. */ @DoNotStrip -public class CxxModuleWrapper implements NativeModule +public class CxxModuleWrapper extends CxxModuleWrapperBase { - static { - SoLoader.loadLibrary(CatalystInstanceImpl.REACT_NATIVE_LIB); - } - - @DoNotStrip - private HybridData mHybridData; - - public CxxModuleWrapper(String library, String factory) { - SoLoader.loadLibrary(library); - mHybridData = - initHybrid(SoLoader.unpackLibraryAndDependencies(library).getAbsolutePath(), factory); - } - - @Override - public native String getName(); - - @Override - public Map getMethods() { - throw new UnsupportedOperationException(); - } - - @Override - public void initialize() { - // do nothing - } - - @Override - public boolean canOverrideExistingModule() { - return false; - } - - @Override - public boolean supportsWebWorkers() { - return false; - } - - @Override - public void onCatalystInstanceDestroy() { - mHybridData.resetNative(); - } - - // For creating a wrapper from C++, or from a derived class. protected CxxModuleWrapper(HybridData hd) { - mHybridData = hd; + super(hd); } - private native HybridData initHybrid(String soPath, String factory); + private static native CxxModuleWrapper makeDsoNative(String soPath, String factory); + + public static CxxModuleWrapper makeDso(String library, String factory) { + SoLoader.loadLibrary(library); + String soPath = SoLoader.unpackLibraryAndDependencies(library).getAbsolutePath(); + return makeDsoNative(soPath, factory); + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapperBase.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapperBase.java new file mode 100644 index 000000000..acb06c0e0 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapperBase.java @@ -0,0 +1,60 @@ +// Copyright 2004-present Facebook. All Rights Reserved. + +package com.facebook.react.cxxbridge; + +import java.util.Map; + +import com.facebook.jni.HybridData; +import com.facebook.proguard.annotations.DoNotStrip; +import com.facebook.react.bridge.NativeModule; +import com.facebook.soloader.SoLoader; + +/** + * A Java Object which represents a cross-platform C++ module + * + * This module implements the NativeModule interface but will never be invoked from Java, + * instead the underlying Cxx module will be extracted by the bridge and called directly. + */ +@DoNotStrip +public class CxxModuleWrapperBase implements NativeModule +{ + static { + SoLoader.loadLibrary(CatalystInstanceImpl.REACT_NATIVE_LIB); + } + + @DoNotStrip + private HybridData mHybridData; + + @Override + public native String getName(); + + @Override + public Map getMethods() { + throw new UnsupportedOperationException(); + } + + @Override + public void initialize() { + // do nothing + } + + @Override + public boolean canOverrideExistingModule() { + return false; + } + + @Override + public boolean supportsWebWorkers() { + return false; + } + + @Override + public void onCatalystInstanceDestroy() { + mHybridData.resetNative(); + } + + // For creating a wrapper from C++, or from a derived class. + protected CxxModuleWrapperBase(HybridData hd) { + mHybridData = hd; + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/NativeModuleRegistry.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/NativeModuleRegistry.java index 140c4efdb..112f45cb0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/NativeModuleRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/NativeModuleRegistry.java @@ -46,7 +46,7 @@ public class NativeModuleRegistry { ArrayList javaModules = new ArrayList<>(); for (Map.Entry, ModuleHolder> entry : mModules.entrySet()) { Class type = entry.getKey(); - if (!CxxModuleWrapper.class.isAssignableFrom(type)) { + if (!CxxModuleWrapperBase.class.isAssignableFrom(type)) { javaModules.add(new JavaModuleWrapper(jsInstance, entry.getValue())); } } @@ -57,7 +57,7 @@ public class NativeModuleRegistry { ArrayList cxxModules = new ArrayList<>(); for (Map.Entry, ModuleHolder> entry : mModules.entrySet()) { Class type = entry.getKey(); - if (CxxModuleWrapper.class.isAssignableFrom(type)) { + if (CxxModuleWrapperBase.class.isAssignableFrom(type)) { cxxModules.add(entry.getValue()); } } diff --git a/ReactAndroid/src/main/jni/xreact/jni/BUCK b/ReactAndroid/src/main/jni/xreact/jni/BUCK index f58ec9e4b..344814039 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/BUCK +++ b/ReactAndroid/src/main/jni/xreact/jni/BUCK @@ -2,6 +2,8 @@ include_defs("//ReactAndroid/DEFS") EXPORTED_HEADERS = [ "CxxModuleWrapper.h", + "CxxModuleWrapperBase.h", + "CxxSharedModuleWrapper.h", "JavaModuleWrapper.h", "JExecutorToken.h", "JSLoader.h", diff --git a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp index 82c7ada5c..a5650ba76 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp @@ -2,27 +2,18 @@ #include "CxxModuleWrapper.h" -#include -#include - #include -#include - #include using namespace facebook::jni; using namespace facebook::xplat::module; -using namespace facebook::react; -void CxxModuleWrapper::registerNatives() { - registerHybrid({ - makeNativeMethod("initHybrid", CxxModuleWrapper::initHybrid), - makeNativeMethod("getName", CxxModuleWrapper::getName) - }); -} +namespace facebook { +namespace react { -CxxModuleWrapper::CxxModuleWrapper(const std::string& soPath, const std::string& fname) { +jni::local_ref CxxModuleWrapper::makeDsoNative( + jni::alias_ref, const std::string& soPath, const std::string& fname) { // soPath is the path of a library which has already been loaded by // java SoLoader.loadLibrary(). So this returns the same handle, // and increments the reference counter. We can't just use @@ -46,9 +37,9 @@ CxxModuleWrapper::CxxModuleWrapper(const std::string& soPath, const std::string& fname.c_str(), soPath.c_str()); } auto factory = reinterpret_cast(sym); - module_.reset((*factory)()); + + return CxxModuleWrapper::newObjectCxxArgs(std::unique_ptr((*factory)())); } -std::string CxxModuleWrapper::getName() { - return module_->getName(); +} } diff --git a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h index 1d5e8ed0c..11551e3df 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h +++ b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h @@ -2,43 +2,31 @@ #pragma once -#include -#include -#include - -#include -#include +#include "CxxModuleWrapperBase.h" namespace facebook { namespace react { -struct JNativeModule : jni::JavaClass { - constexpr static const char *const kJavaDescriptor = - "Lcom/facebook/react/bridge/NativeModule;"; -}; - -class CxxModuleWrapper : - public jni::HybridClass { +class CxxModuleWrapper : public jni::HybridClass { public: constexpr static const char *const kJavaDescriptor = "Lcom/facebook/react/cxxbridge/CxxModuleWrapper;"; - static void registerNatives(); - - CxxModuleWrapper(const std::string& soPath, const std::string& fname); - - static jni::local_ref initHybrid( - jni::alias_ref, const std::string& soPath, const std::string& fname) { - return makeCxxInstance(soPath, fname); + static void registerNatives() { + registerHybrid({ + makeNativeMethod("makeDsoNative", CxxModuleWrapper::makeDsoNative) + }); } - // JNI methods - std::string getName(); + static jni::local_ref makeDsoNative( + jni::alias_ref, const std::string& soPath, const std::string& fname); + + std::string getName() override { + return module_->getName(); + } // This steals ownership of the underlying module for use by the C++ bridge - std::unique_ptr getModule() { - // TODO mhorowitz: remove this (and a lot of other code) once the java - // bridge is dead. + std::unique_ptr getModule() override { return std::move(module_); } diff --git a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapperBase.h b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapperBase.h new file mode 100644 index 000000000..7ae922e4a --- /dev/null +++ b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapperBase.h @@ -0,0 +1,43 @@ +// Copyright 2004-present Facebook. All Rights Reserved. + +#pragma once + +#include +#include + +#include +#include + +namespace facebook { +namespace react { + +struct JNativeModule : jni::JavaClass { + constexpr static const char *const kJavaDescriptor = + "Lcom/facebook/react/bridge/NativeModule;"; +}; + +/** + * The C++ part of a CxxModuleWrapper is not a unique class, but it + * must extend this base class. + */ +class CxxModuleWrapperBase + : public jni::HybridClass { +public: + constexpr static const char *const kJavaDescriptor = + "Lcom/facebook/react/cxxbridge/CxxModuleWrapperBase;"; + + static void registerNatives() { + registerHybrid({ + makeNativeMethod("getName", CxxModuleWrapperBase::getName) + }); + } + + // JNI method + virtual std::string getName() = 0; + + // Called by ModuleRegistryBuilder + virtual std::unique_ptr getModule() = 0; +}; + +} +} diff --git a/ReactAndroid/src/main/jni/xreact/jni/CxxSharedModuleWrapper.h b/ReactAndroid/src/main/jni/xreact/jni/CxxSharedModuleWrapper.h new file mode 100644 index 000000000..81ca0a200 --- /dev/null +++ b/ReactAndroid/src/main/jni/xreact/jni/CxxSharedModuleWrapper.h @@ -0,0 +1,33 @@ +// Copyright 2004-present Facebook. All Rights Reserved. + +#pragma once + +#include + +#include "CxxModuleWrapperBase.h" + +namespace facebook { +namespace react { + +class CxxSharedModuleWrapper: public CxxModuleWrapperBase { + public: + std::string getName() override { + return shared_->getName(); + } + + std::unique_ptr getModule() override { + // Instead of just moving out the stored CxxModule, this creates a + // proxy which passes calls to the shared stored CxxModule. + + return std::make_unique(shared_); + } + +protected: + explicit CxxSharedModuleWrapper(std::unique_ptr module) + : shared_(std::move(module)) {} + + std::shared_ptr shared_; +}; + +} +} diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp index 95e33b714..8633f7833 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryBuilder.cpp @@ -21,9 +21,9 @@ xplat::module::CxxModule::Provider ModuleHolder::getProvider() const { // This is the call which uses the lazy Java Provider to instantiate the // Java CxxModuleWrapper which contains the CxxModule. auto module = method(self); - CHECK(module->isInstanceOf(CxxModuleWrapper::javaClassStatic())) + CHECK(module->isInstanceOf(CxxModuleWrapperBase::javaClassStatic())) << "module isn't a C++ module"; - auto cxxModule = jni::static_ref_cast(module); + auto cxxModule = jni::static_ref_cast(module); // Then, we grab the CxxModule from the wrapper, which is no longer needed. return cxxModule->cthis()->getModule(); }; diff --git a/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp b/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp index dc084b123..7be58ad9f 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp @@ -9,6 +9,7 @@ #include #include #include "CatalystInstanceImpl.h" +#include "CxxModuleWrapper.h" #include "JavaScriptExecutorHolder.h" #include "JSCPerfLogging.h" #include "JSLoader.h" @@ -167,6 +168,7 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) { JSCJavaScriptExecutorHolder::registerNatives(); ProxyJavaScriptExecutorHolder::registerNatives(); CatalystInstanceImpl::registerNatives(); + CxxModuleWrapperBase::registerNatives(); CxxModuleWrapper::registerNatives(); JCallbackImpl::registerNatives(); #ifdef WITH_INSPECTOR diff --git a/ReactAndroid/src/main/jni/xreact/perftests/OnLoad.cpp b/ReactAndroid/src/main/jni/xreact/perftests/OnLoad.cpp index f6d5c0d7c..89610b203 100644 --- a/ReactAndroid/src/main/jni/xreact/perftests/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/xreact/perftests/OnLoad.cpp @@ -16,7 +16,11 @@ using facebook::jni::alias_ref; namespace { // This is a wrapper around the Java proxy to the javascript module. This -// allows us to call functions on the js module from c++. +// allows us to call functions on the js module from c++. Are you seeing +// crashes in this class? Android 6+ crashes when you try to call a +// method on a Proxy. Switch to an older version of Android. If you're +// really desperate, you can fix this by using ToReflectedMethod on the +// underlying jmethodid and invoking that. class JavaJSModule : public jni::JavaClass { public: static constexpr auto kJavaDescriptor =