From 2a638c2ee77ee79238817d3fd9600837fb2748de Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Mon, 30 Jan 2017 06:39:49 -0800 Subject: [PATCH] Remove unused functionality in CxxModuleWrapper Reviewed By: mhorowitz Differential Revision: D4409789 fbshipit-source-id: 91e70d8333141e1e4dcba0e2620ef2c744d0c9d3 --- .../react/cxxbridge/CxxModuleWrapper.java | 30 +-- .../main/jni/xreact/jni/CxxModuleWrapper.cpp | 192 +----------------- .../main/jni/xreact/jni/CxxModuleWrapper.h | 12 +- 3 files changed, 12 insertions(+), 222 deletions(-) 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 418ee04ac..93f8006ce 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CxxModuleWrapper.java @@ -15,6 +15,8 @@ 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 CxxModuleWrapper implements NativeModule @@ -26,28 +28,6 @@ public class CxxModuleWrapper implements NativeModule @DoNotStrip private HybridData mHybridData; - @DoNotStrip - private static class MethodWrapper implements NativeMethod - { - @DoNotStrip - HybridData mHybridData; - - MethodWrapper() { - mHybridData = initHybrid(); - } - - public native HybridData initHybrid(); - - @Override - public native void invoke( - CatalystInstance catalystInstance, - ExecutorToken executorToken, - ReadableNativeArray args); - - @Override - public native String getType(); - } - public CxxModuleWrapper(String library, String factory) { SoLoader.loadLibrary(library); mHybridData = @@ -58,9 +38,9 @@ public class CxxModuleWrapper implements NativeModule public native String getName(); @Override - public native Map getMethods(); - - public native String getConstantsJson(); + public Map getMethods() { + throw new UnsupportedOperationException(); + } @Override public void initialize() { diff --git a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp index 800f00f4b..82c7ada5c 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp @@ -4,171 +4,22 @@ #include #include -#include -#include -#include - -#include #include -#include -#include +#include + #include -#include - -#include "ReadableNativeArray.h" - - using namespace facebook::jni; using namespace facebook::xplat::module; using namespace facebook::react; -namespace { - -class ExecutorToken : public HybridClass { -public: - constexpr static const char *const kJavaDescriptor = "Lcom/facebook/react/bridge/ExecutorToken;"; -}; - -class CxxMethodWrapper : public HybridClass { -public: - constexpr static const char *const kJavaDescriptor = - "Lcom/facebook/react/cxxbridge/CxxModuleWrapper$MethodWrapper;"; - - static local_ref initHybrid(alias_ref) { - return makeCxxInstance(); - } - - static void registerNatives() { - registerHybrid({ - makeNativeMethod("initHybrid", CxxMethodWrapper::initHybrid), - makeNativeMethod("getType", CxxMethodWrapper::getType), - makeNativeMethod("invoke", - "(Lcom/facebook/react/bridge/CatalystInstance;Lcom/facebook/react/bridge/ExecutorToken;Lcom/facebook/react/bridge/ReadableNativeArray;)V", - CxxMethodWrapper::invoke), - }); - } - - std::string getType() { - if (method_->func) { - return "async"; - } else { - return "sync"; - } - } - - void invoke(jobject catalystinstance, ExecutorToken::jhybridobject executorToken, NativeArray* args); - - const CxxModule::Method* method_; -}; - -void CxxMethodWrapper::invoke(jobject jCatalystInstance, ExecutorToken::jhybridobject jExecutorToken, NativeArray* arguments) { - CxxModule::Callback first; - CxxModule::Callback second; - - if (!method_->func) { - throw std::runtime_error( - folly::to("Method ", method_->name, - " is synchronous but invoked asynchronously")); - } - - if (method_->callbacks >= 1) { - auto catalystInstance = make_global(adopt_local(jCatalystInstance)); - global_ref executorToken = make_global(jExecutorToken); - // TODO(10184774): Support ExecutorToken in CxxModules - static auto sCatalystInstanceInvokeCallback = - catalystInstance->getClass()->getMethod( - "invokeCallback"); - - int id1; - - if (!arguments->array[arguments->array.size() - 1].isInt()) { - throwNewJavaException(gJavaLangIllegalArgumentException, - "Expected callback as last argument"); - } - - if (method_->callbacks == 2) { - if (!arguments->array[arguments->array.size() - 2].isInt()) { - throwNewJavaException(gJavaLangIllegalArgumentException, - "Expected callback as penultimate argument"); - return; - } - - id1 = arguments->array[arguments->array.size() - 2].getInt(); - int id2 = arguments->array[arguments->array.size() - 1].getInt(); - second = [catalystInstance, executorToken, id2](std::vector args) mutable { - folly::dynamic argsArray(std::make_move_iterator(args.begin()), - std::make_move_iterator(args.end())); - ThreadScope guard; - sCatalystInstanceInvokeCallback( - catalystInstance.get(), executorToken.get(), id2, - ReadableNativeArray::newObjectCxxArgs(std::move(argsArray)).get()); - catalystInstance.reset(); - executorToken.reset(); - }; - } else { - id1 = arguments->array[arguments->array.size() - 1].getInt(); - } - - first = [catalystInstance, executorToken, id1](std::vector args) mutable { - folly::dynamic argsArray(std::make_move_iterator(args.begin()), - std::make_move_iterator(args.end())); - ThreadScope guard; - sCatalystInstanceInvokeCallback( - catalystInstance.get(), executorToken.get(), id1, - ReadableNativeArray::newObjectCxxArgs(std::move(argsArray)).get()); - // This is necessary because by the time the lambda's dtor runs, - // the guard has been destroyed, and it may not be possible to - // get a JNIEnv* to clean up the captured global_ref. - catalystInstance.reset(); - executorToken.reset(); - }; - } - - // I've got a few flawed options here. I can catch C++ exceptions - // here, and log/convert them to java exceptions. This lets all the - // java handling work ok, but the only info I can capture about the - // C++ exception is the what() string, not the stack. I can let the - // C++ exception escape, crashing 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. 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 - - folly::dynamic dargs = arguments->array; - dargs.resize(arguments->array.size() - method_->callbacks); - - try { - method_->func(dargs, first, second); - } catch (const facebook::xplat::JsArgumentException& ex) { - throwNewJavaException(gJavaLangIllegalArgumentException, ex.what()); - } -} - -} - -// CxxModuleWrapper - void CxxModuleWrapper::registerNatives() { registerHybrid({ makeNativeMethod("initHybrid", CxxModuleWrapper::initHybrid), - makeNativeMethod("getName", CxxModuleWrapper::getName), - makeNativeMethod("getConstantsJson", CxxModuleWrapper::getConstantsJson), - makeNativeMethod("getMethods", "()Ljava/util/Map;", CxxModuleWrapper::getMethods), + makeNativeMethod("getName", CxxModuleWrapper::getName) }); - - CxxMethodWrapper::registerNatives(); } CxxModuleWrapper::CxxModuleWrapper(const std::string& soPath, const std::string& fname) { @@ -196,45 +47,8 @@ CxxModuleWrapper::CxxModuleWrapper(const std::string& soPath, const std::string& } auto factory = reinterpret_cast(sym); module_.reset((*factory)()); - methods_ = module_->getMethods(); } std::string CxxModuleWrapper::getName() { return module_->getName(); } - -std::string CxxModuleWrapper::getConstantsJson() { - std::map constants = module_->getConstants(); - folly::dynamic constsobject = folly::dynamic::object; - - for (auto& c : constants) { - constsobject.insert(std::move(c.first), std::move(c.second)); - } - - return folly::toJson(constsobject); -} - -jobject CxxModuleWrapper::getMethods() { - static auto hashmap = findClassStatic("java/util/HashMap"); - static auto hashmap_put = hashmap->getMethod("put"); - - auto methods = hashmap->newObject(hashmap->getConstructor()); - - std::unordered_set names; - for (const auto& m : methods_) { - if (names.find(m.name) != names.end()) { - throwNewJavaException(gJavaLangIllegalArgumentException, - "C++ Module %s method name already registered: %s", - module_->getName().c_str(), m.name.c_str()); - } - names.insert(m.name); - auto name = make_jstring(m.name); - static auto ctor = - CxxMethodWrapper::javaClassStatic()->getConstructor(); - auto method = CxxMethodWrapper::javaClassStatic()->newObject(ctor); - cthis(method)->method_ = &m; - hashmap_put(methods.get(), name.get(), method.get()); - } - - return methods.release(); -} diff --git a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h index 53a4d5be1..7e8009464 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h +++ b/ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.h @@ -2,12 +2,13 @@ #pragma once -#include -#include #include #include #include +#include +#include + namespace facebook { namespace react { @@ -27,14 +28,11 @@ public: // JNI methods std::string getName(); - std::string getConstantsJson(); - jobject getMethods(); // 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. - methods_.clear(); return std::move(module_); } @@ -42,11 +40,9 @@ protected: friend HybridBase; explicit CxxModuleWrapper(std::unique_ptr module) - : module_(std::move(module)) - , methods_(module_->getMethods()) {} + : module_(std::move(module)) {} std::unique_ptr module_; - std::vector methods_; }; }