Cleanup destruction of URLRequestContextGetter (#12305)

- Add Leak detector
 - Indicate shutdown of request context from Browser Context
 - Change stored references to URLRequestContextGetter to use BrowserContext
 - Destroy session properties explicitly

atom/browser/api/atom_api_cookies.cc

@@ -238,11 +238,11 @@ void SetCookieOnIO(scoped_refptr<net::URLRequestContextGetter> getter,
 }  // namespace
 
 Cookies::Cookies(v8::Isolate* isolate, AtomBrowserContext* browser_context)
-    : browser_context_(browser_context),
+    : browser_context_(browser_context) {
-      request_context_getter_(browser_context->url_request_context_getter()) {
   Init(isolate);
-  cookie_change_subscription_ = browser_context->RegisterCookieChangeCallback(
+  auto subscription = browser_context->RegisterCookieChangeCallback(
       base::Bind(&Cookies::OnCookieChanged, base::Unretained(this)));
+  browser_context->set_cookie_change_subscription(std::move(subscription));
 }
 
 Cookies::~Cookies() {}
@@ -250,34 +250,38 @@ Cookies::~Cookies() {}
 void Cookies::Get(const base::DictionaryValue& filter,
                   const GetCallback& callback) {
   std::unique_ptr<base::DictionaryValue> copied(filter.CreateDeepCopy());
-  auto getter = WrapRefCounted(request_context_getter_);
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(GetCookiesOnIO, getter, Passed(&copied), callback));
+      base::BindOnce(GetCookiesOnIO, base::RetainedRef(getter), Passed(&copied),
+                     callback));
 }
 
 void Cookies::Remove(const GURL& url, const std::string& name,
                      const base::Closure& callback) {
-  auto getter = WrapRefCounted(request_context_getter_);
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(RemoveCookieOnIOThread, getter, url, name, callback));
+      base::BindOnce(RemoveCookieOnIOThread, base::RetainedRef(getter), url,
+                     name, callback));
 }
 
 void Cookies::Set(const base::DictionaryValue& details,
                   const SetCallback& callback) {
   std::unique_ptr<base::DictionaryValue> copied(details.CreateDeepCopy());
-  auto getter = WrapRefCounted(request_context_getter_);
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(SetCookieOnIO, getter, Passed(&copied), callback));
+      base::BindOnce(SetCookieOnIO, base::RetainedRef(getter), Passed(&copied),
+                     callback));
 }
 
 void Cookies::FlushStore(const base::Closure& callback) {
-  auto getter = WrapRefCounted(request_context_getter_);
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(FlushCookieStoreOnIOThread, getter, callback));
+      base::BindOnce(FlushCookieStoreOnIOThread, base::RetainedRef(getter),
+                     callback));
 }
 
 void Cookies::OnCookieChanged(const CookieDetails* details) {

atom/browser/api/atom_api_cookies.h

@@ -58,12 +58,7 @@ class Cookies : public mate::TrackableObject<Cookies> {
   void OnCookieChanged(const CookieDetails*);
 
  private:
-  // Store a reference to ensure this class gets destroyed before the context.
   scoped_refptr<AtomBrowserContext> browser_context_;
-  std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
-      cookie_change_subscription_;
-
-  net::URLRequestContextGetter* request_context_getter_;
 
   DISALLOW_COPY_AND_ASSIGN(Cookies);
 };

atom/browser/api/atom_api_protocol.cc

@@ -33,14 +33,6 @@ namespace {
 // List of registered custom standard schemes.
 std::vector<std::string> g_standard_schemes;
 
-// Clear protocol handlers in IO thread.
-void ClearJobFactoryInIO(
-    scoped_refptr<brightray::URLRequestContextGetter> request_context_getter) {
-  auto job_factory = static_cast<AtomURLRequestJobFactory*>(
-      request_context_getter->job_factory());
-  job_factory->Clear();
-}
-
 }  // namespace
 
 std::vector<std::string> GetStandardSchemes() {
@@ -76,15 +68,11 @@ void RegisterStandardSchemes(const std::vector<std::string>& schemes,
 }
 
 Protocol::Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context)
-    : request_context_getter_(browser_context->GetRequestContext()),
+    : browser_context_(browser_context), weak_factory_(this) {
-      weak_factory_(this) {
   Init(isolate);
 }
 
 Protocol::~Protocol() {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::Bind(ClearJobFactoryInIO, request_context_getter_));
 }
 
 void Protocol::RegisterServiceWorkerSchemes(
@@ -96,12 +84,12 @@ void Protocol::UnregisterProtocol(
     const std::string& scheme, mate::Arguments* args) {
   CompletionCallback callback;
   args->GetNext(&callback);
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTaskAndReplyWithResult(
       content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&Protocol::UnregisterProtocolInIO,
+      base::BindOnce(&Protocol::UnregisterProtocolInIO,
-                 request_context_getter_, scheme),
+                     base::RetainedRef(getter), scheme),
-      base::Bind(&Protocol::OnIOCompleted,
+      base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback));
-                 GetWeakPtr(), callback));
 }
 
 // static
@@ -118,10 +106,11 @@ Protocol::ProtocolError Protocol::UnregisterProtocolInIO(
 
 void Protocol::IsProtocolHandled(const std::string& scheme,
                                  const BooleanCallback& callback) {
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTaskAndReplyWithResult(
       content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&Protocol::IsProtocolHandledInIO,
+      base::Bind(&Protocol::IsProtocolHandledInIO, base::RetainedRef(getter),
-                 request_context_getter_, scheme),
+                 scheme),
       callback);
 }
 
@@ -136,12 +125,12 @@ void Protocol::UninterceptProtocol(
     const std::string& scheme, mate::Arguments* args) {
   CompletionCallback callback;
   args->GetNext(&callback);
+  auto getter = browser_context_->GetRequestContext();
   content::BrowserThread::PostTaskAndReplyWithResult(
       content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&Protocol::UninterceptProtocolInIO,
+      base::BindOnce(&Protocol::UninterceptProtocolInIO,
-                 request_context_getter_, scheme),
+                     base::RetainedRef(getter), scheme),
-      base::Bind(&Protocol::OnIOCompleted,
+      base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback));
-                 GetWeakPtr(), callback));
 }
 
 // static
@@ -181,13 +170,6 @@ std::string Protocol::ErrorCodeToString(ProtocolError error) {
   }
 }
 
-AtomURLRequestJobFactory* Protocol::GetJobFactoryInIO() const {
-  request_context_getter_->GetURLRequestContext();  // Force init.
-  return static_cast<AtomURLRequestJobFactory*>(
-      static_cast<brightray::URLRequestContextGetter*>(
-          request_context_getter_.get())->job_factory());
-}
-
 // static
 mate::Handle<Protocol> Protocol::Create(
     v8::Isolate* isolate, AtomBrowserContext* browser_context) {

atom/browser/api/atom_api_protocol.h

@@ -101,12 +101,12 @@ class Protocol : public mate::TrackableObject<Protocol> {
                         mate::Arguments* args) {
     CompletionCallback callback;
     args->GetNext(&callback);
+    auto getter = browser_context_->GetRequestContext();
     content::BrowserThread::PostTaskAndReplyWithResult(
         content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&Protocol::RegisterProtocolInIO<RequestJob>,
+        base::BindOnce(&Protocol::RegisterProtocolInIO<RequestJob>,
-                   request_context_getter_, isolate(), scheme, handler),
+                       base::RetainedRef(getter), isolate(), scheme, handler),
-        base::Bind(&Protocol::OnIOCompleted,
+        base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback));
-                   GetWeakPtr(), callback));
   }
   template<typename RequestJob>
   static ProtocolError RegisterProtocolInIO(
@@ -147,12 +147,12 @@ class Protocol : public mate::TrackableObject<Protocol> {
                          mate::Arguments* args) {
     CompletionCallback callback;
     args->GetNext(&callback);
+    auto getter = browser_context_->GetRequestContext();
     content::BrowserThread::PostTaskAndReplyWithResult(
         content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&Protocol::InterceptProtocolInIO<RequestJob>,
+        base::BindOnce(&Protocol::InterceptProtocolInIO<RequestJob>,
-                   request_context_getter_, isolate(), scheme, handler),
+                       base::RetainedRef(getter), isolate(), scheme, handler),
-        base::Bind(&Protocol::OnIOCompleted,
+        base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback));
-                   GetWeakPtr(), callback));
   }
   template<typename RequestJob>
   static ProtocolError InterceptProtocolInIO(
@@ -187,13 +187,11 @@ class Protocol : public mate::TrackableObject<Protocol> {
   // Convert error code to string.
   std::string ErrorCodeToString(ProtocolError error);
 
-  AtomURLRequestJobFactory* GetJobFactoryInIO() const;
-
   base::WeakPtr<Protocol> GetWeakPtr() {
     return weak_factory_.GetWeakPtr();
   }
 
-  scoped_refptr<brightray::URLRequestContextGetter> request_context_getter_;
+  scoped_refptr<AtomBrowserContext> browser_context_;
   base::WeakPtrFactory<Protocol> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Protocol);

atom/browser/api/atom_api_session.cc

@@ -342,7 +342,7 @@ void DoCacheActionInIO(
     on_get_backend.Run(net::OK);
 }
 
-void SetProxyInIO(net::URLRequestContextGetter* getter,
+void SetProxyInIO(scoped_refptr<net::URLRequestContextGetter> getter,
                   const net::ProxyConfig& config,
                   const base::Closure& callback) {
   auto proxy_service = getter->GetURLRequestContext()->proxy_service();
@@ -452,6 +452,32 @@ void SetDevToolsNetworkEmulationClientIdInIO(
   network_delegate->SetDevToolsNetworkEmulationClientId(client_id);
 }
 
+// Clear protocol handlers in IO thread.
+void ClearJobFactoryInIO(
+    scoped_refptr<brightray::URLRequestContextGetter> request_context_getter) {
+  auto job_factory = static_cast<AtomURLRequestJobFactory*>(
+      request_context_getter->job_factory());
+  if (job_factory)
+    job_factory->Clear();
+}
+
+void DestroyGlobalHandle(v8::Isolate* isolate,
+                         const v8::Global<v8::Value>& global_handle) {
+  v8::Locker locker(isolate);
+  v8::HandleScope handle_scope(isolate);
+  if (!global_handle.IsEmpty()) {
+    v8::Local<v8::Value> local_handle = global_handle.Get(isolate);
+    if (local_handle->IsObject()) {
+      v8::Local<v8::Object> object = local_handle->ToObject();
+      void* ptr = object->GetAlignedPointerFromInternalField(0);
+      if (!ptr)
+        return;
+      delete static_cast<mate::WrappableBase*>(ptr);
+      object->SetAlignedPointerInInternalField(0, nullptr);
+    }
+  }
+}
+
 }  // namespace
 
 Session::Session(v8::Isolate* isolate, AtomBrowserContext* browser_context)
@@ -468,8 +494,15 @@ Session::Session(v8::Isolate* isolate, AtomBrowserContext* browser_context)
 }
 
 Session::~Session() {
+  auto getter = browser_context_->GetRequestContext();
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(ClearJobFactoryInIO, base::RetainedRef(getter)));
   content::BrowserContext::GetDownloadManager(browser_context())->
       RemoveObserver(this);
+  DestroyGlobalHandle(isolate(), cookies_);
+  DestroyGlobalHandle(isolate(), web_request_);
+  DestroyGlobalHandle(isolate(), protocol_);
   g_sessions.erase(weak_map_id());
 }
 
@@ -533,8 +566,10 @@ void Session::FlushStorageData() {
 void Session::SetProxy(const net::ProxyConfig& config,
                        const base::Closure& callback) {
   auto getter = browser_context_->GetRequestContext();
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+  BrowserThread::PostTask(
-      base::Bind(&SetProxyInIO, base::Unretained(getter), config, callback));
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&SetProxyInIO, base::RetainedRef(getter), config,
+                     callback));
 }
 
 void Session::SetDownloadPath(const base::FilePath& path) {

atom/browser/api/trackable_object.cc

@@ -31,15 +31,16 @@ class IDUserData : public base::SupportsUserData::Data {
 
 TrackableObjectBase::TrackableObjectBase()
     : weak_map_id_(0), weak_factory_(this) {
-  cleanup_ = RegisterDestructionCallback(GetDestroyClosure());
+  atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback(
+      GetDestroyClosure());
 }
 
 TrackableObjectBase::~TrackableObjectBase() {
-  cleanup_.Run();
 }
 
-base::Closure TrackableObjectBase::GetDestroyClosure() {
+base::OnceClosure TrackableObjectBase::GetDestroyClosure() {
-  return base::Bind(&TrackableObjectBase::Destroy, weak_factory_.GetWeakPtr());
+  return base::BindOnce(&TrackableObjectBase::Destroy,
+                        weak_factory_.GetWeakPtr());
 }
 
 void TrackableObjectBase::Destroy() {
@@ -62,10 +63,4 @@ int32_t TrackableObjectBase::GetIDFromWrappedClass(
   return 0;
 }
 
-// static
-base::Closure TrackableObjectBase::RegisterDestructionCallback(
-    const base::Closure& c) {
-  return atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback(c);
-}
-
 }  // namespace mate

atom/browser/api/trackable_object.h

@@ -37,18 +37,13 @@ class TrackableObjectBase {
   virtual ~TrackableObjectBase();
 
   // Returns a closure that can destroy the native class.
-  base::Closure GetDestroyClosure();
+  base::OnceClosure GetDestroyClosure();
-
-  // Register a callback that should be destroyed before JavaScript environment
-  // gets destroyed.
-  static base::Closure RegisterDestructionCallback(const base::Closure& c);
 
   int32_t weak_map_id_;
 
  private:
   void Destroy();
 
-  base::Closure cleanup_;
   base::WeakPtrFactory<TrackableObjectBase> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TrackableObjectBase);

atom/browser/atom_browser_context.h

@@ -59,6 +59,13 @@ class AtomBrowserContext : public brightray::BrowserContext {
 
   AtomBlobReader* GetBlobReader();
 
+  void set_cookie_change_subscription(
+      std::unique_ptr<
+          base::CallbackList<void(const CookieDetails*)>::Subscription>
+          subscription) {
+    cookie_change_subscription_.swap(subscription);
+  }
+
  protected:
   AtomBrowserContext(const std::string& partition, bool in_memory,
                      const base::DictionaryValue& options);
@@ -73,6 +80,8 @@ class AtomBrowserContext : public brightray::BrowserContext {
   bool use_cache_;
 
   base::CallbackList<void(const CookieDetails*)> cookie_change_sub_list_;
+  std::unique_ptr<base::CallbackList<void(const CookieDetails*)>::Subscription>
+      cookie_change_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(AtomBrowserContext);
 };

atom/browser/atom_browser_main_parts.cc

@@ -110,10 +110,9 @@ int AtomBrowserMainParts::GetExitCode() {
   return exit_code_ != nullptr ? *exit_code_ : 0;
 }
 
-base::Closure AtomBrowserMainParts::RegisterDestructionCallback(
+void AtomBrowserMainParts::RegisterDestructionCallback(
-    const base::Closure& callback) {
+    base::OnceClosure callback) {
-  auto iter = destructors_.insert(destructors_.end(), callback);
+  destructors_.insert(destructors_.end(), std::move(callback));
-  return base::Bind(&Erase<std::list<base::Closure>>, &destructors_, iter);
 }
 
 void AtomBrowserMainParts::PreEarlyInitialization() {
@@ -242,9 +241,10 @@ void AtomBrowserMainParts::PostMainMessageLoopRun() {
   // We don't use ranged for loop because iterators are getting invalided when
   // the callback runs.
   for (auto iter = destructors_.begin(); iter != destructors_.end();) {
-    base::Closure& callback = *iter;
+    base::OnceClosure callback = std::move(*iter);
+    if (!callback.is_null())
+      std::move(callback).Run();
     ++iter;
-    callback.Run();
   }
 }
 

atom/browser/atom_browser_main_parts.h

@@ -41,7 +41,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts {
   // Register a callback that should be destroyed before JavaScript environment
   // gets destroyed.
   // Returns a closure that can be used to remove |callback| from the list.
-  base::Closure RegisterDestructionCallback(const base::Closure& callback);
+  void RegisterDestructionCallback(base::OnceClosure callback);
 
   Browser* browser() { return browser_.get(); }
 
@@ -89,7 +89,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts {
   base::Timer gc_timer_;
 
   // List of callbacks should be executed before destroying JS env.
-  std::list<base::Closure> destructors_;
+  std::list<base::OnceClosure> destructors_;
 
   static AtomBrowserMainParts* self_;
 

atom/browser/net/atom_url_request_job_factory.cc

@@ -92,6 +92,7 @@ void AtomURLRequestJobFactory::Clear() {
   for (auto& it : protocol_handler_map_)
     delete it.second;
   protocol_handler_map_.clear();
+  original_protocols_.clear();
 }
 
 net::URLRequestJob* AtomURLRequestJobFactory::MaybeCreateJobWithProtocolHandler(

brightray/browser/browser_context.cc

@@ -92,11 +92,17 @@ BrowserContext::BrowserContext(const std::string& partition, bool in_memory)
 }
 
 BrowserContext::~BrowserContext() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   NotifyWillBeDestroyed(this);
   ShutdownStoragePartitions();
-  BrowserThread::DeleteSoon(BrowserThread::IO,
+  if (BrowserThread::IsMessageLoopValid(BrowserThread::IO)) {
-                            FROM_HERE,
+    BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE,
-                            resource_context_.release());
+                              resource_context_.release());
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&URLRequestContextGetter::NotifyContextShutdownOnIO,
+                       base::RetainedRef(url_request_getter_)));
+  }
 }
 
 void BrowserContext::InitPrefs() {

brightray/browser/url_request_context_getter.cc

@@ -130,7 +130,8 @@ URLRequestContextGetter::URLRequestContextGetter(
       in_memory_(in_memory),
       io_task_runner_(io_task_runner),
       protocol_interceptors_(std::move(protocol_interceptors)),
-      job_factory_(nullptr) {
+      job_factory_(nullptr),
+      context_shutting_down_(false) {
   // Must first be created on the UI thread.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -150,12 +151,24 @@ URLRequestContextGetter::URLRequestContextGetter(
 URLRequestContextGetter::~URLRequestContextGetter() {
 }
 
+void URLRequestContextGetter::NotifyContextShutdownOnIO() {
+  context_shutting_down_ = true;
+  cookie_change_sub_.reset();
+  http_network_session_.reset();
+  http_auth_preferences_.reset();
+  host_mapping_rules_.reset();
+  url_request_context_.reset();
+  storage_.reset();
+  ct_delegate_.reset();
+  net::URLRequestContextGetter::NotifyContextShuttingDown();
+}
+
 void URLRequestContextGetter::OnCookieChanged(
     const net::CanonicalCookie& cookie,
     net::CookieStore::ChangeCause cause) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  if (!delegate_)
+  if (!delegate_ || context_shutting_down_)
     return;
 
   content::BrowserThread::PostTask(
@@ -172,6 +185,9 @@ net::HostResolver* URLRequestContextGetter::host_resolver() {
 net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  if (context_shutting_down_)
+    return nullptr;
+
   if (!url_request_context_.get()) {
     ct_delegate_.reset(new RequireCTDelegate);
     auto& command_line = *base::CommandLine::ForCurrentProcess();
@@ -342,14 +358,14 @@ net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {
     // Set up interceptors in the reverse order.
     std::unique_ptr<net::URLRequestJobFactory> top_job_factory =
         std::move(job_factory);
-    content::URLRequestInterceptorScopedVector::reverse_iterator it;
+    if (!protocol_interceptors_.empty()) {
-    for (it = protocol_interceptors_.rbegin();
+      for (auto it = protocol_interceptors_.rbegin();
-         it != protocol_interceptors_.rend();
+           it != protocol_interceptors_.rend(); ++it) {
-         ++it) {
+        top_job_factory.reset(new net::URLRequestInterceptingJobFactory(
-      top_job_factory.reset(new net::URLRequestInterceptingJobFactory(
+            std::move(top_job_factory), std::move(*it)));
-          std::move(top_job_factory), std::move(*it)));
+      }
+      protocol_interceptors_.clear();
     }
-    protocol_interceptors_.clear();
 
     storage_->set_job_factory(std::move(top_job_factory));
   }

brightray/browser/url_request_context_getter.h

@@ -18,6 +18,10 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
+#if DCHECK_IS_ON()
+#include "base/debug/leak_tracker.h"
+#endif
+
 namespace base {
 class MessageLoop;
 }
@@ -83,6 +87,8 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
   net::HostResolver* host_resolver();
   net::URLRequestJobFactory* job_factory() const { return job_factory_; }
 
+  void NotifyContextShutdownOnIO();
+
  private:
   Delegate* delegate_;
 
@@ -93,6 +99,10 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
 
   std::string user_agent_;
 
+#if DCHECK_IS_ON()
+  base::debug::LeakTracker<URLRequestContextGetter> leak_tracker_;
+#endif
+
   std::unique_ptr<RequireCTDelegate> ct_delegate_;
   std::unique_ptr<net::ProxyConfigService> proxy_config_service_;
   std::unique_ptr<net::URLRequestContextStorage> storage_;
@@ -107,6 +117,8 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
 
   net::URLRequestJobFactory* job_factory_;  // weak ref
 
+  bool context_shutting_down_;
+
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextGetter);
 };