[Downloads] Enforce state transition integrity and state invariants.

content/browser/download/download_item_impl_unittest.cc

Index: content/browser/download/download_item_impl_unittest.cc
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index 0afad1bfca248fea496ee1801387afca60fc2572..1ea59d3c968e9dc02f90db3193f76ed7e5dfb0a3 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -5,11 +5,15 @@
 #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"
@@ -18,26 +22,32 @@
 #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 {
 
@@ -159,7 +169,7 @@ class TestDownloadItemObserver : public DownloadItem::Observer {
               << " download = " << download->DebugString(false);
     destroyed_ = true;
     item_->RemoveObserver(this);
-    item_ = NULL;
+    item_ = nullptr;
   }
 
   DownloadItem* item_;
@@ -182,20 +192,28 @@ ACTION_P2(ScheduleRenameCallback, interrupt_reason, new_path) {
                           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,
@@ -203,11 +221,19 @@ class DownloadItemTest : public testing::Test {
     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.
@@ -217,24 +243,16 @@ class DownloadItemTest : public testing::Test {
     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(_));
@@ -250,7 +268,7 @@ class DownloadItemTest : public testing::Test {
     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.
@@ -266,18 +284,17 @@ class DownloadItemTest : public testing::Test {
   }
 
   // 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));
@@ -287,6 +304,22 @@ class DownloadItemTest : public testing::Test {
     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,
@@ -298,7 +331,7 @@ class DownloadItemTest : public testing::Test {
       if (download_file)
         EXPECT_CALL(*download_file, Cancel());
       item->Cancel(true);
-      loop_.RunUntilIdle();
+      RunAllPendingInMessageLoops();
     }
   }
 
@@ -308,9 +341,7 @@ class DownloadItemTest : public testing::Test {
     delete item;
   }
 
-  void RunAllPendingInMessageLoops() {
-    loop_.RunUntilIdle();
-  }
+  void RunAllPendingInMessageLoops() { base::RunLoop().RunUntilIdle(); }
 
   MockDelegate* mock_delegate() {
     return &delegate_;
@@ -323,11 +354,9 @@ class DownloadItemTest : public testing::Test {
 
  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
@@ -349,8 +378,7 @@ TEST_F(DownloadItemTest, NotificationAfterUpdate) {
 
 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);
 
@@ -358,7 +386,7 @@ TEST_F(DownloadItemTest, NotificationAfterCancel) {
   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);
 
@@ -369,11 +397,10 @@ TEST_F(DownloadItemTest, NotificationAfterCancel) {
 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());
 }
 
@@ -471,9 +498,10 @@ TEST_F(DownloadItemTest, UnresumableInterrupt) {
   // 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.
@@ -494,9 +522,9 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
   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;
 
@@ -517,22 +545,24 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
     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(
@@ -541,6 +571,7 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
     ::testing::Mock::VerifyAndClearExpectations(mock_download_file);
   }
 
+  EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
   EXPECT_EQ(1, observer.interrupt_count());
   CleanupItem(item, nullptr, DownloadItem::INTERRUPTED);
 }
@@ -588,7 +619,7 @@ TEST_F(DownloadItemTest, ResumeUsingFinalURL) {
 
 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);
@@ -601,16 +632,21 @@ TEST_F(DownloadItemTest, NotificationAfterRemove) {
 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());
@@ -618,11 +654,17 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
   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());
@@ -630,8 +672,12 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
   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
@@ -642,10 +688,9 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
 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"));
@@ -693,9 +738,9 @@ TEST_F(DownloadItemTest, NotificationAfterTogglePause) {
 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());
@@ -732,9 +777,9 @@ TEST_F(DownloadItemTest, Start) {
 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"));
@@ -768,9 +813,9 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
 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"));
@@ -814,13 +859,13 @@ TEST_F(DownloadItemTest, Interrupted) {
 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"));
@@ -847,13 +892,13 @@ TEST_F(DownloadItemTest, InterruptedBeforeIntermediateRename_Restart) {
 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"));
@@ -880,13 +925,13 @@ TEST_F(DownloadItemTest, InterruptedBeforeIntermediateRename_Continue) {
 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"));
@@ -910,7 +955,7 @@ TEST_F(DownloadItemTest, InterruptedBeforeIntermediateRename_Failed) {
 
 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());
@@ -979,6 +1024,7 @@ TEST_F(DownloadItemTest, DestinationError) {
 
 TEST_F(DownloadItemTest, DestinationCompleted) {
   DownloadItemImpl* item = CreateDownloadItem();
+  MockDownloadFile* download_file = CallDownloadItemStart(item, nullptr);
   base::WeakPtr<DownloadDestinationObserver> as_observer(
       item->DestinationObserverAsWeakPtr());
   TestDownloadItemObserver observer(item);
@@ -1004,6 +1050,11 @@ TEST_F(DownloadItemTest, DestinationCompleted) {
   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) {
@@ -1020,7 +1071,7 @@ TEST_F(DownloadItemTest, EnabledActionsForNormalDownload) {
   // 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())
@@ -1052,7 +1103,7 @@ TEST_F(DownloadItemTest, EnabledActionsForTemporaryDownload) {
       .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());
@@ -1077,7 +1128,7 @@ TEST_F(DownloadItemTest, EnabledActionsForInterruptedDownload) {
   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) {
@@ -1112,9 +1163,10 @@ TEST_F(DownloadItemTest, CompleteDelegate_ReturnTrue) {
   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())
@@ -1150,9 +1202,10 @@ TEST_F(DownloadItemTest, CompleteDelegate_BlockOnce) {
   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())
@@ -1191,9 +1244,10 @@ TEST_F(DownloadItemTest, CompleteDelegate_SetDanger) {
   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())
@@ -1242,9 +1296,10 @@ TEST_F(DownloadItemTest, CompleteDelegate_BlockTwice) {
   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())
@@ -1319,6 +1374,412 @@ TEST_F(DownloadItemTest, StealInterruptedNonResumableDangerousDownload) {
   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;
 }