Dereference remote objects with native code

Previously we rely on the v8util.setDestructor to dereference the remote
objects in JavaScript, however as documented in V8, it is forbidden to
call V8 APIs in object's destructor (e.g. the weak callback), and doing
so would result in crashs.

This commit removes the JavaScript setDestructor method, and avoids
doing the dereference work with V8.

atom/common/api/atom_api_v8_util.cc

@@ -4,7 +4,9 @@
 
 #include <string>
 
-#include "atom/common/api/object_life_monitor.h"
+#include "atom/common/api/remote_callback_freer.h"
+#include "atom/common/api/remote_object_freer.h"
+#include "atom/common/native_mate_converters/content_converter.h"
 #include "atom/common/node_includes.h"
 #include "native_mate/dictionary.h"
 #include "v8/include/v8-profiler.h"
@@ -51,12 +53,6 @@ int32_t GetObjectHash(v8::Local<v8::Object> object) {
   return object->GetIdentityHash();
 }
 
-void SetDestructor(v8::Isolate* isolate,
-                   v8::Local<v8::Object> object,
-                   v8::Local<v8::Function> callback) {
-  atom::ObjectLifeMonitor::BindTo(isolate, object, callback);
-}
-
 void TakeHeapSnapshot(v8::Isolate* isolate) {
   isolate->GetHeapProfiler()->TakeHeapSnapshot();
 }
@@ -68,8 +64,9 @@ void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
   dict.SetMethod("setHiddenValue", &SetHiddenValue);
   dict.SetMethod("deleteHiddenValue", &DeleteHiddenValue);
   dict.SetMethod("getObjectHash", &GetObjectHash);
-  dict.SetMethod("setDestructor", &SetDestructor);
   dict.SetMethod("takeHeapSnapshot", &TakeHeapSnapshot);
+  dict.SetMethod("setRemoteCallbackFreer", &atom::RemoteCallbackFreer::BindTo);
+  dict.SetMethod("setRemoteObjectFreer", &atom::RemoteObjectFreer::BindTo);
 }
 
 }  // namespace

atom/common/api/object_life_monitor.cc

@@ -10,30 +10,28 @@
 
 namespace atom {
 
-// static
-void ObjectLifeMonitor::BindTo(v8::Isolate* isolate,
-                               v8::Local<v8::Object> target,
-                               v8::Local<v8::Function> destructor) {
-  new ObjectLifeMonitor(isolate, target, destructor);
-}
-
 ObjectLifeMonitor::ObjectLifeMonitor(v8::Isolate* isolate,
-                                     v8::Local<v8::Object> target,
-                                     v8::Local<v8::Function> destructor)
+                                     v8::Local<v8::Object> target)
     : isolate_(isolate),
       context_(isolate, isolate->GetCurrentContext()),
       target_(isolate, target),
-      destructor_(isolate, destructor),
       weak_ptr_factory_(this) {
   target_.SetWeak(this, OnObjectGC, v8::WeakCallbackType::kParameter);
 }
 
+ObjectLifeMonitor::~ObjectLifeMonitor() {
+  if (target_.IsEmpty())
+    return;
+  target_.ClearWeak();
+  target_.Reset();
+}
+
 // static
 void ObjectLifeMonitor::OnObjectGC(
     const v8::WeakCallbackInfo<ObjectLifeMonitor>& data) {
   ObjectLifeMonitor* self = data.GetParameter();
   self->target_.Reset();
-  self->RunCallback();
+  self->RunDestructor();
   data.SetSecondPassCallback(Free);
 }
 
@@ -43,13 +41,4 @@ void ObjectLifeMonitor::Free(
   delete data.GetParameter();
 }
 
-void ObjectLifeMonitor::RunCallback() {
-  v8::HandleScope handle_scope(isolate_);
-  v8::Local<v8::Context> context = v8::Local<v8::Context>::New(
-      isolate_, context_);
-  v8::Context::Scope context_scope(context);
-  v8::Local<v8::Function>::New(isolate_, destructor_)->Call(
-      context->Global(), 0, nullptr);
-}
-
 }  // namespace atom

atom/common/api/object_life_monitor.h

@@ -12,25 +12,19 @@
 namespace atom {
 
 class ObjectLifeMonitor {
- public:
-  static void BindTo(v8::Isolate* isolate,
-                     v8::Local<v8::Object> target,
-                     v8::Local<v8::Function> destructor);
+ protected:
+  ObjectLifeMonitor(v8::Isolate* isolate, v8::Local<v8::Object> target);
+  virtual ~ObjectLifeMonitor();
+
+  virtual void RunDestructor() = 0;
 
  private:
-  ObjectLifeMonitor(v8::Isolate* isolate,
-                    v8::Local<v8::Object> target,
-                    v8::Local<v8::Function> destructor);
-
   static void OnObjectGC(const v8::WeakCallbackInfo<ObjectLifeMonitor>& data);
   static void Free(const v8::WeakCallbackInfo<ObjectLifeMonitor>& data);
 
-  void RunCallback();
-
   v8::Isolate* isolate_;
   v8::Global<v8::Context> context_;
   v8::Global<v8::Object> target_;
-  v8::Global<v8::Function> destructor_;
 
   base::WeakPtrFactory<ObjectLifeMonitor> weak_ptr_factory_;
 

atom/common/api/remote_callback_freer.cc

@@ -0,0 +1,70 @@
+// Copyright (c) 2016 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/common/api/remote_callback_freer.h"
+
+#include "atom/common/api/api_messages.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+
+namespace atom {
+
+// static
+void RemoteCallbackFreer::BindTo(v8::Isolate* isolate,
+                                 v8::Local<v8::Object> target,
+                                 int object_id,
+                                 content::WebContents* web_contents) {
+  new RemoteCallbackFreer(isolate, target, object_id, web_contents);
+}
+
+RemoteCallbackFreer::RemoteCallbackFreer(v8::Isolate* isolate,
+                                         v8::Local<v8::Object> target,
+                                         int object_id,
+                                         content::WebContents* web_contents)
+    : ObjectLifeMonitor(isolate, target),
+      content::WebContentsObserver(web_contents),
+      web_contents_(web_contents),
+      renderer_process_id_(GetRendererProcessID()),
+      object_id_(object_id) {
+}
+
+RemoteCallbackFreer::~RemoteCallbackFreer() {
+}
+
+void RemoteCallbackFreer::RunDestructor() {
+  if (!web_contents_)
+    return;
+
+  if (renderer_process_id_ == GetRendererProcessID()) {
+    base::string16 channel =
+        base::ASCIIToUTF16("ELECTRON_RENDERER_RELEASE_CALLBACK");
+    base::ListValue args;
+    args.AppendInteger(object_id_);
+    Send(new AtomViewMsg_Message(routing_id(), channel, args));
+  }
+  web_contents_ = nullptr;
+}
+
+void RemoteCallbackFreer::WebContentsDestroyed() {
+  if (!web_contents_)
+    return;
+
+  web_contents_ = nullptr;
+  delete this;
+}
+
+int RemoteCallbackFreer::GetRendererProcessID() {
+  if (!web_contents_)
+    return -1;
+
+  auto process = web_contents()->GetRenderProcessHost();
+  if (!process)
+    return -1;
+
+  return process->GetID();
+}
+
+}  // namespace atom

atom/common/api/remote_callback_freer.h

@@ -0,0 +1,44 @@
+// Copyright (c) 2016 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_COMMON_API_REMOTE_CALLBACK_FREER_H_
+#define ATOM_COMMON_API_REMOTE_CALLBACK_FREER_H_
+#include "atom/common/api/object_life_monitor.h"
+#include "content/public/browser/web_contents_observer.h"
+
+namespace atom {
+
+class RemoteCallbackFreer : public ObjectLifeMonitor,
+                            public content::WebContentsObserver {
+ public:
+  static void BindTo(v8::Isolate* isolate,
+                     v8::Local<v8::Object> target,
+                     int object_id,
+                     content::WebContents* web_conents);
+
+ protected:
+  RemoteCallbackFreer(v8::Isolate* isolate,
+                      v8::Local<v8::Object> target,
+                      int object_id,
+                      content::WebContents* web_conents);
+  ~RemoteCallbackFreer() override;
+
+  void RunDestructor() override;
+
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override;
+
+ private:
+  int GetRendererProcessID();
+
+  content::WebContents* web_contents_;
+  int renderer_process_id_;
+  int object_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteCallbackFreer);
+};
+
+}  // namespace atom
+
+#endif  // ATOM_COMMON_API_REMOTE_CALLBACK_FREER_H_

atom/common/api/remote_object_freer.cc

@@ -0,0 +1,63 @@
+// Copyright (c) 2016 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/common/api/remote_object_freer.h"
+
+#include "atom/common/api/api_messages.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "content/public/renderer/render_view.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
+#include "third_party/WebKit/public/web/WebView.h"
+
+using blink::WebLocalFrame;
+using blink::WebView;
+
+namespace atom {
+
+namespace {
+
+content::RenderView* GetCurrentRenderView() {
+  WebLocalFrame* frame = WebLocalFrame::frameForCurrentContext();
+  if (!frame)
+    return nullptr;
+
+  WebView* view = frame->view();
+  if (!view)
+    return nullptr;  // can happen during closing.
+
+  return content::RenderView::FromWebView(view);
+}
+
+}  // namespace
+
+// static
+void RemoteObjectFreer::BindTo(
+    v8::Isolate* isolate, v8::Local<v8::Object> target, int object_id) {
+  new RemoteObjectFreer(isolate, target, object_id);
+}
+
+RemoteObjectFreer::RemoteObjectFreer(
+    v8::Isolate* isolate, v8::Local<v8::Object> target, int object_id)
+    : ObjectLifeMonitor(isolate, target),
+      object_id_(object_id) {
+}
+
+RemoteObjectFreer::~RemoteObjectFreer() {
+}
+
+void RemoteObjectFreer::RunDestructor() {
+  content::RenderView* render_view = GetCurrentRenderView();
+  if (!render_view)
+    return;
+
+  base::string16 channel = base::ASCIIToUTF16("ipc-message");
+  base::ListValue args;
+  args.AppendString("ELECTRON_BROWSER_DEREFERENCE");
+  args.AppendInteger(object_id_);
+  render_view->Send(
+      new AtomViewHostMsg_Message(render_view->GetRoutingID(), channel, args));
+}
+
+}  // namespace atom

atom/common/api/remote_object_freer.h

@@ -0,0 +1,32 @@
+// Copyright (c) 2016 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_
+#define ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_
+
+#include "atom/common/api/object_life_monitor.h"
+
+namespace atom {
+
+class RemoteObjectFreer : public ObjectLifeMonitor {
+ public:
+  static void BindTo(
+      v8::Isolate* isolate, v8::Local<v8::Object> target, int object_id);
+
+ protected:
+  RemoteObjectFreer(
+      v8::Isolate* isolate, v8::Local<v8::Object> target, int object_id);
+  ~RemoteObjectFreer() override;
+
+  void RunDestructor() override;
+
+ private:
+  int object_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteObjectFreer);
+};
+
+}  // namespace atom
+
+#endif  // ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_

atom/common/native_mate_converters/content_converter.cc

@@ -180,4 +180,17 @@ v8::Local<v8::Value> Converter<content::WebContents*>::ToV8(
   return atom::api::WebContents::CreateFrom(isolate, val).ToV8();
 }
 
+// static
+bool Converter<content::WebContents*>::FromV8(
+    v8::Isolate* isolate,
+    v8::Local<v8::Value> val,
+    content::WebContents** out) {
+  atom::api::WebContents* web_contents = nullptr;
+  if (!ConvertFromV8(isolate, val, &web_contents) || !web_contents)
+    return false;
+
+  *out = web_contents->web_contents();
+  return true;
+}
+
 }  // namespace mate

atom/common/native_mate_converters/content_converter.h

@@ -57,6 +57,8 @@ template<>
 struct Converter<content::WebContents*> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
                                    content::WebContents* val);
+  static bool FromV8(v8::Isolate* isolate, v8::Local<v8::Value> val,
+                     content::WebContents** out);
 };
 
 }  // namespace mate

filenames.gypi

@@ -303,6 +303,10 @@
       'atom/common/api/locker.h',
       'atom/common/api/object_life_monitor.cc',
       'atom/common/api/object_life_monitor.h',
+      'atom/common/api/remote_callback_freer.cc',
+      'atom/common/api/remote_callback_freer.h',
+      'atom/common/api/remote_object_freer.cc',
+      'atom/common/api/remote_object_freer.h',
       'atom/common/asar/archive.cc',
       'atom/common/asar/archive.h',
       'atom/common/asar/asar_util.cc',

lib/browser/rpc-server.js

@@ -12,8 +12,19 @@ const FUNCTION_PROPERTIES = [
 ]
 
 // The remote functions in renderer processes.
-// (webContentsId) => {id: Function}
-let rendererFunctions = {}
+// id => Function
+let rendererFunctions = new IDWeakMap()
+
+// Merge two IDs together.
+let mergeIds = function (webContentsId, metaId) {
+  const PADDING_BITS = 20
+  if ((webContentsId << PADDING_BITS) < 0) {
+    throw new Error(`webContents ID is too large: ${webContentsId}`)
+  } else if (metaId > (1 << PADDING_BITS)) {
+    throw new Error(`Object ID is too large: ${metaId}`)
+  }
+  return (webContentsId << PADDING_BITS) + metaId
+}
 
 // Return the description of object's members:
 let getObjectMembers = function (object) {
@@ -165,32 +176,26 @@ var unwrapArgs = function (sender, args) {
           return returnValue
         }
       case 'function': {
+        // Merge webContentsId and meta.id, since meta.id can be the same in
+        // different webContents.
+        const webContentsId = sender.getId()
+        const objectId = mergeIds(webContentsId, meta.id)
+
         // Cache the callbacks in renderer.
-        let webContentsId = sender.getId()
-        let callbacks = rendererFunctions[webContentsId]
-        if (!callbacks) {
-          callbacks = rendererFunctions[webContentsId] = new IDWeakMap()
-          sender.once('render-view-deleted', function (event, id) {
-            callbacks.clear()
-            delete rendererFunctions[id]
-          })
+        if (rendererFunctions.has(objectId)) {
+          return rendererFunctions.get(objectId)
         }
 
-        if (callbacks.has(meta.id)) return callbacks.get(meta.id)
-
         let callIntoRenderer = function (...args) {
-          if ((webContentsId in rendererFunctions) && !sender.isDestroyed()) {
+          if (!sender.isDestroyed() && webContentsId === sender.getId()) {
             sender.send('ELECTRON_RENDERER_CALLBACK', meta.id, valueToMeta(sender, args))
           } else {
             throw new Error(`Attempting to call a function in a renderer window that has been closed or released. Function provided here: ${meta.location}.`)
           }
         }
-        v8Util.setDestructor(callIntoRenderer, function () {
-          if ((webContentsId in rendererFunctions) && !sender.isDestroyed()) {
-            sender.send('ELECTRON_RENDERER_RELEASE_CALLBACK', meta.id)
-          }
-        })
-        callbacks.set(meta.id, callIntoRenderer)
+
+        v8Util.setRemoteCallbackFreer(callIntoRenderer, meta.id, sender)
+        rendererFunctions.set(objectId, callIntoRenderer)
         return callIntoRenderer
       }
       default:

lib/renderer/api/remote.js

@@ -210,9 +210,7 @@ let metaToValue = function (meta) {
 
       // Track delegate object's life time, and tell the browser to clean up
       // when the object is GCed.
-      v8Util.setDestructor(ret, function () {
-        ipcRenderer.send('ELECTRON_BROWSER_DEREFERENCE', meta.id)
-      })
+      v8Util.setRemoteObjectFreer(ret, meta.id)
 
       // Remember object's id.
       v8Util.setHiddenValue(ret, 'atomId', meta.id)