diff --git a/atom/browser/api/atom_api_cookies.cc b/atom/browser/api/atom_api_cookies.cc index c83c10a14623..3b33ec00a7ca 100644 --- a/atom/browser/api/atom_api_cookies.cc +++ b/atom/browser/api/atom_api_cookies.cc @@ -238,11 +238,11 @@ void SetCookieOnIO(scoped_refptr getter, } // namespace Cookies::Cookies(v8::Isolate* isolate, AtomBrowserContext* browser_context) - : browser_context_(browser_context), - request_context_getter_(browser_context->url_request_context_getter()) { + : browser_context_(browser_context) { 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 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 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) { diff --git a/atom/browser/api/atom_api_cookies.h b/atom/browser/api/atom_api_cookies.h index b1fb9e0aaf74..d8a32d8fd4e8 100644 --- a/atom/browser/api/atom_api_cookies.h +++ b/atom/browser/api/atom_api_cookies.h @@ -58,12 +58,7 @@ class Cookies : public mate::TrackableObject { void OnCookieChanged(const CookieDetails*); private: - // Store a reference to ensure this class gets destroyed before the context. scoped_refptr browser_context_; - std::unique_ptr::Subscription> - cookie_change_subscription_; - - net::URLRequestContextGetter* request_context_getter_; DISALLOW_COPY_AND_ASSIGN(Cookies); }; diff --git a/atom/browser/api/atom_api_protocol.cc b/atom/browser/api/atom_api_protocol.cc index b904c88bfcc6..292bfef9bf9b 100644 --- a/atom/browser/api/atom_api_protocol.cc +++ b/atom/browser/api/atom_api_protocol.cc @@ -33,14 +33,6 @@ namespace { // List of registered custom standard schemes. std::vector g_standard_schemes; -// Clear protocol handlers in IO thread. -void ClearJobFactoryInIO( - scoped_refptr request_context_getter) { - auto job_factory = static_cast( - request_context_getter->job_factory()); - job_factory->Clear(); -} - } // namespace std::vector GetStandardSchemes() { @@ -76,15 +68,11 @@ void RegisterStandardSchemes(const std::vector& schemes, } Protocol::Protocol(v8::Isolate* isolate, AtomBrowserContext* browser_context) - : request_context_getter_(browser_context->GetRequestContext()), - weak_factory_(this) { + : browser_context_(browser_context), 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, - request_context_getter_, scheme), - base::Bind(&Protocol::OnIOCompleted, - GetWeakPtr(), callback)); + base::BindOnce(&Protocol::UnregisterProtocolInIO, + base::RetainedRef(getter), scheme), + base::BindOnce(&Protocol::OnIOCompleted, 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, - request_context_getter_, scheme), + base::Bind(&Protocol::IsProtocolHandledInIO, base::RetainedRef(getter), + 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, - request_context_getter_, scheme), - base::Bind(&Protocol::OnIOCompleted, - GetWeakPtr(), callback)); + base::BindOnce(&Protocol::UninterceptProtocolInIO, + base::RetainedRef(getter), scheme), + base::BindOnce(&Protocol::OnIOCompleted, 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( - static_cast( - request_context_getter_.get())->job_factory()); -} - // static mate::Handle Protocol::Create( v8::Isolate* isolate, AtomBrowserContext* browser_context) { diff --git a/atom/browser/api/atom_api_protocol.h b/atom/browser/api/atom_api_protocol.h index dfc32be6bc57..52e30966db97 100644 --- a/atom/browser/api/atom_api_protocol.h +++ b/atom/browser/api/atom_api_protocol.h @@ -101,12 +101,12 @@ class Protocol : public mate::TrackableObject { 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, - request_context_getter_, isolate(), scheme, handler), - base::Bind(&Protocol::OnIOCompleted, - GetWeakPtr(), callback)); + base::BindOnce(&Protocol::RegisterProtocolInIO, + base::RetainedRef(getter), isolate(), scheme, handler), + base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback)); } template static ProtocolError RegisterProtocolInIO( @@ -147,12 +147,12 @@ class Protocol : public mate::TrackableObject { 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, - request_context_getter_, isolate(), scheme, handler), - base::Bind(&Protocol::OnIOCompleted, - GetWeakPtr(), callback)); + base::BindOnce(&Protocol::InterceptProtocolInIO, + base::RetainedRef(getter), isolate(), scheme, handler), + base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback)); } template static ProtocolError InterceptProtocolInIO( @@ -187,13 +187,11 @@ class Protocol : public mate::TrackableObject { // Convert error code to string. std::string ErrorCodeToString(ProtocolError error); - AtomURLRequestJobFactory* GetJobFactoryInIO() const; - base::WeakPtr GetWeakPtr() { return weak_factory_.GetWeakPtr(); } - scoped_refptr request_context_getter_; + scoped_refptr browser_context_; base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(Protocol); diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index f55ecc44403c..4233c6b1f18c 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/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 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 request_context_getter) { + auto job_factory = static_cast( + request_context_getter->job_factory()); + if (job_factory) + job_factory->Clear(); +} + +void DestroyGlobalHandle(v8::Isolate* isolate, + const v8::Global& global_handle) { + v8::Locker locker(isolate); + v8::HandleScope handle_scope(isolate); + if (!global_handle.IsEmpty()) { + v8::Local local_handle = global_handle.Get(isolate); + if (local_handle->IsObject()) { + v8::Local object = local_handle->ToObject(); + void* ptr = object->GetAlignedPointerFromInternalField(0); + if (!ptr) + return; + delete static_cast(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, - base::Bind(&SetProxyInIO, base::Unretained(getter), config, callback)); + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::BindOnce(&SetProxyInIO, base::RetainedRef(getter), config, + callback)); } void Session::SetDownloadPath(const base::FilePath& path) { diff --git a/atom/browser/api/trackable_object.cc b/atom/browser/api/trackable_object.cc index fe78be40ec1c..d305657ba7b6 100644 --- a/atom/browser/api/trackable_object.cc +++ b/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() { - return base::Bind(&TrackableObjectBase::Destroy, weak_factory_.GetWeakPtr()); +base::OnceClosure TrackableObjectBase::GetDestroyClosure() { + 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 diff --git a/atom/browser/api/trackable_object.h b/atom/browser/api/trackable_object.h index 6fcde898a672..7decf3b077d9 100644 --- a/atom/browser/api/trackable_object.h +++ b/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(); - - // Register a callback that should be destroyed before JavaScript environment - // gets destroyed. - static base::Closure RegisterDestructionCallback(const base::Closure& c); + base::OnceClosure GetDestroyClosure(); int32_t weak_map_id_; private: void Destroy(); - base::Closure cleanup_; base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(TrackableObjectBase); diff --git a/atom/browser/atom_browser_context.h b/atom/browser/atom_browser_context.h index c892f2b6a22a..8ac8d167f137 100644 --- a/atom/browser/atom_browser_context.h +++ b/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::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 cookie_change_sub_list_; + std::unique_ptr::Subscription> + cookie_change_subscription_; DISALLOW_COPY_AND_ASSIGN(AtomBrowserContext); }; diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index df4ae834db09..a0bdcb1da854 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/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( - const base::Closure& callback) { - auto iter = destructors_.insert(destructors_.end(), callback); - return base::Bind(&Erase>, &destructors_, iter); +void AtomBrowserMainParts::RegisterDestructionCallback( + base::OnceClosure callback) { + destructors_.insert(destructors_.end(), std::move(callback)); } 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(); } } diff --git a/atom/browser/atom_browser_main_parts.h b/atom/browser/atom_browser_main_parts.h index a4c3bbed5619..11b375428065 100644 --- a/atom/browser/atom_browser_main_parts.h +++ b/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 destructors_; + std::list destructors_; static AtomBrowserMainParts* self_; diff --git a/atom/browser/net/atom_url_request_job_factory.cc b/atom/browser/net/atom_url_request_job_factory.cc index 20680adf7da8..60676d3d8af3 100644 --- a/atom/browser/net/atom_url_request_job_factory.cc +++ b/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( diff --git a/brightray/browser/browser_context.cc b/brightray/browser/browser_context.cc index abf8f698f554..1c5b5916ab35 100644 --- a/brightray/browser/browser_context.cc +++ b/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, - FROM_HERE, - resource_context_.release()); + if (BrowserThread::IsMessageLoopValid(BrowserThread::IO)) { + BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, + resource_context_.release()); + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::BindOnce(&URLRequestContextGetter::NotifyContextShutdownOnIO, + base::RetainedRef(url_request_getter_))); + } } void BrowserContext::InitPrefs() { diff --git a/brightray/browser/url_request_context_getter.cc b/brightray/browser/url_request_context_getter.cc index e39758d737ed..6d8f03f9ed16 100644 --- a/brightray/browser/url_request_context_getter.cc +++ b/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 top_job_factory = std::move(job_factory); - content::URLRequestInterceptorScopedVector::reverse_iterator it; - for (it = protocol_interceptors_.rbegin(); - it != protocol_interceptors_.rend(); - ++it) { - top_job_factory.reset(new net::URLRequestInterceptingJobFactory( - std::move(top_job_factory), std::move(*it))); + if (!protocol_interceptors_.empty()) { + for (auto it = protocol_interceptors_.rbegin(); + it != protocol_interceptors_.rend(); ++it) { + top_job_factory.reset(new net::URLRequestInterceptingJobFactory( + std::move(top_job_factory), std::move(*it))); + } + protocol_interceptors_.clear(); } - protocol_interceptors_.clear(); storage_->set_job_factory(std::move(top_job_factory)); } diff --git a/brightray/browser/url_request_context_getter.h b/brightray/browser/url_request_context_getter.h index 5310e1f48f51..51c70ee92e30 100644 --- a/brightray/browser/url_request_context_getter.h +++ b/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 leak_tracker_; +#endif + std::unique_ptr ct_delegate_; std::unique_ptr proxy_config_service_; std::unique_ptr 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); };