Merge pull request #5491 from electron/key-weak-map

Extend the IDWeakMap to accept arbitrary key type

atom/browser/api/trackable_object.h

@@ -8,7 +8,7 @@
 #include <vector>
 
 #include "atom/browser/api/event_emitter.h"
-#include "atom/common/id_weak_map.h"
+#include "atom/common/key_weak_map.h"
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -113,21 +113,26 @@ class TrackableObject : public TrackableObjectBase,
 
   void AfterInit(v8::Isolate* isolate) override {
     if (!weak_map_) {
-      weak_map_.reset(new atom::IDWeakMap);
+      weak_map_.reset(new atom::KeyWeakMap<int32_t>);
     }
-    weak_map_id_ = weak_map_->Add(isolate, Wrappable<T>::GetWrapper());
+    weak_map_id_ = ++next_id_;
+    weak_map_->Set(isolate, weak_map_id_, Wrappable<T>::GetWrapper());
     if (wrapped_)
       AttachAsUserData(wrapped_);
   }
 
  private:
-  static scoped_ptr<atom::IDWeakMap> weak_map_;
+  static int32_t next_id_;
+  static scoped_ptr<atom::KeyWeakMap<int32_t>> weak_map_;
 
   DISALLOW_COPY_AND_ASSIGN(TrackableObject);
 };
 
 template<typename T>
-scoped_ptr<atom::IDWeakMap> TrackableObject<T>::weak_map_;
+int32_t TrackableObject<T>::next_id_ = 0;
+
+template<typename T>
+scoped_ptr<atom::KeyWeakMap<int32_t>> TrackableObject<T>::weak_map_;
 
 }  // namespace mate
 

atom/common/api/atom_api_id_weak_map.cc

@@ -1,79 +0,0 @@
-// 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(v8::Isolate* isolate) {
-}
-
-IDWeakMap::~IDWeakMap() {
-}
-
-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);
-}
-
-void IDWeakMap::Clear() {
-  id_weak_map_.Clear();
-}
-
-// 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)
-      .SetMethod("clear", &IDWeakMap::Clear);
-}
-
-// static
-mate::WrappableBase* IDWeakMap::Create(v8::Isolate* isolate) {
-  return new IDWeakMap(isolate);
-}
-
-}  // 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

@@ -1,44 +0,0 @@
-// 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<IDWeakMap> {
- public:
-  static mate::WrappableBase* Create(v8::Isolate* isolate);
-
-  static void BuildPrototype(v8::Isolate* isolate,
-                             v8::Local<v8::ObjectTemplate> prototype);
-
- protected:
-  explicit IDWeakMap(v8::Isolate* isolate);
-  ~IDWeakMap();
-
- 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);
-  void Clear();
-
-  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/atom_api_key_weak_map.h

@@ -0,0 +1,65 @@
+// 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_ATOM_API_KEY_WEAK_MAP_H_
+#define ATOM_COMMON_API_ATOM_API_KEY_WEAK_MAP_H_
+
+#include "atom/common/key_weak_map.h"
+#include "native_mate/object_template_builder.h"
+#include "native_mate/handle.h"
+
+namespace atom {
+
+namespace api {
+
+template<typename K>
+class KeyWeakMap : public mate::Wrappable<KeyWeakMap<K>> {
+ public:
+  static mate::Handle<KeyWeakMap<K>> Create(v8::Isolate* isolate) {
+    return mate::CreateHandle(isolate, new KeyWeakMap<K>(isolate));
+  }
+
+  static void BuildPrototype(v8::Isolate* isolate,
+                             v8::Local<v8::ObjectTemplate> prototype) {
+    mate::ObjectTemplateBuilder(isolate, prototype)
+        .SetMethod("set", &KeyWeakMap<K>::Set)
+        .SetMethod("get", &KeyWeakMap<K>::Get)
+        .SetMethod("has", &KeyWeakMap<K>::Has)
+        .SetMethod("remove", &KeyWeakMap<K>::Remove);
+  }
+
+ protected:
+  explicit KeyWeakMap(v8::Isolate* isolate) {
+    mate::Wrappable<KeyWeakMap<K>>::Init(isolate);
+  }
+  ~KeyWeakMap() override {}
+
+ private:
+  // API for KeyWeakMap.
+  void Set(v8::Isolate* isolate, const K& key, v8::Local<v8::Object> object) {
+    key_weak_map_.Set(isolate, key, object);
+  }
+
+  v8::Local<v8::Object> Get(v8::Isolate* isolate, const K& key) {
+    return key_weak_map_.Get(isolate, key).ToLocalChecked();
+  }
+
+  bool Has(const K& key) {
+    return key_weak_map_.Has(key);
+  }
+
+  void Remove(const K& key) {
+    key_weak_map_.Remove(key);
+  }
+
+  atom::KeyWeakMap<K> key_weak_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyWeakMap);
+};
+
+}  // namespace api
+
+}  // namespace atom
+
+#endif  // ATOM_COMMON_API_ATOM_API_KEY_WEAK_MAP_H_

atom/common/api/atom_api_v8_util.cc

@@ -3,14 +3,66 @@
 // found in the LICENSE file.
 
 #include <string>
+#include <utility>
 
+#include "atom/common/api/atom_api_key_weak_map.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 "base/hash.h"
 #include "native_mate/dictionary.h"
 #include "v8/include/v8-profiler.h"
 
+// Following code should be removed after we upgraded to Chrome 50.
+#if !defined(COMPILER_MSVC)
+namespace base {
+
+template <typename T1, typename T2>
+inline size_t HashInts(T1 value1, T2 value2) {
+  // This condition is expected to be compile-time evaluated and optimised away
+  // in release builds.
+  if (sizeof(T1) > sizeof(uint32_t) || (sizeof(T2) > sizeof(uint32_t)))
+    return HashInts64(value1, value2);
+
+  return HashInts32(value1, value2);
+}
+
+}  // namespace base
+
+namespace std {
+
+// The hash function used by DoubleIDWeakMap.
+template <typename Type1, typename Type2>
+struct hash<std::pair<Type1, Type2>> {
+  std::size_t operator()(std::pair<Type1, Type2> value) const {
+    return base::HashInts<Type1, Type2>(value.first, value.second);
+  }
+};
+
+}  // namespace std
+#endif  // defined(COMPILER_MSVC)
+
+namespace mate {
+
+template<typename Type1, typename Type2>
+struct Converter<std::pair<Type1, Type2>> {
+  static bool FromV8(v8::Isolate* isolate,
+                     v8::Local<v8::Value> val,
+                     std::pair<Type1, Type2>* out) {
+    if (!val->IsArray())
+      return false;
+
+    v8::Local<v8::Array> array(v8::Local<v8::Array>::Cast(val));
+    if (array->Length() != 2)
+      return false;
+    return Converter<Type1>::FromV8(isolate, array->Get(0), &out->first) &&
+           Converter<Type2>::FromV8(isolate, array->Get(1), &out->second);
+  }
+};
+
+}  // namespace mate
+
 namespace {
 
 v8::Local<v8::Value> GetHiddenValue(v8::Isolate* isolate,
@@ -67,6 +119,9 @@ void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
   dict.SetMethod("takeHeapSnapshot", &TakeHeapSnapshot);
   dict.SetMethod("setRemoteCallbackFreer", &atom::RemoteCallbackFreer::BindTo);
   dict.SetMethod("setRemoteObjectFreer", &atom::RemoteObjectFreer::BindTo);
+  dict.SetMethod("createIDWeakMap", &atom::api::KeyWeakMap<int32_t>::Create);
+  dict.SetMethod("createDoubleIDWeakMap",
+                 &atom::api::KeyWeakMap<std::pair<int32_t, int32_t>>::Create);
 }
 
 }  // namespace

atom/common/id_weak_map.cc

@@ -1,94 +0,0 @@
-// 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/id_weak_map.h"
-
-#include <utility>
-
-#include "native_mate/converter.h"
-
-namespace atom {
-
-namespace {
-
-struct ObjectKey {
-  ObjectKey(int id, IDWeakMap* map) : id(id), map(map) {}
-  int id;
-  IDWeakMap* map;
-};
-
-void OnObjectGC(const v8::WeakCallbackInfo<ObjectKey>& data) {
-  ObjectKey* key = data.GetParameter();
-  key->map->Remove(key->id);
-  delete key;
-}
-
-}  // namespace
-
-IDWeakMap::IDWeakMap() : next_id_(0) {
-}
-
-IDWeakMap::~IDWeakMap() {
-}
-
-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;
-}
-
-v8::MaybeLocal<v8::Object> IDWeakMap::Get(v8::Isolate* isolate, int32_t id) {
-  auto iter = map_.find(id);
-  if (iter == map_.end())
-    return v8::MaybeLocal<v8::Object>();
-  else
-    return v8::Local<v8::Object>::New(isolate, *iter->second);
-}
-
-bool IDWeakMap::Has(int32_t id) const {
-  return map_.find(id) != map_.end();
-}
-
-std::vector<int32_t> IDWeakMap::Keys() const {
-  std::vector<int32_t> keys;
-  keys.reserve(map_.size());
-  for (const auto& iter : map_)
-    keys.emplace_back(iter.first);
-  return keys;
-}
-
-std::vector<v8::Local<v8::Object>> IDWeakMap::Values(v8::Isolate* isolate) {
-  std::vector<v8::Local<v8::Object>> keys;
-  keys.reserve(map_.size());
-  for (const auto& iter : map_)
-    keys.emplace_back(v8::Local<v8::Object>::New(isolate, *iter.second));
-  return keys;
-}
-
-void IDWeakMap::Remove(int32_t id) {
-  auto iter = map_.find(id);
-  if (iter == map_.end())
-    LOG(WARNING) << "Removing unexist object with ID " << id;
-  else
-    map_.erase(iter);
-}
-
-void IDWeakMap::Clear() {
-  map_.clear();
-}
-
-int32_t IDWeakMap::GetNextID() {
-  return ++next_id_;
-}
-
-}  // namespace atom

atom/common/id_weak_map.h

@@ -1,61 +0,0 @@
-// 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_ID_WEAK_MAP_H_
-#define ATOM_COMMON_ID_WEAK_MAP_H_
-
-#include <unordered_map>
-#include <vector>
-
-#include "base/memory/linked_ptr.h"
-#include "v8/include/v8.h"
-
-namespace atom {
-
-// Like ES6's WeakMap, but the key is Integer and the value is Weak Pointer.
-class IDWeakMap {
- public:
-  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);
-
-  // Gets the object from WeakMap by its |id|.
-  v8::MaybeLocal<v8::Object> Get(v8::Isolate* isolate, int32_t id);
-
-  // Whethere there is an object with |id| in this WeakMap.
-  bool Has(int32_t id) const;
-
-  // Returns IDs of all available objects.
-  std::vector<int32_t> Keys() const;
-
-  // Returns all objects.
-  std::vector<v8::Local<v8::Object>> Values(v8::Isolate* isolate);
-
-  // Remove object with |id| in the WeakMap.
-  void Remove(int32_t key);
-
-  // Clears the weak map.
-  void Clear();
-
- private:
-  // Returns next available ID.
-  int32_t GetNextID();
-
-  // ID of next stored object.
-  int32_t next_id_;
-
-  // Map of stored objects.
-  std::unordered_map<int32_t, linked_ptr<v8::Global<v8::Object>>> map_;
-
-  DISALLOW_COPY_AND_ASSIGN(IDWeakMap);
-};
-
-}  // namespace atom
-
-#endif  // ATOM_COMMON_ID_WEAK_MAP_H_

atom/common/key_weak_map.h

@@ -0,0 +1,96 @@
+// 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_KEY_WEAK_MAP_H_
+#define ATOM_COMMON_KEY_WEAK_MAP_H_
+
+#include <unordered_map>
+#include <utility>
+#include <vector>
+
+#include "base/memory/linked_ptr.h"
+#include "v8/include/v8.h"
+
+namespace atom {
+
+namespace internal {
+
+}  // namespace internal
+
+// Like ES6's WeakMap, but the key is Integer and the value is Weak Pointer.
+template<typename K>
+class KeyWeakMap {
+ public:
+  // Records the key and self, used by SetWeak.
+  struct KeyObject {
+    K key;
+    KeyWeakMap* self;
+  };
+
+  KeyWeakMap() {}
+  virtual ~KeyWeakMap() {
+    for (const auto& p : map_)
+      p.second.second->ClearWeak();
+  }
+
+  // Sets the object to WeakMap with the given |key|.
+  void Set(v8::Isolate* isolate, const K& key, v8::Local<v8::Object> object) {
+    auto value = make_linked_ptr(new v8::Global<v8::Object>(isolate, object));
+    KeyObject key_object = {key, this};
+    auto& p = map_[key] = std::make_pair(key_object, value);
+    value->SetWeak(&(p.first), OnObjectGC, v8::WeakCallbackType::kParameter);
+  }
+
+  // Gets the object from WeakMap by its |key|.
+  v8::MaybeLocal<v8::Object> Get(v8::Isolate* isolate, const K& key) {
+    auto iter = map_.find(key);
+    if (iter == map_.end())
+      return v8::MaybeLocal<v8::Object>();
+    else
+      return v8::Local<v8::Object>::New(isolate, *(iter->second.second));
+  }
+
+  // Whethere there is an object with |key| in this WeakMap.
+  bool Has(const K& key) const {
+    return map_.find(key) != map_.end();
+  }
+
+  // Returns all objects.
+  std::vector<v8::Local<v8::Object>> Values(v8::Isolate* isolate) {
+    std::vector<v8::Local<v8::Object>> keys;
+    keys.reserve(map_.size());
+    for (const auto& iter : map_) {
+      const auto& value = *(iter.second.second);
+      keys.emplace_back(v8::Local<v8::Object>::New(isolate, value));
+    }
+    return keys;
+  }
+
+  // Remove object with |key| in the WeakMap.
+  void Remove(const K& key) {
+    auto iter = map_.find(key);
+    if (iter == map_.end())
+      return;
+
+    iter->second.second->ClearWeak();
+    map_.erase(iter);
+  }
+
+ private:
+  static void OnObjectGC(
+      const v8::WeakCallbackInfo<typename KeyWeakMap<K>::KeyObject>& data) {
+    KeyWeakMap<K>::KeyObject* key_object = data.GetParameter();
+    key_object->self->Remove(key_object->key);
+  }
+
+  // Map of stored objects.
+  std::unordered_map<
+      K, std::pair<KeyObject, linked_ptr<v8::Global<v8::Object>>>> map_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyWeakMap);
+};
+
+}  // namespace atom
+
+#endif  // ATOM_COMMON_KEY_WEAK_MAP_H_

atom/common/node_bindings.cc

@@ -52,7 +52,6 @@ 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);

filenames.gypi

@@ -288,8 +288,7 @@
       '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_key_weak_map.h',
-      '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',
@@ -338,8 +337,7 @@
       'atom/common/draggable_region.cc',
       'atom/common/draggable_region.h',
       'atom/common/google_api_key.h',
-      'atom/common/id_weak_map.cc',
+      'atom/common/key_weak_map.h',
-      'atom/common/id_weak_map.h',
       'atom/common/keyboard_util.cc',
       'atom/common/keyboard_util.h',
       'atom/common/mouse_util.cc',

lib/browser/rpc-server.js

@@ -1,10 +1,10 @@
 'use strict'
 
 const electron = require('electron')
-const ipcMain = electron.ipcMain
-const objectsRegistry = require('./objects-registry')
 const v8Util = process.atomBinding('v8_util')
-const IDWeakMap = process.atomBinding('id_weak_map').IDWeakMap
+const {ipcMain} = electron
+
+const objectsRegistry = require('./objects-registry')
 
 // The internal properties of Function.
 const FUNCTION_PROPERTIES = [
@@ -13,18 +13,7 @@ const FUNCTION_PROPERTIES = [
 
 // The remote functions in renderer processes.
 // id => Function
-let rendererFunctions = new IDWeakMap()
+let rendererFunctions = v8Util.createDoubleIDWeakMap()
-
-// 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) {
@@ -179,7 +168,7 @@ var unwrapArgs = function (sender, args) {
         // 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)
+        const objectId = [webContentsId, meta.id]
 
         // Cache the callbacks in renderer.
         if (rendererFunctions.has(objectId)) {

lib/renderer/api/remote.js

@@ -1,15 +1,13 @@
 'use strict'
 
-const ipcRenderer = require('electron').ipcRenderer
-const CallbacksRegistry = require('electron').CallbacksRegistry
 const v8Util = process.atomBinding('v8_util')
-const IDWeakMap = process.atomBinding('id_weak_map').IDWeakMap
+const {ipcRenderer, CallbacksRegistry} = require('electron')
 
 const callbacksRegistry = new CallbacksRegistry()
 
 var includes = [].includes
 
-var remoteObjectCache = new IDWeakMap()
+var remoteObjectCache = v8Util.createIDWeakMap()
 
 // Check for circular reference.
 var isCircular = function (field, visited) {