DownloadTestObserver should know when the DownloadManager was shutdown

content/public/test/download_test_observer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/public/test/download_test_observer.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
-namespace {
-
-// These functions take scoped_refptr's to DownloadManager because they
-// are posted to message queues, and hence may execute arbitrarily after
-// their actual posting.  Once posted, there is no connection between
-// these routines and the DownloadTestObserver class from which
-// they came, so the DownloadTestObserver's reference to the
-// DownloadManager cannot be counted on to keep the DownloadManager around.
-
-// Fake user click on "Accept".
-void AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager,
-                             int32 download_id) {
-  DownloadItem* download = download_manager->GetDownload(download_id);
-  if (download && (download->GetState() == DownloadItem::IN_PROGRESS))
-    download->DangerousDownloadValidated();
-}
-
-// Fake user click on "Deny".
-void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager,
-                           int32 download_id) {
-  DownloadItem* download = download_manager->GetDownload(download_id);
-  if (download && (download->GetState() == DownloadItem::IN_PROGRESS)) {
-    download->Cancel(true);
-    download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
-  }
-}
-
-}  // namespace
-
 DownloadUpdatedObserver::DownloadUpdatedObserver(
     DownloadItem* item, DownloadUpdatedObserver::EventFilter filter)
     : item_(item),
       filter_(filter),
       waiting_(false),
       event_seen_(false) {
   item->AddObserver(this);
 }
 
 DownloadUpdatedObserver::~DownloadUpdatedObserver() {
@@ skipping 30 matching lines @@
 }
 
 DownloadTestObserver::DownloadTestObserver(
     DownloadManager* download_manager,
     size_t wait_count,
     DangerousDownloadAction dangerous_download_action)
     : download_manager_(download_manager),
       wait_count_(wait_count),
       finished_downloads_at_construction_(0),
       waiting_(false),
-      dangerous_download_action_(dangerous_download_action) {
+      dangerous_download_action_(dangerous_download_action),
+      weak_factory_(this) {
 }
 
 DownloadTestObserver::~DownloadTestObserver() {
   for (DownloadSet::iterator it = downloads_observed_.begin();
        it != downloads_observed_.end(); ++it)
     (*it)->RemoveObserver(this);
 
-  download_manager_->RemoveObserver(this);
+  if (download_manager_)
+    download_manager_->RemoveObserver(this);
 }
 
 void DownloadTestObserver::Init() {
   download_manager_->AddObserver(this);
   std::vector<DownloadItem*> downloads;
   download_manager_->GetAllDownloads(&downloads);
   for (std::vector<DownloadItem*>::iterator it = downloads.begin();
        it != downloads.end(); ++it) {
     OnDownloadCreated(download_manager_, *it);
   }
   finished_downloads_at_construction_ = finished_downloads_.size();
   states_observed_.clear();
 }
 
+void DownloadTestObserver::ManagerGoingDown(DownloadManager* manager) {
+  ASSERT_TRUE(manager == download_manager_);

[benjhayden, 2013/05/21 18:18:58]

Please use CHECK_EQ instead.

+  download_manager_ = NULL;
+  SignalIfFinished();
+}
+
 void DownloadTestObserver::WaitForFinished() {
   if (!IsFinished()) {
     waiting_ = true;
     RunMessageLoop();
     waiting_ = false;
   }
 }
 
 bool DownloadTestObserver::IsFinished() const {
   return (finished_downloads_.size() - finished_downloads_at_construction_ >=
-          wait_count_);
+          wait_count_) || (download_manager_ == NULL);
 }
 
 void DownloadTestObserver::OnDownloadCreated(
     DownloadManager* manager,
     DownloadItem* item) {
   // NOTE: This method is called both by DownloadManager when a download is
   // created as well as in DownloadTestObserver::Init() for downloads that
   // existed before |this| was created.
   OnDownloadUpdated(item);
   DownloadSet::const_iterator finished_it(finished_downloads_.find(item));
@@ skipping 20 matching lines @@
 
     // Calling DangerousDownloadValidated() at this point will
     // cause the download to be completed twice.  Do what the real UI
     // code does: make the call as a delayed task.
     switch (dangerous_download_action_) {
       case ON_DANGEROUS_DOWNLOAD_ACCEPT:
         // Fake user click on "Accept".  Delay the actual click, as the
         // real UI would.
         BrowserThread::PostTask(
             BrowserThread::UI, FROM_HERE,
-            base::Bind(&AcceptDangerousDownload, download_manager_,
+            base::Bind(&DownloadTestObserver::AcceptDangerousDownload,
+                       weak_factory_.GetWeakPtr(),
                        download->GetId()));
         break;
 
       case ON_DANGEROUS_DOWNLOAD_DENY:
         // Fake a user click on "Deny".  Delay the actual click, as the
         // real UI would.
         BrowserThread::PostTask(
             BrowserThread::UI, FROM_HERE,
-            base::Bind(&DenyDangerousDownload, download_manager_,
+            base::Bind(&DownloadTestObserver::DenyDangerousDownload,
+                       weak_factory_.GetWeakPtr(),
                        download->GetId()));
         break;
 
       case ON_DANGEROUS_DOWNLOAD_FAIL:
         ADD_FAILURE() << "Unexpected dangerous download item.";
         break;
 
       case ON_DANGEROUS_DOWNLOAD_IGNORE:
         break;
 
@@ skipping 33 matching lines @@
   states_observed_[download->GetState()]++;  // Initializes to 0 the first time.
 
   SignalIfFinished();
 }
 
 void DownloadTestObserver::SignalIfFinished() {
   if (waiting_ && IsFinished())
     base::MessageLoopForUI::current()->Quit();
 }
 
+void DownloadTestObserver::AcceptDangerousDownload(int32 download_id) {
+  // Download manager was shutdown before the UI thread could accept the
+  // download.
+  if (!download_manager_)
+    return;
+  DownloadItem* download = download_manager_->GetDownload(download_id);
+  if (download && (download->GetState() == DownloadItem::IN_PROGRESS))
+    download->DangerousDownloadValidated();
+}
+
+void DownloadTestObserver::DenyDangerousDownload(int32 download_id) {
+  // Download manager was shutdown before the UI thread could deny the
+  // download.
+  if (!download_manager_)
+    return;
+  DownloadItem* download = download_manager_->GetDownload(download_id);
+  if (download && (download->GetState() == DownloadItem::IN_PROGRESS)) {
+    download->Cancel(true);
+    download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
+  }
+}
+
 DownloadTestObserverTerminal::DownloadTestObserverTerminal(
     DownloadManager* download_manager,
     size_t wait_count,
     DangerousDownloadAction dangerous_download_action)
         : DownloadTestObserver(download_manager,
                                wait_count,
                                dangerous_download_action) {
   // You can't rely on overriden virtual functions in a base class constructor;
   // the virtual function table hasn't been set up yet.  So, we have to do any
   // work that depends on those functions in the derived class constructor
@@ skipping 171 matching lines @@
     base::MessageLoopForUI::current()->Quit();
 }
 
 const DownloadUrlParameters::OnStartedCallback
     DownloadTestItemCreationObserver::callback() {
   return base::Bind(
       &DownloadTestItemCreationObserver::DownloadItemCreationCallback, this);
 }
 
 }  // namespace content