diff options
author | willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-07-26 06:48:44 +0000 |
---|---|---|
committer | willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-07-26 06:48:44 +0000 |
commit | d98261678787d9f149316e3049b56b278f6c5c8c (patch) | |
tree | e88a135bdfc438c2e2e6cb5b99fc56485c05a9ce | |
parent | 5fc549f0469b301169a17a28b725c8ea20d80673 (diff) | |
download | chromium_src-d98261678787d9f149316e3049b56b278f6c5c8c.zip chromium_src-d98261678787d9f149316e3049b56b278f6c5c8c.tar.gz chromium_src-d98261678787d9f149316e3049b56b278f6c5c8c.tar.bz2 |
Make Profile own ProfileIOData.
Notably this makes ChromeURLRequestContextGetter not own its ChromeURLRequestContext, and instead it keeps a base::WeakPtr. If anything is holding onto a ChromeURLRequestContextGetter longer than it should, and tries to use it, then it will crash. Currently no tests fail with this assumption, so hopefully this will prevent regressions. All ChromeURLRequestContextGetters should not outlive the Profile to which they belong.
Note that this will cause problems if there are any URLRequests which outlive the Profile to which they belong, as they will reference URLRequestContexts which point to deleted memory. AFAICT this never happens, and I will enforce this later on by making URLRequests acquire weak pointers to URLRequestContexts. After that, I can stop refcounting URLRequestContext.
I'm hopeful this changelist will fix a large number of leaks in browser_tests. Removing the refcounting of ProfileIOData will bring sanity to the lifecycle management of ProfileIOData and a large number of objects it owns. I've used base::WeakPtr in a number of places in order to prevent use-after-free bugs and force them to be NULL dereferences (crashes). In previous changelists, I've flushed out bugs revealed by this behavior. So currently, this changelist is clean on the trybots, although it's possible new code paths will be discovered via crash reports.
Next steps:
* URLRequests use base::WeakPtr<URLRequestContext>
* Fix all other URLRequestContextGetters not to acquire references to URLRequestContext.
* Stop refcounting URLRequestContext.
BUG=none
TEST=none
Review URL: http://codereview.chromium.org/7432002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@94047 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/io_thread.cc | 38 | ||||
-rw-r--r-- | chrome/browser/io_thread.h | 21 | ||||
-rw-r--r-- | chrome/browser/net/chrome_url_request_context.cc | 37 | ||||
-rw-r--r-- | chrome/browser/net/chrome_url_request_context.h | 14 | ||||
-rw-r--r-- | chrome/browser/profiles/off_the_record_profile_io_data.cc | 13 | ||||
-rw-r--r-- | chrome/browser/profiles/off_the_record_profile_io_data.h | 6 | ||||
-rw-r--r-- | chrome/browser/profiles/profile_impl_io_data.cc | 20 | ||||
-rw-r--r-- | chrome/browser/profiles/profile_impl_io_data.h | 8 | ||||
-rw-r--r-- | chrome/browser/profiles/profile_io_data.cc | 38 | ||||
-rw-r--r-- | chrome/browser/profiles/profile_io_data.h | 61 | ||||
-rw-r--r-- | net/url_request/url_request_context.cc | 4 | ||||
-rw-r--r-- | net/url_request/url_request_context.h | 7 |
12 files changed, 68 insertions, 199 deletions
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc index 85c7e2a..4d716da 100644 --- a/chrome/browser/io_thread.cc +++ b/chrome/browser/io_thread.cc @@ -401,30 +401,6 @@ void IOThread::InitNetworkPredictor( startup_urls, referral_list, preconnect_enabled)); } -void IOThread::RegisterURLRequestContextGetter( - ChromeURLRequestContextGetter* url_request_context_getter) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - std::list<ChromeURLRequestContextGetter*>::const_iterator it = - std::find(url_request_context_getters_.begin(), - url_request_context_getters_.end(), - url_request_context_getter); - DCHECK(it == url_request_context_getters_.end()); - url_request_context_getters_.push_back(url_request_context_getter); -} - -void IOThread::UnregisterURLRequestContextGetter( - ChromeURLRequestContextGetter* url_request_context_getter) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - std::list<ChromeURLRequestContextGetter*>::iterator it = - std::find(url_request_context_getters_.begin(), - url_request_context_getters_.end(), - url_request_context_getter); - DCHECK(it != url_request_context_getters_.end()); - // This does not scale, but we shouldn't have many URLRequestContextGetters in - // the first place, so this should be fine. - url_request_context_getters_.erase(it); -} - void IOThread::ChangedToOnTheRecord() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); message_loop()->PostTask( @@ -532,20 +508,6 @@ void IOThread::CleanUp() { // IO thread only resources they are referencing. BrowserChildProcessHost::TerminateAll(); - std::list<ChromeURLRequestContextGetter*> url_request_context_getters; - url_request_context_getters.swap(url_request_context_getters_); - for (std::list<ChromeURLRequestContextGetter*>::iterator it = - url_request_context_getters.begin(); - it != url_request_context_getters.end(); ++it) { - ChromeURLRequestContextGetter* getter = *it; - // Stop all pending certificate provenance check uploads - net::DnsCertProvenanceChecker* checker = - getter->GetURLRequestContext()->dns_cert_checker(); - if (checker) - checker->Shutdown(); - getter->ReleaseURLRequestContext(); - } - system_url_request_context_getter_ = NULL; // Step 2: Release objects that the net::URLRequestContext could have been diff --git a/chrome/browser/io_thread.h b/chrome/browser/io_thread.h index 6bdd01b..779f4d4 100644 --- a/chrome/browser/io_thread.h +++ b/chrome/browser/io_thread.h @@ -6,7 +6,6 @@ #define CHROME_BROWSER_IO_THREAD_H_ #pragma once -#include <list> #include <string> #include "base/basictypes.h" #include "base/memory/ref_counted.h" @@ -117,21 +116,6 @@ class IOThread : public BrowserProcessSubThread { base::ListValue* referral_list, bool preconnect_enabled); - // Registers |url_request_context_getter| into the IO thread. During - // IOThread::CleanUp(), IOThread will iterate through known getters and - // release their URLRequestContexts. Only called on the IO thread. It does - // not acquire a refcount for |url_request_context_getter|. If - // |url_request_context_getter| is being deleted before IOThread::CleanUp() is - // invoked, then this needs to be balanced with a call to - // UnregisterURLRequestContextGetter(). - void RegisterURLRequestContextGetter( - ChromeURLRequestContextGetter* url_request_context_getter); - - // Unregisters |url_request_context_getter| from the IO thread. Only called - // on the IO thread. - void UnregisterURLRequestContextGetter( - ChromeURLRequestContextGetter* url_request_context_getter); - // Handles changing to On The Record mode, discarding confidential data. void ChangedToOnTheRecord(); @@ -234,11 +218,6 @@ class IOThread : public BrowserProcessSubThread { scoped_refptr<net::URLRequestContextGetter> system_url_request_context_getter_; - // Keeps track of all live ChromeURLRequestContextGetters, so the - // ChromeURLRequestContexts can be released during - // IOThread::CleanUp(). - std::list<ChromeURLRequestContextGetter*> url_request_context_getters_; - ScopedRunnableMethodFactory<IOThread> method_factory_; DISALLOW_COPY_AND_ASSIGN(IOThread); diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc index d96fbc0..06d1f2c 100644 --- a/chrome/browser/net/chrome_url_request_context.cc +++ b/chrome/browser/net/chrome_url_request_context.cc @@ -50,7 +50,7 @@ class FactoryForMain : public ChromeURLRequestContextFactory { } private: - const scoped_refptr<const ProfileIOData> profile_io_data_; + const ProfileIOData* const profile_io_data_; }; // Factory that creates the ChromeURLRequestContext for extensions. @@ -64,7 +64,7 @@ class FactoryForExtensions : public ChromeURLRequestContextFactory { } private: - const scoped_refptr<const ProfileIOData> profile_io_data_; + const ProfileIOData* const profile_io_data_; }; // Factory that creates the ChromeURLRequestContext for a given isolated app. @@ -84,7 +84,7 @@ class FactoryForIsolatedApp : public ChromeURLRequestContextFactory { } private: - const scoped_refptr<const ProfileIOData> profile_io_data_; + const ProfileIOData* const profile_io_data_; const std::string app_id_; scoped_refptr<ChromeURLRequestContextGetter> main_request_context_getter_; @@ -102,7 +102,7 @@ class FactoryForMedia : public ChromeURLRequestContextFactory { } private: - const scoped_refptr<const ProfileIOData> profile_io_data_; + const ProfileIOData* const profile_io_data_; }; } // namespace @@ -115,8 +115,7 @@ ChromeURLRequestContextGetter::ChromeURLRequestContextGetter( Profile* profile, ChromeURLRequestContextFactory* factory) : io_thread_(g_browser_process->io_thread()), - factory_(factory), - url_request_context_(NULL) { + factory_(factory) { DCHECK(factory); DCHECK(profile); RegisterPrefsObserver(profile); @@ -126,17 +125,6 @@ ChromeURLRequestContextGetter::~ChromeURLRequestContextGetter() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); DCHECK(registrar_.IsEmpty()) << "Probably didn't call CleanupOnUIThread"; - - // Either we already transformed the factory into a net::URLRequestContext, or - // we still have a pending factory. - DCHECK((factory_.get() && !url_request_context_.get()) || - (!factory_.get() && url_request_context_.get())); - - if (url_request_context_) - io_thread_->UnregisterURLRequestContextGetter(this); - - // The scoped_refptr / scoped_ptr destructors take care of releasing - // |factory_| and |url_request_context_| now. } // Lazily create a ChromeURLRequestContext using our factory. @@ -145,17 +133,15 @@ net::URLRequestContext* ChromeURLRequestContextGetter::GetURLRequestContext() { if (!url_request_context_) { DCHECK(factory_.get()); - url_request_context_ = factory_->Create(); + url_request_context_ = factory_->Create()->GetWeakPtr(); factory_.reset(); - io_thread_->RegisterURLRequestContextGetter(this); } - return url_request_context_; -} + // Should not be NULL, unless we're trying to use the URLRequestContextGetter + // after the Profile has already been deleted. + CHECK(url_request_context_.get()); -void ChromeURLRequestContextGetter::ReleaseURLRequestContext() { - DCHECK(url_request_context_); - url_request_context_ = NULL; + return url_request_context_; } net::CookieStore* ChromeURLRequestContextGetter::DONTUSEME_GetCookieStore() { @@ -352,8 +338,7 @@ void ChromeURLRequestContextGetter::GetCookieStoreAsyncHelper( // ---------------------------------------------------------------------------- ChromeURLRequestContext::ChromeURLRequestContext() - : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)), - is_incognito_(false) { + : is_incognito_(false) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); } diff --git a/chrome/browser/net/chrome_url_request_context.h b/chrome/browser/net/chrome_url_request_context.h index 22db34c..b8d5a16 100644 --- a/chrome/browser/net/chrome_url_request_context.h +++ b/chrome/browser/net/chrome_url_request_context.h @@ -40,10 +40,6 @@ class ChromeURLRequestContext : public net::URLRequestContext { // Copies the state from |other| into this context. void CopyFrom(ChromeURLRequestContext* other); - base::WeakPtr<ChromeURLRequestContext> GetWeakPtr() { - return weak_ptr_factory_.GetWeakPtr(); - } - bool is_incognito() const { return is_incognito_; } @@ -71,8 +67,6 @@ class ChromeURLRequestContext : public net::URLRequestContext { virtual ~ChromeURLRequestContext(); private: - base::WeakPtrFactory<ChromeURLRequestContext> weak_ptr_factory_; - // --------------------------------------------------------------------------- // Important: When adding any new members below, consider whether they need to // be added to CopyFrom. @@ -115,10 +109,6 @@ class ChromeURLRequestContextGetter : public net::URLRequestContextGetter, virtual net::CookieStore* DONTUSEME_GetCookieStore(); virtual scoped_refptr<base::MessageLoopProxy> GetIOMessageLoopProxy() const; - // Releases |url_request_context_|. It's invalid to call - // GetURLRequestContext() after this point. - void ReleaseURLRequestContext(); - // Convenience overload of GetURLRequestContext() that returns a // ChromeURLRequestContext* rather than a net::URLRequestContext*. ChromeURLRequestContext* GetIOContext() { @@ -203,9 +193,9 @@ class ChromeURLRequestContextGetter : public net::URLRequestContextGetter, scoped_ptr<ChromeURLRequestContextFactory> factory_; // NULL if not yet initialized. Otherwise, it is the net::URLRequestContext - // instance that was lazilly created by GetURLRequestContext. + // instance that was lazily created by GetURLRequestContext(). // Access only from the IO thread. - scoped_refptr<net::URLRequestContext> url_request_context_; + base::WeakPtr<net::URLRequestContext> url_request_context_; DISALLOW_COPY_AND_ASSIGN(ChromeURLRequestContextGetter); }; diff --git a/chrome/browser/profiles/off_the_record_profile_io_data.cc b/chrome/browser/profiles/off_the_record_profile_io_data.cc index 2d9b368..11f8ae3 100644 --- a/chrome/browser/profiles/off_the_record_profile_io_data.cc +++ b/chrome/browser/profiles/off_the_record_profile_io_data.cc @@ -46,8 +46,7 @@ OffTheRecordProfileIOData::Handle::~Handle() { iter->second->CleanupOnUIThread(); } - io_data_->AddRef(); - io_data_.release()->ShutdownOnUIThread(); + io_data_->ShutdownOnUIThread(); } base::Callback<ChromeURLDataManagerBackend*(void)> @@ -56,7 +55,7 @@ GetChromeURLDataManagerBackendGetter() const { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); LazyInitialize(); return base::Bind(&ProfileIOData::GetChromeURLDataManagerBackend, - base::Unretained(io_data_.get())); + base::Unretained(io_data_)); } const content::ResourceContext& @@ -200,11 +199,11 @@ void OffTheRecordProfileIOData::LazyInitializeInternal( extensions_context->set_job_factory(job_factory()); } -scoped_refptr<ProfileIOData::RequestContext> +scoped_refptr<ChromeURLRequestContext> OffTheRecordProfileIOData::InitializeAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const { - AppRequestContext* context = new AppRequestContext(app_id); + AppRequestContext* context = new AppRequestContext; // Copy most state from the main context. context->CopyFrom(main_context); @@ -232,12 +231,12 @@ OffTheRecordProfileIOData::AcquireMediaRequestContext() const { return NULL; } -scoped_refptr<ProfileIOData::RequestContext> +scoped_refptr<ChromeURLRequestContext> OffTheRecordProfileIOData::AcquireIsolatedAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const { // We create per-app contexts on demand, unlike the others above. - scoped_refptr<RequestContext> app_request_context = + scoped_refptr<ChromeURLRequestContext> app_request_context = InitializeAppRequestContext(main_context, app_id); DCHECK(app_request_context); return app_request_context; diff --git a/chrome/browser/profiles/off_the_record_profile_io_data.h b/chrome/browser/profiles/off_the_record_profile_io_data.h index 5e01c15..cbeb632 100644 --- a/chrome/browser/profiles/off_the_record_profile_io_data.h +++ b/chrome/browser/profiles/off_the_record_profile_io_data.h @@ -72,7 +72,7 @@ class OffTheRecordProfileIOData : public ProfileIOData { extensions_request_context_getter_; mutable ChromeURLRequestContextGetterMap app_request_context_getter_map_; - scoped_refptr<OffTheRecordProfileIOData> io_data_; + OffTheRecordProfileIOData* const io_data_; Profile* const profile_; @@ -91,12 +91,12 @@ class OffTheRecordProfileIOData : public ProfileIOData { virtual ~OffTheRecordProfileIOData(); virtual void LazyInitializeInternal(ProfileParams* profile_params) const; - virtual scoped_refptr<RequestContext> InitializeAppRequestContext( + virtual scoped_refptr<ChromeURLRequestContext> InitializeAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const; virtual scoped_refptr<ChromeURLRequestContext> AcquireMediaRequestContext() const; - virtual scoped_refptr<RequestContext> + virtual scoped_refptr<ChromeURLRequestContext> AcquireIsolatedAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const; diff --git a/chrome/browser/profiles/profile_impl_io_data.cc b/chrome/browser/profiles/profile_impl_io_data.cc index f917b1a..fa95deb 100644 --- a/chrome/browser/profiles/profile_impl_io_data.cc +++ b/chrome/browser/profiles/profile_impl_io_data.cc @@ -49,7 +49,7 @@ ProfileImplIOData::Handle::~Handle() { iter->second->CleanupOnUIThread(); } - io_data_.release()->ShutdownOnUIThread(); + io_data_->ShutdownOnUIThread(); } void ProfileImplIOData::Handle::Init(const FilePath& cookie_path, @@ -81,7 +81,7 @@ ProfileImplIOData::Handle::GetChromeURLDataManagerBackendGetter() const { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); LazyInitialize(); return base::Bind(&ProfileIOData::GetChromeURLDataManagerBackend, - base::Unretained(io_data_.get())); + base::Unretained(io_data_)); } const content::ResourceContext& @@ -185,7 +185,7 @@ void ProfileImplIOData::LazyInitializeInternal( ChromeURLRequestContext* main_context = main_request_context(); ChromeURLRequestContext* extensions_context = extensions_request_context(); - media_request_context_ = new RequestContext; + media_request_context_ = new ChromeURLRequestContext; IOThread* const io_thread = profile_params->io_thread; IOThread::Globals* const io_thread_globals = io_thread->globals(); @@ -310,11 +310,11 @@ void ProfileImplIOData::LazyInitializeInternal( lazy_params_.reset(); } -scoped_refptr<ProfileIOData::RequestContext> +scoped_refptr<ChromeURLRequestContext> ProfileImplIOData::InitializeAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const { - ProfileIOData::AppRequestContext* context = new AppRequestContext(app_id); + AppRequestContext* context = new AppRequestContext; // Copy most state from the main context. context->CopyFrom(main_context); @@ -375,19 +375,15 @@ ProfileImplIOData::InitializeAppRequestContext( scoped_refptr<ChromeURLRequestContext> ProfileImplIOData::AcquireMediaRequestContext() const { DCHECK(media_request_context_); - scoped_refptr<ChromeURLRequestContext> context = media_request_context_; - media_request_context_->set_profile_io_data( - const_cast<ProfileImplIOData*>(this)); - media_request_context_ = NULL; - return context; + return media_request_context_; } -scoped_refptr<ProfileIOData::RequestContext> +scoped_refptr<ChromeURLRequestContext> ProfileImplIOData::AcquireIsolatedAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const { // We create per-app contexts on demand, unlike the others above. - scoped_refptr<RequestContext> app_request_context = + scoped_refptr<ChromeURLRequestContext> app_request_context = InitializeAppRequestContext(main_context, app_id); DCHECK(app_request_context); return app_request_context; diff --git a/chrome/browser/profiles/profile_impl_io_data.h b/chrome/browser/profiles/profile_impl_io_data.h index 0e122c1..dea0c79 100644 --- a/chrome/browser/profiles/profile_impl_io_data.h +++ b/chrome/browser/profiles/profile_impl_io_data.h @@ -75,7 +75,7 @@ class ProfileImplIOData : public ProfileIOData { mutable scoped_refptr<ChromeURLRequestContextGetter> extensions_request_context_getter_; mutable ChromeURLRequestContextGetterMap app_request_context_getter_map_; - scoped_refptr<ProfileImplIOData> io_data_; + ProfileImplIOData* const io_data_; Profile* const profile_; @@ -107,12 +107,12 @@ class ProfileImplIOData : public ProfileIOData { virtual ~ProfileImplIOData(); virtual void LazyInitializeInternal(ProfileParams* profile_params) const; - virtual scoped_refptr<RequestContext> InitializeAppRequestContext( + virtual scoped_refptr<ChromeURLRequestContext> InitializeAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const; virtual scoped_refptr<ChromeURLRequestContext> AcquireMediaRequestContext() const; - virtual scoped_refptr<RequestContext> + virtual scoped_refptr<ChromeURLRequestContext> AcquireIsolatedAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const; @@ -120,7 +120,7 @@ class ProfileImplIOData : public ProfileIOData { // Lazy initialization params. mutable scoped_ptr<LazyParams> lazy_params_; - mutable scoped_refptr<RequestContext> media_request_context_; + mutable scoped_refptr<ChromeURLRequestContext> media_request_context_; mutable scoped_ptr<net::HttpTransactionFactory> main_http_factory_; mutable scoped_ptr<net::HttpTransactionFactory> media_http_factory_; diff --git a/chrome/browser/profiles/profile_io_data.cc b/chrome/browser/profiles/profile_io_data.cc index e8695f3..e2c1053 100644 --- a/chrome/browser/profiles/profile_io_data.cc +++ b/chrome/browser/profiles/profile_io_data.cc @@ -272,15 +272,8 @@ void ProfileIOData::InitializeProfileParams(Profile* profile) { profile_params_.reset(params.release()); } -ProfileIOData::RequestContext::RequestContext() {} -ProfileIOData::RequestContext::~RequestContext() {} - -ProfileIOData::AppRequestContext::AppRequestContext(const std::string& app_id) - : app_id_(app_id) {} -ProfileIOData::AppRequestContext::~AppRequestContext() { - DCHECK(ContainsKey(profile_io_data()->app_request_context_map_, app_id_)); - profile_io_data()->app_request_context_map_.erase(app_id_); -} +ProfileIOData::AppRequestContext::AppRequestContext() {} +ProfileIOData::AppRequestContext::~AppRequestContext() {} void ProfileIOData::AppRequestContext::SetCookieStore( net::CookieStore* cookie_store) { @@ -355,10 +348,7 @@ ProfileIOData::GetChromeURLDataManagerBackend() const { scoped_refptr<ChromeURLRequestContext> ProfileIOData::GetMainRequestContext() const { LazyInitialize(); - scoped_refptr<RequestContext> context = main_request_context_; - context->set_profile_io_data(const_cast<ProfileIOData*>(this)); - main_request_context_ = NULL; - return context; + return main_request_context_; } scoped_refptr<ChromeURLRequestContext> @@ -373,11 +363,7 @@ ProfileIOData::GetMediaRequestContext() const { scoped_refptr<ChromeURLRequestContext> ProfileIOData::GetExtensionsRequestContext() const { LazyInitialize(); - scoped_refptr<RequestContext> context = - extensions_request_context_; - context->set_profile_io_data(const_cast<ProfileIOData*>(this)); - extensions_request_context_ = NULL; - return context; + return extensions_request_context_; } scoped_refptr<ChromeURLRequestContext> @@ -389,11 +375,8 @@ ProfileIOData::GetIsolatedAppRequestContext( if (ContainsKey(app_request_context_map_, app_id)) { context = app_request_context_map_[app_id]; } else { - scoped_refptr<RequestContext> request_context = - AcquireIsolatedAppRequestContext(main_context, app_id); - request_context->set_profile_io_data(const_cast<ProfileIOData*>(this)); - app_request_context_map_[app_id] = request_context; - context = request_context; + context = AcquireIsolatedAppRequestContext(main_context, app_id); + app_request_context_map_[app_id] = context; } DCHECK(context); return context; @@ -433,9 +416,8 @@ void ProfileIOData::LazyInitialize() const { const CommandLine& command_line = *CommandLine::ForCurrentProcess(); // Create the common request contexts. - main_request_context_ = new RequestContext; - extensions_request_context_ = new RequestContext; - weak_extensions_request_context_ = extensions_request_context_->GetWeakPtr(); + main_request_context_ = new ChromeURLRequestContext; + extensions_request_context_ = new ChromeURLRequestContext; profile_params_->appcache_service->set_request_context(main_request_context_); @@ -556,7 +538,7 @@ void ProfileIOData::ShutdownOnUIThread() { base::Unretained(g_browser_process->resource_dispatcher_host()), &resource_context_)); bool posted = BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - new ReleaseTask<ProfileIOData>(this)); + new DeleteTask<ProfileIOData>(this)); if (!posted) - Release(); + delete this; } diff --git a/chrome/browser/profiles/profile_io_data.h b/chrome/browser/profiles/profile_io_data.h index b8478be..69c55c9 100644 --- a/chrome/browser/profiles/profile_io_data.h +++ b/chrome/browser/profiles/profile_io_data.h @@ -58,19 +58,13 @@ class DatabaseTracker; // Conceptually speaking, the ProfileIOData represents data that lives on the IO // thread that is owned by a Profile, such as, but not limited to, network -// objects like CookieMonster, HttpTransactionFactory, etc. The Profile -// implementation will maintain a reference to the ProfileIOData. The -// ProfileIOData will originally own a reference to the ChromeURLRequestContexts -// that reference its members. When an accessor for a ChromeURLRequestContext is -// invoked, then ProfileIOData will release its reference to the -// ChromeURLRequestContext and the ChromeURLRequestContext will acquire a -// reference to the ProfileIOData, so they exchange ownership. This is done -// because it's possible for a context's accessor never to be invoked, so this -// ownership reversal prevents shutdown leaks. ProfileIOData will lazily -// initialize its members on the first invocation of a ChromeURLRequestContext -// accessor. -class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { +// objects like CookieMonster, HttpTransactionFactory, etc. Profile owns +// ProfileIOData, but will make sure to delete it on the IO thread (except +// possibly in unit tests where there is no IO thread). +class ProfileIOData { public: + virtual ~ProfileIOData(); + // Returns true if |scheme| is handled in Chrome, or by default handlers in // net::URLRequest. static bool IsHandledProtocol(const std::string& scheme); @@ -105,7 +99,7 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { } ChromeURLRequestContext* extensions_request_context() const { - return weak_extensions_request_context_.get(); + return extensions_request_context_.get(); } BooleanPrefMember* safe_browsing_enabled() const { @@ -113,35 +107,15 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { } protected: - friend class base::RefCountedThreadSafe<ProfileIOData>; - - class RequestContext : public ChromeURLRequestContext { + class AppRequestContext : public ChromeURLRequestContext { public: - RequestContext(); - virtual ~RequestContext(); - - // Setter is used to transfer ownership of the ProfileIOData to the context. - void set_profile_io_data(ProfileIOData* profile_io_data) { - profile_io_data_ = profile_io_data; - } - - protected: - ProfileIOData* profile_io_data() { return profile_io_data_; } - - private: - scoped_refptr<ProfileIOData> profile_io_data_; - }; - - class AppRequestContext : public RequestContext { - public: - explicit AppRequestContext(const std::string& app_id); + AppRequestContext(); virtual ~AppRequestContext(); void SetCookieStore(net::CookieStore* cookie_store); void SetHttpTransactionFactory(net::HttpTransactionFactory* http_factory); private: - const std::string app_id_; scoped_refptr<net::CookieStore> cookie_store_; scoped_ptr<net::HttpTransactionFactory> http_factory_; }; @@ -183,7 +157,6 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { }; explicit ProfileIOData(bool is_incognito); - virtual ~ProfileIOData(); void InitializeProfileParams(Profile* profile); void ApplyProfileParamsToContext(ChromeURLRequestContext* context) const; @@ -237,7 +210,7 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { const ProfileIOData* const io_data_; }; - typedef base::hash_map<std::string, ChromeURLRequestContext*> + typedef base::hash_map<std::string, scoped_refptr<ChromeURLRequestContext> > AppRequestContextMap; // -------------------------------------------- @@ -250,7 +223,7 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { // Does an on-demand initialization of a RequestContext for the given // isolated app. - virtual scoped_refptr<RequestContext> InitializeAppRequestContext( + virtual scoped_refptr<ChromeURLRequestContext> InitializeAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const = 0; @@ -258,7 +231,7 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { // context from ProfileIOData to the URLRequestContextGetter. virtual scoped_refptr<ChromeURLRequestContext> AcquireMediaRequestContext() const = 0; - virtual scoped_refptr<RequestContext> + virtual scoped_refptr<ChromeURLRequestContext> AcquireIsolatedAppRequestContext( scoped_refptr<ChromeURLRequestContext> main_context, const std::string& app_id) const = 0; @@ -301,17 +274,11 @@ class ProfileIOData : public base::RefCountedThreadSafe<ProfileIOData> { // These are only valid in between LazyInitialize() and their accessor being // called. - mutable scoped_refptr<RequestContext> main_request_context_; - mutable scoped_refptr<RequestContext> extensions_request_context_; + mutable scoped_refptr<ChromeURLRequestContext> main_request_context_; + mutable scoped_refptr<ChromeURLRequestContext> extensions_request_context_; // One AppRequestContext per isolated app. mutable AppRequestContextMap app_request_context_map_; - // Weak pointers to the request contexts. Only valid after LazyInitialize. - // These are weak so that they don't hold a reference to the RequestContext, - // because that holds a reference back to ProfileIOData. - mutable base::WeakPtr<ChromeURLRequestContext> - weak_extensions_request_context_; - DISALLOW_COPY_AND_ASSIGN(ProfileIOData); }; diff --git a/net/url_request/url_request_context.cc b/net/url_request/url_request_context.cc index 2349a23..e808a0e 100644 --- a/net/url_request/url_request_context.cc +++ b/net/url_request/url_request_context.cc @@ -4,6 +4,7 @@ #include "net/url_request/url_request_context.h" +#include "base/compiler_specific.h" #include "base/string_util.h" #include "net/base/cookie_store.h" #include "net/base/host_resolver.h" @@ -13,7 +14,8 @@ namespace net { URLRequestContext::URLRequestContext() - : net_log_(NULL), + : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)), + net_log_(NULL), host_resolver_(NULL), cert_verifier_(NULL), origin_bound_cert_service_(NULL), diff --git a/net/url_request/url_request_context.h b/net/url_request/url_request_context.h index d64db3d..0d55a30 100644 --- a/net/url_request/url_request_context.h +++ b/net/url_request/url_request_context.h @@ -13,6 +13,7 @@ #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" +#include "base/memory/weak_ptr.h" #include "base/threading/non_thread_safe.h" #include "net/base/net_api.h" #include "net/base/net_log.h" @@ -45,6 +46,10 @@ class NET_API URLRequestContext public: URLRequestContext(); + base::WeakPtr<URLRequestContext> GetWeakPtr() { + return weak_factory_.GetWeakPtr(); + } + // Copies the state from |other| into this context. void CopyFrom(URLRequestContext* other); @@ -188,6 +193,8 @@ class NET_API URLRequestContext virtual ~URLRequestContext(); private: + base::WeakPtrFactory<URLRequestContext> weak_factory_; + // --------------------------------------------------------------------------- // Important: When adding any new members below, consider whether they need to // be added to CopyFrom. |