Remove additional JSON encoding for native->JS communication

Reviewed By: mhorowitz Differential Revision: D3857323 fbshipit-source-id: 4386cc107b8a1425ecb7297b0f659f6c47f01a78
arv · Sep 19, 2016 · 145109f · 145109f
1 parent bd4cd6e
commit 145109f
Show file tree

Hide file tree

Showing 16 changed files with 224 additions and 100 deletions.
diff --git a/Libraries/BatchedBridge/BatchedBridge.js b/Libraries/BatchedBridge/BatchedBridge.js
@@ -12,12 +12,7 @@
 
 const MessageQueue = require('MessageQueue');
 
-const serializeNativeParams = typeof global.__fbBatchedBridgeSerializeNativeParams !== 'undefined';
-
-const BatchedBridge = new MessageQueue(
-  () => global.__fbBatchedBridgeConfig,
-  serializeNativeParams
-);
+const BatchedBridge = new MessageQueue(() => global.__fbBatchedBridgeConfig);
 
 // TODO: Move these around to solve the cycle in a cleaner way.
 

diff --git a/Libraries/Utilities/MessageQueue.js b/Libraries/Utilities/MessageQueue.js
@@ -19,6 +19,7 @@ const JSTimersExecution = require('JSTimersExecution');
 
 const invariant = require('fbjs/lib/invariant');
 const keyMirror = require('fbjs/lib/keyMirror');
+const deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
 const stringifySafe = require('stringifySafe');
 
 const MODULE_IDS = 0;
@@ -52,15 +53,14 @@ type Config = {
 };
 
 class MessageQueue {
-  constructor(configProvider: () => Config, serializeNativeParams: boolean) {
+  constructor(configProvider: () => Config) {
     this._callableModules = {};
     this._queue = [[], [], [], 0];
     this._callbacks = [];
     this._callbackID = 0;
     this._callID = 0;
     this._lastFlush = 0;
     this._eventLoopStartTime = new Date().getTime();
-    this._serializeNativeParams = serializeNativeParams;
 
     if (__DEV__) {
       this._debugInfo = {};
@@ -165,7 +165,7 @@ class MessageQueue {
   __nativeCall(module, method, params, onFail, onSucc) {
     if (onFail || onSucc) {
       if (__DEV__) {
-        let callId = this._callbackID >> 1;
+        const callId = this._callbackID >> 1;
         this._debugInfo[callId] = [module, method];
         if (callId > DEBUG_INFO_LIMIT) {
           delete this._debugInfo[callId - DEBUG_INFO_LIMIT];
@@ -186,8 +186,14 @@ class MessageQueue {
     this._queue[MODULE_IDS].push(module);
     this._queue[METHOD_IDS].push(method);
 
-    const preparedParams = this._serializeNativeParams ? JSON.stringify(params) : params;
-    this._queue[PARAMS].push(preparedParams);
+    if (__DEV__) {
+      // Any params sent over the bridge should be encodable as JSON
+      JSON.stringify(params);
+
+      // The params object should not be mutated after being queued
+      deepFreezeAndThrowOnMutationInDev(params);
+    }
+    this._queue[PARAMS].push(params);
 
     const now = new Date().getTime();
     if (global.nativeFlushQueueImmediate &&

diff --git a/Libraries/Utilities/PerformanceLogger.js b/Libraries/Utilities/PerformanceLogger.js
@@ -11,9 +11,7 @@
 'use strict';
 
 const BatchedBridge = require('BatchedBridge');
-const fbjsPerformanceNow = require('fbjs/lib/performanceNow');
-
-const performanceNow = global.nativePerformanceNow || fbjsPerformanceNow;
+const performanceNow = global.nativePerformanceNow || require('fbjs/lib/performanceNow');
 
 var timespans = {};
 var extras = {};

diff --git a/ReactAndroid/src/main/jni/xreact/jni/ProxyExecutor.cpp b/ReactAndroid/src/main/jni/xreact/jni/ProxyExecutor.cpp
@@ -58,9 +58,6 @@ ProxyExecutor::ProxyExecutor(jni::global_ref<jobject>&& executorInstance,
   setGlobalVariable(
     "__fbBatchedBridgeConfig",
     folly::make_unique<JSBigStdString>(detail::toStdString(folly::toJson(config))));
-  setGlobalVariable(
-    "__fbBatchedBridgeSerializeNativeParams",
-    folly::make_unique<JSBigStdString>("1"));
 }
 
 ProxyExecutor::~ProxyExecutor() {
@@ -95,7 +92,7 @@ void ProxyExecutor::callFunction(const std::string& moduleId, const std::string&
     std::move(arguments),
   };
   std::string result = executeJSCallWithProxy(m_executor.get(), "callFunctionReturnFlushedQueue", std::move(call));
-  m_delegate->callNativeModules(*this, result, true);
+  m_delegate->callNativeModules(*this, folly::parseJson(result), true);
 }
 
 void ProxyExecutor::invokeCallback(const double callbackId, const folly::dynamic& arguments) {
@@ -104,7 +101,7 @@ void ProxyExecutor::invokeCallback(const double callbackId, const folly::dynamic
     std::move(arguments)
   };
   std::string result = executeJSCallWithProxy(m_executor.get(), "invokeCallbackAndReturnFlushedQueue", std::move(call));
-  m_delegate->callNativeModules(*this, result, true);
+  m_delegate->callNativeModules(*this, folly::parseJson(result), true);
 }
 
 void ProxyExecutor::setGlobalVariable(std::string propName,

diff --git a/ReactCommon/cxxreact/BUCK b/ReactCommon/cxxreact/BUCK
@@ -53,6 +53,9 @@ elif THIS_IS_FBOBJC:
       frameworks = [
         '$SDKROOT/System/Library/Frameworks/JavaScriptCore.framework',
       ],
+      tests = [
+        react_native_xplat_target('cxxreact/tests:tests')
+      ],
       **kwargs_add(
         kwargs,
         preprocessor_flags = DEBUG_PREPROCESSOR_FLAGS,

diff --git a/ReactCommon/cxxreact/Executor.h b/ReactCommon/cxxreact/Executor.h
@@ -45,7 +45,7 @@ class ExecutorDelegate {
   virtual std::vector<std::string> moduleNames() = 0;
   virtual folly::dynamic getModuleConfig(const std::string& name) = 0;
   virtual void callNativeModules(
-    JSExecutor& executor, std::string callJSON, bool isEndOfBatch) = 0;
+    JSExecutor& executor, folly::dynamic&& calls, bool isEndOfBatch) = 0;
   virtual MethodCallResult callSerializableNativeHook(
     JSExecutor& executor, unsigned int moduleId, unsigned int methodId, folly::dynamic&& args) = 0;
 };

diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h
@@ -56,7 +56,7 @@ class Instance {
   void handleMemoryPressureCritical();
 
  private:
-  void callNativeModules(ExecutorToken token, const std::string& calls, bool isEndOfBatch);
+  void callNativeModules(ExecutorToken token, folly::dynamic&& calls, bool isEndOfBatch);
 
   std::shared_ptr<InstanceCallback> callback_;
   std::unique_ptr<NativeToJsBridge> nativeToJsBridge_;

diff --git a/ReactCommon/cxxreact/JSCExecutor.cpp b/ReactCommon/cxxreact/JSCExecutor.cpp
@@ -124,9 +124,6 @@ JSCExecutor::JSCExecutor(std::shared_ptr<ExecutorDelegate> delegate,
   setGlobalVariable(
     "__fbBatchedBridgeConfig",
     folly::make_unique<JSBigStdString>(detail::toStdString(folly::toJson(config))));
-  setGlobalVariable(
-    "__fbBatchedBridgeSerializeNativeParams",
-    folly::make_unique<JSBigStdString>(""));
 }
 
 JSCExecutor::JSCExecutor(
@@ -349,7 +346,7 @@ void JSCExecutor::callNativeModules(Value&& value) {
   SystraceSection s("JSCExecutor::callNativeModules");
   try {
     auto calls = value.toJSONString();
-    m_delegate->callNativeModules(*this, std::move(calls), true);
+    m_delegate->callNativeModules(*this, folly::parseJson(calls), true);
   } catch (...) {
     std::string message = "Error in callNativeModules()";
     try {
@@ -471,8 +468,9 @@ void JSCExecutor::handleMemoryPressureCritical() {
   #endif
 }
 
-void JSCExecutor::flushQueueImmediate(std::string queueJSON) {
-  m_delegate->callNativeModules(*this, std::move(queueJSON), false);
+void JSCExecutor::flushQueueImmediate(Value&& queue) {
+  auto queueStr = queue.toJSONString();
+  m_delegate->callNativeModules(*this, folly::parseJson(queueStr), false);
 }
 
 void JSCExecutor::loadModule(uint32_t moduleId) {
@@ -649,8 +647,7 @@ JSValueRef JSCExecutor::nativeFlushQueueImmediate(
     throw std::invalid_argument("Got wrong number of args");
   }
 
-  std::string resStr = Value(m_context, arguments[0]).toJSONString();
-  flushQueueImmediate(std::move(resStr));
+  flushQueueImmediate(Value(m_context, arguments[0]));
   return JSValueMakeUndefined(m_context);
 }
 

diff --git a/ReactCommon/cxxreact/JSCExecutor.h b/ReactCommon/cxxreact/JSCExecutor.h
@@ -119,7 +119,7 @@ class JSCExecutor : public JSExecutor {
   void bindBridge() throw(JSException);
   void callNativeModules(Value&&);
   void flush();
-  void flushQueueImmediate(std::string queueJSON);
+  void flushQueueImmediate(Value&&);
   void loadModule(uint32_t moduleId);
 
   int addWebWorker(std::string scriptURL, JSValueRef workerRef, JSValueRef globalObjRef);

diff --git a/ReactCommon/cxxreact/MethodCall.cpp b/ReactCommon/cxxreact/MethodCall.cpp
@@ -13,63 +13,56 @@ namespace react {
 #define REQUEST_PARAMSS 2
 #define REQUEST_CALLID 3
 
-std::vector<MethodCall> parseMethodCalls(const std::string& json) throw(std::invalid_argument) {
-  folly::dynamic jsonData = folly::parseJson(json);
-
+std::vector<MethodCall> parseMethodCalls(folly::dynamic&& jsonData) throw(std::invalid_argument) {
   if (jsonData.isNull()) {
     return {};
   }
 
   if (!jsonData.isArray()) {
     throw std::invalid_argument(
-        folly::to<std::string>("Did not get valid calls back from JS: ", jsonData.typeName()));
+      folly::to<std::string>("Did not get valid calls back from JS: ", jsonData.typeName()));
   }
 
   if (jsonData.size() < REQUEST_PARAMSS + 1) {
     throw std::invalid_argument(
-          folly::to<std::string>("Did not get valid calls back from JS: size == ", jsonData.size()));
+      folly::to<std::string>("Did not get valid calls back from JS: size == ", jsonData.size()));
   }
 
-  auto moduleIds = jsonData[REQUEST_MODULE_IDS];
-  auto methodIds = jsonData[REQUEST_METHOD_IDS];
-  auto params = jsonData[REQUEST_PARAMSS];
+  auto& moduleIds = jsonData[REQUEST_MODULE_IDS];
+  auto& methodIds = jsonData[REQUEST_METHOD_IDS];
+  auto& params = jsonData[REQUEST_PARAMSS];
   int  callId = -1;
 
   if (!moduleIds.isArray() || !methodIds.isArray() || !params.isArray()) {
     throw std::invalid_argument(
-          folly::to<std::string>("Did not get valid calls back from JS: ", json.c_str()));
+      folly::to<std::string>("Did not get valid calls back from JS: ", folly::toJson(jsonData)));
   }
 
   if (moduleIds.size() != methodIds.size() || moduleIds.size() != params.size()) {
     throw std::invalid_argument(
-          folly::to<std::string>("Did not get valid calls back from JS: ", json.c_str()));
+      folly::to<std::string>("Did not get valid calls back from JS: ", folly::toJson(jsonData)));
   }
 
   if (jsonData.size() > REQUEST_CALLID) {
     if (!jsonData[REQUEST_CALLID].isInt()) {
       throw std::invalid_argument(
-          folly::to<std::string>("Did not get valid calls back from JS: %s", json.c_str()));
+        folly::to<std::string>("Did not get valid calls back from JS: %s", folly::toJson(jsonData)));
     } else {
       callId = jsonData[REQUEST_CALLID].getInt();
     }
   }
 
   std::vector<MethodCall> methodCalls;
   for (size_t i = 0; i < moduleIds.size(); i++) {
-    if (!params[i].isString()) {
-      throw std::invalid_argument(
-          folly::to<std::string>("Call argument isn't a string"));
-    }
-    auto paramsValue = folly::parseJson(params[i].asString());
-    if (!paramsValue.isArray()) {
+    if (!params[i].isArray()) {
       throw std::invalid_argument(
-          folly::to<std::string>("Parsed params isn't an array"));
+          folly::to<std::string>("Call argument isn't an array"));
     }
 
     methodCalls.emplace_back(
       moduleIds[i].getInt(),
       methodIds[i].getInt(),
-      std::move(paramsValue),
+      std::move(params[i]),
       callId);
 
     // only incremement callid if contains valid callid as callid is optional

diff --git a/ReactCommon/cxxreact/MethodCall.h b/ReactCommon/cxxreact/MethodCall.h
@@ -24,6 +24,6 @@ struct MethodCall {
     , callId(cid) {}
 };
 
-std::vector<MethodCall> parseMethodCalls(const std::string& json) throw(std::invalid_argument);
+std::vector<MethodCall> parseMethodCalls(folly::dynamic&& calls) throw(std::invalid_argument);
 
 } }
diff --git a/ReactCommon/cxxreact/NativeToJsBridge.cpp b/ReactCommon/cxxreact/NativeToJsBridge.cpp
@@ -53,13 +53,13 @@ class JsToNativeBridge : public react::ExecutorDelegate {
   }
 
   void callNativeModules(
-      JSExecutor& executor, std::string callJSON, bool isEndOfBatch) override {
+      JSExecutor& executor, folly::dynamic&& calls, bool isEndOfBatch) override {
     ExecutorToken token = m_nativeToJs->getTokenForExecutor(executor);
-    m_nativeQueue->runOnQueue([this, token, callJSON=std::move(callJSON), isEndOfBatch] {
+    m_nativeQueue->runOnQueue([this, token, calls=std::move(calls), isEndOfBatch] () mutable {
       // An exception anywhere in here stops processing of the batch.  This
       // was the behavior of the Android bridge, and since exception handling
       // terminates the whole bridge, there's not much point in continuing.
-      for (auto& call : react::parseMethodCalls(callJSON)) {
+      for (auto& call : react::parseMethodCalls(std::move(calls))) {
         m_registry->callNativeMethod(
           token, call.moduleId, call.methodId, std::move(call.arguments), call.callId);
       }

diff --git a/ReactCommon/cxxreact/tests/BUCK b/ReactCommon/cxxreact/tests/BUCK
@@ -1,24 +1,42 @@
-include_defs('//ReactAndroid/DEFS')
-include_defs('//ReactAndroid/TEST_DEFS')
+TEST_SRCS = [
+  'CxxMessageQueueTest.cpp',
+  'jsarg_helpers.cpp',
+  'jscexecutor.cpp',
+  'jsclogging.cpp',
+  'methodcall.cpp',
+  'value.cpp',
+]
 
-jni_instrumentation_test_lib(
-  name = 'tests',
-  class_under_test = 'com/facebook/react/XplatBridgeTest',
-  soname = 'libxplat-bridge.so',
-  srcs = [
-    'CxxMessageQueueTest.cpp',
-    'value.cpp',
-    'methodcall.cpp',
-    'jsclogging.cpp',
-    'jscexecutor.cpp',
-  ],
-  compiler_flags = [
-    '-fexceptions',
-  ],
-  deps = [
-    '//native/third-party/android-ndk:android',
-    '//xplat/third-party/gmock:gtest',
-    react_native_xplat_target('cxxreact:bridge'),
-  ],
-  visibility = ['//instrumentation_tests/...'],
-)
+if THIS_IS_FBANDROID:
+  include_defs('//ReactAndroid/DEFS')
+  include_defs('//ReactAndroid/TEST_DEFS')
+
+  jni_instrumentation_test_lib(
+    name = 'tests',
+    class_under_test = 'com/facebook/react/XplatBridgeTest',
+    soname = 'libxplat-bridge.so',
+    srcs = TEST_SRCS,
+    compiler_flags = [
+      '-fexceptions',
+    ],
+    deps = [
+      '//native/third-party/android-ndk:android',
+      '//xplat/third-party/gmock:gtest',
+      react_native_xplat_target('cxxreact:bridge'),
+    ],
+    visibility = ['//instrumentation_tests/...'],
+  )
+
+if THIS_IS_FBOBJC:
+  fb_xplat_cxx_test(
+    name = 'tests',
+    srcs = TEST_SRCS,
+    compiler_flags = [
+      '-fexceptions',
+    ],
+    deps = [
+      '//xplat/third-party/gmock:gtest',
+      react_native_xplat_target('cxxreact:bridge'),
+    ],
+    visibility = [react_native_xplat_target('cxxreact/...')],
+  )