Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
deepak1556 authored and ckerr committed Mar 30, 2018
1 parent fc00a2b commit 171230e
Show file tree
Hide file tree
Showing 14 changed files with 144 additions and 96 deletions.
26 changes: 15 additions & 11 deletions atom/browser/api/atom_api_cookies.cc
Expand Up @@ -238,46 +238,50 @@ void SetCookieOnIO(scoped_refptr<net::URLRequestContextGetter> 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() {}

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) {
Expand Down
5 changes: 0 additions & 5 deletions atom/browser/api/atom_api_cookies.h
Expand Up @@ -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);
};
Expand Down
42 changes: 12 additions & 30 deletions atom/browser/api/atom_api_protocol.cc
Expand Up @@ -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() {
Expand Down Expand Up @@ -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()),
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(
Expand All @@ -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
Expand All @@ -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);
}

Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
20 changes: 9 additions & 11 deletions atom/browser/api/atom_api_protocol.h
Expand Up @@ -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>,
request_context_getter_, isolate(), scheme, handler),
base::Bind(&Protocol::OnIOCompleted,
GetWeakPtr(), callback));
base::BindOnce(&Protocol::RegisterProtocolInIO<RequestJob>,
base::RetainedRef(getter), isolate(), scheme, handler),
base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback));
}
template<typename RequestJob>
static ProtocolError RegisterProtocolInIO(
Expand Down Expand Up @@ -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>,
request_context_getter_, isolate(), scheme, handler),
base::Bind(&Protocol::OnIOCompleted,
GetWeakPtr(), callback));
base::BindOnce(&Protocol::InterceptProtocolInIO<RequestJob>,
base::RetainedRef(getter), isolate(), scheme, handler),
base::BindOnce(&Protocol::OnIOCompleted, GetWeakPtr(), callback));
}
template<typename RequestJob>
static ProtocolError InterceptProtocolInIO(
Expand Down Expand Up @@ -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);
Expand Down
41 changes: 38 additions & 3 deletions atom/browser/api/atom_api_session.cc
Expand Up @@ -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();
Expand Down Expand Up @@ -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)
Expand All @@ -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());
}

Expand Down Expand Up @@ -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) {
Expand Down
15 changes: 5 additions & 10 deletions atom/browser/api/trackable_object.cc
Expand Up @@ -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() {
Expand All @@ -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
7 changes: 1 addition & 6 deletions atom/browser/api/trackable_object.h
Expand Up @@ -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<TrackableObjectBase> weak_factory_;

DISALLOW_COPY_AND_ASSIGN(TrackableObjectBase);
Expand Down
9 changes: 9 additions & 0 deletions atom/browser/atom_browser_context.h
Expand Up @@ -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);
Expand All @@ -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);
};
Expand Down
12 changes: 6 additions & 6 deletions atom/browser/atom_browser_main_parts.cc
Expand Up @@ -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<std::list<base::Closure>>, &destructors_, iter);
void AtomBrowserMainParts::RegisterDestructionCallback(
base::OnceClosure callback) {
destructors_.insert(destructors_.end(), std::move(callback));
}

void AtomBrowserMainParts::PreEarlyInitialization() {
Expand Down Expand Up @@ -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();
}
}

Expand Down

0 comments on commit 171230e

Please sign in to comment.