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

chrome/browser/download/download_manager_unittest.cc

@@ -30,6 +30,8 @@
 #include "content/browser/download/download_status_updater.h"
 #include "content/browser/download/mock_download_manager.h"
 #include "grit/generated_resources.h"
+#include "net/base/io_buffer.h"
+#include "net/base/mock_file_stream.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -67,8 +69,9 @@ class DownloadManagerTest : public testing::Test {
     file_manager()->downloads_[id] = download_file;
   }
 
-  void OnAllDataSaved(int32 download_id, int64 size, const std::string& hash) {
-    download_manager_->OnAllDataSaved(download_id, size, hash);
+  void OnResponseCompleted(int32 download_id, int64 size,
+                           const std::string& hash) {
+    download_manager_->OnResponseCompleted(download_id, size, hash);
   }
 
   void FileSelected(const FilePath& path, void* params) {
@@ -79,6 +82,30 @@ class DownloadManagerTest : public testing::Test {
     download_manager_->ContinueDownloadWithPath(download, path);
   }
 
+  void UpdateData(int32 id, const char* data, size_t length) {
+    // We are passing ownership of this buffer to the download file manager.
+    net::IOBuffer* io_buffer = new net::IOBuffer(length);
+    // We need |AddRef()| because we do a |Release()| in |UpdateDownload()|.
+    io_buffer->AddRef();
+    memcpy(io_buffer->data(), data, length);
+
+    {
+      base::AutoLock auto_lock(download_buffer_.lock);
+
+      download_buffer_.contents.push_back(
+          std::make_pair(io_buffer, length));
+    }
+
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        NewRunnableMethod(file_manager_.get(),
+                          &DownloadFileManager::UpdateDownload,
+                          id,
+                          &download_buffer_));
+
+    message_loop_.RunAllPending();
+  }
+
   void OnDownloadError(int32 download_id, int64 size, int os_error) {
     download_manager_->OnDownloadError(download_id, size, os_error);
   }
@@ -99,6 +126,7 @@ class DownloadManagerTest : public testing::Test {
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
+  DownloadBuffer download_buffer_;
 
   DownloadFileManager* file_manager() {
     if (!file_manager_) {
@@ -126,6 +154,45 @@ const char* DownloadManagerTest::kTestData = "a;sdlfalsdfjalsdkfjad";
 const size_t DownloadManagerTest::kTestDataLen =
     strlen(DownloadManagerTest::kTestData);
 
+// A DownloadFile that we can inject errors into.  Uses MockFileStream.
+// Note:  This can't be in an anonymous namespace because it must be declared
+// as a friend of |DownloadFile| in order to access its private members.
+class DownloadFileWithMockStream : public DownloadFile {
+ public:
+  DownloadFileWithMockStream(DownloadCreateInfo* info,
+                             DownloadManager* manager,
+                             net::testing::MockFileStream* stream);
+
+  virtual ~DownloadFileWithMockStream() {}
+
+  void SetForcedError(int error);
+
+ protected:
+  // This version creates a |MockFileStream| instead of a |FileStream|.
+  virtual void CreateFileStream() OVERRIDE;
+};
+
+DownloadFileWithMockStream::DownloadFileWithMockStream(
+    DownloadCreateInfo* info,
+    DownloadManager* manager,
+    net::testing::MockFileStream* stream)
+        : DownloadFile(info, manager) {
+  DCHECK(file_stream_ == NULL);
+  file_stream_.reset(stream);
+}
+
+void DownloadFileWithMockStream::SetForcedError(int error)
+{
+  // |file_stream_| can only be set in the constructor and in
+  // CreateFileStream(), both of which insure that it is a |MockFileStream|.
+  net::testing::MockFileStream* mock_stream =
+    static_cast<net::testing::MockFileStream *>(file_stream_.get());
+  mock_stream->set_forced_error(error);
+}
+void DownloadFileWithMockStream::CreateFileStream() {
+  file_stream_.reset(new net::testing::MockFileStream);
+}
+
 namespace {
 
 const struct {
@@ -384,13 +451,13 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
     int32* id_ptr = new int32;
     *id_ptr = i;  // Deleted in FileSelected().
     if (kDownloadRenameCases[i].finish_before_rename) {
-      OnAllDataSaved(i, 1024, std::string("fake_hash"));
+      OnResponseCompleted(i, 1024, std::string("fake_hash"));
       message_loop_.RunAllPending();
       FileSelected(new_path, id_ptr);
     } else {
       FileSelected(new_path, id_ptr);
       message_loop_.RunAllPending();
-      OnAllDataSaved(i, 1024, std::string("fake_hash"));
+      OnResponseCompleted(i, 1024, std::string("fake_hash"));
     }
 
     message_loop_.RunAllPending();
@@ -482,6 +549,91 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
 }
 
+// Test the behavior of DownloadFileManager and DownloadManager in the event
+// of a file error while writing the download to disk.
+TEST_F(DownloadManagerTest, DownloadFileErrorTest) {
+  // Create a temporary file and a mock stream.
+  FilePath path;
+  ASSERT_TRUE(file_util::CreateTemporaryFile(&path));
+
+  // This file stream will be used, until the first rename occurs.
+  net::testing::MockFileStream* mock_stream = new net::testing::MockFileStream;
+  ASSERT_EQ(0, mock_stream->Open(
+      path,
+      base::PLATFORM_FILE_OPEN_ALWAYS | base::PLATFORM_FILE_WRITE));
+
+  // Normally, the download system takes ownership of info, and is
+  // responsible for deleting it.  In these unit tests, however, we
+  // don't call the function that deletes it, so we do so ourselves.
+  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+  int32 id = 0;
+  info->download_id = id;
+  info->prompt_user_for_save_location = false;
+  info->url_chain.push_back(GURL());
+  info->total_bytes = static_cast<int64>(kTestDataLen * 3);
+  info->save_info.file_path = path;
+
+  // Create a download file that we can insert errors into.
+  DownloadFileWithMockStream* download_file(new DownloadFileWithMockStream(
+      info.get(), download_manager_, mock_stream));
+  AddDownloadToFileManager(id, download_file);
+
+  // |download_file| is owned by DownloadFileManager.
+  download_manager_->CreateDownloadItem(info.get());
+
+  DownloadItem* download = GetActiveDownloadItem(0);
+  ASSERT_TRUE(download != NULL);
+  // This will keep track of what should be displayed on the shelf.
+  scoped_ptr<DownloadItemModel> download_item_model(
+      new DownloadItemModel(download));
+
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
+  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
+
+  // Add some data before finalizing the file name.
+  UpdateData(id, kTestData, kTestDataLen);
+
+  // Finalize the file name.
+  ContinueDownloadWithPath(download, path);
+  message_loop_.RunAllPending();
+  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
+
+  // Add more data.
+  UpdateData(id, kTestData, kTestDataLen);
+
+  // Add more data, but an error occurs.
+  download_file->SetForcedError(net::ERR_FAILED);
+  UpdateData(id, kTestData, kTestDataLen);
+
+  // Check the state.  The download should have been interrupted.
+  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
+  EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
+  EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
+  EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
+  EXPECT_TRUE(observer->was_updated());
+  EXPECT_FALSE(observer->was_opened());
+  EXPECT_FALSE(download->file_externally_removed());
+  EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
+
+  // Check the download shelf's information.
+  size_t error_size = kTestDataLen * 3;
+  ui::DataUnits amount_units = ui::GetByteDisplayUnits(kTestDataLen);
+  string16 simple_size =
+      ui::FormatBytesWithUnits(error_size, amount_units, false);
+  string16 simple_total = base::i18n::GetDisplayStringInLTRDirectionality(
+      ui::FormatBytesWithUnits(error_size, amount_units, true));
+  EXPECT_EQ(l10n_util::GetStringFUTF16(IDS_DOWNLOAD_STATUS_INTERRUPTED,
+                                       simple_size,
+                                       simple_total),
+            download_item_model->GetStatusText());
+
+  // Clean up.
+  download->Cancel(true);
+  message_loop_.RunAllPending();
+}
+
 TEST_F(DownloadManagerTest, DownloadCancelTest) {
   using ::testing::_;
   using ::testing::CreateFunctor;
@@ -609,7 +761,7 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   download_file->AppendDataToFile(kTestData, kTestDataLen);
 
   // Finish the download.
-  OnAllDataSaved(0, kTestDataLen, "");
+  OnResponseCompleted(0, kTestDataLen, "");
   message_loop_.RunAllPending();
 
   // Download is complete.
@@ -685,7 +837,7 @@ TEST_F(DownloadManagerTest, DownloadRemoveTest) {
   download_file->AppendDataToFile(kTestData, kTestDataLen);
 
   // Finish the download.
-  OnAllDataSaved(0, kTestDataLen, "");
+  OnResponseCompleted(0, kTestDataLen, "");
   message_loop_.RunAllPending();
 
   // Download is complete.

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) {

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.

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();
+}

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) {

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.

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();
   }
 

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

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_);
 }
 

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();

net/base/file_stream.h

@@ -40,31 +40,31 @@ class NET_EXPORT FileStream {
   // is destructed.
   FileStream(base::PlatformFile file, int flags);
 
-  ~FileStream();
+  virtual ~FileStream();
 
   // Call this method to close the FileStream.  It is OK to call Close
   // multiple times.  Redundant calls are ignored.
   // Note that if there are any pending async operations, they'll be aborted.
-  void Close();
+  virtual void Close();
 
   // Call this method to open the FileStream.  The remaining methods
   // cannot be used unless this method returns OK.  If the file cannot be
   // opened then an error code is returned.
   // open_flags is a bitfield of base::PlatformFileFlags
-  int Open(const FilePath& path, int open_flags);
+  virtual int Open(const FilePath& path, int open_flags);
 
   // Returns true if Open succeeded and Close has not been called.
-  bool IsOpen() const;
+  virtual bool IsOpen() const;
 
   // Adjust the position from where data is read.  Upon success, the stream
   // position relative to the start of the file is returned.  Otherwise, an
   // error code is returned.  It is not valid to call Seek while a Read call
   // has a pending completion.
-  int64 Seek(Whence whence, int64 offset);
+  virtual int64 Seek(Whence whence, int64 offset);
 
   // Returns the number of bytes available to read from the current stream
   // position until the end of the file.  Otherwise, an error code is returned.
-  int64 Available();
+  virtual int64 Available();
 
   // Call this method to read data from the current stream position.  Up to
   // buf_len bytes will be copied into buf.  (In other words, partial reads are
@@ -85,14 +85,14 @@ class NET_EXPORT FileStream {
   // This method should not be called if the stream was opened WRITE_ONLY.
   //
   // You can pass NULL as the callback for synchronous I/O.
-  int Read(char* buf, int buf_len, CompletionCallback* callback);
+  virtual int Read(char* buf, int buf_len, CompletionCallback* callback);
 
   // Performs the same as Read, but ensures that exactly buf_len bytes
   // are copied into buf.  A partial read may occur, but only as a result of
   // end-of-file or fatal error.  Returns the number of bytes copied into buf,
   // 0 if at end-of-file and no bytes have been read into buf yet,
   // or an error code if the operation could not be performed.
-  int ReadUntilComplete(char *buf, int buf_len);
+  virtual int ReadUntilComplete(char *buf, int buf_len);
 
   // Call this method to write data at the current stream position.  Up to
   // buf_len bytes will be written from buf. (In other words, partial writes are
@@ -113,14 +113,14 @@ class NET_EXPORT FileStream {
   // This method should not be called if the stream was opened READ_ONLY.
   //
   // You can pass NULL as the callback for synchronous I/O.
-  int Write(const char* buf, int buf_len, CompletionCallback* callback);
+  virtual int Write(const char* buf, int buf_len, CompletionCallback* callback);
 
   // Truncates the file to be |bytes| length. This is only valid for writable
   // files. After truncation the file stream is positioned at |bytes|. The new
   // position is retured, or a value < 0 on error.
   // WARNING: one may not truncate a file beyond its current length on any
   //   platform with this call.
-  int64 Truncate(int64 bytes);
+  virtual int64 Truncate(int64 bytes);
 
   // Forces out a filesystem sync on this file to make sure that the file was
   // written out to disk and is not currently sitting in the buffer. This does
@@ -130,7 +130,7 @@ class NET_EXPORT FileStream {
   /// Returns an error code if the operation could not be performed.
   //
   // This method should not be called if the stream was opened READ_ONLY.
-  int Flush();
+  virtual int Flush();
 
  private:
   class AsyncContext;

net/base/mock_file_stream.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/mock_file_stream.h"
+
+namespace net {
+
+namespace testing {
+
+int MockFileStream::Open(const FilePath& path, int open_flags) {
+  path_ = path;
+  return ReturnError(net::FileStream::Open(path, open_flags));
+}
+
+int64 MockFileStream::Seek(net::Whence whence, int64 offset) {
+  return ReturnError64(net::FileStream::Seek(whence, offset));
+}
+
+int64 MockFileStream::Available() {
+  return ReturnError64(net::FileStream::Available());
+}
+
+int MockFileStream::Read(char* buf,
+                 int buf_len,
+                 net::CompletionCallback* callback) {
+  return ReturnError(net::FileStream::Read(buf, buf_len, callback));
+}
+
+int MockFileStream::ReadUntilComplete(char *buf, int buf_len) {
+  return ReturnError(net::FileStream::ReadUntilComplete(buf, buf_len));
+}
+
+int MockFileStream::Write(const char* buf,
+                  int buf_len,
+                  net::CompletionCallback* callback) {
+  return ReturnError(net::FileStream::Write(buf, buf_len, callback));
+}
+
+int64 MockFileStream::Truncate(int64 bytes) {
+  return ReturnError64(net::FileStream::Truncate(bytes));
+}
+
+int MockFileStream::Flush() {
+  return ReturnError(net::FileStream::Flush());
+}
+
+}  // namespace testing
+
+}  // namespace net

net/base/mock_file_stream.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This file defines MockFileStream, a test class for FileStream.
+
+#ifndef NET_BASE_MOCK_FILE_STREAM_H_
+#define NET_BASE_MOCK_FILE_STREAM_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/file_path.h"
+#include "net/base/file_stream.h"
+#include "net/base/net_errors.h"
+
+namespace net {
+
+namespace testing {
+
+class MockFileStream : public net::FileStream {
+ public:
+  MockFileStream() : forced_error_(net::OK) {}
+
+  MockFileStream(base::PlatformFile file, int flags)
+      : net::FileStream(file, flags), forced_error_(net::OK) {}
+
+  // FileStream methods.
+  virtual int Open(const FilePath& path, int open_flags) OVERRIDE;
+  virtual int64 Seek(net::Whence whence, int64 offset) OVERRIDE;
+  virtual int64 Available() OVERRIDE;
+  virtual int Read(char* buf,
+                   int buf_len,
+                   net::CompletionCallback* callback) OVERRIDE;
+  virtual int ReadUntilComplete(char *buf, int buf_len) OVERRIDE;
+  virtual int Write(const char* buf,
+                    int buf_len,
+                    net::CompletionCallback* callback) OVERRIDE;
+  virtual int64 Truncate(int64 bytes) OVERRIDE;
+  virtual int Flush() OVERRIDE;
+
+  void set_forced_error(int error) { forced_error_ = error; }
+  void clear_forced_error() { forced_error_ = net::OK; }
+  const FilePath& get_path() const { return path_; }
+
+ private:
+  int ReturnError(int function_error) {
+    if (forced_error_ != net::OK) {
+      int ret = forced_error_;
+      clear_forced_error();
+      return ret;
+    }
+
+    return function_error;
+  }
+
+  int64 ReturnError64(int64 function_error) {
+    if (forced_error_ != net::OK) {
+      int64 ret = forced_error_;
+      clear_forced_error();
+      return ret;
+    }
+
+    return function_error;
+  }
+
+  int forced_error_;
+  FilePath path_;
+};
+
+}  // namespace testing
+
+}  // namespace net
+
+#endif  // NET_BASE_MOCK_FILE_STREAM_H_

net/net.gyp

@@ -1241,6 +1241,8 @@
         'base/cookie_monster_store_test.h',
         'base/cookie_store_test_helpers.cc',
         'base/cookie_store_test_helpers.h',
+        'base/mock_file_stream.cc',
+        'base/mock_file_stream.h',
         'base/mock_host_resolver.cc',
         'base/mock_host_resolver.h',
         'base/net_test_suite.cc',