Detect file system errors during downloads.

Moving towards giving the user feedback when a file system error occurs during a download.

Split from CL 7134019.

BUG=85240
TEST=None


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

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

9 files changed, 204 insertions, 95 deletions

diff --git a/content/browser/download/base_file.cc b/content/browser/download/base_file.cc
index 0a40369..87d60f8 100644
--- a/content/browser/download/base_file.cc
+++ b/content/browser/download/base_file.cc
@@ -203,6 +203,10 @@ void BaseFile::AnnotateWithSourceInformation() {
 #endif
 }
 
+void BaseFile::CreateFileStream() {
+  file_stream_.reset(new net::FileStream);
+}
+
 bool BaseFile::Open() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(!detached_);
@@ -210,7 +214,7 @@ bool BaseFile::Open() {
 
   // Create a new file stream if it is not provided.
   if (!file_stream_.get()) {
-    file_stream_.reset(new net::FileStream);
+    CreateFileStream();
     if (file_stream_->Open(full_path_,
                            base::PLATFORM_FILE_OPEN_ALWAYS |
                                base::PLATFORM_FILE_WRITE) != net::OK) {
diff --git a/content/browser/download/base_file.h b/content/browser/download/base_file.h
index d07fb1d..ff119d4 100644
--- a/content/browser/download/base_file.h
+++ b/content/browser/download/base_file.h
@@ -65,6 +65,7 @@ class BaseFile {
   virtual std::string DebugString() const;
 
  protected:
+  virtual void CreateFileStream();  // For testing.
   bool Open();
   void Close();
 
@@ -72,6 +73,9 @@ class BaseFile {
   FilePath full_path_;
 
  private:
+  friend class BaseFileTest;
+  friend class DownloadFileWithMockStream;
+
   static const size_t kSha256HashLen = 32;
 
   // Source URL for the file being downloaded.
diff --git a/content/browser/download/base_file_unittest.cc b/content/browser/download/base_file_unittest.cc
index ce8acb5..5848f6f 100644
--- a/content/browser/download/base_file_unittest.cc
+++ b/content/browser/download/base_file_unittest.cc
@@ -2,13 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "content/browser/download/base_file.h"
+
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
 #include "base/string_number_conversions.h"
 #include "content/browser/browser_thread.h"
-#include "content/browser/download/base_file.h"
 #include "net/base/file_stream.h"
+#include "net/base/mock_file_stream.h"
+#include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -17,6 +20,8 @@ const char kTestData1[] = "Let's write some data to the file!\n";
 const char kTestData2[] = "Writing more data.\n";
 const char kTestData3[] = "Final line.";
 
+}  // namespace
+
 class BaseFileTest : public testing::Test {
  public:
   BaseFileTest()
@@ -51,16 +56,41 @@ class BaseFileTest : public testing::Test {
     EXPECT_EQ(expect_file_survives_, file_util::PathExists(full_path));
   }
 
-  void AppendDataToFile(const std::string& data) {
-    ASSERT_TRUE(base_file_->in_progress());
-    base_file_->AppendDataToFile(data.data(), data.size());
+  bool OpenMockFileStream() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+    FilePath path;
+    if (!file_util::CreateTemporaryFile(&path))
+      return false;
+
+    // Create a new file stream.
+    mock_file_stream_.reset(new net::testing::MockFileStream);
+    if (mock_file_stream_->Open(
+        path,
+        base::PLATFORM_FILE_OPEN_ALWAYS | base::PLATFORM_FILE_WRITE) != 0) {
+      mock_file_stream_.reset();
+      return false;
+    }
+
+    return true;
+  }
+
+  void ForceError(net::Error error) {
+    mock_file_stream_->set_forced_error(error);
+  }
+
+  bool AppendDataToFile(const std::string& data) {
+    EXPECT_TRUE(base_file_->in_progress());
+    bool appended = base_file_->AppendDataToFile(data.data(), data.size());
     expected_data_ += data;
     EXPECT_EQ(static_cast<int64>(expected_data_.size()),
               base_file_->bytes_so_far());
+    return appended;
   }
 
  protected:
   linked_ptr<net::FileStream> file_stream_;
+  linked_ptr<net::testing::MockFileStream> mock_file_stream_;
 
   // BaseClass instance we are testing.
   scoped_ptr<BaseFile> base_file_;
@@ -100,7 +130,7 @@ TEST_F(BaseFileTest, Cancel) {
 // automatically when base_file_ is destructed.
 TEST_F(BaseFileTest, WriteAndDetach) {
   ASSERT_TRUE(base_file_->Initialize(false));
-  AppendDataToFile(kTestData1);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
   base_file_->Finish();
   base_file_->Detach();
   expect_file_survives_ = true;
@@ -109,7 +139,7 @@ TEST_F(BaseFileTest, WriteAndDetach) {
 // 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);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
   base_file_->Finish();
 
   std::string hash;
@@ -130,7 +160,7 @@ TEST_F(BaseFileTest, WriteThenRenameAndDetach) {
   FilePath new_path(temp_dir_.path().AppendASCII("NewFile"));
   EXPECT_FALSE(file_util::PathExists(new_path));
 
-  AppendDataToFile(kTestData1);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
 
   EXPECT_TRUE(base_file_->Rename(new_path));
   EXPECT_FALSE(file_util::PathExists(initial_path));
@@ -144,16 +174,16 @@ TEST_F(BaseFileTest, WriteThenRenameAndDetach) {
 // Write data to the file once.
 TEST_F(BaseFileTest, SingleWrite) {
   ASSERT_TRUE(base_file_->Initialize(false));
-  AppendDataToFile(kTestData1);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
   base_file_->Finish();
 }
 
 // Write data to the file multiple times.
 TEST_F(BaseFileTest, MultipleWrites) {
   ASSERT_TRUE(base_file_->Initialize(false));
-  AppendDataToFile(kTestData1);
-  AppendDataToFile(kTestData2);
-  AppendDataToFile(kTestData3);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
+  ASSERT_TRUE(AppendDataToFile(kTestData2));
+  ASSERT_TRUE(AppendDataToFile(kTestData3));
   std::string hash;
   EXPECT_FALSE(base_file_->GetSha256Hash(&hash));
   base_file_->Finish();
@@ -162,7 +192,7 @@ TEST_F(BaseFileTest, MultipleWrites) {
 // Write data to the file once and calculate its sha256 hash.
 TEST_F(BaseFileTest, SingleWriteWithHash) {
   ASSERT_TRUE(base_file_->Initialize(true));
-  AppendDataToFile(kTestData1);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
   base_file_->Finish();
 
   std::string hash;
@@ -176,9 +206,9 @@ TEST_F(BaseFileTest, MultipleWritesWithHash) {
   std::string hash;
 
   ASSERT_TRUE(base_file_->Initialize(true));
-  AppendDataToFile(kTestData1);
-  AppendDataToFile(kTestData2);
-  AppendDataToFile(kTestData3);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
+  ASSERT_TRUE(AppendDataToFile(kTestData2));
+  ASSERT_TRUE(AppendDataToFile(kTestData3));
   // no hash before Finish() is called either.
   EXPECT_FALSE(base_file_->GetSha256Hash(&hash));
   base_file_->Finish();
@@ -197,7 +227,7 @@ TEST_F(BaseFileTest, WriteThenRename) {
   FilePath new_path(temp_dir_.path().AppendASCII("NewFile"));
   EXPECT_FALSE(file_util::PathExists(new_path));
 
-  AppendDataToFile(kTestData1);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
 
   EXPECT_TRUE(base_file_->Rename(new_path));
   EXPECT_FALSE(file_util::PathExists(initial_path));
@@ -215,16 +245,29 @@ TEST_F(BaseFileTest, RenameWhileInProgress) {
   FilePath new_path(temp_dir_.path().AppendASCII("NewFile"));
   EXPECT_FALSE(file_util::PathExists(new_path));
 
-  AppendDataToFile(kTestData1);
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
 
   EXPECT_TRUE(base_file_->in_progress());
   EXPECT_TRUE(base_file_->Rename(new_path));
   EXPECT_FALSE(file_util::PathExists(initial_path));
   EXPECT_TRUE(file_util::PathExists(new_path));
 
-  AppendDataToFile(kTestData2);
+  ASSERT_TRUE(AppendDataToFile(kTestData2));
 
   base_file_->Finish();
 }
 
-}  // namespace
+// Write data to the file multiple times.
+TEST_F(BaseFileTest, MultipleWritesWithError) {
+  ASSERT_TRUE(OpenMockFileStream());
+  base_file_.reset(new BaseFile(mock_file_stream_->get_path(),
+                                GURL(), GURL(), 0, mock_file_stream_));
+  ASSERT_TRUE(base_file_->Initialize(false));
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
+  ASSERT_TRUE(AppendDataToFile(kTestData2));
+  ForceError(net::ERR_FAILED);
+  ASSERT_FALSE(AppendDataToFile(kTestData3));
+  std::string hash;
+  EXPECT_FALSE(base_file_->GetSha256Hash(&hash));
+  base_file_->Finish();
+}
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 7847c5f..f21c67b1 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -153,11 +153,38 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
   }
 
   DownloadFile* download_file = GetDownloadFile(id);
+  bool had_error = false;
   for (size_t i = 0; i < contents.size(); ++i) {
     net::IOBuffer* data = contents[i].first;
     const int data_len = contents[i].second;
-    if (download_file)
-      download_file->AppendDataToFile(data->data(), data_len);
+    if (!had_error && download_file) {
+      bool write_succeeded =
+          download_file->AppendDataToFile(data->data(), data_len);
+      if (!write_succeeded) {
+        // Write failed: interrupt the download.
+        DownloadManager* download_manager = download_file->GetDownloadManager();
+        had_error = true;
+
+        int64 bytes_downloaded = download_file->bytes_so_far();
+        // Calling this here in case we get more data, to avoid
+        // processing data after an error.  That could lead to
+        // files that are corrupted if the later processing succeeded.
+        CancelDownload(id);
+        download_file = NULL;  // Was deleted in |CancelDownload|.
+
+        if (download_manager) {
+          BrowserThread::PostTask(
+              BrowserThread::UI,
+              FROM_HERE,
+              NewRunnableMethod(
+                  download_manager,
+                  &DownloadManager::OnDownloadError,
+                  id,
+                  bytes_downloaded,
+                  net::ERR_FAILED));
+        }
+      }
+    }
     data->Release();
   }
 }
@@ -165,10 +192,10 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
 void DownloadFileManager::OnResponseCompleted(
     int id,
     DownloadBuffer* buffer,
-    int os_error,
+    int net_error,
     const std::string& security_info) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id
-           << " os_error = " << os_error
+           << " net_error = " << net_error
            << " security_info = \"" << security_info << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   delete buffer;
@@ -188,11 +215,32 @@ void DownloadFileManager::OnResponseCompleted(
   if (!download_file->GetSha256Hash(&hash))
     hash.clear();
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(
-        download_manager, &DownloadManager::OnResponseCompleted,
-        id, download_file->bytes_so_far(), os_error, hash));
+  // ERR_CONNECTION_CLOSED is allowed since a number of servers in the wild
+  // advertise a larger Content-Length than the amount of bytes in the message
+  // body, and then close the connection. Other browsers - IE8, Firefox 4.0.1,
+  // and Safari 5.0.4 - treat the download as complete in this case, so we
+  // follow their lead.
+  if (net_error == net::OK || net_error == net::ERR_CONNECTION_CLOSED) {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        NewRunnableMethod(
+            download_manager,
+            &DownloadManager::OnResponseCompleted,
+            id,
+            download_file->bytes_so_far(),
+            hash));
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        NewRunnableMethod(
+            download_manager,
+            &DownloadManager::OnDownloadError,
+            id,
+            download_file->bytes_so_far(),
+            net_error));
+  }
   // We need to keep the download around until the UI thread has finalized
   // the name.
 }
@@ -278,7 +326,7 @@ void DownloadFileManager::RenameInProgressDownloadFile(
   if (!download_file->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.
-    CancelDownloadOnRename(id);
+    CancelDownloadOnRename(id, net::ERR_FAILED);
   }
 }
 
@@ -326,7 +374,7 @@ void DownloadFileManager::RenameCompletingDownloadFile(
   if (!download_file->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);
+    CancelDownloadOnRename(id, net::ERR_FAILED);
     return;
   }
 
@@ -345,7 +393,7 @@ void DownloadFileManager::RenameCompletingDownloadFile(
 
 // Called only from RenameInProgressDownloadFile and
 // RenameCompletingDownloadFile on the FILE thread.
-void DownloadFileManager::CancelDownloadOnRename(int id) {
+void DownloadFileManager::CancelDownloadOnRename(int id, int rename_error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   DownloadFile* download_file = GetDownloadFile(id);
@@ -356,7 +404,7 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   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.
+    // history before canceling the request.
     download_file->CancelDownloadRequest();
     return;
   }
@@ -364,7 +412,10 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::CancelDownload, id));
+                        &DownloadManager::OnDownloadError,
+                        id,
+                        download_file->bytes_so_far(),
+                        rename_error));
 }
 
 void DownloadFileManager::EraseDownload(int id) {
diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h
index 0c14dca..f32a2f2 100644
--- a/content/browser/download/download_file_manager.h
+++ b/content/browser/download/download_file_manager.h
@@ -82,14 +82,14 @@ class DownloadFileManager
   // Handlers for notifications sent from the IO thread and run on the
   // FILE thread.
   void UpdateDownload(int id, DownloadBuffer* buffer);
-  // |os_error| is 0 for normal completions, and non-0 for errors.
+  // |net_error| is 0 for normal completions, and non-0 for errors.
   // |security_info| contains SSL information (cert_id, cert_status,
   // security_bits, ssl_connection_status), which can be used to
   // fine-tune the error message.  It is empty if the transaction
   // was not performed securely.
   void OnResponseCompleted(int id,
                            DownloadBuffer* buffer,
-                           int os_error,
+                           int net_error,
                            const std::string& security_info);
 
   // Handlers for notifications sent from the UI thread and run on the
@@ -148,7 +148,8 @@ class DownloadFileManager
 
   // Called only from RenameInProgressDownloadFile and
   // RenameCompletingDownloadFile on the FILE thread.
-  void CancelDownloadOnRename(int id);
+  // |rename_error| indicates what network error caused the cancel.
+  void CancelDownloadOnRename(int id, int rename_error);
 
   // Erases the download file with the given the download |id| and removes
   // it from the maps.
diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc
index ab3159e..9f3f9dd 100644
--- a/content/browser/download/download_file_unittest.cc
+++ b/content/browser/download/download_file_unittest.cc
@@ -78,8 +78,8 @@ class DownloadFileTest : public testing::Test {
                                             &disk_data));
     EXPECT_EQ(expected_data_, disk_data);
 
-    // Make sure the mock BrowserThread outlives the DownloadFile to satisfy
-    // thread checks inside it.
+    // Make sure the Browser and File threads outlive the DownloadFile
+    // to satisfy thread checks inside it.
     file->reset();
   }
 
diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index 5476aa1..13e884b 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -181,9 +181,9 @@ class DownloadItem {
   void OnDownloadedFileRemoved();
 
   // Download operation had an error.
-  // |size| is the amount of data received so far, and |os_error| is the error
-  // code that the operation received.
-  void Interrupted(int64 size, int os_error);
+  // |size| is the amount of data received at interruption.
+  // |error| is the network error code that the operation received.
+  void Interrupted(int64 size, int error);
 
   // Deletes the file from disk and removes the download from the views and
   // history.  |user| should be true if this is the result of the user clicking
diff --git a/content/browser/download/download_manager.cc b/content/browser/download/download_manager.cc
index bc12c56..420d397 100644
--- a/content/browser/download/download_manager.cc
+++ b/content/browser/download/download_manager.cc
@@ -350,24 +350,7 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
 
 void DownloadManager::OnResponseCompleted(int32 download_id,
                                           int64 size,
-                                          int os_error,
                                           const std::string& hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // ERR_CONNECTION_CLOSED is allowed since a number of servers in the wild
-  // advertise a larger Content-Length than the amount of bytes in the message
-  // body, and then close the connection. Other browsers - IE8, Firefox 4.0.1,
-  // and Safari 5.0.4 - treat the download as complete in this case, so we
-  // follow their lead.
-  if (os_error == 0 || os_error == net::ERR_CONNECTION_CLOSED) {
-    OnAllDataSaved(download_id, size, hash);
-  } else {
-    OnDownloadError(download_id, size, os_error);
-  }
-}
-
-void DownloadManager::OnAllDataSaved(int32 download_id,
-                                     int64 size,
-                                     const std::string& hash) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " size = " << size;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -512,8 +495,9 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
 }
 
 void DownloadManager::CancelDownload(int32 download_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadItem* download = GetDownloadItem(download_id);
+  DownloadItem* download = GetActiveDownload(download_id);
+  // A cancel at the right time could remove the download from the
+  // |active_downloads_| map before we get here.
   if (!download)
     return;
 
@@ -522,61 +506,63 @@ void DownloadManager::CancelDownload(int32 download_id) {
 
 void DownloadManager::DownloadCancelledInternal(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  int download_id = download->id();
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  // Clean up will happen when the history system create callback runs if we
-  // don't have a valid db_handle yet.
-  if (download->db_handle() != DownloadItem::kUninitializedHandle) {
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
-    UpdateDownloadProgress();  // Reflect removal from in_progress_.
-    delegate_->UpdateItemInPersistentStore(download);
-
-    // This function is called from the DownloadItem, so DI state
-    // should already have been updated.
-    AssertQueueStateConsistent(download);
-  }
+  RemoveFromActiveList(download);
+  // This function is called from the DownloadItem, so DI state
+  // should already have been updated.
+  AssertQueueStateConsistent(download);
 
   download->OffThreadCancel(file_manager_);
 }
 
 void DownloadManager::OnDownloadError(int32 download_id,
                                       int64 size,
-                                      int os_error) {
+                                      int error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadItem* download = GetActiveDownload(download_id);
+  if (!download)
+    return;
+
+  VLOG(20) << __FUNCTION__ << "()" << " Error " << error
+           << " at offset " << download->received_bytes()
+           << " size = " << size
+           << " download = " << download->DebugString(true);
+
+  RemoveFromActiveList(download);
+  download->Interrupted(size, error);
+  download->OffThreadCancel(file_manager_);
+}
+
+DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
-  // A cancel at the right time could remove the download from the
-  // |active_downloads_| map before we get here.
   if (it == active_downloads_.end())
-    return;
+    return NULL;
 
   DownloadItem* download = it->second;
 
-  VLOG(20) << __FUNCTION__ << "()" << " Error " << os_error
-           << " at offset " << download->received_bytes()
-           << " for download = " << download->DebugString(true);
+  DCHECK(download);
+  DCHECK_EQ(download_id, download->id());
+
+  return download;
+}
 
-  download->Interrupted(size, os_error);
+void DownloadManager::RemoveFromActiveList(DownloadItem* download) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(download);
 
-  // TODO(ahendrickson) - Remove this when we add resuming of interrupted
-  // downloads, as we will keep the download item around in that case.
-  //
   // Clean up will happen when the history system create callback runs if we
   // don't have a valid db_handle yet.
   if (download->db_handle() != DownloadItem::kUninitializedHandle) {
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
+    in_progress_.erase(download->id());
+    active_downloads_.erase(download->id());
     UpdateDownloadProgress();  // Reflect removal from in_progress_.
     delegate_->UpdateItemInPersistentStore(download);
   }
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
 }
 
 void DownloadManager::UpdateDownloadProgress() {
@@ -765,6 +751,11 @@ void DownloadManager::FileSelectionCanceled(void* params) {
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
+  // TODO(ahendrickson) -- This currently has no effect, as the download is
+  // not put on the active list until the file selection is complete.  Need
+  // to put it on the active list earlier in the process.
+  RemoveFromActiveList(download);
+
   download->OffThreadCancel(file_manager_);
 }
 
diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h
index 7f92423..b08a143 100644
--- a/content/browser/download/download_manager.h
+++ b/content/browser/download/download_manager.h
@@ -112,15 +112,24 @@ class DownloadManager
   // Notifications sent from the download thread to the UI thread
   void StartDownload(int32 id);
   void UpdateDownload(int32 download_id, int64 size);
+
+  // |download_id| is the ID of the download.
+  // |size| is the number of bytes that have been downloaded.
   // |hash| is sha256 hash for the downloaded file. It is empty when the hash
   // is not available.
-  void OnResponseCompleted(int32 download_id, int64 size, int os_error,
+  void OnResponseCompleted(int32 download_id, int64 size,
                            const std::string& hash);
 
   // Offthread target for cancelling a particular download.  Will be a no-op
   // if the download has already been cancelled.
   void CancelDownload(int32 download_id);
 
+  // Called when there is an error in the download.
+  // |download_id| is the ID of the download.
+  // |size| is the number of bytes that are currently downloaded.
+  // |error| is a download error code.  Indicates what caused the interruption.
+  void OnDownloadError(int32 download_id, int64 size, int error);
+
   // Called from DownloadItem to handle the DownloadManager portion of a
   // Cancel; should not be called from other locations.
   void DownloadCancelledInternal(DownloadItem* download);
@@ -290,8 +299,14 @@ class DownloadManager
   // is not available.
   void OnAllDataSaved(int32 download_id, int64 size, const std::string& hash);
 
-  // An error occurred in the download.
-  void OnDownloadError(int32 download_id, int64 size, int os_error);
+  // Retrieves the download from the |download_id|.
+  // Returns NULL if the download is not active.
+  DownloadItem* GetActiveDownload(int32 download_id);
+
+  // Removes |download| from the active and in progress maps.
+  // Called when the download is cancelled or has an error.
+  // Does nothing if the download is not in the history DB.
+  void RemoveFromActiveList(DownloadItem* download);
 
   // Updates the delegate about the overall download progress.
   void UpdateDownloadProgress();