Merge pull request #3251 from deepak1556/remote_callback_patch

remote: track listeners on browser side

atom/browser/lib/rpc-server.coffee

@@ -2,6 +2,10 @@ ipc = require 'ipc'
 path = require 'path'
 objectsRegistry = require './objects-registry.js'
 v8Util = process.atomBinding 'v8_util'
+IDWeakMap = process.atomBinding('id_weak_map').IDWeakMap
+
+# Object mapping from webcontents id to their renderer callbacks weakmap.
+rendererRegistry = {}
 
 # Convert a real value into meta data.
 valueToMeta = (sender, value, optimizeSimpleObject=false) ->
@@ -70,18 +74,30 @@ unwrapArgs = (sender, args) ->
         returnValue = metaToValue meta.value
         -> returnValue
       when 'function'
+        webContentsId = sender.getId()
+        rendererCallbacks = rendererRegistry[webContentsId]
+        if not rendererCallbacks?
+          # Weak reference to callbacks with their ID
+          rendererCallbacks = new IDWeakMap()
+          rendererRegistry[webContentsId] = rendererCallbacks
+
+        if rendererCallbacks.has(meta.id)
+          return rendererCallbacks.get(meta.id)
+
         rendererReleased = false
-        objectsRegistry.once "clear-#{sender.getId()}", ->
+        objectsRegistry.once "clear-#{webContentsId}", ->
           rendererReleased = true
 
         ret = ->
           if rendererReleased
             throw new Error("Attempting to call a function in a renderer window
-              that has been closed or released. Function provided here: #{meta.id}.")
+              that has been closed or released. Function provided here: #{meta.location}.")
           sender.send 'ATOM_RENDERER_CALLBACK', meta.id, valueToMeta(sender, arguments)
         v8Util.setDestructor ret, ->
           return if rendererReleased
+          rendererCallbacks.remove meta.id
           sender.send 'ATOM_RENDERER_RELEASE_CALLBACK', meta.id
+        rendererCallbacks.set meta.id, ret
         ret
       else throw new TypeError("Unknown type: #{meta.type}")
 
@@ -100,6 +116,8 @@ callFunction = (event, func, caller, args) ->
 
 # Send by BrowserWindow when its render view is deleted.
 process.on 'ATOM_BROWSER_RELEASE_RENDER_VIEW', (id) ->
+  if rendererRegistry.id?
+    delete rendererRegistry.id
   objectsRegistry.clear id
 
 ipc.on 'ATOM_BROWSER_REQUIRE', (event, module) ->

atom/common/api/atom_api_id_weak_map.cc

@@ -0,0 +1,79 @@
+// Copyright (c) 2015 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/atom_api_id_weak_map.h"
+
+#include "atom/common/node_includes.h"
+#include "native_mate/constructor.h"
+#include "native_mate/dictionary.h"
+
+namespace atom {
+
+namespace api {
+
+IDWeakMap::IDWeakMap() : id_weak_map_(new atom::IDWeakMap) {
+}
+
+IDWeakMap::~IDWeakMap() {
+  id_weak_map_ = nullptr;
+}
+
+void IDWeakMap::Set(v8::Isolate* isolate,
+                    int32_t id,
+                    v8::Local<v8::Object> object) {
+  id_weak_map_->Set(isolate, id, object);
+}
+
+v8::Local<v8::Object> IDWeakMap::Get(v8::Isolate* isolate, int32_t id) {
+  return id_weak_map_->Get(isolate, id).ToLocalChecked();
+}
+
+bool IDWeakMap::Has(int32_t id) {
+  return id_weak_map_->Has(id);
+}
+
+void IDWeakMap::Remove(int32_t id) {
+  id_weak_map_->Remove(id);
+}
+
+bool IDWeakMap::IsDestroyed() const {
+  return !id_weak_map_;
+}
+
+// static
+void IDWeakMap::BuildPrototype(v8::Isolate* isolate,
+                               v8::Local<v8::ObjectTemplate> prototype) {
+  mate::ObjectTemplateBuilder(isolate, prototype)
+      .SetMethod("set", &IDWeakMap::Set)
+      .SetMethod("get", &IDWeakMap::Get)
+      .SetMethod("has", &IDWeakMap::Has)
+      .SetMethod("remove", &IDWeakMap::Remove);
+}
+
+// static
+mate::Wrappable* IDWeakMap::Create(v8::Isolate* isolate) {
+  return new IDWeakMap;
+}
+
+}  // namespace api
+
+}  // namespace atom
+
+namespace {
+
+using atom::api::IDWeakMap;
+
+void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
+                v8::Local<v8::Context> context, void* priv) {
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::Local<v8::Function> constructor = mate::CreateConstructor<IDWeakMap>(
+      isolate, "IDWeakMap", base::Bind(&IDWeakMap::Create));
+  mate::Dictionary id_weak_map(isolate, constructor);
+  mate::Dictionary dict(isolate, exports);
+  dict.Set("IDWeakMap", id_weak_map);
+}
+
+}  // namespace
+
+NODE_MODULE_CONTEXT_AWARE_BUILTIN(atom_common_id_weak_map, Initialize)

atom/common/api/atom_api_id_weak_map.h

@@ -0,0 +1,46 @@
+// Copyright (c) 2015 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_ATOM_API_ID_WEAK_MAP_H_
+#define ATOM_COMMON_API_ATOM_API_ID_WEAK_MAP_H_
+
+#include "atom/common/id_weak_map.h"
+#include "native_mate/object_template_builder.h"
+#include "native_mate/handle.h"
+
+namespace atom {
+
+namespace api {
+
+class IDWeakMap : public mate::Wrappable {
+ public:
+  static mate::Wrappable* Create(v8::Isolate* isolate);
+
+  static void BuildPrototype(v8::Isolate* isolate,
+                             v8::Local<v8::ObjectTemplate> prototype);
+
+ protected:
+  IDWeakMap();
+  ~IDWeakMap();
+
+  // mate::Wrappable:
+  bool IsDestroyed() const override;
+
+ private:
+  // Api for IDWeakMap.
+  void Set(v8::Isolate* isolate, int32_t id, v8::Local<v8::Object> object);
+  v8::Local<v8::Object> Get(v8::Isolate* isolate, int32_t id);
+  bool Has(int32_t id);
+  void Remove(int32_t id);
+
+  atom::IDWeakMap* id_weak_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(IDWeakMap);
+};
+
+}  // namespace api
+
+}  // namespace atom
+
+#endif  // ATOM_COMMON_API_ATOM_API_ID_WEAK_MAP_H_

atom/common/api/lib/callbacks-registry.coffee

@@ -1,3 +1,5 @@
+v8Util = process.atomBinding 'v8_util'
+
 module.exports =
 class CallbacksRegistry
   constructor: ->
@@ -5,6 +7,9 @@ class CallbacksRegistry
     @callbacks = {}
 
   add: (callback) ->
+    if v8Util.getHiddenValue(callback, 'metaId')?
+      return v8Util.getHiddenValue(callback, 'metaId')
+
     id = ++@nextId
 
     # Capture the location of the function and put it in the ID string,
@@ -17,10 +22,11 @@ class CallbacksRegistry
       continue if location.indexOf('(native)') isnt -1
       continue if location.indexOf('atom.asar') isnt -1
       [x, filenameAndLine] = /([^/^\)]*)\)?$/gi.exec(location)
-      id = "#{filenameAndLine} (#{id})"
       break
 
     @callbacks[id] = callback
+    v8Util.setHiddenValue callback, 'metaId', id
+    v8Util.setHiddenValue callback, 'location', filenameAndLine
     id
 
   get: (id) ->

atom/common/id_weak_map.cc

@@ -32,12 +32,18 @@ IDWeakMap::IDWeakMap() : next_id_(0) {
 IDWeakMap::~IDWeakMap() {
 }
 
-int32_t IDWeakMap::Add(v8::Isolate* isolate, v8::Local<v8::Object> object) {
-  int32_t id = GetNextID();
+void IDWeakMap::Set(v8::Isolate* isolate,
+                    int32_t id,
+                    v8::Local<v8::Object> object) {
   auto global = make_linked_ptr(new v8::Global<v8::Object>(isolate, object));
   ObjectKey* key = new ObjectKey(id, this);
   global->SetWeak(key, OnObjectGC, v8::WeakCallbackType::kParameter);
   map_[id] = global;
+}
+
+int32_t IDWeakMap::Add(v8::Isolate* isolate, v8::Local<v8::Object> object) {
+  int32_t id = GetNextID();
+  Set(isolate, id, object);
   return id;
 }
 

atom/common/id_weak_map.h

@@ -19,6 +19,9 @@ class IDWeakMap {
   IDWeakMap();
   ~IDWeakMap();
 
+  // Sets the object to WeakMap with the given |id|.
+  void Set(v8::Isolate* isolate, int32_t id, v8::Local<v8::Object> object);
+
   // Adds |object| to WeakMap and returns its allocated |id|.
   int32_t Add(v8::Isolate* isolate, v8::Local<v8::Object> object);
 

atom/common/node_bindings.cc

@@ -48,6 +48,7 @@ REFERENCE_MODULE(atom_browser_window);
 REFERENCE_MODULE(atom_common_asar);
 REFERENCE_MODULE(atom_common_clipboard);
 REFERENCE_MODULE(atom_common_crash_reporter);
+REFERENCE_MODULE(atom_common_id_weak_map);
 REFERENCE_MODULE(atom_common_native_image);
 REFERENCE_MODULE(atom_common_screen);
 REFERENCE_MODULE(atom_common_shell);

atom/renderer/api/lib/remote.coffee

@@ -33,7 +33,7 @@ wrapArgs = (args, visited=[]) ->
     else if typeof value is 'function' and v8Util.getHiddenValue value, 'returnValue'
       type: 'function-with-return-value', value: valueToMeta(value())
     else if typeof value is 'function'
-      type: 'function', id: callbacksRegistry.add(value)
+      type: 'function', id: callbacksRegistry.add(value), location: v8Util.getHiddenValue value, 'location'
     else
       type: 'value', value: value
 

filenames.gypi

@@ -255,6 +255,8 @@
       'atom/common/api/atom_api_asar.cc',
       'atom/common/api/atom_api_clipboard.cc',
       'atom/common/api/atom_api_crash_reporter.cc',
+      'atom/common/api/atom_api_id_weak_map.cc',
+      'atom/common/api/atom_api_id_weak_map.h',
       'atom/common/api/atom_api_native_image.cc',
       'atom/common/api/atom_api_native_image.h',
       'atom/common/api/atom_api_native_image_mac.mm',

spec/api-ipc-spec.coffee

@@ -88,3 +88,16 @@ describe 'ipc module', ->
         w.destroy()
         done()
       w.loadUrl 'file://' + path.join(fixtures, 'api', 'send-sync-message.html')
+
+  describe 'remote listeners', ->
+    it 'can be added and removed correctly', ->
+      count = 0
+      w = new BrowserWindow(show: false)
+      listener = () ->
+        count += 1
+        w.removeListener 'blur', listener
+      w.on 'blur', listener
+      w.emit 'blur'
+      w.emit 'blur'
+      assert.equal count, 1
+      w.destroy()