From 861b4d0a0b0cf9e5224d004eb99ce768221000ed Mon Sep 17 00:00:00 2001 From: "asanka@chromium.org" Date: Fri, 13 Dec 2013 20:10:41 +0000 Subject: [Downloads] Make ChromeDownloadManagerDelegate owned by DownloadService. Currently ChromeDownloadManagerDelegate is reference counted. Since it also posts long running tasks, this causes it to outlive the associated profile on occasion. Since there are multiple references to Profile/BrowserContext owned objects from within CDMD, this can introduce subtle use-after-free errors. This change is to make CDMD owned by DownloadService so that it doesn't outlive the Profile. BUG=317913 Review URL: https://codereview.chromium.org/69793006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@240726 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/download/download_service.cc | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) (limited to 'chrome/browser/download/download_service.cc') diff --git a/chrome/browser/download/download_service.cc b/chrome/browser/download/download_service.cc index a33ed163..13503dc 100644 --- a/chrome/browser/download/download_service.cc +++ b/chrome/browser/download/download_service.cc @@ -43,7 +43,7 @@ ChromeDownloadManagerDelegate* DownloadService::GetDownloadManagerDelegate() { // In case the delegate has already been set by // SetDownloadManagerDelegateForTesting. if (!manager_delegate_.get()) - manager_delegate_ = new ChromeDownloadManagerDelegate(profile_); + manager_delegate_.reset(new ChromeDownloadManagerDelegate(profile_)); manager_delegate_->SetDownloadManager(manager); @@ -55,8 +55,8 @@ ChromeDownloadManagerDelegate* DownloadService::GetDownloadManagerDelegate() { if (!profile_->IsOffTheRecord()) { HistoryService* history = HistoryServiceFactory::GetForProfile( profile_, Profile::EXPLICIT_ACCESS); - history->GetNextDownloadId(base::Bind( - &ChromeDownloadManagerDelegate::SetNextId, manager_delegate_)); + history->GetNextDownloadId( + manager_delegate_->GetDownloadIdReceiverCallback()); download_history_.reset(new DownloadHistory( manager, scoped_ptr( @@ -135,17 +135,13 @@ void DownloadService::CancelAllDownloads() { } void DownloadService::SetDownloadManagerDelegateForTesting( - ChromeDownloadManagerDelegate* new_delegate) { - // Set the new delegate first so that if BrowserContext::GetDownloadManager() - // causes a new download manager to be created, we won't create a redundant - // ChromeDownloadManagerDelegate(). - manager_delegate_ = new_delegate; - // Guarantee everything is properly initialized. + scoped_ptr new_delegate) { + manager_delegate_.swap(new_delegate); DownloadManager* dm = BrowserContext::GetDownloadManager(profile_); - if (dm->GetDelegate() != new_delegate) { - dm->SetDelegate(new_delegate); - new_delegate->SetDownloadManager(dm); - } + dm->SetDelegate(manager_delegate_.get()); + manager_delegate_->SetDownloadManager(dm); + if (new_delegate) + new_delegate->Shutdown(); } bool DownloadService::IsShelfEnabled() { @@ -169,6 +165,6 @@ void DownloadService::Shutdown() { #if !defined(OS_ANDROID) extension_event_router_.reset(); #endif - manager_delegate_ = NULL; + manager_delegate_.reset(); download_history_.reset(); } -- cgit v1.1