Refactor CxxNativeModule out of android-specific code into common code

Differential Revision: D3574789

fbshipit-source-id: 0cb5965d20dcf7accb6a94514486b8fda1126b7b

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

@@ -49,6 +49,10 @@ class JInstanceCallback : public InstanceCallback {
   }
 
   void incrementPendingJSCalls() override {
+    // For C++ modules, this can be called from an arbitrary thread
+    // managed by the module, via callJSCallback or callJSFunction.  So,
+    // we ensure that it is registered with the JVM.
+    jni::ThreadScope guard;
     static auto method =
       ReactCallback::javaClassStatic()->getMethod<void()>("incrementPendingJSCalls");
     method(jobj_);

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

@@ -45,6 +45,10 @@ JMessageQueueThread::JMessageQueueThread(alias_ref<JavaMessageQueueThread::javao
 }
 
 void JMessageQueueThread::runOnQueue(std::function<void()>&& runnable) {
+  // For C++ modules, this can be called from an arbitrary thread
+  // managed by the module, via callJSCallback or callJSFunction.  So,
+  // we ensure that it is registered with the JVM.
+  jni::ThreadScope guard;
   static auto method = JavaMessageQueueThread::javaClassStatic()->
     getMethod<void(Runnable::javaobject)>("runOnQueue");
   method(m_jobj, JNativeRunnable::newObjectCxxArgs(wrapRunnable(std::move(runnable))).get());

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

@@ -6,7 +6,8 @@
 #include <fbsystrace.h>
 #endif
 
-#include "ModuleRegistryHolder.h"
+#include <cxxreact/CxxNativeModule.h>
+
 #include "JCallback.h"
 #include "JExecutorToken.h"
 #include "ReadableNativeArray.h"

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

@@ -7,6 +7,7 @@
 #include <fb/fbjni.h>
 
 #include <cxxreact/CxxModule.h>
+#include <cxxreact/CxxNativeModule.h>
 #include <cxxreact/Instance.h>
 #include <cxxreact/JsArgumentHelpers.h>
 #include <cxxreact/NativeModule.h>
@@ -170,115 +171,6 @@ class NewJavaNativeModule : public NativeModule {
   }
 };
 
-class CxxNativeModule : public NativeModule {
- public:
-  CxxNativeModule(std::weak_ptr<Instance> instance,
-                  std::unique_ptr<CxxModule> module)
-      : instance_(instance)
-      , module_(std::move(module))
-      , methods_(module_->getMethods()) {}
-
-  std::string getName() override {
-    return module_->getName();
-  }
-
-  virtual std::vector<MethodDescriptor> getMethods() override {
-    // Same as MessageQueue.MethodTypes.remote
-    static const auto kMethodTypeRemote = "remote";
-
-    std::vector<MethodDescriptor> descs;
-    for (auto& method : methods_) {
-      descs.emplace_back(method.name, kMethodTypeRemote);
-    }
-    return descs;
-  }
-
-  virtual folly::dynamic getConstants() override {
-    folly::dynamic constants = folly::dynamic::object();
-    for (auto& pair : module_->getConstants()) {
-      constants.insert(std::move(pair.first), std::move(pair.second));
-    }
-    return constants;
-  }
-
-  virtual bool supportsWebWorkers() override {
-    // TODO(andrews): web worker support in cxxmodules
-    return true;
-  }
-
-  // TODO mhorowitz: do we need initialize()/onCatalystInstanceDestroy() in C++
-  // or only Java?
-  virtual void invoke(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) override {
-    if (reactMethodId >= methods_.size()) {
-      throw std::invalid_argument(
-        folly::to<std::string>("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]"));
-    }
-    if (!params.isArray()) {
-      throw std::invalid_argument(
-        folly::to<std::string>("method parameters should be array, but are ", params.typeName()));
-    }
-
-    CxxModule::Callback first;
-    CxxModule::Callback second;
-
-    const auto& method = methods_[reactMethodId];
-
-    if (params.size() < method.callbacks) {
-      throw std::invalid_argument(
-        folly::to<std::string>("Expected ", method.callbacks, " callbacks, but only ",
-                               params.size(), " parameters provided"));
-    }
-
-    if (method.callbacks == 1) {
-      first = makeCallback(instance_, token, params[params.size() - 1]);
-    } else if (method.callbacks == 2) {
-      first = makeCallback(instance_, token, params[params.size() - 2]);
-      second = makeCallback(instance_, token, params[params.size() - 1]);
-    }
-
-    params.resize(params.size() - method.callbacks);
-
-    // I've got a few flawed options here.  I can let the C++ exception
-    // propogate, and the registry will log/convert them to java exceptions.
-    // This lets all the java and red box handling work ok, but the only info I
-    // can capture about the C++ exception is the what() string, not the stack.
-    // I can std::terminate() the app.  This causes the full, accurate C++
-    // stack trace to be added to logcat by debuggerd.  The java state is lost,
-    // but in practice, the java stack is always the same in this case since
-    // the javascript stack is not visible, and the crash is unfriendly to js
-    // developers, but crucial to C++ developers.  The what() value is also
-    // lost.  Finally, I can catch, log the java stack, then rethrow the C++
-    // exception.  In this case I get java and C++ stack data, but the C++
-    // stack is as of the rethrow, not the original throw, both the C++ and
-    // java stacks always look the same.
-    //
-    // I am going with option 2, since that seems like the most useful
-    // choice.  It would be nice to be able to get what() and the C++
-    // stack.  I'm told that will be possible in the future.  TODO
-    // mhorowitz #7128529: convert C++ exceptions to Java
-
-    try {
-      method.func(std::move(params), first, second);
-    } catch (const facebook::xplat::JsArgumentException& ex) {
-      // This ends up passed to the onNativeException callback.
-      throw;
-    } catch (...) {
-      // This means some C++ code is buggy.  As above, we fail hard so the C++
-      // developer can debug and fix it.
-      std::terminate();
-    }
-  }
-
-  MethodCallResult callSerializableNativeHook(ExecutorToken token, unsigned int hookId, folly::dynamic&& args) override {
-    throw std::runtime_error("Not supported");
-  }
-
- private:
-  std::weak_ptr<Instance> instance_;
-  std::unique_ptr<CxxModule> module_;
-  std::vector<CxxModule::Method> methods_;
-};
-
 }
 
 jni::local_ref<JReflectMethod::javaobject> JMethodDescriptor::getMethod() const {
@@ -324,19 +216,5 @@ ModuleRegistryHolder::ModuleRegistryHolder(
   registry_ = std::make_shared<ModuleRegistry>(std::move(modules));
 }
 
-Callback makeCallback(std::weak_ptr<Instance> instance, ExecutorToken token, const folly::dynamic& callbackId) {
-  if (!callbackId.isInt()) {
-    throw std::invalid_argument("Expected callback(s) as final argument");
-  }
-
-  auto id = callbackId.getInt();
-  return [winstance = std::move(instance), token, id](folly::dynamic args) {
-    if (auto instance = winstance.lock()) {
-      jni::ThreadScope guard;
-      instance->callJSCallback(token, id, std::move(args));
-    }
-  };
-}
-
 }
 }

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

@@ -83,12 +83,7 @@ class ModuleRegistryHolder : public jni::HybridClass<ModuleRegistryHolder> {
       jni::alias_ref<jni::JCollection<JavaModuleWrapper::javaobject>::javaobject> javaModules,
       jni::alias_ref<jni::JCollection<CxxModuleWrapper::javaobject>::javaobject> cxxModules);
 
-  facebook::xplat::module::CxxModule::Callback makeCallback(const folly::dynamic& callbackId);
-
   std::shared_ptr<ModuleRegistry> registry_;
 };
 
-using Callback = std::function<void(folly::dynamic)>;
-Callback makeCallback(std::weak_ptr<Instance> instance, ExecutorToken token, const folly::dynamic& callbackId);
-
 }}

ReactCommon/cxxreact/Android.mk

@@ -5,6 +5,7 @@ include $(CLEAR_VARS)
 LOCAL_MODULE := libreactnativefb
 
 LOCAL_SRC_FILES := \
+  CxxNativeModule.cpp \
   Executor.cpp \
   Instance.cpp \
   JSCExecutor.cpp \

ReactCommon/cxxreact/BUCK

@@ -109,6 +109,7 @@ react_library(
   force_static = True,
   srcs = [
     'CxxMessageQueue.cpp',
+    'CxxNativeModule.cpp',
     'Executor.cpp',
     'Instance.cpp',
     'JSCExecutor.cpp',
@@ -136,6 +137,7 @@ react_library(
   ],
   exported_headers = [
     'CxxMessageQueue.h',
+    'CxxNativeModule.h',
     'Executor.h',
     'ExecutorToken.h',
     'ExecutorTokenFactory.h',
@@ -165,6 +167,7 @@ react_library(
     '-frtti',
   ] + REACT_LIBRARY_EXTRA_COMPILER_FLAGS,
   deps = [
+    ':module',
     '//xplat/fbsystrace:fbsystrace',
     react_native_xplat_target('microprofiler:microprofiler'),
   ],

ReactCommon/cxxreact/CxxNativeModule.cpp

@@ -0,0 +1,128 @@
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+#include "CxxNativeModule.h"
+#include "Instance.h"
+
+#include <cxxreact/JsArgumentHelpers.h>
+
+using facebook::xplat::module::CxxModule;
+
+namespace facebook {
+namespace react {
+
+std::function<void(folly::dynamic)> makeCallback(
+    std::weak_ptr<Instance> instance, ExecutorToken token, const folly::dynamic& callbackId) {
+  if (!callbackId.isInt()) {
+    throw std::invalid_argument("Expected callback(s) as final argument");
+  }
+
+  auto id = callbackId.getInt();
+  return [winstance = std::move(instance), token, id](folly::dynamic args) {
+    if (auto instance = winstance.lock()) {
+      instance->callJSCallback(token, id, std::move(args));
+    }
+  };
+}
+
+CxxNativeModule::CxxNativeModule(std::weak_ptr<Instance> instance,
+                                 std::unique_ptr<CxxModule> module)
+  : instance_(instance)
+  , module_(std::move(module))
+  , methods_(module_->getMethods()) {}
+
+std::string CxxNativeModule::getName() {
+  return module_->getName();
+}
+
+std::vector<MethodDescriptor> CxxNativeModule::getMethods() {
+  // Same as MessageQueue.MethodTypes.remote
+  static const auto kMethodTypeRemote = "remote";
+
+  std::vector<MethodDescriptor> descs;
+  for (auto& method : methods_) {
+    descs.emplace_back(method.name, kMethodTypeRemote);
+  }
+  return descs;
+}
+
+folly::dynamic CxxNativeModule::getConstants() {
+  folly::dynamic constants = folly::dynamic::object();
+  for (auto& pair : module_->getConstants()) {
+    constants.insert(std::move(pair.first), std::move(pair.second));
+  }
+  return constants;
+}
+
+bool CxxNativeModule::supportsWebWorkers() {
+  // TODO(andrews): web worker support in cxxmodules
+  return false;
+}
+
+void CxxNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) {
+  if (reactMethodId >= methods_.size()) {
+    throw std::invalid_argument(
+      folly::to<std::string>("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]"));
+  }
+  if (!params.isArray()) {
+    throw std::invalid_argument(
+      folly::to<std::string>("method parameters should be array, but are ", params.typeName()));
+  }
+
+  CxxModule::Callback first;
+  CxxModule::Callback second;
+
+  const auto& method = methods_[reactMethodId];
+
+  if (params.size() < method.callbacks) {
+    throw std::invalid_argument(
+      folly::to<std::string>("Expected ", method.callbacks, " callbacks, but only ",
+                             params.size(), " parameters provided"));
+  }
+
+  if (method.callbacks == 1) {
+    first = makeCallback(instance_, token, params[params.size() - 1]);
+  } else if (method.callbacks == 2) {
+    first = makeCallback(instance_, token, params[params.size() - 2]);
+    second = makeCallback(instance_, token, params[params.size() - 1]);
+  }
+
+  params.resize(params.size() - method.callbacks);
+
+  // I've got a few flawed options here.  I can let the C++ exception
+  // propogate, and the registry will log/convert them to java exceptions.
+  // This lets all the java and red box handling work ok, but the only info I
+  // can capture about the C++ exception is the what() string, not the stack.
+  // I can std::terminate() the app.  This causes the full, accurate C++
+  // stack trace to be added to logcat by debuggerd.  The java state is lost,
+  // but in practice, the java stack is always the same in this case since
+  // the javascript stack is not visible, and the crash is unfriendly to js
+  // developers, but crucial to C++ developers.  The what() value is also
+  // lost.  Finally, I can catch, log the java stack, then rethrow the C++
+  // exception.  In this case I get java and C++ stack data, but the C++
+  // stack is as of the rethrow, not the original throw, both the C++ and
+  // java stacks always look the same.
+  //
+  // I am going with option 2, since that seems like the most useful
+  // choice.  It would be nice to be able to get what() and the C++
+  // stack.  I'm told that will be possible in the future.  TODO
+  // mhorowitz #7128529: convert C++ exceptions to Java
+
+  try {
+    method.func(std::move(params), first, second);
+  } catch (const facebook::xplat::JsArgumentException& ex) {
+    // This ends up passed to the onNativeException callback.
+    throw;
+  } catch (...) {
+    // This means some C++ code is buggy.  As above, we fail hard so the C++
+    // developer can debug and fix it.
+    std::terminate();
+  }
+}
+
+MethodCallResult CxxNativeModule::callSerializableNativeHook(ExecutorToken token, unsigned int hookId, folly::dynamic&& args) {
+  throw std::runtime_error("Not supported");
+}
+
+}
+}
+

ReactCommon/cxxreact/CxxNativeModule.h

@@ -0,0 +1,37 @@
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+#pragma once
+
+#include "NativeModule.h"
+
+#include <cxxreact/CxxModule.h>
+
+namespace facebook {
+namespace react {
+
+class Instance;
+
+std::function<void(folly::dynamic)> makeCallback(
+  std::weak_ptr<Instance> instance, ExecutorToken token, const folly::dynamic& callbackId);
+
+class CxxNativeModule : public NativeModule {
+public:
+  CxxNativeModule(std::weak_ptr<Instance> instance,
+                  std::unique_ptr<xplat::module::CxxModule> module);
+
+  std::string getName() override;
+  std::vector<MethodDescriptor> getMethods() override;
+  folly::dynamic getConstants() override;
+  bool supportsWebWorkers() override;
+  void invoke(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) override;
+  MethodCallResult callSerializableNativeHook(
+    ExecutorToken token, unsigned int hookId, folly::dynamic&& args) override;
+
+private:
+  std::weak_ptr<Instance> instance_;
+  std::unique_ptr<xplat::module::CxxModule> module_;
+  std::vector<xplat::module::CxxModule::Method> methods_;
+};
+
+}
+}