From da2684f0e7e75c6c27f575293665ee171f1d9f7e Mon Sep 17 00:00:00 2001 From: Marc Horowitz Date: Wed, 3 Aug 2016 15:58:45 -0700 Subject: [PATCH] Refactor CxxNativeModule out of android-specific code into common code Differential Revision: D3574789 fbshipit-source-id: 0cb5965d20dcf7accb6a94514486b8fda1126b7b --- .../jni/xreact/jni/CatalystInstanceImpl.cpp | 4 + .../jni/xreact/jni/JMessageQueueThread.cpp | 4 + .../src/main/jni/xreact/jni/MethodInvoker.cpp | 3 +- .../jni/xreact/jni/ModuleRegistryHolder.cpp | 124 +---------------- .../jni/xreact/jni/ModuleRegistryHolder.h | 5 - ReactCommon/cxxreact/Android.mk | 1 + ReactCommon/cxxreact/BUCK | 3 + ReactCommon/cxxreact/CxxNativeModule.cpp | 128 ++++++++++++++++++ ReactCommon/cxxreact/CxxNativeModule.h | 37 +++++ 9 files changed, 180 insertions(+), 129 deletions(-) create mode 100644 ReactCommon/cxxreact/CxxNativeModule.cpp create mode 100644 ReactCommon/cxxreact/CxxNativeModule.h diff --git a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp index 196e333a5..cd859b98b 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp +++ b/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("incrementPendingJSCalls"); method(jobj_); diff --git a/ReactAndroid/src/main/jni/xreact/jni/JMessageQueueThread.cpp b/ReactAndroid/src/main/jni/xreact/jni/JMessageQueueThread.cpp index 2510c2841..0937589a0 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/JMessageQueueThread.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/JMessageQueueThread.cpp @@ -45,6 +45,10 @@ JMessageQueueThread::JMessageQueueThread(alias_ref&& 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("runOnQueue"); method(m_jobj, JNativeRunnable::newObjectCxxArgs(wrapRunnable(std::move(runnable))).get()); diff --git a/ReactAndroid/src/main/jni/xreact/jni/MethodInvoker.cpp b/ReactAndroid/src/main/jni/xreact/jni/MethodInvoker.cpp index e7d3d2c2d..75233fbf0 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/MethodInvoker.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/MethodInvoker.cpp @@ -6,7 +6,8 @@ #include #endif -#include "ModuleRegistryHolder.h" +#include + #include "JCallback.h" #include "JExecutorToken.h" #include "ReadableNativeArray.h" diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.cpp b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.cpp index eebed41df..a37191d15 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -170,115 +171,6 @@ class NewJavaNativeModule : public NativeModule { } }; -class CxxNativeModule : public NativeModule { - public: - CxxNativeModule(std::weak_ptr instance, - std::unique_ptr module) - : instance_(instance) - , module_(std::move(module)) - , methods_(module_->getMethods()) {} - - std::string getName() override { - return module_->getName(); - } - - virtual std::vector getMethods() override { - // Same as MessageQueue.MethodTypes.remote - static const auto kMethodTypeRemote = "remote"; - - std::vector 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("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]")); - } - if (!params.isArray()) { - throw std::invalid_argument( - folly::to("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("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_; - std::unique_ptr module_; - std::vector methods_; -}; - } jni::local_ref JMethodDescriptor::getMethod() const { @@ -324,19 +216,5 @@ ModuleRegistryHolder::ModuleRegistryHolder( registry_ = std::make_shared(std::move(modules)); } -Callback makeCallback(std::weak_ptr 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)); - } - }; -} - } } diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h index 565d9e769..29ff1b62e 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h +++ b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h @@ -83,12 +83,7 @@ class ModuleRegistryHolder : public jni::HybridClass { jni::alias_ref::javaobject> javaModules, jni::alias_ref::javaobject> cxxModules); - facebook::xplat::module::CxxModule::Callback makeCallback(const folly::dynamic& callbackId); - std::shared_ptr registry_; }; -using Callback = std::function; -Callback makeCallback(std::weak_ptr instance, ExecutorToken token, const folly::dynamic& callbackId); - }} diff --git a/ReactCommon/cxxreact/Android.mk b/ReactCommon/cxxreact/Android.mk index 30cd232db..83f99b152 100644 --- a/ReactCommon/cxxreact/Android.mk +++ b/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 \ diff --git a/ReactCommon/cxxreact/BUCK b/ReactCommon/cxxreact/BUCK index c787885f4..dc4733746 100644 --- a/ReactCommon/cxxreact/BUCK +++ b/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'), ], diff --git a/ReactCommon/cxxreact/CxxNativeModule.cpp b/ReactCommon/cxxreact/CxxNativeModule.cpp new file mode 100644 index 000000000..b18721394 --- /dev/null +++ b/ReactCommon/cxxreact/CxxNativeModule.cpp @@ -0,0 +1,128 @@ +// Copyright 2004-present Facebook. All Rights Reserved. + +#include "CxxNativeModule.h" +#include "Instance.h" + +#include + +using facebook::xplat::module::CxxModule; + +namespace facebook { +namespace react { + +std::function makeCallback( + std::weak_ptr 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, + std::unique_ptr module) + : instance_(instance) + , module_(std::move(module)) + , methods_(module_->getMethods()) {} + +std::string CxxNativeModule::getName() { + return module_->getName(); +} + +std::vector CxxNativeModule::getMethods() { + // Same as MessageQueue.MethodTypes.remote + static const auto kMethodTypeRemote = "remote"; + + std::vector 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("methodId ", reactMethodId, " out of range [0..", methods_.size(), "]")); + } + if (!params.isArray()) { + throw std::invalid_argument( + folly::to("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("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"); +} + +} +} + diff --git a/ReactCommon/cxxreact/CxxNativeModule.h b/ReactCommon/cxxreact/CxxNativeModule.h new file mode 100644 index 000000000..6f6b939a1 --- /dev/null +++ b/ReactCommon/cxxreact/CxxNativeModule.h @@ -0,0 +1,37 @@ +// Copyright 2004-present Facebook. All Rights Reserved. + +#pragma once + +#include "NativeModule.h" + +#include + +namespace facebook { +namespace react { + +class Instance; + +std::function makeCallback( + std::weak_ptr instance, ExecutorToken token, const folly::dynamic& callbackId); + +class CxxNativeModule : public NativeModule { +public: + CxxNativeModule(std::weak_ptr instance, + std::unique_ptr module); + + std::string getName() override; + std::vector 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_; + std::unique_ptr module_; + std::vector methods_; +}; + +} +}