Merging the safe and dangerous download paths.

Manual testing matrix:

+--------------------------------------+------+-------+-------------+-------------+
|                                      |Accept/Decline|  Cancelled  |             |
| Test                                 |Before| After |Before|After |  Completed  |
|                                      |Full download |Full download|             |
+--------------------------------------+------+-------+-------------+-------------+
|Drag & Drop                           |     N/A      |     N/A     |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Safe                                  |     N/A      |      |      |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Safe                                  |     N/A      |  Y   | N/A  |             |
+--------------------------------------+------+-------+-------------+-------------+
|Safe with uniquification              |     N/A      |      | N/A  |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Safe with uniquification              |     N/A      |  Y   | N/A  |             |
+--------------------------------------+------+-------+-------------+-------------+
|Dangerous Accepted                    |  Y   |       |     N/A     |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Dangerous Accepted                    |      |   Y   |     N/A     |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Dangerous Accepted with uniquification|  Y   |       |     N/A     |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Dangerous Accepted with uniquification|      |   Y   |     N/A     |      Y      |
+--------------------------------------+------+-------+-------------+-------------+
|Dangerous Declined                    |  Y   |       |     N/A     |     N/A     |
+--------------------------------------+------+-------+-------------+-------------+
|Dangerous Declined                    |      |   Y   |     N/A     |     N/A     |
+--------------------------------------+------+-------+-------------+-------------+

I couldn't figure out how to get Drag & Drop to work in Windows.  All I got was a URL shortcut when I tried.

BUG=49394, 49568, 75277
TEST=Passes all download tests.  Passes all tests in the above matrix.

Review URL: http://codereview.chromium.org/6588020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@79313 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 394 insertions, 278 deletions

diff --git a/chrome/browser/download/base_file.cc b/chrome/browser/download/base_file.cc
index 9229aca..5f5a53b 100644
--- a/chrome/browser/download/base_file.cc
+++ b/chrome/browser/download/base_file.cc
@@ -6,6 +6,7 @@
 
 #include "base/crypto/secure_hash.h"
 #include "base/file_util.h"
+#include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "net/base/file_stream.h"
@@ -30,20 +31,23 @@ BaseFile::BaseFile(const FilePath& full_path,
       file_stream_(file_stream),
       bytes_so_far_(received_bytes),
       power_save_blocker_(true),
-      calculate_hash_(false) {
+      calculate_hash_(false),
+      detached_(false) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   memset(sha256_hash_, 0, sizeof(sha256_hash_));
 }
 
 BaseFile::~BaseFile() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  if (in_progress())
-    Cancel();
-  Close();
+  if (detached_)
+    Close();
+  else
+    Cancel();  // Will delete the file.
 }
 
 bool BaseFile::Initialize(bool calculate_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!detached_);
 
   calculate_hash_ = calculate_hash;
 
@@ -58,6 +62,7 @@ bool BaseFile::Initialize(bool calculate_hash) {
 
 bool BaseFile::AppendDataToFile(const char* data, size_t data_len) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!detached_);
 
   if (!file_stream_.get())
     return false;
@@ -140,9 +145,16 @@ bool BaseFile::Rename(const FilePath& new_path) {
   return true;
 }
 
+void BaseFile::Detach() {
+  detached_ = true;
+}
+
 void BaseFile::Cancel() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!detached_);
+
   Close();
+
   if (!full_path_.empty())
     file_util::Delete(full_path_, false);
 }
@@ -157,6 +169,7 @@ void BaseFile::Finish() {
 }
 
 bool BaseFile::GetSha256Hash(std::string* hash) {
+  DCHECK(!detached_);
   if (!calculate_hash_ || in_progress())
     return false;
   hash->assign(reinterpret_cast<const char*>(sha256_hash_),
@@ -166,6 +179,8 @@ bool BaseFile::GetSha256Hash(std::string* hash) {
 
 void BaseFile::AnnotateWithSourceInformation() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!detached_);
+
 #if defined(OS_WIN)
   // Sets the Zone to tell Windows that this file comes from the internet.
   // We ignore the return value because a failure is not fatal.
@@ -180,9 +195,10 @@ void BaseFile::AnnotateWithSourceInformation() {
 
 bool BaseFile::Open() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!detached_);
   DCHECK(!full_path_.empty());
 
-  // Create a new file steram if it is not provided.
+  // Create a new file stream if it is not provided.
   if (!file_stream_.get()) {
     file_stream_.reset(new net::FileStream);
     if (file_stream_->Open(full_path_,
@@ -220,7 +236,11 @@ void BaseFile::Close() {
 }
 
 std::string BaseFile::DebugString() const {
-  return base::StringPrintf("{ source_url_ = \"%s\" full_path_ = \"%s\" }",
+  return base::StringPrintf("{ source_url_ = \"%s\""
+                            " full_path_ = \"%" PRFilePath "\""
+                            " bytes_so_far_ = %" PRId64 " detached_ = %c }",
                             source_url_.spec().c_str(),
-                            full_path_.value().c_str());
+                            full_path_.value().c_str(),
+                            bytes_so_far_,
+                            detached_ ? 'T' : 'F');
 }
diff --git a/chrome/browser/download/base_file.h b/chrome/browser/download/base_file.h
index cf4bb24..1fc25aa 100644
--- a/chrome/browser/download/base_file.h
+++ b/chrome/browser/download/base_file.h
@@ -42,6 +42,9 @@ class BaseFile {
   // Rename the download file. Returns true on success.
   virtual bool Rename(const FilePath& full_path);
 
+  // Detach the file so it is not deleted on destruction.
+  virtual void Detach();
+
   // Abort the download and automatically close the file.
   void Cancel();
 
@@ -95,6 +98,10 @@ class BaseFile {
 
   unsigned char sha256_hash_[kSha256HashLen];
 
+  // Indicates that this class no longer owns the associated file, and so
+  // won't delete it on destruction.
+  bool detached_;
+
   DISALLOW_COPY_AND_ASSIGN(BaseFile);
 };
 
diff --git a/chrome/browser/download/base_file_unittest.cc b/chrome/browser/download/base_file_unittest.cc
index c6176fb..5eeb685 100644
--- a/chrome/browser/download/base_file_unittest.cc
+++ b/chrome/browser/download/base_file_unittest.cc
@@ -19,7 +19,9 @@ const char kTestData3[] = "Final line.";
 
 class BaseFileTest : public testing::Test {
  public:
-  BaseFileTest() : file_thread_(BrowserThread::FILE, &message_loop_) {
+  BaseFileTest()
+      : expect_file_survives_(false),
+        file_thread_(BrowserThread::FILE, &message_loop_) {
   }
 
   virtual void SetUp() {
@@ -33,17 +35,20 @@ class BaseFileTest : public testing::Test {
     EXPECT_EQ(static_cast<int64>(expected_data_.size()),
               base_file_->bytes_so_far());
 
+    FilePath full_path = base_file_->full_path();
+
     if (!expected_data_.empty()) {
       // Make sure the data has been properly written to disk.
       std::string disk_data;
-      EXPECT_TRUE(file_util::ReadFileToString(base_file_->full_path(),
-                                              &disk_data));
+      EXPECT_TRUE(file_util::ReadFileToString(full_path, &disk_data));
       EXPECT_EQ(expected_data_, disk_data);
     }
 
     // Make sure the mock BrowserThread outlives the BaseFile to satisfy
     // thread checks inside it.
     base_file_.reset();
+
+    EXPECT_EQ(expect_file_survives_, file_util::PathExists(full_path));
   }
 
   void AppendDataToFile(const std::string& data) {
@@ -63,6 +68,9 @@ class BaseFileTest : public testing::Test {
   // Temporary directory for renamed downloads.
   ScopedTempDir temp_dir_;
 
+  // Expect the file to survive deletion of the BaseFile instance.
+  bool expect_file_survives_;
+
  private:
   // Keep track of what data should be saved to the disk file.
   std::string expected_data_;
@@ -88,6 +96,51 @@ TEST_F(BaseFileTest, Cancel) {
   EXPECT_NE(FilePath().value(), base_file_->full_path().value());
 }
 
+// Write data to the file and detach it, so it doesn't get deleted
+// automatically when base_file_ is destructed.
+TEST_F(BaseFileTest, WriteAndDetach) {
+  ASSERT_TRUE(base_file_->Initialize(false));
+  AppendDataToFile(kTestData1);
+  base_file_->Finish();
+  base_file_->Detach();
+  expect_file_survives_ = true;
+}
+
+// Write data to the file and detach it, and calculate its sha256 hash.
+TEST_F(BaseFileTest, WriteWithHashAndDetach) {
+  ASSERT_TRUE(base_file_->Initialize(true));
+  AppendDataToFile(kTestData1);
+  base_file_->Finish();
+
+  std::string hash;
+  base_file_->GetSha256Hash(&hash);
+  EXPECT_EQ("0B2D3F3F7943AD64B860DF94D05CB56A8A97C6EC5768B5B70B930C5AA7FA9ADE",
+            base::HexEncode(hash.data(), hash.size()));
+
+  base_file_->Detach();
+  expect_file_survives_ = true;
+}
+
+// Rename the file after writing to it, then detach.
+TEST_F(BaseFileTest, WriteThenRenameAndDetach) {
+  ASSERT_TRUE(base_file_->Initialize(false));
+
+  FilePath initial_path(base_file_->full_path());
+  EXPECT_TRUE(file_util::PathExists(initial_path));
+  FilePath new_path(temp_dir_.path().AppendASCII("NewFile"));
+  EXPECT_FALSE(file_util::PathExists(new_path));
+
+  AppendDataToFile(kTestData1);
+
+  EXPECT_TRUE(base_file_->Rename(new_path));
+  EXPECT_FALSE(file_util::PathExists(initial_path));
+  EXPECT_TRUE(file_util::PathExists(new_path));
+
+  base_file_->Finish();
+  base_file_->Detach();
+  expect_file_survives_ = true;
+}
+
 // Write data to the file once.
 TEST_F(BaseFileTest, SingleWrite) {
   ASSERT_TRUE(base_file_->Initialize(false));
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 84ae546..8d346b5 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -147,7 +147,7 @@ class DownloadsObserver : public DownloadManager::Observer,
     }
   }
 
-  virtual void SelectFileDialogDisplayed() {
+  virtual void SelectFileDialogDisplayed(int32 /* id */) {
     select_file_dialog_seen_ = true;
     SignalIfFinished();
   }
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index 4b7e860..7a1c298 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -54,7 +54,6 @@ DownloadFileManager::DownloadFileManager(ResourceDispatcherHost* rdh)
 
 DownloadFileManager::~DownloadFileManager() {
   DCHECK(downloads_.empty());
-  DCHECK(downloads_with_final_name_.empty());
 }
 
 void DownloadFileManager::Shutdown() {
@@ -67,7 +66,6 @@ void DownloadFileManager::Shutdown() {
 void DownloadFileManager::OnShutdown() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   StopUpdateTimer();
-  downloads_with_final_name_.clear();
   STLDeleteValues(&downloads_);
 }
 
@@ -211,31 +209,27 @@ void DownloadFileManager::OnResponseCompleted(int id, DownloadBuffer* buffer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   delete buffer;
   DownloadFileMap::iterator it = downloads_.find(id);
-  if (it != downloads_.end()) {
-    DownloadFile* download = it->second;
-    download->Finish();
-
-    DownloadManager* download_manager = download->GetDownloadManager();
-    if (download_manager) {
-      std::string hash;
-      if (!download->GetSha256Hash(&hash))
-        hash.clear();
-
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          NewRunnableMethod(
-              download_manager, &DownloadManager::OnAllDataSaved,
-              id, download->bytes_so_far(), hash));
-    }
+  if (it == downloads_.end())
+    return;
+
+  DownloadFile* download = it->second;
+  download->Finish();
 
-    // We need to keep the download around until the UI thread has finalized
-    // the name.
-    if (ContainsKey(downloads_with_final_name_, id))
-      EraseDownload(id);
+  DownloadManager* download_manager = download->GetDownloadManager();
+  if (!download_manager) {
+    CancelDownload(id);
+    return;
   }
 
-  if (downloads_.empty())
-    StopUpdateTimer();
+  std::string hash;
+  if (!download->GetSha256Hash(&hash))
+    hash.clear();
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      NewRunnableMethod(
+          download_manager, &DownloadManager::OnAllDataSaved,
+          id, download->bytes_so_far(), hash));
 }
 
 // This method will be sent via a user action, or shutdown on the UI thread, and
@@ -245,18 +239,32 @@ void DownloadFileManager::CancelDownload(int id) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DownloadFileMap::iterator it = downloads_.find(id);
-  if (it != downloads_.end()) {
-    DownloadFile* download = it->second;
-    VLOG(20) << __FUNCTION__ << "()"
-             << " download = " << download->DebugString();
-    download->Cancel();
-
-    if (ContainsKey(downloads_with_final_name_, id))
-      EraseDownload(id);
-  }
+  if (it == downloads_.end())
+    return;
 
-  if (downloads_.empty())
-    StopUpdateTimer();
+  DownloadFile* download = it->second;
+  VLOG(20) << __FUNCTION__ << "()"
+           << " download = " << download->DebugString();
+  download->Cancel();
+
+  EraseDownload(id);
+}
+
+void DownloadFileManager::CompleteDownload(int id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  if (!ContainsKey(downloads_, id))
+    return;
+
+  DownloadFile* download_file = downloads_[id];
+
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " id = " << id
+           << " download_file = " << download_file->DebugString();
+
+  download_file->Detach();
+
+  EraseDownload(id);
 }
 
 void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
@@ -271,7 +279,6 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
     if (download_file->GetDownloadManager() == manager) {
       download_file->CancelDownloadRequest(resource_dispatcher_host_);
       to_remove.insert(download_file);
-      downloads_with_final_name_.erase(download_file->id());
     }
   }
 
@@ -286,18 +293,22 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 
 // The DownloadManager in the UI thread has provided an intermediate .crdownload
 // name for the download specified by 'id'. Rename the in progress download.
-void DownloadFileManager::OnIntermediateDownloadName(
-    int id, const FilePath& full_path, DownloadManager* download_manager) {
+//
+// There are 2 possible rename cases where this method can be called:
+// 1. tmp -> foo.crdownload (not final, safe)
+// 2. tmp-> Unconfirmed.xxx.crdownload (not final, dangerous)
+void DownloadFileManager::RenameInProgressDownloadFile(
+    int id, const FilePath& full_path) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DownloadFileMap::iterator it = downloads_.find(id);
-  if (it == downloads_.end())
+
+  DownloadFile* download = GetDownloadFile(id);
+  if (!download)
     return;
-  DCHECK(!ContainsKey(downloads_with_final_name_, id));
 
-  DownloadFile* download = it->second;
   VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString();
+
   if (!download->Rename(full_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
@@ -306,16 +317,14 @@ void DownloadFileManager::OnIntermediateDownloadName(
 }
 
 // The DownloadManager in the UI thread has provided a final name for the
-// download specified by 'id'. Rename the in progress download, and remove it
-// from our table if it has been completed or cancelled already.
+// download specified by 'id'. Rename the completed download.
 //
 // There are 2 possible rename cases where this method can be called:
-// 1. foo.crdownload -> foo
-// 2. tmp-> Unconfirmed.xxx.crdownload
-// We don't call this function before a safe temp file has been renamed (in
-// that case tmp -> foo.crdownload occurs in |OnIntermediateDownloadName|).
-void DownloadFileManager::OnFinalDownloadName(
-    int id, const FilePath& full_path, DownloadManager* download_manager) {
+// 1. foo.crdownload -> foo (final, safe)
+// 2. Unconfirmed.xxx.crdownload -> xxx (final, validated)
+void DownloadFileManager::RenameFinishedDownloadFile(
+    int id, const FilePath& full_path)
+{
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -323,36 +332,48 @@ void DownloadFileManager::OnFinalDownloadName(
   DownloadFile* download = GetDownloadFile(id);
   if (!download)
     return;
+
+  DCHECK(download->GetDownloadManager());
+  DownloadManager* download_manager = download->GetDownloadManager();
+
   VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString();
-  DCHECK(!ContainsKey(downloads_with_final_name_, id));
-  if (download->Rename(full_path)) {
-    downloads_with_final_name_[id] = download;
-#if defined(OS_MACOSX)
-    // Done here because we only want to do this once; see
-    // http://crbug.com/13120 for details.
-    download->AnnotateWithSourceInformation();
-#endif
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        NewRunnableMethod(
-            download_manager, &DownloadManager::DownloadRenamedToFinalName, id,
-            full_path));
-  } else {
+
+  int uniquifier = 0;
+  FilePath new_path = full_path;
+  // Make our name unique at this point, as if a dangerous file is
+  // downloading and a 2nd download is started for a file with the same
+  // name, they would have the same path.  This is because we uniquify
+  // the name on download start, and at that time the first file does
+  // not exists yet, so the second file gets the same name.
+  // This should not happen in the SAFE case, and we check for that in the UI
+  // thread.
+  uniquifier = download_util::GetUniquePathNumber(new_path);
+  if (uniquifier > 0) {
+    download_util::AppendNumberToPath(&new_path, uniquifier);
+  }
+
+  // Rename the file, overwriting if necessary.
+  if (!download->Rename(new_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
     CancelDownloadOnRename(id);
+    return;
   }
 
-  // If the download has completed before we got this final name, we remove it
-  // from our in progress map.
-  if (!download->in_progress())
-    EraseDownload(id);
+#if defined(OS_MACOSX)
+  // Done here because we only want to do this once; see
+  // http://crbug.com/13120 for details.
+  download->AnnotateWithSourceInformation();
+#endif
 
-  if (downloads_.empty())
-    StopUpdateTimer();
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      NewRunnableMethod(
+          download_manager, &DownloadManager::OnDownloadRenamedToFinalName, id,
+          new_path, uniquifier));
 }
 
-// Called only from OnFinalDownloadName or OnIntermediateDownloadName
+// Called only from RenameInProgressDownloadFile and RenameFinishedDownloadFile
 // on the FILE thread.
 void DownloadFileManager::CancelDownloadOnRename(int id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -363,6 +384,9 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
 
   DownloadManager* download_manager = download->GetDownloadManager();
   if (!download_manager) {
+    // Without a download manager, we can't cancel the request normally, so we
+    // need to do it here.  The normal path will also update the download
+    // history before cancelling the request.
     download->CancelDownloadRequest(resource_dispatcher_host_);
     return;
   }
@@ -374,15 +398,21 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
 }
 
 void DownloadFileManager::EraseDownload(int id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
   if (!ContainsKey(downloads_, id))
     return;
 
   DownloadFile* download_file = downloads_[id];
 
-  downloads_.erase(id);
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " id = " << id
+           << " download_file = " << download_file->DebugString();
 
-  if (ContainsKey(downloads_with_final_name_, id))
-    downloads_with_final_name_.erase(id);
+  downloads_.erase(id);
 
   delete download_file;
+
+  if (downloads_.empty())
+    StopUpdateTimer();
 }
diff --git a/chrome/browser/download/download_file_manager.h b/chrome/browser/download/download_file_manager.h
index a0abc99..7b8a0b4 100644
--- a/chrome/browser/download/download_file_manager.h
+++ b/chrome/browser/download/download_file_manager.h
@@ -43,6 +43,7 @@
 #include <map>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/hash_tables.h"
 #include "base/ref_counted.h"
 #include "base/timer.h"
@@ -61,10 +62,6 @@ class URLRequestContextGetter;
 // Manages all in progress downloads.
 class DownloadFileManager
     : public base::RefCountedThreadSafe<DownloadFileManager> {
-
-  // For testing.
-  friend class DownloadManagerTest;
-
  public:
   explicit DownloadFileManager(ResourceDispatcherHost* rdh);
 
@@ -78,26 +75,34 @@ class DownloadFileManager
   void StartDownload(DownloadCreateInfo* info);
 
   // Handlers for notifications sent from the IO thread and run on the
-  // download thread.
+  // FILE thread.
   void UpdateDownload(int id, DownloadBuffer* buffer);
-  void CancelDownload(int id);
   void OnResponseCompleted(int id, DownloadBuffer* buffer);
 
+  // Handlers for notifications sent from the UI thread and run on the
+  // FILE thread.  These are both terminal actions with respect to the
+  // download file, as far as the DownloadFileManager is concerned -- if
+  // anything happens to the download file after they are called, it will
+  // be ignored.
+  void CancelDownload(int id);
+  void CompleteDownload(int id);
+
   // Called on FILE thread by DownloadManager at the beginning of its shutdown.
   void OnDownloadManagerShutdown(DownloadManager* manager);
 
   // The DownloadManager in the UI thread has provided an intermediate
-  // .crdownload name for the download specified by 'id'.
-  void OnIntermediateDownloadName(int id, const FilePath& full_path,
-                                  DownloadManager* download_manager);
+  // .crdownload name for the download specified by |id|.
+  void RenameInProgressDownloadFile(int id, const FilePath& full_path);
 
-  // The download manager has provided a final name for a download. Sent from
-  // the UI thread and run on the download thread.
-  void OnFinalDownloadName(int id, const FilePath& full_path,
-                           DownloadManager* download_manager);
+  // The DownloadManager in the UI thread has provided a final name for the
+  // download specified by |id|.  Sent from the UI thread and run on the
+  // FILE thread.
+  void RenameFinishedDownloadFile(int id, const FilePath& full_path);
 
  private:
   friend class base::RefCountedThreadSafe<DownloadFileManager>;
+  friend class DownloadManagerTest;
+  FRIEND_TEST_ALL_PREFIXES(DownloadManagerTest, StartDownload);
 
   ~DownloadFileManager();
 
@@ -122,12 +127,12 @@ class DownloadFileManager
   // Called only on the download thread.
   DownloadFile* GetDownloadFile(int id);
 
-  // Called only from OnFinalDownloadName or OnIntermediateDownloadName
-  // on the FILE thread.
+  // Called only from RenameInProgressDownloadFile and
+  // RenameFinishedDownloadFile on the FILE thread.
   void CancelDownloadOnRename(int id);
 
-  // Erases the download file with the given the download |id| and removes
-  // it from the maps.
+  // Called when the DownloadFileManager has finished with the download file.
+  // Will delete the download file if it hasn't been detached.
   void EraseDownload(int id);
 
   // Unique ID for each DownloadFile.
@@ -135,13 +140,9 @@ class DownloadFileManager
 
   typedef base::hash_map<int, DownloadFile*> DownloadFileMap;
 
-  // A map of all in progress downloads.  It owns the download files,
-  // although they may also show up in |downloads_with_final_name_|.
+  // A map of all in progress downloads.  It owns the download files.
   DownloadFileMap downloads_;
 
-  // download files are owned by |downloads_|.
-  DownloadFileMap downloads_with_final_name_;
-
   // Schedule periodic updates of the download progress. This timer
   // is controlled from the FILE thread, and posts updates to the UI thread.
   base::RepeatingTimer<DownloadFileManager> update_timer_;
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index d593608..3c303d0 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -359,6 +359,8 @@ void DownloadItem::OnAllDataSaved(int64 size) {
 }
 
 void DownloadItem::Finished() {
+  VLOG(20) << " " << __FUNCTION__ << "() "
+           << DebugString(false);
   // Handle chrome extensions explicitly and skip the shell execute.
   if (is_extension_install()) {
     download_util::OpenChromeExtension(download_manager_->profile(),
@@ -433,7 +435,7 @@ int DownloadItem::PercentComplete() const {
 
 void DownloadItem::Rename(const FilePath& full_path) {
   VLOG(20) << " " << __FUNCTION__ << "()"
-           << " full_path = " << full_path.value()
+           << " full_path = \"" << full_path.value() << "\""
            << DebugString(true);
   DCHECK(!full_path.empty());
   full_path_ = full_path;
@@ -447,6 +449,8 @@ void DownloadItem::TogglePause() {
 }
 
 void DownloadItem::OnNameFinalized() {
+  VLOG(20) << " " << __FUNCTION__ << "() "
+           << DebugString(true);
   name_finalized_ = true;
 
   // The download file is meant to be completed if both the filename is
@@ -459,34 +463,43 @@ void DownloadItem::OnNameFinalized() {
   }
 }
 
-void DownloadItem::OnSafeDownloadFinished(DownloadFileManager* file_manager) {
-  DCHECK_EQ(SAFE, safety_state());
+void DownloadItem::OnDownloadFinished(DownloadFileManager* file_manager) {
+  VLOG(20) << " " << __FUNCTION__ << "() "
+           << " needs rename = " << NeedsRename()
+           << " " << DebugString(true);
+  DCHECK_NE(DANGEROUS, safety_state());
   DCHECK(file_manager);
+
   if (NeedsRename()) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(
-            file_manager, &DownloadFileManager::OnFinalDownloadName,
-            id(), GetTargetFilePath(), make_scoped_refptr(download_manager_)));
+            file_manager, &DownloadFileManager::RenameFinishedDownloadFile,
+            id(), GetTargetFilePath()));
     return;
   }
 
   Finished();
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(
+          file_manager, &DownloadFileManager::CompleteDownload, id()));
 }
 
 void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
   VLOG(20) << " " << __FUNCTION__ << "()"
-           << " full_path = " << full_path.value();
-  bool needed_rename = NeedsRename();
+           << " full_path = " << full_path.value()
+           << " needed rename = " << NeedsRename()
+           << " " << DebugString(false);
+  DCHECK(NeedsRename());
 
   Rename(full_path);
   OnNameFinalized();
 
-  if (needed_rename && safety_state() == SAFE) {
-    // This was called from OnSafeDownloadFinished; continue to call
-    // DownloadFinished.
-    Finished();
-  }
+  // This was called from OnDownloadFinished; continue to call
+  // DownloadFinished.
+  Finished();
 }
 
 bool DownloadItem::MatchesQuery(const string16& query) const {
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index 5eadea8..9ffff60 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -202,10 +202,10 @@ class DownloadItem {
   // Called when the name of the download is finalized.
   void OnNameFinalized();
 
-  // Called when the download is finished for safe downloads.
+  // Called when the download is finished.
   // This may perform final rename if necessary and will eventually call
   // DownloadManager::DownloadFinished().
-  void OnSafeDownloadFinished(DownloadFileManager* file_manager);
+  void OnDownloadFinished(DownloadFileManager* file_manager);
 
   // Called when the file name for the download is renamed to its final name.
   void OnDownloadRenamedToFinalName(const FilePath& full_path);
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index f2e6ec7..c24f096 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -439,7 +439,8 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) {
                                     info->suggested_path,
                                     &file_type_info, 0, FILE_PATH_LITERAL(""),
                                     owning_window, info);
-    FOR_EACH_OBSERVER(Observer, observers_, SelectFileDialogDisplayed());
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      SelectFileDialogDisplayed(info->download_id));
   } else {
     // No prompting for download, just continue with the suggested name.
     info->path = info->suggested_path;
@@ -485,30 +486,29 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info) {
   UpdateAppIcon();  // Reflect entry into in_progress_.
 
   // Rename to intermediate name.
+  FilePath download_path;
   if (info->IsDangerous()) {
     // The download is not safe.  We can now rename the file to its
-    // tentative name using OnFinalDownloadName (the actual final
-    // name after user confirmation will be set in
-    // ProceedWithFinishedDangerousDownload).
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(
-            file_manager_, &DownloadFileManager::OnFinalDownloadName,
-            download->id(), info->path, make_scoped_refptr(this)));
+    // tentative name using RenameInProgressDownloadFile.
+    // NOTE: The |Rename| below will be a no-op for dangerous files, as we're
+    // renaming it to the same name.
+    download_path = info->path;
   } else {
     // The download is a safe download.  We need to
     // rename it to its intermediate '.crdownload' path.  The final
     // name after user confirmation will be set from
-    // DownloadItem::OnSafeDownloadFinished.
-    FilePath download_path = download_util::GetCrDownloadPath(info->path);
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(
-            file_manager_, &DownloadFileManager::OnIntermediateDownloadName,
-            download->id(), download_path, make_scoped_refptr(this)));
-    download->Rename(download_path);
+    // DownloadItem::OnDownloadFinished.
+    download_path = download_util::GetCrDownloadPath(info->path);
   }
 
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(
+          file_manager_, &DownloadFileManager::RenameInProgressDownloadFile,
+          download->id(), download_path));
+
+  download->Rename(download_path);
+
   download_history_->AddEntry(*info, download,
       NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete));
 }
@@ -627,30 +627,8 @@ void DownloadManager::MaybeCompleteDownload(DownloadItem* download) {
   download->MarkAsComplete();
   download_history_->UpdateEntry(download);
 
-  switch (download->safety_state()) {
-    case DownloadItem::DANGEROUS:
-      // If this a dangerous download not yet validated by the user, don't do
-      // anything. When the user notifies us, it will trigger a call to
-      // ProceedWithFinishedDangerousDownload.
-      NOTREACHED();
-      return;
-    case DownloadItem::DANGEROUS_BUT_VALIDATED:
-      // The dangerous download has been validated by the user.  We first
-      // need to rename the downloaded file from its temporary name to
-      // its final name.  We will continue the download processing in the
-      // callback.
-      BrowserThread::PostTask(
-          BrowserThread::FILE, FROM_HERE,
-          NewRunnableMethod(
-              this, &DownloadManager::ProceedWithFinishedDangerousDownload,
-              download->db_handle(),
-              download->full_path(), download->target_name()));
-      return;
-    case DownloadItem::SAFE:
-      // The download is safe; just finish it.
-      download->OnSafeDownloadFinished(file_manager_);
-      return;
-  }
+  // Finish the download.
+  download->OnDownloadFinished(file_manager_);
 }
 
 void DownloadManager::RemoveFromActiveList(int32 download_id) {
@@ -658,72 +636,39 @@ void DownloadManager::RemoveFromActiveList(int32 download_id) {
   active_downloads_.erase(download_id);
 }
 
-void DownloadManager::DownloadRenamedToFinalName(int download_id,
-                                                 const FilePath& full_path) {
+void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
+                                                   const FilePath& full_path,
+                                                   int uniquifier) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
-           << " full_path = \"" << full_path.value() << "\"";
+           << " full_path = \"" << full_path.value() << "\""
+           << " uniquifier = " << uniquifier;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   DownloadItem* item = GetDownloadItem(download_id);
   if (!item)
     return;
-  item->OnDownloadRenamedToFinalName(full_path);
-}
-
-// Called on the file thread.  Renames the downloaded file to its original name.
-void DownloadManager::ProceedWithFinishedDangerousDownload(
-    int64 download_handle,
-    const FilePath& path,
-    const FilePath& original_name) {
-  bool success = false;
-  FilePath new_path;
-  int uniquifier = 0;
-  if (file_util::PathExists(path)) {
-    new_path = path.DirName().Append(original_name);
-    // Make our name unique at this point, as if a dangerous file is downloading
-    // and a 2nd download is started for a file with the same name, they would
-    // have the same path.  This is because we uniquify the name on download
-    // start, and at that time the first file does not exists yet, so the second
-    // file gets the same name.
-    uniquifier = download_util::GetUniquePathNumber(new_path);
-    if (uniquifier > 0)
-      download_util::AppendNumberToPath(&new_path, uniquifier);
-    success = file_util::Move(path, new_path);
-  } else {
-    NOTREACHED();
-  }
 
   BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(this, &DownloadManager::DangerousDownloadRenamed,
-                        download_handle, success, new_path, uniquifier));
-}
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(
+          file_manager_, &DownloadFileManager::CompleteDownload, download_id));
 
-// Call from the file thread when the finished dangerous download was renamed.
-void DownloadManager::DangerousDownloadRenamed(int64 download_handle,
-                                               bool success,
-                                               const FilePath& new_path,
-                                               int new_path_uniquifier) {
-  VLOG(20) << __FUNCTION__ << "()" << " download_handle = " << download_handle
-           << " success = " << success
-           << " new_path = \"" << new_path.value() << "\""
-           << " new_path_uniquifier = " << new_path_uniquifier;
-  DownloadMap::iterator it = history_downloads_.find(download_handle);
-  if (it == history_downloads_.end()) {
-    NOTREACHED();
+  if ((item->safety_state() == DownloadItem::SAFE) && (uniquifier != 0)) {
+    // File name conflict: the file name we expected to use at the start
+    // of the safe download was taken.
+    // TODO(ahendrickson): Warn the user that we're cancelling the download.
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        NewRunnableMethod(
+            file_manager_, &DownloadFileManager::CancelDownload, download_id));
     return;
   }
 
-  DownloadItem* download = it->second;
-  // If we failed to rename the file, we'll just keep the name as is.
-  if (success) {
-    // We need to update the path uniquifier so that the UI shows the right
-    // name when calling GetFileNameToReportUser().
-    download->set_path_uniquifier(new_path_uniquifier);
-    RenameDownload(download, new_path);
-  }
+  if (uniquifier)
+    item->set_path_uniquifier(uniquifier);
 
-  // Continue the download finished sequence.
-  download->Finished();
+  item->OnDownloadRenamedToFinalName(full_path);
+  download_history_->UpdateDownloadPath(item, full_path);
 }
 
 void DownloadManager::DownloadCancelled(int32 download_id) {
@@ -753,6 +698,7 @@ void DownloadManager::DownloadCancelled(int32 download_id) {
 void DownloadManager::DownloadCancelledInternal(int download_id,
                                                 int render_process_id,
                                                 int request_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // Cancel the network request.  RDH is guaranteed to outlive the IO thread.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
@@ -791,12 +737,6 @@ void DownloadManager::UpdateAppIcon() {
     status_updater_->Update();
 }
 
-void DownloadManager::RenameDownload(DownloadItem* download,
-                                     const FilePath& new_path) {
-  download->Rename(new_path);
-  download_history_->UpdateDownloadPath(download, new_path);
-}
-
 void DownloadManager::PauseDownloadRequest(ResourceDispatcherHost* rdh,
                                            int render_process_id,
                                            int request_id,
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 070693cd..42d4ed9 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -83,7 +83,8 @@ class DownloadManager
 
     // Called immediately after the DownloadManager puts up a select file
     // dialog.
-    virtual void SelectFileDialogDisplayed() {}
+    // |id| indicates which download opened the dialog.
+    virtual void SelectFileDialogDisplayed(int32 id) {}
 
    protected:
     virtual ~Observer() {}
@@ -135,7 +136,11 @@ class DownloadManager
   void MaybeCompleteDownload(DownloadItem* download);
 
   // Called when the download is renamed to its final name.
-  void DownloadRenamedToFinalName(int download_id, const FilePath& full_path);
+  // |uniquifier| is a number used to make unique names for the file.  It is
+  // only valid for the DANGEROUS_BUT_VALIDATED state of the download item.
+  void OnDownloadRenamedToFinalName(int download_id,
+                                    const FilePath& full_path,
+                                    int uniquifier);
 
   // Remove downloads after remove_begin (inclusive) and before remove_end
   // (exclusive). You may pass in null Time values to do an unbounded delete
@@ -282,26 +287,9 @@ class DownloadManager
                                  int render_process_id,
                                  int request_id);
 
-  // Renames a finished dangerous download from its temporary file name to its
-  // real file name.
-  // Invoked on the file thread.
-  void ProceedWithFinishedDangerousDownload(int64 download_handle,
-                                            const FilePath& path,
-                                            const FilePath& original_name);
-
-  // Invoked on the UI thread when a dangerous downloaded file has been renamed.
-  void DangerousDownloadRenamed(int64 download_handle,
-                                bool success,
-                                const FilePath& new_path,
-                                int new_path_uniquifier);
-
   // Updates the app icon about the overall download progress.
   void UpdateAppIcon();
 
-  // Changes the paths and file name of the specified |download|, propagating
-  // the change to the history system.
-  void RenameDownload(DownloadItem* download, const FilePath& new_path);
-
   // Makes the ResourceDispatcherHost pause/un-pause a download request.
   // Called on the IO thread.
   void PauseDownloadRequest(ResourceDispatcherHost* rdh,
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 683a993..a857d34 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include <string>
+#include <set>
 
+#include "base/scoped_ptr.h"
 #include "base/string_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/download/download_file.h"
@@ -28,7 +30,8 @@ class DownloadManagerTest : public testing::Test {
   DownloadManagerTest()
       : profile_(new TestingProfile()),
         download_manager_(new MockDownloadManager(&download_status_updater_)),
-        ui_thread_(BrowserThread::UI, &message_loop_) {
+        ui_thread_(BrowserThread::UI, &message_loop_),
+        file_thread_(BrowserThread::FILE, &message_loop_) {
     download_manager_->Init(profile_.get());
   }
 
@@ -52,6 +55,7 @@ class DownloadManagerTest : public testing::Test {
   scoped_refptr<DownloadFileManager> file_manager_;
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
+  BrowserThread file_thread_;
 
   DownloadFileManager* file_manager() {
     if (!file_manager_) {
@@ -126,34 +130,6 @@ const struct {
     true, },
 };
 
-}  // namespace
-
-TEST_F(DownloadManagerTest, StartDownload) {
-  BrowserThread io_thread(BrowserThread::IO, &message_loop_);
-  PrefService* prefs = profile_->GetPrefs();
-  prefs->SetFilePath(prefs::kDownloadDefaultDirectory, FilePath());
-  download_manager_->download_prefs()->EnableAutoOpenBasedOnExtension(
-      FilePath(FILE_PATH_LITERAL("example.pdf")));
-
-  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) {
-    prefs->SetBoolean(prefs::kPromptForDownload,
-                      kStartDownloadCases[i].prompt_for_download);
-
-    DownloadCreateInfo info;
-    info.prompt_user_for_save_location = kStartDownloadCases[i].save_as;
-    info.url = GURL(kStartDownloadCases[i].url);
-    info.mime_type = kStartDownloadCases[i].mime_type;
-
-    download_manager_->StartDownload(&info);
-    message_loop_.RunAllPending();
-
-    EXPECT_EQ(kStartDownloadCases[i].expected_save_as,
-        info.prompt_user_for_save_location);
-  }
-}
-
-namespace {
-
 const struct {
   FilePath::StringType suggested_path;
   bool is_dangerous_file;
@@ -191,8 +167,8 @@ const struct {
 
 class MockDownloadFile : public DownloadFile {
  public:
-  explicit MockDownloadFile(DownloadCreateInfo* info)
-      : DownloadFile(info, NULL), renamed_count_(0) { }
+  MockDownloadFile(DownloadCreateInfo* info, DownloadManager* manager)
+      : DownloadFile(info, manager), renamed_count_(0) { }
   virtual ~MockDownloadFile() { Destructed(); }
   MOCK_METHOD1(Rename, bool(const FilePath&));
   MOCK_METHOD0(Destructed, void());
@@ -210,16 +186,87 @@ class MockDownloadFile : public DownloadFile {
   int renamed_count_;
 };
 
+// This is an observer that records what download IDs have opened a select
+// file dialog.
+class SelectFileObserver : public DownloadManager::Observer {
+ public:
+  explicit SelectFileObserver(DownloadManager* download_manager)
+      : download_manager_(download_manager) {
+    DCHECK(download_manager_);
+    download_manager_->AddObserver(this);
+  }
+
+  ~SelectFileObserver() {
+    download_manager_->RemoveObserver(this);
+  }
+
+  // Downloadmanager::Observer functions.
+  virtual void ModelChanged() {}
+  virtual void ManagerGoingDown() {}
+  virtual void SelectFileDialogDisplayed(int32 id) {
+    file_dialog_ids_.insert(id);
+  }
+
+  bool ShowedFileDialogForId(int32 id) {
+    return file_dialog_ids_.find(id) != file_dialog_ids_.end();
+  }
+
+ private:
+  std::set<int32> file_dialog_ids_;
+  DownloadManager* download_manager_;
+};
+
 }  // namespace
 
+TEST_F(DownloadManagerTest, StartDownload) {
+  BrowserThread io_thread(BrowserThread::IO, &message_loop_);
+  PrefService* prefs = profile_->GetPrefs();
+  prefs->SetFilePath(prefs::kDownloadDefaultDirectory, FilePath());
+  download_manager_->download_prefs()->EnableAutoOpenBasedOnExtension(
+      FilePath(FILE_PATH_LITERAL("example.pdf")));
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) {
+    prefs->SetBoolean(prefs::kPromptForDownload,
+                      kStartDownloadCases[i].prompt_for_download);
+
+    SelectFileObserver observer(download_manager_);
+    DownloadCreateInfo* info = new DownloadCreateInfo;
+    info->download_id = static_cast<int>(i);
+    info->prompt_user_for_save_location = kStartDownloadCases[i].save_as;
+    info->url = GURL(kStartDownloadCases[i].url);
+    info->mime_type = kStartDownloadCases[i].mime_type;
+    download_manager_->CreateDownloadItem(info);
+
+    DownloadFile* download(new DownloadFile(info, download_manager_));
+    AddDownloadToFileManager(info->download_id, download);
+    download->Initialize(false);
+    download_manager_->StartDownload(info);
+    message_loop_.RunAllPending();
+
+    // NOTE: At this point, |AttachDownloadItem| will have been run if we don't
+    // need to prompt the user, so |info| could have been destructed.
+    // This means that we can't check any of its values.
+    // However, SelectFileObserver will have recorded any attempt to open the
+    // select file dialog.
+    EXPECT_EQ(kStartDownloadCases[i].expected_save_as,
+              observer.ShowedFileDialogForId(i));
+
+    // If the Save As dialog pops up, it never reached
+    // DownloadManager::AttachDownloadItem(), and never deleted info or
+    // completed.  This cleans up info.
+    // Note that DownloadManager::FileSelectionCanceled() is never called.
+    if (observer.ShowedFileDialogForId(i)) {
+      delete info;
+    }
+  }
+}
+
 TEST_F(DownloadManagerTest, DownloadRenameTest) {
   using ::testing::_;
   using ::testing::CreateFunctor;
   using ::testing::Invoke;
   using ::testing::Return;
 
-  BrowserThread file_thread(BrowserThread::FILE, &message_loop_);
-
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDownloadRenameCases); ++i) {
     // |info| will be destroyed in download_manager_.
     DownloadCreateInfo* info(new DownloadCreateInfo);
@@ -229,7 +276,7 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
     info->is_dangerous_url = kDownloadRenameCases[i].is_dangerous_url;
     FilePath new_path(kDownloadRenameCases[i].suggested_path);
 
-    MockDownloadFile* download(new MockDownloadFile(info));
+    MockDownloadFile* download(new MockDownloadFile(info, download_manager_));
     AddDownloadToFileManager(info->download_id, download);
 
     // |download| is owned by DownloadFileManager.
@@ -253,9 +300,11 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
 
     if (kDownloadRenameCases[i].finish_before_rename) {
       download_manager_->OnAllDataSaved(i, 1024, std::string("fake_hash"));
+      message_loop_.RunAllPending();
       download_manager_->FileSelected(new_path, i, info);
     } else {
       download_manager_->FileSelected(new_path, i, info);
+      message_loop_.RunAllPending();
       download_manager_->OnAllDataSaved(i, 1024, std::string("fake_hash"));
     }
 
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index f00b40b..62ff15f 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -757,7 +757,9 @@ void CancelDownloadRequest(ResourceDispatcherHost* rdh,
                            int render_process_id,
                            int request_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  rdh->CancelRequest(render_process_id, request_id, false);
+  // |rdh| may be NULL in unit tests.
+  if (rdh)
+    rdh->CancelRequest(render_process_id, request_id, false);
 }
 
 void NotifyDownloadInitiated(int render_process_id, int render_view_id) {
diff --git a/chrome/browser/download/save_file_manager.cc b/chrome/browser/download/save_file_manager.cc
index 8240b9f..dfce49d 100644
--- a/chrome/browser/download/save_file_manager.cc
+++ b/chrome/browser/download/save_file_manager.cc
@@ -262,10 +262,16 @@ void SaveFileManager::SaveFinished(int save_id,
                                    const GURL& save_url,
                                    int render_process_id,
                                    bool is_success) {
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " save_id = " << save_id
+           << " save_url = \"" << save_url.spec() << "\""
+           << " is_success = " << is_success;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   SaveFileMap::iterator it = save_file_map_.find(save_id);
   if (it != save_file_map_.end()) {
     SaveFile* save_file = it->second;
+  VLOG(20) << " " << __FUNCTION__ << "()"
+           << " save_file = " << save_file->DebugString();
     BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
         NewRunnableMethod(
@@ -273,6 +279,7 @@ void SaveFileManager::SaveFinished(int save_id,
             save_file->bytes_so_far(), is_success));
 
     save_file->Finish();
+    save_file->Detach();
   } else if (save_id == -1) {
     // Before saving started, we got error. We still call finish process.
     DCHECK(!save_url.is_empty());
@@ -438,6 +445,7 @@ void SaveFileManager::SaveLocalFile(const GURL& original_file_url,
 
   // Close the save file before the copy operation.
   save_file->Finish();
+  save_file->Detach();
 
   DCHECK(original_file_url.SchemeIsFile());
   FilePath file_path;
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index abb0a21..0c29107 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -1018,6 +1018,9 @@ void SavePackage::OnReceivedSerializedHtmlData(const GURL& frame_url,
   if (flag == WebPageSerializerClient::AllFramesAreFinished) {
     for (SaveUrlItemMap::iterator it = in_progress_items_.begin();
          it != in_progress_items_.end(); ++it) {
+      VLOG(20) << " " << __FUNCTION__ << "()"
+               << " save_id = " << it->second->save_id()
+               << " url = \"" << it->second->url().spec() << "\"";
       BrowserThread::PostTask(
           BrowserThread::FILE, FROM_HERE,
           NewRunnableMethod(file_manager_,
@@ -1053,6 +1056,9 @@ void SavePackage::OnReceivedSerializedHtmlData(const GURL& frame_url,
 
   // Current frame is completed saving, call finish in file thread.
   if (flag == WebPageSerializerClient::CurrentFrameIsFinished) {
+    VLOG(20) << " " << __FUNCTION__ << "()"
+             << " save_id = " << save_item->save_id()
+             << " url = \"" << save_item->url().spec() << "\"";
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(file_manager_,
diff --git a/chrome/browser/ui/gtk/dialogs_gtk.cc b/chrome/browser/ui/gtk/dialogs_gtk.cc
index c33a7d6..98a4025 100644
--- a/chrome/browser/ui/gtk/dialogs_gtk.cc
+++ b/chrome/browser/ui/gtk/dialogs_gtk.cc
@@ -163,8 +163,7 @@ FilePath* SelectFileDialogImpl::last_opened_path_ = NULL;
 
 // static
 SelectFileDialog* SelectFileDialog::Create(Listener* listener) {
-  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return new SelectFileDialogImpl(listener);
 }