Support invoking and returning arbitrary types from Java sync hooks

Reviewed By: mhorowitz

Differential Revision: D4409900

fbshipit-source-id: 347e33c442b32f64355d343308c218c15cf5a70f

ReactAndroid/src/androidTest/java/com/facebook/react/tests/BUCK

@@ -8,6 +8,7 @@ deps = [
   react_native_target('java/com/facebook/react:react'),
   react_native_target('java/com/facebook/react/bridge:bridge'),
   react_native_target('java/com/facebook/react/common:common'),
+  react_native_target('java/com/facebook/react/module/annotations:annotations'),
   react_native_target('java/com/facebook/react/modules/core:core'),
   react_native_target('java/com/facebook/react/modules/datepicker:datepicker'),
   react_native_target('java/com/facebook/react/modules/share:share'),

ReactAndroid/src/androidTest/java/com/facebook/react/tests/CatalystNativeJSToJavaParametersTestCase.java

@@ -41,11 +41,15 @@ import com.facebook.react.views.view.ReactViewManager;
 
 /**
  * Integration test to verify passing various types of parameters from JS to Java works
+ *
+ * TODO: we should run these tests with isBlockingSynchronousMethod = true as well,
+ * since they currrently use a completely different codepath
  */
 public class CatalystNativeJSToJavaParametersTestCase extends ReactIntegrationTestCase {
 
   private interface TestJSToJavaParametersModule extends JavaScriptModule {
     void returnBasicTypes();
+    void returnBoxedTypes();
     void returnDynamicTypes();
 
     void returnArrayWithBasicTypes();
@@ -116,6 +120,19 @@ public class CatalystNativeJSToJavaParametersTestCase extends ReactIntegrationTe
     assertNull(args[3]);
   }
 
+  public void testBoxedTypes() {
+    mCatalystInstance.getJSModule(TestJSToJavaParametersModule.class).returnBoxedTypes();
+    waitForBridgeAndUIIdle();
+
+    List<Object[]> boxedTypesCalls = mRecordingTestModule.getBoxedTypesCalls();
+    assertEquals(1, boxedTypesCalls.size());
+
+    Object[] args = boxedTypesCalls.get(0);
+    assertEquals(Integer.valueOf(42), args[0]);
+    assertEquals(Double.valueOf(3.14), args[1]);
+    assertEquals(Boolean.valueOf(true), args[2]);
+  }
+
   public void testDynamicType() {
     mCatalystInstance.getJSModule(TestJSToJavaParametersModule.class).returnDynamicTypes();
     waitForBridgeAndUIIdle();
@@ -682,9 +699,10 @@ public class CatalystNativeJSToJavaParametersTestCase extends ReactIntegrationTe
     }
   }
 
-  private class RecordingTestModule extends BaseJavaModule {
+  private static class RecordingTestModule extends BaseJavaModule {
 
     private final List<Object[]> mBasicTypesCalls = new ArrayList<Object[]>();
+    private final List<Object[]> mBoxedTypesCalls = new ArrayList<Object[]>();
     private final List<ReadableArray> mArrayCalls = new ArrayList<ReadableArray>();
     private final List<ReadableMap> mMapCalls = new ArrayList<ReadableMap>();
     private final List<Dynamic> mDynamicCalls = new ArrayList<Dynamic>();
@@ -699,6 +717,11 @@ public class CatalystNativeJSToJavaParametersTestCase extends ReactIntegrationTe
       mBasicTypesCalls.add(new Object[]{s, d, b, nullableString});
     }
 
+    @ReactMethod
+    public void receiveBoxedTypes(Integer i, Double d, Boolean b) {
+      mBoxedTypesCalls.add(new Object[]{i, d, b});
+    }
+
     @ReactMethod
     public void receiveArray(ReadableArray array) {
       mArrayCalls.add(array);
@@ -718,6 +741,10 @@ public class CatalystNativeJSToJavaParametersTestCase extends ReactIntegrationTe
       return mBasicTypesCalls;
     }
 
+    public List<Object[]> getBoxedTypesCalls() {
+      return mBoxedTypesCalls;
+    }
+
     public List<ReadableArray> getArrayCalls() {
       return mArrayCalls;
     }

ReactAndroid/src/androidTest/java/com/facebook/react/tests/CatalystNativeJavaToJSReturnValuesTestCase.java

@@ -0,0 +1,163 @@
+/**
+ * Copyright (c) 2014-present, Facebook, Inc.
+ * All rights reserved.
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package com.facebook.react.tests;
+
+import java.util.ArrayList;
+
+import com.facebook.react.bridge.BaseJavaModule;
+import com.facebook.react.bridge.CatalystInstance;
+import com.facebook.react.bridge.JavaScriptModule;
+import com.facebook.react.bridge.ReactMethod;
+import com.facebook.react.bridge.WritableArray;
+import com.facebook.react.bridge.WritableMap;
+import com.facebook.react.bridge.WritableNativeArray;
+import com.facebook.react.bridge.WritableNativeMap;
+import com.facebook.react.module.annotations.ReactModule;
+import com.facebook.react.testing.AssertModule;
+import com.facebook.react.testing.FakeWebSocketModule;
+import com.facebook.react.testing.ReactIntegrationTestCase;
+import com.facebook.react.testing.ReactTestHelper;
+import com.facebook.react.uimanager.UIImplementationProvider;
+import com.facebook.react.uimanager.UIManagerModule;
+import com.facebook.react.uimanager.ViewManager;
+
+/**
+ * Test marshalling return values from Java to JS
+ */
+public class CatalystNativeJavaToJSReturnValuesTestCase extends ReactIntegrationTestCase {
+
+  private interface TestJavaToJSReturnValuesModule extends JavaScriptModule {
+    void callMethod(String methodName, String expectedReturnType, String expectedJSON);
+    void triggerException();
+  }
+
+  @ReactModule(name = "TestModule")
+  private static class TestModule extends BaseJavaModule {
+    @Override
+    public String getName() {
+      return "TestModule";
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    boolean getBoolean() {
+      return true;
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    Boolean getBoxedBoolean() {
+      return Boolean.valueOf(true);
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    int getInt() {
+      return 42;
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    Integer getBoxedInt() {
+      return Integer.valueOf(42);
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    double getDouble() {
+      return 3.14159;
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    Double getBoxedDouble() {
+      return Double.valueOf(3.14159);
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    String getString() {
+      return "Hello world!";
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    WritableArray getArray() {
+      WritableArray arr = new WritableNativeArray();
+      arr.pushString("a");
+      arr.pushBoolean(true);
+      return arr;
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    WritableMap getMap() {
+      WritableMap map = new WritableNativeMap();
+      map.putBoolean("a", true);
+      map.putBoolean("b", false);
+      return map;
+    }
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    boolean triggerException() {
+      throw new RuntimeException("Exception triggered");
+    }
+  }
+
+  private AssertModule mAssertModule;
+  private CatalystInstance mInstance;
+
+  @Override
+  protected void setUp() throws Exception {
+    super.setUp();
+
+    final UIManagerModule mUIManager = new UIManagerModule(
+        getContext(),
+        new ArrayList<ViewManager>(),
+        new UIImplementationProvider(),
+        false);
+
+    mAssertModule = new AssertModule();
+
+    mInstance = ReactTestHelper.catalystInstanceBuilder(this)
+        .addNativeModule(mAssertModule)
+        .addNativeModule(new FakeWebSocketModule())
+        .addJSModule(TestJavaToJSReturnValuesModule.class)
+        .addNativeModule(mUIManager)
+        .addNativeModule(new TestModule())
+        .build();
+  }
+
+  public void testGetPrimitives() {
+    TestJavaToJSReturnValuesModule m = mInstance.getJSModule(TestJavaToJSReturnValuesModule.class);
+
+    // jboolean is actually an unsigned char, so we don't get JS booleans
+    m.callMethod("getBoolean", "number", "1");
+    m.callMethod("getBoxedBoolean", "number", "1");
+
+    m.callMethod("getInt", "number", "42");
+    m.callMethod("getBoxedInt", "number", "42");
+
+    m.callMethod("getDouble", "number", "3.14159");
+    m.callMethod("getBoxedDouble", "number", "3.14159");
+
+    waitForBridgeAndUIIdle();
+    mAssertModule.verifyAssertsAndReset();
+  }
+
+  public void testObjectTypes() {
+    TestJavaToJSReturnValuesModule m = mInstance.getJSModule(TestJavaToJSReturnValuesModule.class);
+
+    m.callMethod("getString", "string", "\"Hello world!\"");
+    m.callMethod("getArray", "object", "[\"a\",true]");
+    m.callMethod("getMap", "object", "{\"b\":false,\"a\":true}");
+
+    waitForBridgeAndUIIdle();
+    mAssertModule.verifyAssertsAndReset();
+  }
+
+  public void testThrowsException() {
+    TestJavaToJSReturnValuesModule m = mInstance.getJSModule(TestJavaToJSReturnValuesModule.class);
+    m.triggerException();
+
+    waitForBridgeAndUIIdle();
+    mAssertModule.verifyAssertsAndReset();
+  }
+}

ReactAndroid/src/androidTest/js/TestBundle.js

@@ -18,6 +18,7 @@ require('ViewRenderingTestModule');
 require('TestJavaToJSArgumentsModule');
 require('TestJSLocaleModule');
 require('TestJSToJavaParametersModule');
+require('TestJavaToJSReturnValuesModule');
 require('UIManagerTestModule');
 
 require('CatalystRootViewTestModule');

ReactAndroid/src/androidTest/js/TestJSToJavaParametersModule.js

@@ -18,6 +18,9 @@ var TestJSToJavaParametersModule = {
   returnBasicTypes: function() {
     Recording.receiveBasicTypes('foo', 3.14, true, null);
   },
+  returnBoxedTypes: function() {
+    Recording.receiveBoxedTypes(42, 3.14, true);
+  },
   returnDynamicTypes: function() {
     Recording.receiveDynamic('foo');
     Recording.receiveDynamic(3.14);

ReactAndroid/src/androidTest/js/TestJavaToJSReturnValuesModule.js

@@ -0,0 +1,40 @@
+/**
+ * Copyright (c) 2013-present, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @providesModule TestJavaToJSReturnValuesModule
+ */
+
+'use strict';
+
+const BatchedBridge = require('BatchedBridge');
+
+const {assertEquals, assertTrue} = require('Asserts');
+const {TestModule} = require('NativeModules');
+
+var TestJavaToJSReturnValuesModule = {
+  callMethod: function(methodName, expectedType, expectedJSON) {
+    const result = TestModule[methodName]();
+    assertEquals(expectedType, typeof result);
+    assertEquals(expectedJSON, JSON.stringify(result));
+  },
+
+  triggerException: function() {
+    try {
+      TestModule.triggerException();
+    } catch (ex) {
+      assertTrue(ex.message.indexOf('Exception triggered') !== -1);
+    }
+  }
+};
+
+BatchedBridge.registerCallableModule(
+  'TestJavaToJSReturnValuesModule',
+  TestJavaToJSReturnValuesModule
+);
+
+module.exports = TestJavaToJSReturnValuesModule;

ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java

@@ -461,6 +461,7 @@ public abstract class BaseJavaModule implements NativeModule {
   }
 
   private static char returnTypeToChar(Class returnClass) {
+    // Keep this in sync with MethodInvoker
     char tryCommon = commonTypeToChar(returnClass);
     if (tryCommon != '\0') {
       return tryCommon;

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

@@ -2,10 +2,8 @@
 
 #include "JavaModuleWrapper.h"
 
-#include <folly/json.h>
-
 #include <fb/fbjni.h>
-
+#include <folly/json.h>
 #include <cxxreact/CxxModule.h>
 #include <cxxreact/CxxNativeModule.h>
 #include <cxxreact/Instance.h>
@@ -42,9 +40,30 @@ std::string JavaNativeModule::getName() {
 
 std::vector<MethodDescriptor> JavaNativeModule::getMethods() {
   std::vector<MethodDescriptor> ret;
+  syncMethods_.clear();
   auto descs = wrapper_->getMethodDescriptors();
   for (const auto& desc : *descs) {
-    ret.emplace_back(desc->getName(), desc->getType());
+    auto methodName = desc->getName();
+    auto methodType = desc->getType();
+
+    if (methodType == "sync") {
+      // allow for the sync methods vector to have empty values, resize on demand
+      size_t methodIndex = ret.size();
+      if (methodIndex >= syncMethods_.size()) {
+        syncMethods_.resize(methodIndex + 1);
+      }
+      syncMethods_.insert(syncMethods_.begin() + methodIndex, MethodInvoker(
+        desc->getMethod(),
+        desc->getSignature(),
+        getName() + "." + methodName,
+        true
+      ));
+    }
+
+    ret.emplace_back(
+      std::move(methodName),
+      std::move(methodType)
+    );
   }
   return ret;
 }
@@ -75,7 +94,15 @@ void JavaNativeModule::invoke(ExecutorToken token, unsigned int reactMethodId, f
 }
 
 MethodCallResult JavaNativeModule::callSerializableNativeHook(ExecutorToken token, unsigned int reactMethodId, folly::dynamic&& params) {
-  throw std::runtime_error("Unsupported operation.");
+    // TODO: evaluate whether calling through invoke is potentially faster
+    if (reactMethodId >= syncMethods_.size()) {
+      throw std::invalid_argument(
+        folly::to<std::string>("methodId ", reactMethodId, " out of range [0..", syncMethods_.size(), "]"));
+    }
+
+    auto& method = syncMethods_[reactMethodId];
+    CHECK(method.hasValue() && method->isSyncHook()) << "Trying to invoke a asynchronous method as synchronous hook";
+    return method->invoke(instance_, wrapper_->getModule(), token, params);
 }
 
 NewJavaNativeModule::NewJavaNativeModule(

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

@@ -4,6 +4,7 @@
 
 #include <cxxreact/NativeModule.h>
 #include <fb/fbjni.h>
+#include <folly/Optional.h>
 
 #include "MethodInvoker.h"
 
@@ -54,6 +55,7 @@ class JavaNativeModule : public NativeModule {
  private:
   std::weak_ptr<Instance> instance_;
   jni::global_ref<JavaModuleWrapper::javaobject> wrapper_;
+  std::vector<folly::Optional<MethodInvoker>> syncMethods_;
 };
 
 // Experimental new implementation that uses direct method invocation

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

@@ -12,6 +12,8 @@
 #include "JExecutorToken.h"
 #include "ReadableNativeArray.h"
 #include "ReadableNativeMap.h"
+#include "WritableNativeArray.h"
+#include "WritableNativeMap.h"
 
 namespace facebook {
 namespace react {
@@ -57,30 +59,6 @@ jni::local_ref<JPromiseImpl::javaobject> extractPromise(std::weak_ptr<Instance>&
   return JPromiseImpl::create(resolve, reject);
 }
 
-jobject valueOf(jboolean value) {
-  static auto kClass = jni::findClassStatic("java/lang/Boolean");
-  static auto kValueOf = kClass->getStaticMethod<jobject(jboolean)>("valueOf");
-  return kValueOf(kClass, value).release();
-}
-
-jobject valueOf(jint value) {
-  static auto kClass = jni::findClassStatic("java/lang/Integer");
-  static auto kValueOf = kClass->getStaticMethod<jobject(jint)>("valueOf");
-  return kValueOf(kClass, value).release();
-}
-
-jobject valueOf(jdouble value) {
-  static auto kClass = jni::findClassStatic("java/lang/Double");
-  static auto kValueOf = kClass->getStaticMethod<jobject(jdouble)>("valueOf");
-  return kValueOf(kClass, value).release();
-}
-
-jobject valueOf(jfloat value) {
-  static auto kClass = jni::findClassStatic("java/lang/Float");
-  static auto kValueOf = kClass->getStaticMethod<jobject(jfloat)>("valueOf");
-  return kValueOf(kClass, value).release();
-}
-
 bool isNullable(char type) {
   switch (type) {
     case 'Z':
@@ -118,25 +96,25 @@ jvalue extract(std::weak_ptr<Instance>& instance, ExecutorToken token, char type
       value.z = static_cast<jboolean>(arg.getBool());
       break;
     case 'Z':
-      value.l = valueOf(static_cast<jboolean>(arg.getBool()));
+      value.l = JBoolean::valueOf(static_cast<jboolean>(arg.getBool())).release();
       break;
     case 'i':
       value.i = static_cast<jint>(arg.getInt());
       break;
     case 'I':
-      value.l = valueOf(static_cast<jint>(arg.getInt()));
+      value.l = JInteger::valueOf(static_cast<jint>(arg.getInt())).release();
       break;
     case 'f':
       value.f = static_cast<jfloat>(extractDouble(arg));
       break;
     case 'F':
-      value.l = valueOf(static_cast<jfloat>(extractDouble(arg)));
+      value.l = JFloat::valueOf(static_cast<jfloat>(extractDouble(arg))).release();
       break;
     case 'd':
       value.d = extractDouble(arg);
       break;
     case 'D':
-      value.l = valueOf(extractDouble(arg));
+      value.l = JDouble::valueOf(extractDouble(arg)).release();
       break;
     case 'S':
       value.l = jni::make_jstring(arg.getString().c_str()).release();
@@ -182,10 +160,10 @@ MethodInvoker::MethodInvoker(jni::alias_ref<JReflectMethod::javaobject> method,
  traceName_(std::move(traceName)),
  isSync_(isSync) {
    CHECK(signature_.at(1) == '.') << "Improper module method signature";
-   CHECK(!isSync || signature_.at(0) == 'v') << "Non-sync hooks cannot have a non-void return type";
+   CHECK(isSync || signature_.at(0) == 'v') << "Non-sync hooks cannot have a non-void return type";
  }
 
-MethodCallResult MethodInvoker::invoke(std::weak_ptr<Instance>& instance, JBaseJavaModule::javaobject module, ExecutorToken token, const folly::dynamic& params) {
+MethodCallResult MethodInvoker::invoke(std::weak_ptr<Instance>& instance, jni::alias_ref<JBaseJavaModule::javaobject> module, ExecutorToken token, const folly::dynamic& params) {
   #ifdef WITH_FBSYSTRACE
   fbsystrace::FbSystraceSection s(
       TRACE_TAG_REACT_CXX_BRIDGE,
@@ -193,11 +171,14 @@ MethodCallResult MethodInvoker::invoke(std::weak_ptr<Instance>& instance, JBaseJ
       "method",
       traceName_);
   #endif
+
   if (params.size() != jsArgCount_) {
     throw std::invalid_argument(folly::to<std::string>("expected ", jsArgCount_, " arguments, got ", params.size()));
   }
+
+  auto env = jni::Environment::current();
   auto argCount = signature_.size() - 2;
-  jni::JniLocalScope scope(jni::Environment::current(), argCount);
+  jni::JniLocalScope scope(env, argCount);
   jvalue args[argCount];
   std::transform(
     signature_.begin() + 2,
@@ -207,24 +188,45 @@ MethodCallResult MethodInvoker::invoke(std::weak_ptr<Instance>& instance, JBaseJ
       return extract(instance, token, type, it, end);
   });
 
-  // TODO(t10768795): Use fbjni here
-  folly::dynamic ret = folly::dynamic::object();
-  bool isReturnUndefined = false;
+#define CASE_PRIMITIVE(KEY, TYPE, METHOD)                                      \
+  case KEY: {                                                                  \
+    auto result = env->Call ## METHOD ## MethodA(module.get(), method_, args); \
+    jni::throwPendingJniExceptionAsCppException();                             \
+    return MethodCallResult {result, false};                                   \
+  }
+
+#define CASE_OBJECT(KEY, JNI_CLASS, ACTIONS)                                \
+  case KEY: {                                                               \
+    auto jobject = env->CallObjectMethodA(module.get(), method_, args);     \
+    jni::throwPendingJniExceptionAsCppException();                          \
+    auto result = adopt_local(static_cast<JNI_CLASS::javaobject>(jobject)); \
+    return MethodCallResult {result->ACTIONS, false};                       \
+  }
+
   char returnType = signature_.at(0);
   switch (returnType) {
     case 'v':
-      jni::Environment::current()->CallVoidMethodA(module, method_, args);
-      ret = nullptr;
-      isReturnUndefined = true;
-      break;
+      env->CallVoidMethodA(module.get(), method_, args);
+      jni::throwPendingJniExceptionAsCppException();
+      return MethodCallResult {nullptr, true};
+
+    CASE_PRIMITIVE('z', jboolean, Boolean)
+    CASE_OBJECT('Z', JBoolean, value())
+    CASE_PRIMITIVE('i', jint, Int)
+    CASE_OBJECT('I', JInteger, value())
+    CASE_PRIMITIVE('d', jdouble, Double)
+    CASE_OBJECT('D', JDouble, value())
+    CASE_PRIMITIVE('f', jfloat, Float)
+    CASE_OBJECT('F', JFloat, value())
+
+    CASE_OBJECT('S', JString, toStdString())
+    CASE_OBJECT('M', WritableNativeMap, cthis()->consume())
+    CASE_OBJECT('A', WritableNativeArray, cthis()->array)
+
     default:
       LOG(FATAL) << "Unknown return type: " << returnType;
-    // TODO: other cases
+      return MethodCallResult {nullptr, true};
   }
-
-  jni::throwPendingJniExceptionAsCppException();
-
-  return MethodCallResult{ret, isReturnUndefined};
 }
 
 }

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

@@ -31,7 +31,7 @@ class MethodInvoker {
 public:
   MethodInvoker(jni::alias_ref<JReflectMethod::javaobject> method, std::string signature, std::string traceName, bool isSync);
 
-  MethodCallResult invoke(std::weak_ptr<Instance>& instance, JBaseJavaModule::javaobject module, ExecutorToken token, const folly::dynamic& params);
+  MethodCallResult invoke(std::weak_ptr<Instance>& instance, jni::alias_ref<JBaseJavaModule::javaobject> module, ExecutorToken token, const folly::dynamic& params);
 
   bool isSyncHook() const {
     return isSync_;

ReactCommon/cxxreact/JSCNativeModules.cpp

@@ -2,7 +2,6 @@
 
 #include "JSCNativeModules.h"
 
-#include <jschelpers/JavaScriptCore.h>
 #include <string>
 
 namespace facebook {

ReactCommon/jschelpers/Value.cpp

@@ -84,7 +84,7 @@ JSValueRef Value::fromDynamic(JSContextRef ctx, const folly::dynamic& value) {
 
 JSValueRef Value::fromDynamicInner(JSContextRef ctx, const folly::dynamic& obj) {
   switch (obj.type()) {
-    // For premitive types (and strings), just create and return an equivalent JSValue
+    // For primitive types (and strings), just create and return an equivalent JSValue
     case folly::dynamic::Type::NULLT:
       return JSC_JSValueMakeNull(ctx);