Fix DCHECK when shutting down safe browsing DownloadMetadataManager.

chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc

 // Copyright 2014 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 "chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 162 matching lines @@
     // Capture the metadata manager's observer on the download manager.
     EXPECT_CALL(download_manager_, AddObserver(&manager_))
         .WillOnce(SaveArg<0>(&dm_observer_));
     manager_.AddDownloadManager(&download_manager_);
   }
 
   // Shuts down the DownloadManager. Safe to call any number of times.
   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.

[robertshield, 2014/11/05 01:04:50]

I don't know enough gmock to know whether this is work-aroundable. Can
expectations be added for these calls? MAD claims knowledge of such things.

[grt (UTC plus 2), 2014/11/05 01:25:35]

The right thing when you don't want an expectation (something that would make
the test fail) but rather to specify the mock's behavior is to use ON_CALL
(https://code.google.com/p/googlemock/wiki/CookBook#Setting_the_Default_Action...)
as on line 200. This returns the desired value and will emit the "Uninteresting"
warning as noted in the comment. The right way to suppress that warning is to
use a NiceMock
(https://code.google.com/p/googlemock/wiki/CookBook#The_Nice,_the_Strict,_and_...),
which means "don't let me know about calls that don't have expectations." The
problem, as I perhaps poorly explained in the comment here, is that
MockDownloadItem's dtor runs callbacks which will invoke methods on the item.
These work just fine, but the NiceMock's dtor (which removes the "suppress
unintersting calls" hook for this mock) has already run at that point, so the
uninteresting calls are logged.

+      other_item_.reset();
+      test_item_.reset();
       dm_observer_ = nullptr;
     }
   }
 
   // Adds two test DownloadItems to the DownloadManager.
   void AddDownloadItems() {
     ASSERT_NE(nullptr, dm_observer_);
     // Add the item under test.
     test_item_.reset(new NiceMock<content::MockDownloadItem>);
     ON_CALL(*test_item_, GetId())
         .WillByDefault(Return(kTestDownloadId));
     ON_CALL(*test_item_, GetBrowserContext())
         .WillByDefault(Return(&profile_));
     ON_CALL(*test_item_, GetEndTime())
         .WillByDefault(Return(base::Time::FromJsTime(kTestDownloadEndTimeMs)));
     dm_observer_->OnDownloadCreated(&download_manager_, test_item_.get());
 
     // Add another item.
     other_item_.reset(new NiceMock<content::MockDownloadItem>);
     ON_CALL(*other_item_, GetId())
         .WillByDefault(Return(kOtherDownloadId));
     ON_CALL(*other_item_, GetBrowserContext())
         .WillByDefault(Return(&profile_));
     ON_CALL(*test_item_, GetEndTime())
         .WillByDefault(Return(base::Time::FromJsTime(kTestDownloadEndTimeMs)));
     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_;
   NiceMock<content::MockDownloadManager> download_manager_;
   scoped_ptr<content::MockDownloadItem> test_item_;
   scoped_ptr<content::MockDownloadItem> other_item_;
   content::DownloadManager::Observer* dm_observer_;
 };
 
 // A parameterized test that exercises GetDownloadDetails. The parameters
@@ skipping 94 matching lines @@
                             ResultOf(GetDetailsOpenTime, Eq(0)))));
     }
   } else {
     // No file on disk, so expect that the callback is invoked with null.
     EXPECT_CALL(details_getter, OnDownloadDetails(IsNull()));
   }
 
   // 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();
 }
 
 INSTANTIATE_TEST_CASE_P(
     DownloadMetadataManager,
     GetDetailsTest,
     testing::Combine(
         testing::Values("absent", "present"),
         testing::Values("not_managed", "managed"),
         testing::Values("not_created", "created", "opened", "removed"),
@@ skipping 104 matching lines @@
     // Expect that the callback is invoked with details for this item.
     EXPECT_CALL(
         details_getter,
         OnDownloadDetails(ResultOf(GetDetailsDownloadUrl, StrEq(kNewUrl))));
   } else {
     // Expect that the callback is invoked with null to clear stale metadata.
     EXPECT_CALL(details_getter, OnDownloadDetails(IsNull()));
   }
   manager_.GetDownloadDetails(&profile_, details_getter.GetCallback());
 
-  DestroyDownloadItems();
   ShutdownDownloadManager();
 
   scoped_ptr<DownloadMetadata> metadata(ReadTestMetadataFile());
   if (set_request_) {
     // Expect that the file contains metadata for the download.
     ASSERT_TRUE(metadata);
     EXPECT_EQ(kTestDownloadId, metadata->download_id());
     EXPECT_STREQ(kNewUrl, metadata->download().download().url().c_str());
   } else {
     // Expect that the file is not present.
     ASSERT_FALSE(metadata);
   }
 }
 
 INSTANTIATE_TEST_CASE_P(
     DownloadMetadataManager,
     SetRequestTest,
     testing::Combine(testing::Values("absent", "this", "other", "unknown"),
                      testing::Values("none", "pending"),
                      testing::Values("none", "pending"),
                      testing::Values("waiting", "loaded"),
                      testing::Values("clear", "set")));
 
 }  // namespace safe_browsing