[Downloads] Enforce state transition integrity and state invariants.

content/browser/download/download_item_impl_unittest.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/browser/download/download_item_impl.h"
 
 #include <stdint.h>
+
+#include <iterator>
+#include <queue>
 #include <utility>
 
 #include "base/callback.h"
 #include "base/feature_list.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/threading/thread.h"
 #include "content/browser/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/mock_download_file.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_destination_observer.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/common/content_features.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/test_browser_context.h"
-#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::testing::_;
+using ::testing::DoAll;
 using ::testing::NiceMock;
 using ::testing::Property;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::StrictMock;
+using ::testing::WithArg;
+using ::testing::_;
 
 const int kDownloadChunkSize = 1000;
 const int kDownloadSpeed = 1000;
-const base::FilePath::CharType kDummyPath[] = FILE_PATH_LITERAL("/testpath");
+const base::FilePath::CharType kDummyTargetPath[] =
+    FILE_PATH_LITERAL("/testpath");
+const base::FilePath::CharType kDummyIntermediatePath[] =
+    FILE_PATH_LITERAL("/testpathx");
 
 namespace content {
 
 namespace {
 
 class MockDelegate : public DownloadItemImplDelegate {
  public:
   MockDelegate() : DownloadItemImplDelegate() {
     SetDefaultExpectations();
   }
@@ skipping 101 matching lines @@
   void OnDownloadOpened(DownloadItem* download) override {
     DVLOG(20) << " " << __FUNCTION__
               << " download = " << download->DebugString(false);
   }
 
   void OnDownloadDestroyed(DownloadItem* download) override {
     DVLOG(20) << " " << __FUNCTION__
               << " download = " << download->DebugString(false);
     destroyed_ = true;
     item_->RemoveObserver(this);
-    item_ = NULL;
+    item_ = nullptr;
   }
 
   DownloadItem* item_;
   DownloadItem::DownloadState last_state_;
   bool removed_;
   bool destroyed_;
   bool updated_;
   int interrupt_count_;
   int resume_count_;
 };
 
 // Schedules a task to invoke the RenameCompletionCallback with |new_path| on
 // the UI thread. Should only be used as the action for
 // MockDownloadFile::Rename as follows:
 //   EXPECT_CALL(download_file, Rename*(_,_))
 //       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
 //                                        new_path));
 ACTION_P2(ScheduleRenameCallback, interrupt_reason, new_path) {
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(arg1, interrupt_reason, new_path));
 }
 
+// Schedules a task to invoke a callback that's bound to the specified
+// parameter.
+// E.g.:
+//
+//   EXPECT_CALL(foo, Bar(1, _))
+//     .WithArg<1>(InvokeCallbackWithParam(0));
+//
+//   .. will invoke the second argument to Bar with 0 as the parameter.
+ACTION_P(InvokeCallbackWithParam, param) {
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(arg0, param));
+}
+
+ACTION_P(InvokeClosure, closure) {
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, closure);
+}
+
 }  // namespace
 
 class DownloadItemTest : public testing::Test {
  public:
-  DownloadItemTest()
-      : ui_thread_(BrowserThread::UI, &loop_),
-        file_thread_(BrowserThread::FILE, &loop_),
-        delegate_() {
-  }
-
-  ~DownloadItemTest() {
-  }
-
-  virtual void SetUp() {
+  DownloadItemTest() {
     base::FeatureList::ClearInstanceForTesting();
     scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
     feature_list->InitializeFromCommandLine(features::kDownloadResumption.name,
                                             std::string());
     base::FeatureList::SetInstance(std::move(feature_list));
   }
 
-  virtual void TearDown() {
-    ui_thread_.DeprecatedGetThreadObject()->message_loop()->RunUntilIdle();
+  ~DownloadItemTest() {
+    RunAllPendingInMessageLoops();
     STLDeleteElements(&allocated_downloads_);
   }
 
+  DownloadItemImpl* CreateDownloadItemWithCreateInfo(
+      scoped_ptr<DownloadCreateInfo> info) {
+    DownloadItemImpl* download = new DownloadItemImpl(
+        &delegate_, next_download_id_++, *(info.get()), net::BoundNetLog());
+    allocated_downloads_.insert(download);
+    return download;
+  }
+
   // This class keeps ownership of the created download item; it will
   // be torn down at the end of the test unless DestroyDownloadItem is
   // called.
   DownloadItemImpl* CreateDownloadItem() {
     scoped_ptr<DownloadCreateInfo> info;
 
     info.reset(new DownloadCreateInfo());
     info->save_info = scoped_ptr<DownloadSaveInfo>(new DownloadSaveInfo());
     info->save_info->prompt_for_save_location = false;
-    info->url_chain.push_back(GURL());
+    info->url_chain.push_back(GURL("http://example.com/download"));
     info->etag = "SomethingToSatisfyResumption";
 
     return CreateDownloadItemWithCreateInfo(std::move(info));
   }
 
-  DownloadItemImpl* CreateDownloadItemWithCreateInfo(
-      scoped_ptr<DownloadCreateInfo> info) {
-    DownloadItemImpl* download = new DownloadItemImpl(
-        &delegate_, next_download_id_++, *(info.get()), net::BoundNetLog());
-    allocated_downloads_.insert(download);
-    return download;
-  }
-
   // Add DownloadFile to DownloadItem
-  MockDownloadFile* AddDownloadFileToDownloadItem(
+  MockDownloadFile* CallDownloadItemStart(
       DownloadItemImpl* item,
-      DownloadItemImplDelegate::DownloadTargetCallback *callback) {
+      DownloadItemImplDelegate::DownloadTargetCallback* callback) {
     MockDownloadFile* mock_download_file(new StrictMock<MockDownloadFile>);
     scoped_ptr<DownloadFile> download_file(mock_download_file);
     EXPECT_CALL(*mock_download_file, Initialize(_));
     if (callback) {
       // Save the callback.
       EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _))
           .WillOnce(SaveArg<1>(callback));
     } else {
       // Drop it on the floor.
       EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _));
     }
 
     scoped_ptr<DownloadRequestHandleInterface> request_handle(
         new NiceMock<MockRequestHandle>);
     item->Start(std::move(download_file), std::move(request_handle));
-    loop_.RunUntilIdle();
+    RunAllPendingInMessageLoops();
 
     // So that we don't have a function writing to a stack variable
     // lying around if the above failed.
     mock_delegate()->VerifyAndClearExpectations();
     EXPECT_CALL(*mock_delegate(), AssertStateConsistent(_))
         .WillRepeatedly(Return());
     EXPECT_CALL(*mock_delegate(), ShouldOpenFileBasedOnExtension(_))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(_, _))
         .WillRepeatedly(Return(true));
 
     return mock_download_file;
   }
 
   // Perform the intermediate rename for |item|. The target path for the
-  // download will be set to kDummyPath. Returns the MockDownloadFile* that was
+  // download will be set to kDummyTargetPath. Returns the MockDownloadFile*
+  // that was
   // added to the DownloadItem.
   MockDownloadFile* DoIntermediateRename(DownloadItemImpl* item,
                                          DownloadDangerType danger_type) {
     EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
     EXPECT_TRUE(item->GetTargetFilePath().empty());
     DownloadItemImplDelegate::DownloadTargetCallback callback;
-    MockDownloadFile* download_file =
-        AddDownloadFileToDownloadItem(item, &callback);
-    base::FilePath target_path(kDummyPath);
-    base::FilePath intermediate_path(
-        target_path.InsertBeforeExtensionASCII("x"));
+    MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
+    base::FilePath target_path(kDummyTargetPath);
+    base::FilePath intermediate_path(kDummyIntermediatePath);
     EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
         .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                          intermediate_path));
     callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                  danger_type, intermediate_path);
     RunAllPendingInMessageLoops();
     return download_file;
   }
 
+  void DoDestinationComplete(DownloadItemImpl* item,
+                             MockDownloadFile* download_file) {
+    EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(_, _))
+        .WillOnce(Return(true));
+    base::FilePath final_path(kDummyTargetPath);
+    EXPECT_CALL(*download_file, RenameAndAnnotate(_, _))
+        .WillOnce(
+            ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE, final_path));
+    EXPECT_CALL(*download_file, FullPath())
+        .WillRepeatedly(Return(base::FilePath(kDummyTargetPath)));
+    EXPECT_CALL(*download_file, Detach());
+
+    item->DestinationObserverAsWeakPtr()->DestinationCompleted("");
+    RunAllPendingInMessageLoops();
+  }
+
   // Cleanup a download item (specifically get rid of the DownloadFile on it).
   // The item must be in the expected state.
   void CleanupItem(DownloadItemImpl* item,
                    MockDownloadFile* download_file,
                    DownloadItem::DownloadState expected_state) {
     EXPECT_EQ(expected_state, item->GetState());
 
     if (expected_state == DownloadItem::IN_PROGRESS) {
       if (download_file)
         EXPECT_CALL(*download_file, Cancel());
       item->Cancel(true);
-      loop_.RunUntilIdle();
+      RunAllPendingInMessageLoops();
     }
   }
 
   // Destroy a previously created download item.
   void DestroyDownloadItem(DownloadItem* item) {
     allocated_downloads_.erase(item);
     delete item;
   }
 
-  void RunAllPendingInMessageLoops() {
-    loop_.RunUntilIdle();
-  }
+  void RunAllPendingInMessageLoops() { base::RunLoop().RunUntilIdle(); }
 
   MockDelegate* mock_delegate() {
     return &delegate_;
   }
 
   void OnDownloadFileAcquired(base::FilePath* return_path,
                               const base::FilePath& path) {
     *return_path = path;
   }
 
  private:
   int next_download_id_ = DownloadItem::kInvalidId + 1;
-  base::MessageLoopForUI loop_;
-  TestBrowserThread ui_thread_;    // UI thread
-  TestBrowserThread file_thread_;  // FILE thread
   StrictMock<MockDelegate> delegate_;
   std::set<DownloadItem*> allocated_downloads_;
+  TestBrowserThreadBundle thread_bundle_;
 };
 
 // Tests to ensure calls that change a DownloadItem generate an update to
 // observers.
 // State changing functions not tested:
 //  void OpenDownload();
 //  void ShowDownloadInShell();
 //  void CompleteDelayedDownload();
 //  set_* mutators
 
 TEST_F(DownloadItemTest, NotificationAfterUpdate) {
   DownloadItemImpl* item = CreateDownloadItem();
   TestDownloadItemObserver observer(item);
 
   item->DestinationUpdate(kDownloadChunkSize, kDownloadSpeed, std::string());
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
   EXPECT_EQ(kDownloadSpeed, item->CurrentSpeed());
 }
 
 TEST_F(DownloadItemTest, NotificationAfterCancel) {
   DownloadItemImpl* user_cancel = CreateDownloadItem();
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(user_cancel, NULL);
+  MockDownloadFile* download_file = CallDownloadItemStart(user_cancel, nullptr);
   EXPECT_CALL(*download_file, Cancel());
   TestDownloadItemObserver observer1(user_cancel);
 
   user_cancel->Cancel(true);
   ASSERT_TRUE(observer1.CheckAndResetDownloadUpdated());
 
   DownloadItemImpl* system_cancel = CreateDownloadItem();
-  download_file = AddDownloadFileToDownloadItem(system_cancel, NULL);
+  download_file = CallDownloadItemStart(system_cancel, nullptr);
   EXPECT_CALL(*download_file, Cancel());
   TestDownloadItemObserver observer2(system_cancel);
 
   system_cancel->Cancel(false);
   ASSERT_TRUE(observer2.CheckAndResetDownloadUpdated());
 }
 
 TEST_F(DownloadItemTest, NotificationAfterComplete) {
   DownloadItemImpl* item = CreateDownloadItem();
   TestDownloadItemObserver observer(item);
-
-  item->OnAllDataSaved(DownloadItem::kEmptyFileHash);
+  MockDownloadFile* download_file =
+      DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
-
-  item->MarkAsComplete();
+  DoDestinationComplete(item, download_file);
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
 }
 
 TEST_F(DownloadItemTest, NotificationAfterDownloadedFileRemoved) {
   DownloadItemImpl* item = CreateDownloadItem();
   TestDownloadItemObserver observer(item);
 
   item->OnDownloadedFileRemoved();
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
 }
@@ skipping 77 matching lines @@
 // Check we do correct cleanup for RESUME_MODE_INVALID interrupts.
 TEST_F(DownloadItemTest, UnresumableInterrupt) {
   DownloadItemImpl* item = CreateDownloadItem();
   TestDownloadItemObserver observer(item);
   MockDownloadFile* download_file =
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   // Fail final rename with unresumable reason.
   EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(item, _))
       .WillOnce(Return(true));
-  EXPECT_CALL(*download_file, RenameAndAnnotate(base::FilePath(kDummyPath), _))
+  EXPECT_CALL(*download_file,
+              RenameAndAnnotate(base::FilePath(kDummyTargetPath), _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath()));
   EXPECT_CALL(*download_file, Cancel());
 
   // Complete download to trigger final rename.
   item->DestinationObserverAsWeakPtr()->DestinationCompleted(std::string());
   RunAllPendingInMessageLoops();
 
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
   // Should not try to auto-resume.
   ASSERT_EQ(1, observer.interrupt_count());
   ASSERT_EQ(0, observer.resume_count());
 
   CleanupItem(item, nullptr, DownloadItem::INTERRUPTED);
 }
 
 TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
   TestBrowserContext test_browser_context;
   DownloadItemImpl* item = CreateDownloadItem();
   base::WeakPtr<DownloadDestinationObserver> as_observer(
       item->DestinationObserverAsWeakPtr());
   TestDownloadItemObserver observer(item);
-  MockDownloadFile* mock_download_file(NULL);
+  MockDownloadFile* mock_download_file(nullptr);
   scoped_ptr<DownloadFile> download_file;
-  MockRequestHandle* mock_request_handle(NULL);
+  MockRequestHandle* mock_request_handle(nullptr);
   scoped_ptr<DownloadRequestHandleInterface> request_handle;
   DownloadItemImplDelegate::DownloadTargetCallback callback;
 
   EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _))
       .WillRepeatedly(SaveArg<1>(&callback));
   EXPECT_CALL(*mock_delegate(), GetBrowserContext())
       .WillRepeatedly(Return(&test_browser_context));
   EXPECT_CALL(*mock_delegate(), MockResumeInterruptedDownload(_, _))
       .Times(DownloadItemImpl::kMaxAutoResumeAttempts);
   for (int i = 0; i < (DownloadItemImpl::kMaxAutoResumeAttempts + 1); ++i) {
     DVLOG(20) << "Loop iteration " << i;
 
     mock_download_file = new NiceMock<MockDownloadFile>;
     download_file.reset(mock_download_file);
     mock_request_handle = new NiceMock<MockRequestHandle>;
     request_handle.reset(mock_request_handle);
 
     ON_CALL(*mock_download_file, FullPath())
         .WillByDefault(Return(base::FilePath()));
 
-    // Copied key parts of DoIntermediateRename & AddDownloadFileToDownloadItem
+    // Copied key parts of DoIntermediateRename & CallDownloadItemStart
     // to allow for holding onto the request handle.
     item->Start(std::move(download_file), std::move(request_handle));
     RunAllPendingInMessageLoops();
+
+    base::FilePath target_path(kDummyTargetPath);
+    base::FilePath intermediate_path(kDummyIntermediatePath);
     if (i == 0) {
-      // Target determination is only done the first time through.
-      base::FilePath target_path(kDummyPath);
-      base::FilePath intermediate_path(
-          target_path.InsertBeforeExtensionASCII("x"));
+      // RenameAndUniquify is only called the first time. In all the subsequent
+      // iterations, the intermediate file already has the correct name, hence
+      // no rename is necessary.
       EXPECT_CALL(*mock_download_file, RenameAndUniquify(intermediate_path, _))
           .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                            intermediate_path));
-      callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                   DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
-      RunAllPendingInMessageLoops();
     }
+    callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                 DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
+    RunAllPendingInMessageLoops();
 
     // Use a continuable interrupt.
     item->DestinationObserverAsWeakPtr()->DestinationError(
         DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR);
 
     ::testing::Mock::VerifyAndClearExpectations(mock_download_file);
   }
 
+  EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_EQ(1, observer.interrupt_count());
   CleanupItem(item, nullptr, DownloadItem::INTERRUPTED);
 }
 
 // Test that resumption uses the final URL in a URL chain when resuming.
 TEST_F(DownloadItemTest, ResumeUsingFinalURL) {
   TestBrowserContext test_browser_context;
   scoped_ptr<DownloadCreateInfo> create_info(new DownloadCreateInfo);
   create_info->save_info = scoped_ptr<DownloadSaveInfo>(new DownloadSaveInfo());
   create_info->save_info->prompt_for_save_location = false;
@@ skipping 27 matching lines @@
   // ResumeInterruptedDownload() being called is sufficient for verifying that
   // the resumption was triggered.
   RunAllPendingInMessageLoops();
 
   // The download is currently in RESUMING_INTERNAL, which maps to IN_PROGRESS.
   CleanupItem(item, nullptr, DownloadItem::IN_PROGRESS);
 }
 
 TEST_F(DownloadItemTest, NotificationAfterRemove) {
   DownloadItemImpl* item = CreateDownloadItem();
-  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  MockDownloadFile* download_file = CallDownloadItemStart(item, nullptr);
   EXPECT_CALL(*download_file, Cancel());
   EXPECT_CALL(*mock_delegate(), DownloadRemoved(_));
   TestDownloadItemObserver observer(item);
 
   item->Remove();
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
   ASSERT_TRUE(observer.download_removed());
 }
 
 TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
   // Setting to NOT_DANGEROUS does not trigger a notification.
   DownloadItemImpl* safe_item = CreateDownloadItem();
+  MockDownloadFile* download_file =
+      DoIntermediateRename(safe_item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
   TestDownloadItemObserver safe_observer(safe_item);
 
   safe_item->OnAllDataSaved(std::string());
   EXPECT_TRUE(safe_observer.CheckAndResetDownloadUpdated());
   safe_item->OnContentCheckCompleted(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
   EXPECT_TRUE(safe_observer.CheckAndResetDownloadUpdated());
+  CleanupItem(safe_item, download_file, DownloadItem::IN_PROGRESS);
 
   // Setting to unsafe url or unsafe file should trigger a notification.
   DownloadItemImpl* unsafeurl_item =
       CreateDownloadItem();
+  download_file =
+      DoIntermediateRename(unsafeurl_item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
   TestDownloadItemObserver unsafeurl_observer(unsafeurl_item);
 
   unsafeurl_item->OnAllDataSaved(std::string());
   EXPECT_TRUE(unsafeurl_observer.CheckAndResetDownloadUpdated());
   unsafeurl_item->OnContentCheckCompleted(DOWNLOAD_DANGER_TYPE_DANGEROUS_URL);
   EXPECT_TRUE(unsafeurl_observer.CheckAndResetDownloadUpdated());
 
+  EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(_, _))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*download_file, RenameAndAnnotate(_, _));
   unsafeurl_item->ValidateDangerousDownload();
   EXPECT_TRUE(unsafeurl_observer.CheckAndResetDownloadUpdated());
+  CleanupItem(unsafeurl_item, download_file, DownloadItem::IN_PROGRESS);
 
   DownloadItemImpl* unsafefile_item =
       CreateDownloadItem();
+  download_file =
+      DoIntermediateRename(unsafefile_item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
   TestDownloadItemObserver unsafefile_observer(unsafefile_item);
 
   unsafefile_item->OnAllDataSaved(std::string());
   EXPECT_TRUE(unsafefile_observer.CheckAndResetDownloadUpdated());
   unsafefile_item->OnContentCheckCompleted(DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE);
   EXPECT_TRUE(unsafefile_observer.CheckAndResetDownloadUpdated());
 
+  EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(_, _))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*download_file, RenameAndAnnotate(_, _));
   unsafefile_item->ValidateDangerousDownload();
   EXPECT_TRUE(unsafefile_observer.CheckAndResetDownloadUpdated());
+  CleanupItem(unsafefile_item, download_file, DownloadItem::IN_PROGRESS);
 }
 
 // DownloadItemImpl::OnDownloadTargetDetermined will schedule a task to run
 // DownloadFile::Rename(). Once the rename
 // completes, DownloadItemImpl receives a notification with the new file
 // name. Check that observers are updated when the new filename is available and
 // not before.
 TEST_F(DownloadItemTest, NotificationAfterOnDownloadTargetDetermined) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
   TestDownloadItemObserver observer(item);
-  base::FilePath target_path(kDummyPath);
+  base::FilePath target_path(kDummyTargetPath);
   base::FilePath intermediate_path(target_path.InsertBeforeExtensionASCII("x"));
   base::FilePath new_intermediate_path(
       target_path.InsertBeforeExtensionASCII("y"));
   EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                        new_intermediate_path));
 
   // Currently, a notification would be generated if the danger type is anything
   // other than NOT_DANGEROUS.
   callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
@@ skipping 27 matching lines @@
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
 
   RunAllPendingInMessageLoops();
 
   CleanupItem(item, mock_download_file, DownloadItem::IN_PROGRESS);
 }
 
 TEST_F(DownloadItemTest, DisplayName) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
-  base::FilePath target_path(base::FilePath(kDummyPath).AppendASCII("foo.bar"));
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
+  base::FilePath target_path(
+      base::FilePath(kDummyTargetPath).AppendASCII("foo.bar"));
   base::FilePath intermediate_path(target_path.InsertBeforeExtensionASCII("x"));
   EXPECT_EQ(FILE_PATH_LITERAL(""),
             item->GetFileNameToReportUser().value());
   EXPECT_CALL(*download_file, RenameAndUniquify(_, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                        intermediate_path));
   callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
   EXPECT_EQ(FILE_PATH_LITERAL("foo.bar"),
@@ skipping 16 matching lines @@
   item->Start(std::move(download_file), std::move(request_handle));
   RunAllPendingInMessageLoops();
 
   CleanupItem(item, mock_download_file, DownloadItem::IN_PROGRESS);
 }
 
 // Test that the delegate is invoked after the download file is renamed.
 TEST_F(DownloadItemTest, CallbackAfterRename) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
-  base::FilePath final_path(base::FilePath(kDummyPath).AppendASCII("foo.bar"));
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
+  base::FilePath final_path(
+      base::FilePath(kDummyTargetPath).AppendASCII("foo.bar"));
   base::FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
   base::FilePath new_intermediate_path(
       final_path.InsertBeforeExtensionASCII("y"));
   EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                        new_intermediate_path));
 
   callback.Run(final_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
@@ skipping 13 matching lines @@
   RunAllPendingInMessageLoops();
   ::testing::Mock::VerifyAndClearExpectations(download_file);
   mock_delegate()->VerifyAndClearExpectations();
 }
 
 // Test that the delegate is invoked after the download file is renamed and the
 // download item is in an interrupted state.
 TEST_F(DownloadItemTest, CallbackAfterInterruptedRename) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
-  base::FilePath final_path(base::FilePath(kDummyPath).AppendASCII("foo.bar"));
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
+  base::FilePath final_path(
+      base::FilePath(kDummyTargetPath).AppendASCII("foo.bar"));
   base::FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
   base::FilePath new_intermediate_path(
       final_path.InsertBeforeExtensionASCII("y"));
   EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,
                                        new_intermediate_path));
   EXPECT_CALL(*download_file, Cancel())
       .Times(1);
 
   callback.Run(final_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
@@ skipping 23 matching lines @@
   item->Cancel(true);
   EXPECT_EQ(DownloadItem::CANCELLED, item->GetState());
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_USER_CANCELED, item->GetLastReason());
 }
 
 // Destination errors that occur before the intermediate rename shouldn't cause
 // the download to be marked as interrupted until after the intermediate rename.
 TEST_F(DownloadItemTest, InterruptedBeforeIntermediateRename_Restart) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
   ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
 
-  base::FilePath final_path(base::FilePath(kDummyPath).AppendASCII("foo.bar"));
+  base::FilePath final_path(
+      base::FilePath(kDummyTargetPath).AppendASCII("foo.bar"));
   base::FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
   base::FilePath new_intermediate_path(
       final_path.InsertBeforeExtensionASCII("y"));
   EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                        new_intermediate_path));
   EXPECT_CALL(*download_file, Cancel())
       .Times(1);
 
   callback.Run(final_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
   // All the callbacks should have happened by now.
   ::testing::Mock::VerifyAndClearExpectations(download_file);
   mock_delegate()->VerifyAndClearExpectations();
   EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_TRUE(item->GetFullPath().empty());
   EXPECT_EQ(final_path, item->GetTargetFilePath());
 }
 
 // As above. But if the download can be resumed by continuing, then the
 // intermediate path should be retained when the download is interrupted after
 // the intermediate rename succeeds.
 TEST_F(DownloadItemTest, InterruptedBeforeIntermediateRename_Continue) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED);
   ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
 
-  base::FilePath final_path(base::FilePath(kDummyPath).AppendASCII("foo.bar"));
+  base::FilePath final_path(
+      base::FilePath(kDummyTargetPath).AppendASCII("foo.bar"));
   base::FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
   base::FilePath new_intermediate_path(
       final_path.InsertBeforeExtensionASCII("y"));
   EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                        new_intermediate_path));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath(new_intermediate_path)));
   EXPECT_CALL(*download_file, Detach());
 
   callback.Run(final_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
   // All the callbacks should have happened by now.
   ::testing::Mock::VerifyAndClearExpectations(download_file);
   mock_delegate()->VerifyAndClearExpectations();
   EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_EQ(new_intermediate_path, item->GetFullPath());
   EXPECT_EQ(final_path, item->GetTargetFilePath());
 }
 
 // As above. If the intermediate rename fails, then the interrupt reason should
 // be set to the destination error and the intermediate path should be empty.
 TEST_F(DownloadItemTest, InterruptedBeforeIntermediateRename_Failed) {
   DownloadItemImpl* item = CreateDownloadItem();
   DownloadItemImplDelegate::DownloadTargetCallback callback;
-  MockDownloadFile* download_file =
-      AddDownloadFileToDownloadItem(item, &callback);
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED);
   ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
 
-  base::FilePath final_path(base::FilePath(kDummyPath).AppendASCII("foo.bar"));
+  base::FilePath final_path(
+      base::FilePath(kDummyTargetPath).AppendASCII("foo.bar"));
   base::FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
   base::FilePath new_intermediate_path(
       final_path.InsertBeforeExtensionASCII("y"));
   EXPECT_CALL(*download_file, RenameAndUniquify(intermediate_path, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,
                                        new_intermediate_path));
   EXPECT_CALL(*download_file, Cancel())
       .Times(1);
 
   callback.Run(final_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
   RunAllPendingInMessageLoops();
   // All the callbacks should have happened by now.
   ::testing::Mock::VerifyAndClearExpectations(download_file);
   mock_delegate()->VerifyAndClearExpectations();
   EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED, item->GetLastReason());
   EXPECT_TRUE(item->GetFullPath().empty());
   EXPECT_EQ(final_path, item->GetTargetFilePath());
 }
 
 TEST_F(DownloadItemTest, Canceled) {
   DownloadItemImpl* item = CreateDownloadItem();
-  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  MockDownloadFile* download_file = CallDownloadItemStart(item, nullptr);
 
   // Confirm cancel sets state properly.
   EXPECT_CALL(*download_file, Cancel());
   item->Cancel(true);
   EXPECT_EQ(DownloadItem::CANCELLED, item->GetState());
 }
 
 TEST_F(DownloadItemTest, FileRemoved) {
   DownloadItemImpl* item = CreateDownloadItem();
 
@@ skipping 48 matching lines @@
       DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
   mock_delegate()->VerifyAndClearExpectations();
   EXPECT_TRUE(observer.CheckAndResetDownloadUpdated());
   EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
             item->GetLastReason());
 }
 
 TEST_F(DownloadItemTest, DestinationCompleted) {
   DownloadItemImpl* item = CreateDownloadItem();
+  MockDownloadFile* download_file = CallDownloadItemStart(item, nullptr);
   base::WeakPtr<DownloadDestinationObserver> as_observer(
       item->DestinationObserverAsWeakPtr());
   TestDownloadItemObserver observer(item);
 
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_EQ("", item->GetHash());
   EXPECT_EQ("", item->GetHashState());
   EXPECT_FALSE(item->AllDataSaved());
   EXPECT_FALSE(observer.CheckAndResetDownloadUpdated());
 
   as_observer->DestinationUpdate(10, 20, "deadbeef");
   EXPECT_TRUE(observer.CheckAndResetDownloadUpdated());
   EXPECT_FALSE(observer.CheckAndResetDownloadUpdated());  // Confirm reset.
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_EQ("", item->GetHash());
   EXPECT_EQ("deadbeef", item->GetHashState());
   EXPECT_FALSE(item->AllDataSaved());
 
   as_observer->DestinationCompleted("livebeef");
   mock_delegate()->VerifyAndClearExpectations();
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_TRUE(observer.CheckAndResetDownloadUpdated());
   EXPECT_EQ("livebeef", item->GetHash());
   EXPECT_EQ("", item->GetHashState());
   EXPECT_TRUE(item->AllDataSaved());
+
+  // Even though the DownloadItem receives a DestinationCompleted() event,
+  // target determination hasn't completed, hence the download item is stuck in
+  // TARGET_PENDING.
+  CleanupItem(item, download_file, DownloadItem::IN_PROGRESS);
 }
 
 TEST_F(DownloadItemTest, EnabledActionsForNormalDownload) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockDownloadFile* download_file =
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   // InProgress
   ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   ASSERT_FALSE(item->GetTargetFilePath().empty());
   EXPECT_TRUE(item->CanShowInFolder());
   EXPECT_TRUE(item->CanOpenDownload());
 
   // Complete
   EXPECT_CALL(*download_file, RenameAndAnnotate(_, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath(kDummyTargetPath)));
   EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   item->DestinationObserverAsWeakPtr()->DestinationCompleted(std::string());
   RunAllPendingInMessageLoops();
 
   ASSERT_EQ(DownloadItem::COMPLETE, item->GetState());
   EXPECT_TRUE(item->CanShowInFolder());
@@ skipping 11 matching lines @@
   ASSERT_FALSE(item->GetTargetFilePath().empty());
   ASSERT_TRUE(item->IsTemporary());
   EXPECT_FALSE(item->CanShowInFolder());
   EXPECT_FALSE(item->CanOpenDownload());
 
   // Complete Temporary
   EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, RenameAndAnnotate(_, _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath(kDummyTargetPath)));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   item->DestinationObserverAsWeakPtr()->DestinationCompleted(std::string());
   RunAllPendingInMessageLoops();
 
   ASSERT_EQ(DownloadItem::COMPLETE, item->GetState());
   EXPECT_FALSE(item->CanShowInFolder());
   EXPECT_FALSE(item->CanOpenDownload());
 }
 
 TEST_F(DownloadItemTest, EnabledActionsForInterruptedDownload) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockDownloadFile* download_file =
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   EXPECT_CALL(*download_file, Cancel());
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
   RunAllPendingInMessageLoops();
 
   ASSERT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   ASSERT_FALSE(item->GetTargetFilePath().empty());
   EXPECT_FALSE(item->CanShowInFolder());
-  EXPECT_FALSE(item->CanOpenDownload());
+  EXPECT_TRUE(item->CanOpenDownload());
 }
 
 TEST_F(DownloadItemTest, EnabledActionsForCancelledDownload) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockDownloadFile* download_file =
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   EXPECT_CALL(*download_file, Cancel());
   item->Cancel(true);
   RunAllPendingInMessageLoops();
@@ skipping 14 matching lines @@
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   // Drive the delegate interaction.
   EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(item, _))
       .WillOnce(Return(true));
   item->DestinationObserverAsWeakPtr()->DestinationCompleted(std::string());
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_FALSE(item->IsDangerous());
 
   // Make sure the download can complete.
-  EXPECT_CALL(*download_file, RenameAndAnnotate(base::FilePath(kDummyPath), _))
+  EXPECT_CALL(*download_file,
+              RenameAndAnnotate(base::FilePath(kDummyTargetPath), _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath(kDummyTargetPath)));
   EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   RunAllPendingInMessageLoops();
   EXPECT_EQ(DownloadItem::COMPLETE, item->GetState());
 }
 
 // Just delaying completion.
@@ skipping 15 matching lines @@
   ASSERT_FALSE(delegate_callback.is_null());
   copy_delegate_callback = delegate_callback;
   delegate_callback.Reset();
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   copy_delegate_callback.Run();
   ASSERT_TRUE(delegate_callback.is_null());
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_FALSE(item->IsDangerous());
 
   // Make sure the download can complete.
-  EXPECT_CALL(*download_file, RenameAndAnnotate(base::FilePath(kDummyPath), _))
+  EXPECT_CALL(*download_file,
+              RenameAndAnnotate(base::FilePath(kDummyTargetPath), _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath(kDummyTargetPath)));
   EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   RunAllPendingInMessageLoops();
   EXPECT_EQ(DownloadItem::COMPLETE, item->GetState());
 }
 
 // Delay and set danger.
@@ skipping 18 matching lines @@
   EXPECT_FALSE(item->IsDangerous());
   item->OnContentCheckCompleted(
       content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE);
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   copy_delegate_callback.Run();
   ASSERT_TRUE(delegate_callback.is_null());
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_TRUE(item->IsDangerous());
 
   // Make sure the download doesn't complete until we've validated it.
-  EXPECT_CALL(*download_file, RenameAndAnnotate(base::FilePath(kDummyPath), _))
+  EXPECT_CALL(*download_file,
+              RenameAndAnnotate(base::FilePath(kDummyTargetPath), _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath(kDummyTargetPath)));
   EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   RunAllPendingInMessageLoops();
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_TRUE(item->IsDangerous());
 
   item->ValidateDangerousDownload();
@@ skipping 28 matching lines @@
   ASSERT_FALSE(delegate_callback.is_null());
   copy_delegate_callback = delegate_callback;
   delegate_callback.Reset();
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   copy_delegate_callback.Run();
   ASSERT_TRUE(delegate_callback.is_null());
   EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
   EXPECT_FALSE(item->IsDangerous());
 
   // Make sure the download can complete.
-  EXPECT_CALL(*download_file, RenameAndAnnotate(base::FilePath(kDummyPath), _))
+  EXPECT_CALL(*download_file,
+              RenameAndAnnotate(base::FilePath(kDummyTargetPath), _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
-                                       base::FilePath(kDummyPath)));
+                                       base::FilePath(kDummyTargetPath)));
   EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   RunAllPendingInMessageLoops();
   EXPECT_EQ(DownloadItem::COMPLETE, item->GetState());
 }
 
 TEST_F(DownloadItemTest, StealDangerousDownload) {
@@ skipping 54 matching lines @@
   EXPECT_CALL(*mock_delegate(), DownloadRemoved(_));
   base::WeakPtrFactory<DownloadItemTest> weak_ptr_factory(this);
   item->StealDangerousDownload(
       base::Bind(&DownloadItemTest::OnDownloadFileAcquired,
                  weak_ptr_factory.GetWeakPtr(),
                  base::Unretained(&returned_path)));
   RunAllPendingInMessageLoops();
   EXPECT_TRUE(returned_path.empty());
 }
 
+namespace {

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Why?  It seems inconsistent with the rest of the file.

[asanka, 2016/02/12 05:04:27]

Hmm? The namespace only covers the set of definitions below. I placed them here
next to the test fixture so that it would be easier to follow.

[Randy Smith (Not in Mondays), 2016/02/12 17:37:02]

Ah, ignore me; it's a combination of a prejudice against anonymous namespaces
other than at the top of the file and a bias towards having tests at file scope
but not in an anonymous namespace, neither one of which I can really defend.

+

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

It might be useful to have a motivating comment before diving into the
infrastructure.

[asanka, 2016/02/12 05:04:27]

Done.

[Randy Smith (Not in Mondays), 2016/02/12 17:37:02]

Nice, thank you.

+// A call to a DownloadDestinationObserver method that's missing the
+// DownloadDestinationObserver object. Currying this way allows us to bind a
+// call prior to constructing the object on which the method would be invoked.
+// This is necessary since we are going to construct various permutations of
+// observer calls that will then be applied to a DownloadItem in a state as yet
+// undetermined.
+using CurriedObservation =
+    base::Callback<void(base::WeakPtr<DownloadDestinationObserver>)>;
+
+// A set of observations that are to be made during some event in the
+// DownloadItemImpl control flow. Ordering of the observations is significant.
+using ObservationSet = std::deque<CurriedObservation>;
+
+// An ordered list of events.
+//
+// An "event" in this context refers to some stage in the DownloadItemImpl's
+// workflow. For example: immediately after the initialization of the
+// DownloadFile.
+using EventList = std::deque<ObservationSet>;

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

This is a bit confusing, given that the EventList has no way to specify what the
event was.  I presume that means that exactly what the event is is context
dependent and specified by ordering within that context?

[asanka, 2016/02/12 05:04:26]

Yeah. It's the ordering. I'll make a note of it.

+
+// The following functions help us with currying the calls to
+// DownloadDestinationObserver. If std::bind was allowed along with
+// std::placeholders, it is possible to avoid these functions, but currently
+// Chromium doesn't allow using std::bind for good reasons.
+void DestinationUpdateInvoker(
+    int64_t bytes_so_far,
+    int64_t bytes_per_sec,
+    const std::string& hash_state,
+    base::WeakPtr<DownloadDestinationObserver> observer) {
+  if (observer)
+    observer->DestinationUpdate(bytes_so_far, bytes_per_sec, hash_state);
+}
+
+void DestinationErrorInvoker(
+    DownloadInterruptReason reason,
+    base::WeakPtr<DownloadDestinationObserver> observer) {
+  if (observer)
+    observer->DestinationError(reason);
+}
+
+void DestinationCompletedInvoker(
+    const std::string& final_hash,
+    base::WeakPtr<DownloadDestinationObserver> observer) {
+  if (observer)
+    observer->DestinationCompleted(final_hash);
+}
+
+// Given a set of observations (via the range |begin|..|end|), constructs a list
+// of EventLists such that:
+//
+// * There are exactly |event_count| ObservationSets in each EventList.
+//
+// * Each ObservationSet in each EventList contains a subrange of observations

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

suggest: "... subrange (possibly empty) of observations ..."

[asanka, 2016/02/12 05:04:27]

Done.

+//   from the input range, in the same order as the input range.
+//
+// * The ordering of the ObservationSet in each EventList is such that all
+//   observations in one ObservationSet occur earlier than all observations in
+//   an ObservationSet that follows it.
+//
+// * The list of EventLists together describe all the possible ways in which the
+//   list of observations can be distributed into |event_count| events.

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

I'm tempted to title this "Thor's hammer" except that hammer was very powerful,
not immensely large :-}.

[asanka, 2016/02/12 05:04:27]

Hehe.

+std::vector<EventList> DistributeObservationsIntoEvents(
+    const std::vector<CurriedObservation>::iterator begin,
+    const std::vector<CurriedObservation>::iterator end,
+    int event_count) {
+  std::vector<EventList> all_observation_sets;
+  for (auto partition = begin;; ++partition) {
+    ObservationSet first_group_of_observations(begin, partition);
+    if (event_count > 1) {
+      std::vector<EventList> subsequent_bins =
+          DistributeObservationsIntoEvents(partition, end, event_count - 1);
+      for (const auto other_bins : subsequent_bins) {

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Reference?  It's copied anyway below (IIUC).

[asanka, 2016/02/12 05:04:27]

Done.

+        EventList observation_set;
+        observation_set = other_bins;
+        observation_set.push_front(first_group_of_observations);
+        all_observation_sets.push_back(observation_set);
+      }
+    } else {
+      EventList observation_set;
+      observation_set.push_front(first_group_of_observations);
+      all_observation_sets.push_back(observation_set);
+    }
+    if (partition == end)
+      break;
+  }
+  return all_observation_sets;
+}
+
+// For the purpose of this tests, we are only concerned with 3 events:
+//
+// 1. Immediately after the DownloadFile is initialized.
+// 2. Immediately after the DownloadTargetCallback is invoked.
+// 3. Immediately after the intermediate file is renamed.
+//
+// We are going to take a couple of sets of DownloadDestinationObserver events
+// and distribute them into the three events described above. And then we are
+// going to invoke the observations while a DownloadItemImpl is carefully
+// stepped through its stages.
+
+std::vector<EventList> GenerateSuccessfulEventLists() {
+  std::vector<CurriedObservation> all_observations;
+  all_observations.push_back(
+      base::Bind(&DestinationUpdateInvoker, 100, 100, "abc"));
+  all_observations.push_back(
+      base::Bind(&DestinationUpdateInvoker, 200, 100, "def"));
+  all_observations.push_back(
+      base::Bind(&DestinationCompletedInvoker, "final-hash-1"));
+  return DistributeObservationsIntoEvents(all_observations.begin(),
+                                          all_observations.end(), 3);

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Where does event_count == 3 come from?  Isn't that dependent on the caller?  Or
is there some link between the event count and the number of observations?

ETA: Ah, looks like all your tests have the same three events.  Maybe worthwhile
defining that above and using a constant here?

[asanka, 2016/02/12 05:04:27]

Gave it a name.

+}
+
+std::vector<EventList> GenerateFailingEventLists() {
+  std::vector<CurriedObservation> all_observations;
+  all_observations.push_back(
+      base::Bind(&DestinationUpdateInvoker, 100, 100, "abc"));
+  all_observations.push_back(base::Bind(
+      &DestinationErrorInvoker, DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED));
+  return DistributeObservationsIntoEvents(all_observations.begin(),
+                                          all_observations.end(), 3);
+}
+
+class DownloadItemDestinationUpdateRaceTest
+    : public DownloadItemTest,
+      public ::testing::WithParamInterface<EventList> {
+ public:
+  DownloadItemDestinationUpdateRaceTest()
+      : DownloadItemTest(),
+        item_(CreateDownloadItem()),
+        file_(new ::testing::StrictMock<MockDownloadFile>()),
+        request_handle_(new ::testing::StrictMock<MockRequestHandle>()) {
+    DCHECK_EQ(GetParam().size(), 3u);
+    EXPECT_CALL(*request_handle_, GetWebContents())
+        .WillRepeatedly(Return(nullptr));
+  }
+
+ protected:
+  const ObservationSet& PostInitializeFileObservations() {
+    return GetParam().front();
+  }
+  const ObservationSet& PostTargetDeterminationObservations() {
+    return *(GetParam().begin() + 1);
+  }
+  const ObservationSet& PostIntermediateRenameObservations() {
+    return *(GetParam().begin() + 2);
+  }
+
+  // Apply all the observations in |observations| to |observer|, but do so
+  // asynchronously so that the events are applied in order behind any tasks
+  // that are already scheduled.
+  void ScheduleObservations(
+      const ObservationSet& observations,
+      base::WeakPtr<DownloadDestinationObserver> observer) {
+    for (const auto action : observations)
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                              base::Bind(action, observer));
+  }
+
+  DownloadItemImpl* item_;
+  scoped_ptr<MockDownloadFile> file_;
+  scoped_ptr<MockRequestHandle> request_handle_;
+
+  std::queue<base::Closure> successful_update_events_;
+  std::queue<base::Closure> failing_update_events_;
+};
+
+INSTANTIATE_TEST_CASE_P(Success,
+                        DownloadItemDestinationUpdateRaceTest,
+                        ::testing::ValuesIn(GenerateSuccessfulEventLists()));
+
+INSTANTIATE_TEST_CASE_P(Failure,
+                        DownloadItemDestinationUpdateRaceTest,
+                        ::testing::ValuesIn(GenerateFailingEventLists()));
+
+}  // namespace
+
+TEST_P(DownloadItemDestinationUpdateRaceTest, DumpParameters) {

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

Quick comment before each test sketching out what it does?

[asanka, 2016/02/12 05:04:27]

Done.

+  const auto parameter = GetParam();
+  class Dump : public DownloadDestinationObserver {
+   public:
+    void DestinationUpdate(int64_t bytes_so_far,
+                           int64_t bytes_per_sec,
+                           const std::string& hash_state) override {
+      DVLOG(20) << "  DestinationUpdate(bytes_so_far:" << bytes_so_far
+                << " bytes_per_sec:" << bytes_per_sec
+                << " hash_state:" << hash_state << ")";
+    };
+
+    void DestinationError(DownloadInterruptReason reason) override {
+      DVLOG(20) << "  DestinationError(reason:"
+                << DownloadInterruptReasonToString(reason) << ")";
+    }
+
+    void DestinationCompleted(const std::string& final_hash) override {
+      DVLOG(20) << "  DestinationCompleted(final_hash:" << final_hash << ")";
+    }
+  } dumper;
+  base::WeakPtrFactory<DownloadDestinationObserver> factory(&dumper);
+  base::WeakPtr<DownloadDestinationObserver> dumper_ptr = factory.GetWeakPtr();
+
+  for (const auto bin : parameter) {
+    DVLOG(20) << "New bin: " << bin.size() << " elements";
+    for (const auto invoker : bin)
+      invoker.Run(dumper_ptr);
+  }
+}
+
+TEST_P(DownloadItemDestinationUpdateRaceTest, InitDownloadFileFails) {

[Randy Smith (Not in Mondays), 2016/02/12 00:03:11]

I'm sure there's something I'm missing, but is there a reason to run this test
for each param?  It looks like it does the same thing no matter what parameter
it gets.

[asanka, 2016/02/12 05:04:27]

Yeah, this one was wasteful. I moved it to a regular DownloadItemTest.

+  // Expect that the download file and the request will be cancelled as a
+  // result.
+  EXPECT_CALL(*file_, Cancel());
+  EXPECT_CALL(*request_handle_, CancelRequest());
+
+  EXPECT_CALL(*file_, Initialize(_))
+      .WillOnce(InvokeCallbackWithParam(
+          DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED));
+  item_->Start(std::move(file_), std::move(request_handle_));
+
+  RunAllPendingInMessageLoops();
+
+  // Since the download file initialization failed, no destination updates are
+  // expected. No need to drain any action buckets.
+  EXPECT_EQ(DownloadItem::INTERRUPTED, item_->GetState());
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
+            item_->GetLastReason());
+}
+
+TEST_P(DownloadItemDestinationUpdateRaceTest, DownloadCancelledByUser) {
+  // Expect that the download file and the request will be cancelled as a
+  // result.
+  EXPECT_CALL(*file_, Cancel());
+  EXPECT_CALL(*request_handle_, CancelRequest());
+
+  base::RunLoop initialize_loop;
+  DownloadFile::InitializeCallback initialize_callback;
+  EXPECT_CALL(*file_, Initialize(_))
+      .WillOnce(DoAll(SaveArg<0>(&initialize_callback),
+                      InvokeClosure(initialize_loop.QuitClosure())));
+
+  item_->Start(std::move(file_), std::move(request_handle_));
+  initialize_loop.Run();
+  base::WeakPtr<DownloadDestinationObserver> destination_observer =
+      item_->DestinationObserverAsWeakPtr();
+
+  base::RunLoop target_determination_loop;
+  DownloadItemImplDelegate::DownloadTargetCallback target_callback;
+  EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(_, _))
+      .WillOnce(DoAll(SaveArg<1>(&target_callback),
+                      InvokeClosure(target_determination_loop.QuitClosure())));
+  ScheduleObservations(PostInitializeFileObservations(), destination_observer);
+  initialize_callback.Run(DOWNLOAD_INTERRUPT_REASON_NONE);
+  target_determination_loop.Run();
+
+  RunAllPendingInMessageLoops();
+
+  ASSERT_FALSE(target_callback.is_null());
+  ScheduleObservations(PostTargetDeterminationObservations(),
+                       destination_observer);
+  target_callback.Run(base::FilePath(),
+                      DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                      DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, base::FilePath());
+  RunAllPendingInMessageLoops();
+
+  EXPECT_EQ(DownloadItem::CANCELLED, item_->GetState());
+}
+
+TEST_P(DownloadItemDestinationUpdateRaceTest, IntermediateRenameFails) {
+  // Expect that the download file and the request will be cancelled as a
+  // result.
+  EXPECT_CALL(*file_, Cancel());
+  EXPECT_CALL(*request_handle_, CancelRequest());
+
+  // Intermediate rename loop is not used immediately, but let's set up the
+  // DownloadFile expectations since we are about to transfer its ownership to
+  // the DownloadItem.
+  base::RunLoop intermediate_rename_loop;
+  DownloadFile::RenameCompletionCallback intermediate_rename_callback;
+  EXPECT_CALL(*file_, RenameAndUniquify(_, _))
+      .WillOnce(DoAll(SaveArg<1>(&intermediate_rename_callback),
+                      InvokeClosure(intermediate_rename_loop.QuitClosure())));
+
+  base::RunLoop initialize_loop;
+  DownloadFile::InitializeCallback initialize_callback;
+  EXPECT_CALL(*file_, Initialize(_))
+      .WillOnce(DoAll(SaveArg<0>(&initialize_callback),
+                      InvokeClosure(initialize_loop.QuitClosure())));
+
+  item_->Start(std::move(file_), std::move(request_handle_));
+  initialize_loop.Run();
+  base::WeakPtr<DownloadDestinationObserver> destination_observer =
+      item_->DestinationObserverAsWeakPtr();
+
+  base::RunLoop target_determination_loop;
+  DownloadItemImplDelegate::DownloadTargetCallback target_callback;
+  EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(_, _))
+      .WillOnce(DoAll(SaveArg<1>(&target_callback),
+                      InvokeClosure(target_determination_loop.QuitClosure())));
+  ScheduleObservations(PostInitializeFileObservations(), destination_observer);
+  initialize_callback.Run(DOWNLOAD_INTERRUPT_REASON_NONE);
+  target_determination_loop.Run();
+
+  RunAllPendingInMessageLoops();
+  ASSERT_FALSE(target_callback.is_null());
+
+  ScheduleObservations(PostTargetDeterminationObservations(),
+                       destination_observer);
+  target_callback.Run(base::FilePath(kDummyTargetPath),
+                      DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                      DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                      base::FilePath(kDummyIntermediatePath));
+
+  intermediate_rename_loop.Run();
+  ASSERT_FALSE(intermediate_rename_callback.is_null());
+
+  ScheduleObservations(PostIntermediateRenameObservations(),
+                       destination_observer);
+  intermediate_rename_callback.Run(DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,
+                                   base::FilePath());
+  RunAllPendingInMessageLoops();
+
+  EXPECT_EQ(DownloadItem::INTERRUPTED, item_->GetState());
+}
+
+TEST_P(DownloadItemDestinationUpdateRaceTest, IntermediateRenameSucceeds) {
+  // We expect either that the download will fail (in which case the request and
+  // the download file will be cancelled), or it will succeed (in which case the
+  // DownloadFile will Detach()). It depends on the list of observations that
+  // are given to us.
+  EXPECT_CALL(*file_, Cancel()).Times(::testing::AnyNumber());
+  EXPECT_CALL(*request_handle_, CancelRequest()).Times(::testing::AnyNumber());
+  EXPECT_CALL(*file_, Detach()).Times(::testing::AnyNumber());
+
+  EXPECT_CALL(*file_, FullPath())
+      .WillRepeatedly(Return(base::FilePath(kDummyIntermediatePath)));
+
+  // Intermediate rename loop is not used immediately, but let's set up the
+  // DownloadFile expectations since we are about to transfer its ownership to
+  // the DownloadItem.
+  base::RunLoop intermediate_rename_loop;
+  DownloadFile::RenameCompletionCallback intermediate_rename_callback;
+  EXPECT_CALL(*file_, RenameAndUniquify(_, _))
+      .WillOnce(DoAll(SaveArg<1>(&intermediate_rename_callback),
+                      InvokeClosure(intermediate_rename_loop.QuitClosure())));
+
+  base::RunLoop initialize_loop;
+  DownloadFile::InitializeCallback initialize_callback;
+  EXPECT_CALL(*file_, Initialize(_))
+      .WillOnce(DoAll(SaveArg<0>(&initialize_callback),
+                      InvokeClosure(initialize_loop.QuitClosure())));
+
+  item_->Start(std::move(file_), std::move(request_handle_));
+  initialize_loop.Run();
+  base::WeakPtr<DownloadDestinationObserver> destination_observer =
+      item_->DestinationObserverAsWeakPtr();
+
+  base::RunLoop target_determination_loop;
+  DownloadItemImplDelegate::DownloadTargetCallback target_callback;
+  EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(_, _))
+      .WillOnce(DoAll(SaveArg<1>(&target_callback),
+                      InvokeClosure(target_determination_loop.QuitClosure())));
+  ScheduleObservations(PostInitializeFileObservations(), destination_observer);
+  initialize_callback.Run(DOWNLOAD_INTERRUPT_REASON_NONE);
+  target_determination_loop.Run();
+
+  RunAllPendingInMessageLoops();
+  ASSERT_FALSE(target_callback.is_null());
+
+  ScheduleObservations(PostTargetDeterminationObservations(),
+                       destination_observer);
+  target_callback.Run(base::FilePath(kDummyTargetPath),
+                      DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                      DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                      base::FilePath(kDummyIntermediatePath));
+
+  intermediate_rename_loop.Run();
+  ASSERT_FALSE(intermediate_rename_callback.is_null());
+
+  // This may or may not be called, depending on whether there are any errors in
+  // our action list.
+  EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(_, _))
+      .Times(::testing::AnyNumber());
+
+  ScheduleObservations(PostIntermediateRenameObservations(),
+                       destination_observer);
+  intermediate_rename_callback.Run(DOWNLOAD_INTERRUPT_REASON_NONE,
+                                   base::FilePath(kDummyIntermediatePath));
+  RunAllPendingInMessageLoops();
+
+  // The state of the download depends on the observer events that were played
+  // back to the DownloadItemImpl. Hence we can't establish a single expectation
+  // here. On Debug builds, the DCHECKs will verify that the state transitions
+  // were correct, and on Release builds, the tests should run to completion
+  // without crashing.
+  EXPECT_TRUE(item_->GetState() == DownloadItem::IN_PROGRESS ||
+              item_->GetState() == DownloadItem::INTERRUPTED);
+  if (item_->GetState() == DownloadItem::INTERRUPTED)
+    EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED, item_->GetLastReason());
+
+  item_->Cancel(true);
+  RunAllPendingInMessageLoops();
+}
+
+// TODO(asanka): Write tests for resuming and auto-resuming downloads which have
+// their own brand of race conditions.
+
 TEST(MockDownloadItem, Compiles) {
   MockDownloadItem mock_item;
 }
 
 }  // namespace content