Fix DCHECK when shutting down safe browsing DownloadMetadataManager.

Chrome announces that a DownloadManager is going down before it destroys
its items. This change makes the unit test behave more like chrome does
and makes the metadata manager handle this shutdown order properly.

BUG=389123, 386915, 389643
R=robertshield@chromium.org

Review URL: https://codereview.chromium.org/703463006

Cr-Commit-Position: refs/heads/master@{#302746}

chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.cc

@@ -293,8 +293,12 @@ DownloadMetadataManager::DownloadMetadataManager(
 }
 
 DownloadMetadataManager::~DownloadMetadataManager() {
-  // All download managers must have gone down prior to this.
-  DCHECK(contexts_.empty());
+  // Destruction may have taken place before managers have gone down.
+  for (const auto& value : contexts_) {
+    value.first->RemoveObserver(this);
+    value.second->Detach();
+  }
+  contexts_.clear();
 }
 
 void DownloadMetadataManager::AddDownloadManager(
@@ -505,11 +509,17 @@ void DownloadMetadataManager::ManagerContext::OnDownloadDestroyed(
 }
 
 DownloadMetadataManager::ManagerContext::~ManagerContext() {
-  // All downloads must have been destroyed prior to deleting the context and
-  // all recorded operations and callbacks must have been handled.
+  // A context should not be deleted while waiting for a load to complete.
   DCHECK(pending_items_.empty());
-  DCHECK_EQ(item_, static_cast<content::DownloadItem*>(nullptr));
   DCHECK(get_details_callbacks_.empty());
+
+  // The context may have detached while still observing the item of interest
+  // since a DownloadManager announces that it's going down before it destroyes
+  // its items.
+  if (item_) {
+    item_->RemoveObserver(this);
+    item_ = nullptr;
+  }
 }
 
 void DownloadMetadataManager::ManagerContext::ClearPendingItems() {

chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc

@@ -180,6 +180,14 @@ class DownloadMetadataManagerTestBase : public ::testing::Test {
   void ShutdownDownloadManager() {
     if (dm_observer_) {
       dm_observer_->ManagerGoingDown(&download_manager_);
+      // Note: these calls may result in "Uninteresting mock function call"
+      // warnings as a result of MockDownloadItem invoking observers in its
+      // dtor. This happens after the NiceMock wrapper has removed its niceness
+      // hook. These can safely be ignored, as they are entirely expected. The
+      // values specified by ON_CALL invocations in AddDownloadItems are
+      // returned as desired.
+      other_item_.reset();
+      test_item_.reset();
       dm_observer_ = nullptr;
     }
   }
@@ -208,13 +216,6 @@ class DownloadMetadataManagerTestBase : public ::testing::Test {
     dm_observer_->OnDownloadCreated(&download_manager_, other_item_.get());
   }
 
-  // Destroyes the DownloadItems added to the manager. Safe to call any number
-  // of times.
-  void DestroyDownloadItems() {
-    other_item_.reset();
-    test_item_.reset();
-  }
-
   content::TestBrowserThreadBundle thread_bundle_;
   NiceMock<MockDownloadMetadataManager> manager_;
   TestingProfile profile_;
@@ -329,17 +330,14 @@ TEST_P(GetDetailsTest, GetDownloadDetails) {
   // Fire in the hole!
   manager_.GetDownloadDetails(&profile_, details_getter.GetCallback());
 
-  // Destroy download items and shutdown the download manager, if relevant.
-  if (early_shutdown_) {
-    DestroyDownloadItems();
+  // Shutdown the download manager, if relevant.
+  if (early_shutdown_)
     ShutdownDownloadManager();
-  }
 
   // Allow the read task and the response callback to run.
   RunAllTasks();
 
-  // Destroy download items and shutdown the download manager, if relevant.
-  DestroyDownloadItems();
+  // Shutdown the download manager, if relevant.
   ShutdownDownloadManager();
 }
 
@@ -464,7 +462,6 @@ TEST_P(SetRequestTest, SetRequest) {
   }
   manager_.GetDownloadDetails(&profile_, details_getter.GetCallback());
 
-  DestroyDownloadItems();
   ShutdownDownloadManager();
 
   scoped_ptr<DownloadMetadata> metadata(ReadTestMetadataFile());