From d23d9dcf014d35b2a00bb59dbde38f1f6ab38525 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 1 Aug 2016 19:35:51 +0900 Subject: [PATCH 1/3] Do not garbage collect sessions --- lib/browser/api/session.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index 86a7902ef5..ab2d277859 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -2,7 +2,7 @@ const {EventEmitter} = require('events') const {app} = require('electron') const {fromPartition, _setWrapSession} = process.atomBinding('session') -// Returns the default session. +// Public API. Object.defineProperties(exports, { defaultSession: { enumerable: true, @@ -14,9 +14,15 @@ Object.defineProperties(exports, { } }) +const sessions = [] + // Wraps native Session class. _setWrapSession(function (session) { // Session is an EventEmitter. Object.setPrototypeOf(session, EventEmitter.prototype) app.emit('session-created', session) + + // The Sessions should never be garbage collected, since the common pattern is + // to use partition strings, instead of using the Session object directly. + sessions.push(session) }) From ffed3e9c0c17ec109d5d8e34b7554a10a57fee4f Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 1 Aug 2016 20:11:17 +0900 Subject: [PATCH 2/3] Move the code to native --- atom/browser/api/atom_api_session.cc | 11 +++++++++++ lib/browser/api/session.js | 6 ------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 02fafddf94..9117ff58a9 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -4,6 +4,7 @@ #include "atom/browser/api/atom_api_session.h" +#include #include #include @@ -173,6 +174,9 @@ const char kPersistPrefix[] = "persist:"; using WrapSessionCallback = base::Callback)>; WrapSessionCallback g_wrap_session; +// Referenced session objects. +std::map> g_sessions; + class ResolveProxyHelper { public: ResolveProxyHelper(AtomBrowserContext* browser_context, @@ -346,6 +350,7 @@ Session::Session(v8::Isolate* isolate, AtomBrowserContext* browser_context) Session::~Session() { content::BrowserContext::GetDownloadManager(browser_context())-> RemoveObserver(this); + g_sessions.erase(weak_map_id()); } void Session::OnDownloadCreated(content::DownloadManager* manager, @@ -537,6 +542,12 @@ mate::Handle Session::CreateFrom( auto handle = mate::CreateHandle( isolate, new Session(isolate, browser_context)); g_wrap_session.Run(handle.ToV8()); + + // The Sessions should never be garbage collected, since the common pattern is + // to use partition strings, instead of using the Session object directly. + g_sessions[handle->weak_map_id()] = + v8::Global(isolate, handle.ToV8()); + return handle; } diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index ab2d277859..d82e6d0154 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -14,15 +14,9 @@ Object.defineProperties(exports, { } }) -const sessions = [] - // Wraps native Session class. _setWrapSession(function (session) { // Session is an EventEmitter. Object.setPrototypeOf(session, EventEmitter.prototype) app.emit('session-created', session) - - // The Sessions should never be garbage collected, since the common pattern is - // to use partition strings, instead of using the Session object directly. - sessions.push(session) }) From e1152ae96c1ac55fa36b1178ffec2564b0ee67e3 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 1 Aug 2016 20:26:06 +0900 Subject: [PATCH 3/3] Remove usages of linked_ptr It is no longer needed since we now have move semantic. --- atom/browser/api/atom_api_download_item.cc | 11 ++++------ atom/common/key_weak_map.h | 24 ++++++++++------------ 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index 81107abcea..79ea3fa5fd 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -11,7 +11,6 @@ #include "atom/common/native_mate_converters/file_path_converter.h" #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/node_includes.h" -#include "base/memory/linked_ptr.h" #include "base/message_loop/message_loop.h" #include "base/strings/utf_string_conversions.h" #include "native_mate/dictionary.h" @@ -56,7 +55,7 @@ namespace { using WrapDownloadItemCallback = base::Callback)>; WrapDownloadItemCallback g_wrap_download_item; -std::map>> g_download_item_objects; +std::map> g_download_item_objects; } // namespace @@ -76,9 +75,7 @@ DownloadItem::~DownloadItem() { } // Remove from the global map. - auto iter = g_download_item_objects.find(weak_map_id()); - if (iter != g_download_item_objects.end()) - g_download_item_objects.erase(iter); + g_download_item_objects.erase(weak_map_id()); } void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) { @@ -202,8 +199,8 @@ mate::Handle DownloadItem::Create( g_wrap_download_item.Run(handle.ToV8()); // Reference this object in case it got garbage collected. - g_download_item_objects[handle->weak_map_id()] = make_linked_ptr( - new v8::Global(isolate, handle.ToV8())); + g_download_item_objects[handle->weak_map_id()] = + v8::Global(isolate, handle.ToV8()); return handle; } diff --git a/atom/common/key_weak_map.h b/atom/common/key_weak_map.h index 009ba099c9..8e879d656e 100644 --- a/atom/common/key_weak_map.h +++ b/atom/common/key_weak_map.h @@ -9,7 +9,7 @@ #include #include -#include "base/memory/linked_ptr.h" +#include "base/macros.h" #include "v8/include/v8.h" namespace atom { @@ -26,16 +26,16 @@ class KeyWeakMap { KeyWeakMap() {} virtual ~KeyWeakMap() { - for (const auto& p : map_) - p.second.second->ClearWeak(); + for (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 object) { - auto value = make_linked_ptr(new v8::Global(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); + auto& p = map_[key] = + std::make_pair(key_object, v8::Global(isolate, object)); + p.second.SetWeak(&(p.first), OnObjectGC, v8::WeakCallbackType::kParameter); } // Gets the object from WeakMap by its |key|. @@ -44,7 +44,7 @@ class KeyWeakMap { if (iter == map_.end()) return v8::MaybeLocal(); else - return v8::Local::New(isolate, *(iter->second.second)); + return v8::Local::New(isolate, iter->second.second); } // Whethere there is an object with |key| in this WeakMap. @@ -56,10 +56,8 @@ class KeyWeakMap { std::vector> Values(v8::Isolate* isolate) const { std::vector> keys; keys.reserve(map_.size()); - for (const auto& iter : map_) { - const auto& value = *(iter.second.second); - keys.emplace_back(v8::Local::New(isolate, value)); - } + for (const auto& it : map_) + keys.emplace_back(v8::Local::New(isolate, it.second.second)); return keys; } @@ -69,7 +67,7 @@ class KeyWeakMap { if (iter == map_.end()) return; - iter->second.second->ClearWeak(); + iter->second.second.ClearWeak(); map_.erase(iter); } @@ -82,7 +80,7 @@ class KeyWeakMap { // Map of stored objects. std::unordered_map< - K, std::pair>>> map_; + K, std::pair>> map_; DISALLOW_COPY_AND_ASSIGN(KeyWeakMap); };