From d63d4f0e9cc78887f3f5cccdf7b853cf2c6cf90d Mon Sep 17 00:00:00 2001 From: Alexander Blom Date: Tue, 21 Jun 2016 10:08:31 -0700 Subject: [PATCH] Bind methods instead of using eval Reviewed By: astreet Differential Revision: D3417452 fbshipit-source-id: 437daa2dfcd01efb749465a94c1a72ce8a2cb315 --- .../react/XReactInstanceManagerImpl.java | 8 ++- .../java/com/facebook/react/cxxbridge/BUCK | 3 +- .../react/cxxbridge/JSBundleLoader.java | 9 ++- .../devsupport/DebugServerException.java | 21 ++++++- .../react/devsupport/DevServerHelper.java | 14 ++--- ReactCommon/cxxreact/JSCExecutor.cpp | 62 +++++++++---------- ReactCommon/cxxreact/JSCExecutor.h | 6 ++ ReactCommon/cxxreact/Value.cpp | 13 +++- ReactCommon/cxxreact/Value.h | 14 ++++- 9 files changed, 101 insertions(+), 49 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java index 782fd25421b..410a3c59fdc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java @@ -914,8 +914,14 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; return null; } }).get(); - } catch (InterruptedException | ExecutionException e) { + } catch (InterruptedException e) { throw new RuntimeException(e); + } catch (ExecutionException e) { + if (e.getCause() instanceof RuntimeException) { + throw (RuntimeException) e.getCause(); + } else { + throw new RuntimeException(e); + } } return reactContext; diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/BUCK b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/BUCK index 898715d8955..54b234ec6c1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/BUCK +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/BUCK @@ -27,7 +27,8 @@ android_library( react_native_dep('third-party/java/okhttp:okhttp3-ws'), 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/devsupport:devsupport'), +], visibility = [ 'PUBLIC', ], diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java index 09fd6a4a64f..c55b30ce692 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java @@ -11,6 +11,9 @@ package com.facebook.react.cxxbridge; import android.content.Context; +import com.facebook.react.devsupport.DebugServerException; +import com.facebook.react.devsupport.DevServerHelper; + /** * A class that stores JS bundle information and allows {@link CatalystInstance} to load a correct * bundle through {@link ReactBridge}. @@ -55,7 +58,11 @@ public abstract class JSBundleLoader { return new JSBundleLoader() { @Override public void loadScript(CatalystInstanceImpl instance) { - instance.loadScriptFromFile(cachedFileLocation, sourceURL); + try { + instance.loadScriptFromFile(cachedFileLocation, sourceURL); + } catch (Exception e) { + throw DebugServerException.makeGeneric(e.getMessage(), e); + } } @Override diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DebugServerException.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DebugServerException.java index 1cf1ff387f1..88baa042460 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DebugServerException.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DebugServerException.java @@ -25,7 +25,22 @@ import org.json.JSONObject; * Tracks errors connecting to or received from the debug derver. * The debug server returns errors as json objects. This exception represents that error. */ -public class DebugServerException extends IOException { +public class DebugServerException extends RuntimeException { + private static final String GENERIC_ERROR_MESSAGE = + "\n\nTry the following to fix the issue:\n" + + "\u2022 Ensure that the packager server is running\n" + + "\u2022 Ensure that your device/emulator is connected to your machine and has USB debugging enabled - run 'adb devices' to see a list of connected devices\n" + + "\u2022 If you're on a physical device connected to the same machine, run 'adb reverse tcp:8081 tcp:8081' to forward requests from your device\n" + + "\u2022 If your device is on the same Wi-Fi network, set 'Debug server host & port for device' in 'Dev settings' to your machine's IP address and the port of the local dev server - e.g. 10.0.1.1:8081\n\n"; + + public static DebugServerException makeGeneric(String reason, Throwable t) { + return makeGeneric(reason, "", t); + } + + public static DebugServerException makeGeneric(String reason, String extra, Throwable t) { + return new DebugServerException(reason + GENERIC_ERROR_MESSAGE + extra, t); + } + private DebugServerException(String description, String fileName, int lineNumber, int column) { super(description + "\n at " + fileName + ":" + lineNumber + ":" + column); } @@ -34,6 +49,10 @@ public class DebugServerException extends IOException { super(description); } + public DebugServerException(String detailMessage, Throwable throwable) { + super(detailMessage, throwable); + } + /** * Parse a DebugServerException from the server json string. * @param str json string returned by the debug server diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java index 95d347852ee..a7c5565f5e4 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java @@ -47,7 +47,6 @@ import okio.Sink; * - Genymotion emulator with default settings: 10.0.3.2 */ public class DevServerHelper { - public static final String RELOAD_APP_EXTRA_JS_PROXY = "jsproxy"; private static final String RELOAD_APP_ACTION_SUFFIX = ".RELOAD_APP_ACTION"; @@ -185,15 +184,10 @@ public class DevServerHelper { } mDownloadBundleFromURLCall = null; - StringBuilder sb = new StringBuilder(); - sb.append("Could not connect to development server.\n\n") - .append("Try the following to fix the issue:\n") - .append("\u2022 Ensure that the packager server is running\n") - .append("\u2022 Ensure that your device/emulator is connected to your machine and has USB debugging enabled - run 'adb devices' to see a list of connected devices\n") - .append("\u2022 If you're on a physical device connected to the same machine, run 'adb reverse tcp:8081 tcp:8081' to forward requests from your device\n") - .append("\u2022 If your device is on the same Wi-Fi network, set 'Debug server host & port for device' in 'Dev settings' to your machine's IP address and the port of the local dev server - e.g. 10.0.1.1:8081\n\n") - .append("URL: ").append(call.request().url().toString()); - callback.onFailure(new DebugServerException(sb.toString())); + callback.onFailure(DebugServerException.makeGeneric( + "Could not connect to development server.", + "URL: " + call.request().url().toString(), + e)); } @Override diff --git a/ReactCommon/cxxreact/JSCExecutor.cpp b/ReactCommon/cxxreact/JSCExecutor.cpp index c244532aca6..b3a8a2db66e 100644 --- a/ReactCommon/cxxreact/JSCExecutor.cpp +++ b/ReactCommon/cxxreact/JSCExecutor.cpp @@ -90,22 +90,6 @@ static JSValueRef nativeInjectHMRUpdate( const JSValueRef arguments[], JSValueRef *exception); -static std::string executeJSCallWithJSC( - JSGlobalContextRef ctx, - const std::string& methodName, - const std::vector& arguments) throw(JSException) { - SystraceSection s("JSCExecutor.executeJSCall", - "method", methodName); - - // Evaluate script with JSC - folly::dynamic jsonArgs(arguments.begin(), arguments.end()); - auto js = folly::to( - "__fbBatchedBridge.", methodName, ".apply(null, ", - folly::toJson(jsonArgs), ")"); - auto result = evaluateScript(ctx, String(js.c_str()), nullptr); - return Value(ctx, result).toJSONString(); -} - std::unique_ptr JSCExecutorFactory::createJSExecutor( std::shared_ptr delegate, std::shared_ptr jsQueue) { return std::unique_ptr( @@ -289,6 +273,9 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr scrip String jsSourceURL(sourceURL.c_str()); evaluateScript(m_context, jsScript, jsSourceURL); + + bindBridge(); + flush(); ReactMarker::logMarker("CREATE_REACT_CONTEXT_END"); } @@ -300,32 +287,41 @@ void JSCExecutor::setJSModulesUnbundle(std::unique_ptr unbund m_unbundle = std::move(unbundle); } +void JSCExecutor::bindBridge() throw(JSException) { + auto global = Object::getGlobalObject(m_context); + auto batchedBridgeValue = global.getProperty("__fbBatchedBridge"); + if (batchedBridgeValue.isUndefined()) { + throwJSExecutionException("Could not get BatchedBridge, make sure your bundle is packaged correctly"); + } + + auto batchedBridge = batchedBridgeValue.asObject(); + m_callFunctionReturnFlushedQueueJS = batchedBridge.getProperty("callFunctionReturnFlushedQueue").asObject(); + m_invokeCallbackAndReturnFlushedQueueJS = batchedBridge.getProperty("invokeCallbackAndReturnFlushedQueue").asObject(); + m_flushedQueueJS = batchedBridge.getProperty("flushedQueue").asObject(); +} + void JSCExecutor::flush() throw(JSException) { - // TODO: Make this a first class function instead of evaling. #9317773 - std::string calls = executeJSCallWithJSC(m_context, "flushedQueue", std::vector()); + auto result = m_flushedQueueJS->callAsFunction({}); + auto calls = Value(m_context, result).toJSONString(); m_delegate->callNativeModules(*this, std::move(calls), true); } void JSCExecutor::callFunction(const std::string& moduleId, const std::string& methodId, const folly::dynamic& arguments) throw(JSException) { - // TODO: Make this a first class function instead of evaling. #9317773 - // TODO(cjhopman): This copies args. - std::vector call{ - moduleId, - methodId, - std::move(arguments), - }; - std::string calls = executeJSCallWithJSC(m_context, "callFunctionReturnFlushedQueue", std::move(call)); + auto result = m_callFunctionReturnFlushedQueueJS->callAsFunction({ + Value(m_context, String::createExpectingAscii(moduleId)), + Value(m_context, String::createExpectingAscii(methodId)), + Value::fromDynamic(m_context, std::move(arguments)) + }); + auto calls = Value(m_context, result).toJSONString(); m_delegate->callNativeModules(*this, std::move(calls), true); } void JSCExecutor::invokeCallback(const double callbackId, const folly::dynamic& arguments) throw(JSException) { - // TODO: Make this a first class function instead of evaling. #9317773 - // TODO(cjhopman): This copies args. - std::vector call{ - (double) callbackId, - std::move(arguments) - }; - std::string calls = executeJSCallWithJSC(m_context, "invokeCallbackAndReturnFlushedQueue", std::move(call)); + auto result = m_invokeCallbackAndReturnFlushedQueueJS->callAsFunction({ + JSValueMakeNumber(m_context, callbackId), + Value::fromDynamic(m_context, std::move(arguments)) + }); + auto calls = Value(m_context, result).toJSONString(); m_delegate->callNativeModules(*this, std::move(calls), true); } diff --git a/ReactCommon/cxxreact/JSCExecutor.h b/ReactCommon/cxxreact/JSCExecutor.h index 816bec1a545..b1dfcdb56d5 100644 --- a/ReactCommon/cxxreact/JSCExecutor.h +++ b/ReactCommon/cxxreact/JSCExecutor.h @@ -9,6 +9,7 @@ #include #include +#include #include "Executor.h" #include "ExecutorToken.h" @@ -91,6 +92,10 @@ private: std::unique_ptr m_unbundle; folly::dynamic m_jscConfig; + folly::Optional m_invokeCallbackAndReturnFlushedQueueJS; + folly::Optional m_callFunctionReturnFlushedQueueJS; + folly::Optional m_flushedQueueJS; + /** * WebWorker constructor. Must be invoked from thread this Executor will run on. */ @@ -105,6 +110,7 @@ private: void initOnJSVMThread() throw(JSException); void terminateOnJSVMThread(); + void bindBridge() throw(JSException); void flush() throw(JSException); void flushQueueImmediate(std::string queueJSON); void loadModule(uint32_t moduleId); diff --git a/ReactCommon/cxxreact/Value.cpp b/ReactCommon/cxxreact/Value.cpp index 9b236b0cee4..40990ad1341 100644 --- a/ReactCommon/cxxreact/Value.cpp +++ b/ReactCommon/cxxreact/Value.cpp @@ -1,5 +1,7 @@ // Copyright 2004-present Facebook. All Rights Reserved. +#include + #include "Value.h" #include "JSCHelpers.h" @@ -49,6 +51,11 @@ Value Value::fromJSON(JSContextRef ctx, const String& json) throw(JSException) { return Value(ctx, result); } +Value Value::fromDynamic(JSContextRef ctx, folly::dynamic value) throw(JSException) { + auto json = folly::toJson(value); + return fromJSON(ctx, String(json.c_str())); +} + Object Value::asObject() { JSValueRef exn; JSObjectRef jsObj = JSValueToObject(context(), m_value, &exn); @@ -65,7 +72,11 @@ Object::operator Value() const { return Value(m_context, m_obj); } -Value Object::callAsFunction(int nArgs, JSValueRef args[]) { +Value Object::callAsFunction(std::initializer_list args) const { + return callAsFunction(args.size(), args.begin()); +} + +Value Object::callAsFunction(int nArgs, const JSValueRef args[]) const { JSValueRef exn; JSValueRef result = JSObjectCallAsFunction(m_context, m_obj, NULL, nArgs, args, &exn); if (!result) { diff --git a/ReactCommon/cxxreact/Value.h b/ReactCommon/cxxreact/Value.h index 6291aabbeda..cfa74f4ac7d 100644 --- a/ReactCommon/cxxreact/Value.h +++ b/ReactCommon/cxxreact/Value.h @@ -12,6 +12,8 @@ #include #include +#include + #include "noncopyable.h" #if WITH_FBJSCEXTENSIONS @@ -148,6 +150,13 @@ public: } } + Object& operator=(Object&& other) { + std::swap(m_context, other.m_context); + std::swap(m_obj, other.m_obj); + std::swap(m_isProtected, other.m_isProtected); + return *this; + } + operator JSObjectRef() const { return m_obj; } @@ -158,7 +167,9 @@ public: return JSObjectIsFunction(m_context, m_obj); } - Value callAsFunction(int nArgs, JSValueRef args[]); + Value callAsFunction(std::initializer_list args) const; + + Value callAsFunction(int nArgs, const JSValueRef args[]) const; Value getProperty(const String& propName) const; Value getProperty(const char *propName) const; @@ -253,6 +264,7 @@ public: std::string toJSONString(unsigned indent = 0) const throw(JSException); static Value fromJSON(JSContextRef ctx, const String& json) throw(JSException); + static Value fromDynamic(JSContextRef ctx, folly::dynamic value) throw(JSException); protected: JSContextRef context() const; JSContextRef m_context;