perf: don't use JSON to send the result of ipcRenderer.sendSync. (#8953)

* Don't use JSON to send the result of `ipcRenderer.sendSync`.

- Change the return type of AtomViewHostMsg_Message_Sync from `base::string16`
  to `base::ListValue`
- Adjust lib/browser/api/web-contents.js and /lib/renderer/api/ipc-renderer.js
  to wrap/unwrap return values to/from array, instead of
  serializing/deserializing JSON.

This change can greatly improve `ipcRenderer.sendSync` calls where the return
value contains Buffer instances, because those are converted to Array before
being serialized to JSON(which has no efficient way of representing byte
arrays).

A simple benchmark where remote.require('fs') was used to read a 16mb file got
at least 5x faster, not to mention it used a lot less memory.  This difference
tends increases with larger buffers.

* Don't base64 encode Buffers

* Don't allocate V8ValueConverter on the heap

* Replace hidden global.sandbox with NodeBindings::IsInitialized()

* Refactoring: check NodeBindings::IsInitialized() in V8ValueConverter

* Refactor problematic test to make it more reliable

* Add tests for NaN and Infinity

atom/browser/api/event.cc

@@ -6,6 +6,7 @@
 
 #include "atom/common/api/api_messages.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
+#include "atom/common/native_mate_converters/value_converter.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "native_mate/object_template_builder.h"
@@ -52,11 +53,11 @@ void Event::PreventDefault(v8::Isolate* isolate) {
   GetWrapper()->Set(StringToV8(isolate, "defaultPrevented"), v8::True(isolate));
 }
 
-bool Event::SendReply(const base::string16& json) {
+bool Event::SendReply(const base::ListValue& result) {
   if (message_ == nullptr || sender_ == nullptr)
     return false;
 
-  AtomFrameHostMsg_Message_Sync::WriteReplyParams(message_, json);
+  AtomFrameHostMsg_Message_Sync::WriteReplyParams(message_, result);
   bool success = sender_->Send(message_);
   message_ = nullptr;
   sender_ = nullptr;

atom/browser/api/event.h

@@ -29,8 +29,8 @@ class Event : public Wrappable<Event>, public content::WebContentsObserver {
   // event.PreventDefault().
   void PreventDefault(v8::Isolate* isolate);
 
-  // event.sendReply(json), used for replying synchronous message.
+  // event.sendReply(array), used for replying synchronous message.
-  bool SendReply(const base::string16& json);
+  bool SendReply(const base::ListValue& result);
 
  protected:
   explicit Event(v8::Isolate* isolate);

atom/common/api/api_messages.h

@@ -31,7 +31,7 @@ IPC_MESSAGE_ROUTED2(AtomFrameHostMsg_Message,
 IPC_SYNC_MESSAGE_ROUTED2_1(AtomFrameHostMsg_Message_Sync,
                            base::string16 /* channel */,
                            base::ListValue /* arguments */,
-                           base::string16 /* result (in JSON) */)
+                           base::ListValue /* result */)
 
 IPC_MESSAGE_ROUTED3(AtomFrameMsg_Message,
                     bool /* send_to_all */,

atom/common/native_mate_converters/v8_value_converter.cc

@@ -13,6 +13,7 @@
 #include "base/values.h"
 #include "native_mate/dictionary.h"
 
+#include "atom/common/node_bindings.h"
 #include "atom/common/node_includes.h"
 
 namespace atom {
@@ -136,10 +137,6 @@ void V8ValueConverter::SetStripNullFromObjects(bool val) {
   strip_null_from_objects_ = val;
 }
 
-void V8ValueConverter::SetDisableNode(bool val) {
-  disable_node_ = val;
-}
-
 v8::Local<v8::Value> V8ValueConverter::ToV8Value(
     const base::Value* value,
     v8::Local<v8::Context> context) const {
@@ -253,7 +250,7 @@ v8::Local<v8::Value> V8ValueConverter::ToArrayBuffer(
   const char* data = value->GetBlob().data();
   size_t length = value->GetBlob().size();
 
-  if (!disable_node_) {
+  if (NodeBindings::IsInitialized()) {
     return node::Buffer::Copy(isolate, data, length).ToLocalChecked();
   }
 

atom/common/native_mate_converters/v8_value_converter.h

@@ -24,7 +24,6 @@ class V8ValueConverter {
   void SetRegExpAllowed(bool val);
   void SetFunctionAllowed(bool val);
   void SetStripNullFromObjects(bool val);
-  void SetDisableNode(bool val);
   v8::Local<v8::Value> ToV8Value(const base::Value* value,
                                  v8::Local<v8::Context> context) const;
   base::Value* FromV8Value(v8::Local<v8::Value> value,
@@ -63,13 +62,6 @@ class V8ValueConverter {
   // If true, we will convert Function JavaScript objects to dictionaries.
   bool function_allowed_ = false;
 
-  // If true, will not use node::Buffer::Copy to deserialize byte arrays.
-  // node::Buffer::Copy depends on a working node.js environment, and this is
-  // not desirable in sandboxed renderers. That means Buffer instances sent from
-  // browser process will be deserialized as browserify-based Buffer(which are
-  // wrappers around Uint8Array).
-  bool disable_node_ = false;
-
   // If true, undefined and null values are ignored when converting v8 objects
   // into Values.
   bool strip_null_from_objects_ = false;

atom/common/node_bindings.cc

@@ -104,6 +104,8 @@ void stop_and_close_uv_loop(uv_loop_t* loop) {
   uv_loop_close(loop);
 }
 
+bool g_is_initialized = false;
+
 }  // namespace
 
 namespace atom {
@@ -178,6 +180,10 @@ void NodeBindings::RegisterBuiltinModules() {
 #undef V
 }
 
+bool NodeBindings::IsInitialized() {
+  return g_is_initialized;
+}
+
 void NodeBindings::Initialize() {
   // Open node's error reporting system for browser process.
   node::g_standalone_mode = browser_env_ == BROWSER;
@@ -203,6 +209,8 @@ void NodeBindings::Initialize() {
   if (browser_env_ == BROWSER || env->HasVar("ELECTRON_DEFAULT_ERROR_MODE"))
     SetErrorMode(GetErrorMode() & ~SEM_NOGPFAULTERRORBOX);
 #endif
+
+  g_is_initialized = true;
 }
 
 node::Environment* NodeBindings::CreateEnvironment(

atom/common/node_bindings.h

@@ -32,6 +32,7 @@ class NodeBindings {
 
   static NodeBindings* Create(BrowserEnvironment browser_env);
   static void RegisterBuiltinModules();
+  static bool IsInitialized();
 
   virtual ~NodeBindings();
 

atom/renderer/api/atom_api_renderer_ipc.cc

@@ -6,6 +6,7 @@
 #include "atom/common/api/api_messages.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
+#include "atom/common/node_bindings.h"
 #include "atom/common/node_includes.h"
 #include "content/public/renderer/render_frame.h"
 #include "native_mate/dictionary.h"
@@ -40,23 +41,23 @@ void Send(mate::Arguments* args,
     args->ThrowError("Unable to send AtomFrameHostMsg_Message");
 }
 
-base::string16 SendSync(mate::Arguments* args,
+base::ListValue SendSync(mate::Arguments* args,
-                        const base::string16& channel,
+                         const base::string16& channel,
-                        const base::ListValue& arguments) {
+                         const base::ListValue& arguments) {
-  base::string16 json;
+  base::ListValue result;
 
   RenderFrame* render_frame = GetCurrentRenderFrame();
   if (render_frame == nullptr)
-    return json;
+    return result;
 
   IPC::SyncMessage* message = new AtomFrameHostMsg_Message_Sync(
-      render_frame->GetRoutingID(), channel, arguments, &json);
+      render_frame->GetRoutingID(), channel, arguments, &result);
   bool success = render_frame->Send(message);
 
   if (!success)
     args->ThrowError("Unable to send AtomFrameHostMsg_Message_Sync");
 
-  return json;
+  return result;
 }
 
 void Initialize(v8::Local<v8::Object> exports,

atom/renderer/api/atom_api_renderer_ipc.h

@@ -16,9 +16,9 @@ void Send(mate::Arguments* args,
           const base::string16& channel,
           const base::ListValue& arguments);
 
-base::string16 SendSync(mate::Arguments* args,
+base::ListValue SendSync(mate::Arguments* args,
-                        const base::string16& channel,
+                         const base::string16& channel,
-                        const base::ListValue& arguments);
+                         const base::ListValue& arguments);
 
 void Initialize(v8::Local<v8::Object> exports,
                 v8::Local<v8::Value> unused,

atom/renderer/atom_sandboxed_renderer_client.cc

@@ -7,7 +7,6 @@
 #include "atom/common/api/api_messages.h"
 #include "atom/common/api/atom_bindings.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
-#include "atom/common/native_mate_converters/v8_value_converter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "atom/common/node_bindings.h"
 #include "atom/common/options_switches.h"
@@ -100,10 +99,7 @@ class AtomSandboxedRenderFrameObserver : public AtomRenderFrameObserver {
   AtomSandboxedRenderFrameObserver(content::RenderFrame* render_frame,
                                    AtomSandboxedRendererClient* renderer_client)
       : AtomRenderFrameObserver(render_frame, renderer_client),
-        v8_converter_(new atom::V8ValueConverter),
+        renderer_client_(renderer_client) {}
-        renderer_client_(renderer_client) {
-    v8_converter_->SetDisableNode(true);
-  }
 
  protected:
   void EmitIPCEvent(blink::WebLocalFrame* frame,
@@ -117,14 +113,13 @@ class AtomSandboxedRenderFrameObserver : public AtomRenderFrameObserver {
     auto context = frame->MainWorldScriptContext();
     v8::Context::Scope context_scope(context);
     v8::Local<v8::Value> argv[] = {mate::ConvertToV8(isolate, channel),
-                                   v8_converter_->ToV8Value(&args, context)};
+                                   mate::ConvertToV8(isolate, args)};
     renderer_client_->InvokeIpcCallback(
         context, "onMessage",
-        std::vector<v8::Local<v8::Value>>(argv, argv + 2));
+        std::vector<v8::Local<v8::Value>>(argv, argv + node::arraysize(argv)));
   }
 
  private:
-  std::unique_ptr<atom::V8ValueConverter> v8_converter_;
   AtomSandboxedRendererClient* renderer_client_;
   DISALLOW_COPY_AND_ASSIGN(AtomSandboxedRenderFrameObserver);
 };

lib/browser/api/web-contents.js

@@ -280,7 +280,7 @@ WebContents.prototype._init = function () {
   this.on('ipc-message-sync', function (event, [channel, ...args]) {
     Object.defineProperty(event, 'returnValue', {
       set: function (value) {
-        return event.sendReply(JSON.stringify(value))
+        return event.sendReply([value])
       },
       get: function () {}
     })

lib/common/buffer-utils.js

@@ -26,25 +26,31 @@ function getType (value) {
   return null
 }
 
+function getBuffer (value) {
+  if (value instanceof Buffer) {
+    return value
+  } else if (value instanceof ArrayBuffer) {
+    return Buffer.from(value)
+  } else {
+    return Buffer.from(value.buffer, value.byteOffset, value.byteLength)
+  }
+}
+
 exports.isBuffer = function (value) {
   return ArrayBuffer.isView(value) || value instanceof ArrayBuffer
 }
 
 exports.bufferToMeta = function (value) {
-  const buffer = (value instanceof ArrayBuffer)
-    ? Buffer.from(value)
-    : Buffer.from(value.buffer, value.byteOffset, value.byteLength)
-
   return {
     type: getType(value),
-    data: buffer.toString('base64'),
+    data: getBuffer(value),
     length: value.length
   }
 }
 
 exports.metaToBuffer = function (value) {
   const constructor = typedArrays[value.type]
-  const data = Buffer.from(value.data, 'base64')
+  const data = getBuffer(value.data)
 
   if (constructor === Buffer) {
     return data

lib/renderer/api/ipc-renderer.js

@@ -11,7 +11,7 @@ ipcRenderer.send = function (...args) {
 }
 
 ipcRenderer.sendSync = function (...args) {
-  return JSON.parse(binding.sendSync('ipc-message-sync', args))
+  return binding.sendSync('ipc-message-sync', args)[0]
 }
 
 ipcRenderer.sendToHost = function (...args) {

spec/api-browser-window-spec.js

@@ -1381,24 +1381,25 @@ describe('BrowserWindow module', () => {
         })
         let htmlPath = path.join(fixtures, 'api', 'sandbox.html?verify-ipc-sender')
         const pageUrl = 'file://' + htmlPath
-        w.loadURL(pageUrl)
+        let childWc
         w.webContents.once('new-window', (e, url, frameName, disposition, options) => {
-          let parentWc = w.webContents
+          childWc = options.webContents
-          let childWc = options.webContents
+          assert.notEqual(w.webContents, childWc)
-          assert.notEqual(parentWc, childWc)
-          ipcMain.once('parent-ready', function (event) {
-            assert.equal(parentWc, event.sender)
-            parentWc.send('verified')
-          })
-          ipcMain.once('child-ready', function (event) {
-            assert.equal(childWc, event.sender)
-            childWc.send('verified')
-          })
-          waitForEvents(ipcMain, [
-            'parent-answer',
-            'child-answer'
-          ], done)
         })
+        ipcMain.once('parent-ready', function (event) {
+          assert.equal(w.webContents, event.sender)
+          event.sender.send('verified')
+        })
+        ipcMain.once('child-ready', function (event) {
+          assert(childWc)
+          assert.equal(childWc, event.sender)
+          event.sender.send('verified')
+        })
+        waitForEvents(ipcMain, [
+          'parent-answer',
+          'child-answer'
+        ], done)
+        w.loadURL(pageUrl)
       })
 
       describe('event handling', () => {

spec/api-remote-spec.js

@@ -237,6 +237,16 @@ describe('remote module', () => {
     const print = path.join(fixtures, 'module', 'print_name.js')
     const printName = remote.require(print)
 
+    it('converts NaN to undefined', () => {
+      assert.strictEqual(printName.getNaN(), undefined)
+      assert.strictEqual(printName.echo(NaN), undefined)
+    })
+
+    it('converts Infinity to undefined', () => {
+      assert.strictEqual(printName.getInfinity(), undefined)
+      assert.strictEqual(printName.echo(Infinity), undefined)
+    })
+
     it('keeps its constructor name for objects', () => {
       const buf = Buffer.from('test')
       assert.equal(printName.print(buf), 'Buffer')

spec/fixtures/api/sandbox.html

@@ -98,10 +98,8 @@
         popup.ipcRenderer.once('verified', () => {
           popup.ipcRenderer.send('child-answer')
         })
-        setTimeout(() => {
+        ipcRenderer.send('parent-ready')
-          ipcRenderer.send('parent-ready')
+        popup.ipcRenderer.send('child-ready')
-          popup.ipcRenderer.send('child-ready')
-        }, 50)
       }
     }
 

spec/fixtures/module/print_name.js

@@ -26,3 +26,11 @@ exports.typedArray = function (type, values) {
   }
   return array
 }
+
+exports.getNaN = function () {
+  return NaN
+}
+
+exports.getInfinity = function () {
+  return Infinity
+}