[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

8 files changed, 63 insertions, 50 deletions

diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 0d1c0c5..efcc701 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -216,6 +216,8 @@ ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
 }
 
 ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() {
+  // If a DownloadManager was set for this, Shutdown() must be called.
+  DCHECK(!download_manager_);
 }
 
 void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) {
@@ -225,6 +227,13 @@ void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) {
 void ChromeDownloadManagerDelegate::Shutdown() {
   download_prefs_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
+  download_manager_ = NULL;
+}
+
+content::DownloadIdCallback
+ChromeDownloadManagerDelegate::GetDownloadIdReceiverCallback() {
+  return base::Bind(&ChromeDownloadManagerDelegate::SetNextId,
+                    weak_ptr_factory_.GetWeakPtr());
 }
 
 void ChromeDownloadManagerDelegate::SetNextId(uint32 next_id) {
diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h
index 38b91f7..095f391 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.h
+++ b/chrome/browser/download/chrome_download_manager_delegate.h
@@ -7,7 +7,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/containers/hash_tables.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/download/download_path_reservation_tracker.h"
@@ -48,12 +47,12 @@ struct hash<extensions::CrxInstaller*> {
 
 // This is the Chrome side helper for the download system.
 class ChromeDownloadManagerDelegate
-    : public base::RefCountedThreadSafe<ChromeDownloadManagerDelegate>,
-      public content::DownloadManagerDelegate,
+    : public content::DownloadManagerDelegate,
       public content::NotificationObserver,
       public DownloadTargetDeterminerDelegate {
  public:
   explicit ChromeDownloadManagerDelegate(Profile* profile);
+  virtual ~ChromeDownloadManagerDelegate();
 
   // Should be called before the first call to ShouldCompleteDownload() to
   // disable SafeBrowsing checks for |item|.
@@ -61,9 +60,9 @@ class ChromeDownloadManagerDelegate
 
   void SetDownloadManager(content::DownloadManager* dm);
 
-  // Callbacks passed to GetNextId() will not be called until SetNextId() is
-  // called.
-  void SetNextId(uint32 next_id);
+  // Callbacks passed to GetNextId() will not be called until the returned
+  // callback is called.
+  content::DownloadIdCallback GetDownloadIdReceiverCallback();
 
   // content::DownloadManagerDelegate
   virtual void Shutdown() OVERRIDE;
@@ -105,9 +104,6 @@ class ChromeDownloadManagerDelegate
   DownloadPrefs* download_prefs() { return download_prefs_.get(); }
 
  protected:
-  // So that test classes can inherit from this for override purposes.
-  virtual ~ChromeDownloadManagerDelegate();
-
   // So that test classes that inherit from this for override purposes
   // can call back into the DownloadManager.
   content::DownloadManager* download_manager_;
@@ -165,6 +161,8 @@ class ChromeDownloadManagerDelegate
     uint32 download_id,
     const base::Closure& user_complete_callback);
 
+  void SetNextId(uint32 id);
+
   void ReturnNextId(const content::DownloadIdCallback& callback);
 
   void OnDownloadTargetDetermined(
diff --git a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
index 1e96837..7ef30fc 100644
--- a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
@@ -75,6 +75,8 @@ class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate {
       : ChromeDownloadManagerDelegate(profile) {
   }
 
+  virtual ~TestChromeDownloadManagerDelegate() {}
+
   virtual safe_browsing::DownloadProtectionService*
       GetDownloadProtectionService() OVERRIDE {
     return NULL;
@@ -118,9 +120,6 @@ class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate {
           content::DownloadItem*,
           const base::FilePath&,
           const DownloadTargetDeterminerDelegate::FileSelectedCallback&));
-
- private:
-  ~TestChromeDownloadManagerDelegate() {}
 };
 
 class ChromeDownloadManagerDelegateTest :
@@ -164,7 +163,7 @@ class ChromeDownloadManagerDelegateTest :
   TestingPrefServiceSyncable* pref_service_;
   base::ScopedTempDir test_download_dir_;
   scoped_ptr<content::MockDownloadManager> download_manager_;
-  scoped_refptr<TestChromeDownloadManagerDelegate> delegate_;
+  scoped_ptr<TestChromeDownloadManagerDelegate> delegate_;
   MockWebContentsDelegate web_contents_delegate_;
 };
 
@@ -176,7 +175,7 @@ void ChromeDownloadManagerDelegateTest::SetUp() {
   ChromeRenderViewHostTestHarness::SetUp();
 
   CHECK(profile());
-  delegate_ = new TestChromeDownloadManagerDelegate(profile());
+  delegate_.reset(new TestChromeDownloadManagerDelegate(profile()));
   delegate_->SetDownloadManager(download_manager_.get());
   pref_service_ = profile()->GetTestingPrefService();
   web_contents()->SetDelegate(&web_contents_delegate_);
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<DownloadHistory::HistoryAdapter>(
@@ -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<ChromeDownloadManagerDelegate> 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();
 }
diff --git a/chrome/browser/download/download_service.h b/chrome/browser/download/download_service.h
index c382486..72b0de0 100644
--- a/chrome/browser/download/download_service.h
+++ b/chrome/browser/download/download_service.h
@@ -7,7 +7,6 @@
 
 #include "base/basictypes.h"
 #include "base/callback_forward.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 
@@ -58,7 +57,7 @@ class DownloadService : public BrowserContextKeyedService {
   // its DownloadManager.  Takes ownership of |delegate|, and destroys
   // the previous delegate.  For testing.
   void SetDownloadManagerDelegateForTesting(
-      ChromeDownloadManagerDelegate* delegate);
+      scoped_ptr<ChromeDownloadManagerDelegate> delegate);
 
   // Will be called to release references on other services as part
   // of Profile shutdown.
@@ -75,7 +74,7 @@ class DownloadService : public BrowserContextKeyedService {
   // ChromeDownloadManagerDelegate may be the target of callbacks from
   // the history service/DB thread and must be kept alive for those
   // callbacks.
-  scoped_refptr<ChromeDownloadManagerDelegate> manager_delegate_;
+  scoped_ptr<ChromeDownloadManagerDelegate> manager_delegate_;
 
   scoped_ptr<DownloadHistory> download_history_;
 
diff --git a/chrome/browser/download/download_test_file_activity_observer.cc b/chrome/browser/download/download_test_file_activity_observer.cc
index 5768726..db68b6f 100644
--- a/chrome/browser/download/download_test_file_activity_observer.cc
+++ b/chrome/browser/download/download_test_file_activity_observer.cc
@@ -24,11 +24,15 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
   explicit MockDownloadManagerDelegate(Profile* profile)
       : ChromeDownloadManagerDelegate(profile),
         file_chooser_enabled_(false),
-        file_chooser_displayed_(false) {
+        file_chooser_displayed_(false),
+        weak_ptr_factory_(this) {
     if (!profile->IsOffTheRecord())
-      SetNextId(content::DownloadItem::kInvalidId + 1);
+      GetDownloadIdReceiverCallback().Run(
+          content::DownloadItem::kInvalidId + 1);
   }
 
+  virtual ~MockDownloadManagerDelegate() {}
+
   void EnableFileChooser(bool enable) {
     file_chooser_enabled_ = enable;
   }
@@ -39,6 +43,10 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
     return did_show;
   }
 
+  base::WeakPtr<MockDownloadManagerDelegate> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
  protected:
 
   virtual void PromptUserForDownloadPath(content::DownloadItem* item,
@@ -54,26 +62,30 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
   virtual void OpenDownload(content::DownloadItem* item) OVERRIDE {}
 
  private:
-  virtual ~MockDownloadManagerDelegate() {}
-
   bool file_chooser_enabled_;
   bool file_chooser_displayed_;
+  base::WeakPtrFactory<MockDownloadManagerDelegate> weak_ptr_factory_;
 };
 
 DownloadTestFileActivityObserver::DownloadTestFileActivityObserver(
     Profile* profile) {
-  test_delegate_ = new MockDownloadManagerDelegate(profile);
+  scoped_ptr<MockDownloadManagerDelegate> mock_delegate(
+      new MockDownloadManagerDelegate(profile));
+  test_delegate_ = mock_delegate->GetWeakPtr();
   DownloadServiceFactory::GetForBrowserContext(profile)->
-      SetDownloadManagerDelegateForTesting(test_delegate_.get());
+      SetDownloadManagerDelegateForTesting(
+          mock_delegate.PassAs<ChromeDownloadManagerDelegate>());
 }
 
 DownloadTestFileActivityObserver::~DownloadTestFileActivityObserver() {
 }
 
 void DownloadTestFileActivityObserver::EnableFileChooser(bool enable) {
-  test_delegate_->EnableFileChooser(enable);
+  if (test_delegate_.get())
+    test_delegate_->EnableFileChooser(enable);
 }
 
 bool DownloadTestFileActivityObserver::TestAndResetDidShowFileChooser() {
-  return test_delegate_->TestAndResetDidShowFileChooser();
+  return test_delegate_.get() &&
+      test_delegate_->TestAndResetDidShowFileChooser();
 }
diff --git a/chrome/browser/download/download_test_file_activity_observer.h b/chrome/browser/download/download_test_file_activity_observer.h
index 736b9fd..28e07d8 100644
--- a/chrome/browser/download/download_test_file_activity_observer.h
+++ b/chrome/browser/download/download_test_file_activity_observer.h
@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_FILE_ACTIVITY_OBSERVER_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_FILE_ACTIVITY_OBSERVER_H_
 
-#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 
 class Profile;
 
@@ -34,7 +34,7 @@ class DownloadTestFileActivityObserver {
  private:
   class MockDownloadManagerDelegate;
 
-  scoped_refptr<MockDownloadManagerDelegate> test_delegate_;
+  base::WeakPtr<MockDownloadManagerDelegate> test_delegate_;
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_FILE_ACTIVITY_OBSERVER_H_
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index 70636b9..8eb7341 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -448,26 +448,28 @@ class DelayingDownloadManagerDelegate : public ChromeDownloadManagerDelegate {
   explicit DelayingDownloadManagerDelegate(Profile* profile)
     : ChromeDownloadManagerDelegate(profile) {
   }
+  virtual ~DelayingDownloadManagerDelegate() {}
+
   virtual bool ShouldCompleteDownload(
       content::DownloadItem* item,
       const base::Closure& user_complete_callback) OVERRIDE {
     return false;
   }
 
- protected:
-  virtual ~DelayingDownloadManagerDelegate() {}
-
  private:
   DISALLOW_COPY_AND_ASSIGN(DelayingDownloadManagerDelegate);
 };
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyTabDestroy) {
   GURL url = NavigateToMockURL("a");
-  DownloadManager* manager(GetDownloadManager());
-  scoped_refptr<DelayingDownloadManagerDelegate> delaying_delegate(
+  scoped_ptr<DelayingDownloadManagerDelegate> delaying_delegate(
       new DelayingDownloadManagerDelegate(browser()->profile()));
-  delaying_delegate->SetNextId(content::DownloadItem::kInvalidId + 1);
-  manager->SetDelegate(delaying_delegate.get());
+  delaying_delegate->GetDownloadIdReceiverCallback().Run(
+      content::DownloadItem::kInvalidId + 1);
+  DownloadServiceFactory::GetForBrowserContext(browser()->profile())->
+      SetDownloadManagerDelegateForTesting(
+          delaying_delegate.PassAs<ChromeDownloadManagerDelegate>());
+  DownloadManager* manager(GetDownloadManager());
   std::vector<DownloadItem*> downloads;
   manager->GetAllDownloads(&downloads);
   ASSERT_EQ(0u, downloads.size());
@@ -487,8 +489,6 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnlyTabDestroy) {
 
   EXPECT_FALSE(base::PathExists(full_file_name));
   EXPECT_FALSE(base::PathExists(dir));
-
-  manager->SetDelegate(NULL);
 }
 
 // Disabled on Windows due to flakiness. http://crbug.com/162323