Refactor CxxModuleWrapper to support different use cases

Reviewed By: javache

Differential Revision: D4807680

fbshipit-source-id: 48eccfb382814a0c4082f56617e0359e61345da7

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<String, NativeMethod> 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);
+  }
 }

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<String, NativeMethod> 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;
+  }
+}

ReactAndroid/src/main/java/com/facebook/react/cxxbridge/NativeModuleRegistry.java

@@ -46,7 +46,7 @@ public class NativeModuleRegistry {
     ArrayList<JavaModuleWrapper> javaModules = new ArrayList<>();
     for (Map.Entry<Class<? extends NativeModule>, 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<ModuleHolder> cxxModules = new ArrayList<>();
     for (Map.Entry<Class<? extends NativeModule>, ModuleHolder> entry : mModules.entrySet()) {
       Class<?> type = entry.getKey();
-      if (CxxModuleWrapper.class.isAssignableFrom(type)) {
+      if (CxxModuleWrapperBase.class.isAssignableFrom(type)) {
         cxxModules.add(entry.getValue());
       }
     }

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",

ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp

@@ -2,27 +2,18 @@
 
 #include "CxxModuleWrapper.h"
 
-#include <fb/fbjni.h>
-#include <fb/Environment.h>
-
 #include <folly/ScopeGuard.h>
 
-#include <cxxreact/CxxModule.h>
-
 #include <dlfcn.h>
 
 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::javaobject> CxxModuleWrapper::makeDsoNative(
+    jni::alias_ref<jclass>, 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<CxxModule* (*)()>(sym);
-  module_.reset((*factory)());
+
+  return CxxModuleWrapper::newObjectCxxArgs(std::unique_ptr<CxxModule>((*factory)()));
 }
 
-std::string CxxModuleWrapper::getName() {
-  return module_->getName();
+}
 }

ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h

@@ -2,43 +2,31 @@
 
 #pragma once
 
-#include <memory>
-#include <string>
-#include <vector>
-
-#include <cxxreact/CxxModule.h>
-#include <fb/fbjni.h>
+#include "CxxModuleWrapperBase.h"
 
 namespace facebook {
 namespace react {
 
-struct JNativeModule : jni::JavaClass<JNativeModule> {
-  constexpr static const char *const kJavaDescriptor =
-    "Lcom/facebook/react/bridge/NativeModule;";
-};
-
-class CxxModuleWrapper :
-    public jni::HybridClass<CxxModuleWrapper, JNativeModule> {
+class CxxModuleWrapper : public jni::HybridClass<CxxModuleWrapper, CxxModuleWrapperBase> {
 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<jhybriddata> initHybrid(
-      jni::alias_ref<jhybridobject>, 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<CxxModuleWrapper::javaobject> makeDsoNative(
+    jni::alias_ref<jclass>, 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<xplat::module::CxxModule> getModule() {
-    // TODO mhorowitz: remove this (and a lot of other code) once the java
-    // bridge is dead.
+  std::unique_ptr<xplat::module::CxxModule> getModule() override {
     return std::move(module_);
   }
 

ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapperBase.h

@@ -0,0 +1,43 @@
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+#pragma once
+
+#include <memory>
+#include <string>
+
+#include <cxxreact/CxxModule.h>
+#include <fb/fbjni.h>
+
+namespace facebook {
+namespace react {
+
+struct JNativeModule : jni::JavaClass<JNativeModule> {
+  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<CxxModuleWrapperBase, JNativeModule> {
+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<xplat::module::CxxModule> getModule() = 0;
+};
+
+}
+}

ReactAndroid/src/main/jni/xreact/jni/CxxSharedModuleWrapper.h

@@ -0,0 +1,33 @@
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+#pragma once
+
+#include <cxxreact/SharedProxyCxxModule.h>
+
+#include "CxxModuleWrapperBase.h"
+
+namespace facebook {
+namespace react {
+
+class CxxSharedModuleWrapper: public CxxModuleWrapperBase {
+ public:
+  std::string getName() override {
+    return shared_->getName();
+  }
+
+  std::unique_ptr<xplat::module::CxxModule> 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<xplat::module::SharedProxyCxxModule>(shared_);
+  }
+
+protected:
+  explicit CxxSharedModuleWrapper(std::unique_ptr<xplat::module::CxxModule> module)
+    : shared_(std::move(module)) {}
+
+  std::shared_ptr<xplat::module::CxxModule> shared_;
+};
+
+}
+}

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<CxxModuleWrapper::javaobject>(module);
+    auto cxxModule = jni::static_ref_cast<CxxModuleWrapperBase::javaobject>(module);
     // Then, we grab the CxxModule from the wrapper, which is no longer needed.
     return cxxModule->cthis()->getModule();
   };

ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp

@@ -9,6 +9,7 @@
 #include <cxxreact/Platform.h>
 #include <jschelpers/Value.h>
 #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

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<JavaJSModule> {
 public:
   static constexpr auto kJavaDescriptor =