Merge pull request #6674 from electron/session-no-gc

Do not garbage collect sessions

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<void(v8::Local<v8::Value>)>;
 WrapDownloadItemCallback g_wrap_download_item;
 
-std::map<uint32_t, linked_ptr<v8::Global<v8::Value>>> g_download_item_objects;
+std::map<uint32_t, v8::Global<v8::Object>> 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> 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<v8::Value>(isolate, handle.ToV8()));
+  g_download_item_objects[handle->weak_map_id()] =
+      v8::Global<v8::Object>(isolate, handle.ToV8());
   return handle;
 }
 

atom/browser/api/atom_api_session.cc

@@ -4,6 +4,7 @@
 
 #include "atom/browser/api/atom_api_session.h"
 
+#include <map>
 #include <string>
 #include <vector>
 
@@ -173,6 +174,9 @@ const char kPersistPrefix[] = "persist:";
 using WrapSessionCallback = base::Callback<void(v8::Local<v8::Value>)>;
 WrapSessionCallback g_wrap_session;
 
+// Referenced session objects.
+std::map<uint32_t, v8::Global<v8::Object>> 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> 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<v8::Object>(isolate, handle.ToV8());
+
   return handle;
 }
 

atom/common/key_weak_map.h

@@ -9,7 +9,7 @@
 #include <utility>
 #include <vector>
 
-#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<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);
+    auto& p = map_[key] =
+        std::make_pair(key_object, v8::Global<v8::Object>(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<v8::Object>();
     else
-      return v8::Local<v8::Object>::New(isolate, *(iter->second.second));
+      return v8::Local<v8::Object>::New(isolate, iter->second.second);
   }
 
   // Whethere there is an object with |key| in this WeakMap.
@@ -56,10 +56,8 @@ class KeyWeakMap {
   std::vector<v8::Local<v8::Object>> Values(v8::Isolate* isolate) const {
     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));
-    }
+    for (const auto& it : map_)
+      keys.emplace_back(v8::Local<v8::Object>::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<KeyObject, linked_ptr<v8::Global<v8::Object>>>> map_;
+      K, std::pair<KeyObject, v8::Global<v8::Object>>> map_;
 
   DISALLOW_COPY_AND_ASSIGN(KeyWeakMap);
 };

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,