Detect file system errors during downloads.

chrome/browser/download/download_manager_unittest.cc

Index: chrome/browser/download/download_manager_unittest.cc
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 58c39fa36df05fc6b64bca79419515ae2768b082..4938db0ea1e76fca82df45f77fb588f22c1ffa26 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/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,31 @@ class DownloadManagerTest : public testing::Test {
     download_manager_->ContinueDownloadWithPath(download, path);
   }
 
+  void UpdateData(int32 id, const void* data, size_t length) {

[cbentzel, 2011/08/29 15:05:27]

Should |data| be const char* to be consistent with IOBuffer's representation?

[ahendrickson, 2011/08/29 16:30:53]

I don't have a strong opinion about this.

Changed.

+    // We are passing ownership of this buffer to the download file manager.
+    net::IOBuffer* io_buffer = new net::IOBuffer(length);
+    // We need |AddRef()| because io_buffer is not a |scoped_refptr|, and we

[cbentzel, 2011/08/29 15:05:27]

This comment isn't quite correct. If io_buffer were a scoped_refptr, than the
Release() would be called at function termination. You need the Release which is
called in DownloadFileManager::UpdateDownload. 

I took a look at the DownloadResourceHandler as a result of this, and it
maintains the refcount in a funky way - it uses a scoped_refptr to bump it to 1,
and then does a swap [should be a release to be clearer] to a raw pointer,
followed by the release in DownloadFileManager::UpdateDownload.

[ahendrickson, 2011/08/29 16:30:53]

Yeah, it's funky.

Changed.

+    // will do a |Release()| in |UpdateDownload()|.
+    io_buffer->AddRef();
+    memcpy(io_buffer->data(), data, length);
+
+    {
+      base::AutoLock auto_lock(download_buffer_.lock);

[cbentzel, 2011/08/29 15:05:27]

This is kinda gross, although it already existed. Seems like DownloadBuffer
should move to a thread-safe class which hides when locks are acquired.

[ahendrickson, 2011/08/29 16:30:53]

I think that's outside the scope of this CL.

[Randy Smith (Not in Mondays), 2011/08/29 16:57:57]

Agreed with both points (suggested refactor, and that it's outside the scope of
this CL).

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

[cbentzel, 2011/08/29 15:05:27]

Why do you need to do this inside of UpdateData?

[ahendrickson, 2011/08/29 16:30:53]

For other operations.  Most tests don't need to use UpdateData(), and the one
that does also calls ContinueDownloadWithPath() and DownloadItem::Cancel(), both
of which need to run the message loop.

+  }
+
   void OnDownloadError(int32 download_id, int64 size, int os_error) {
     download_manager_->OnDownloadError(download_id, size, os_error);
   }
@@ -99,6 +127,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 +155,35 @@ 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.

[cbentzel, 2011/08/29 15:05:27]

Could you just add a DownloadFile constructor which accepts a FileStream
argument? The BaseFile one does. Then you wouldn't need to make this a friend
class of BaseFile. 

[ahendrickson, 2011/08/29 16:30:53]

It needs more information than just the file stream, for example, the
DownloadManager.

+class DownloadFileWithMockStream : public DownloadFile {
+ public:
+  DownloadFileWithMockStream(DownloadCreateInfo* info,
+                             DownloadManager* manager,
+                             testing::MockFileStream* stream)
+      : DownloadFile(info, manager) {
+    DCHECK(file_stream_ == NULL);
+    file_stream_.reset(stream);
+  }
+  virtual ~DownloadFileWithMockStream() {
+  }
+
+  void SetForcedError(int error)
+  {
+    testing::MockFileStream* mock_stream =

[Randy Smith (Not in Mondays), 2011/08/28 22:14:03]

Please put in a comment as to why the upcast is ok (i.e. that the only places
this can be set from are the constructor and the reset below, and in both cases
you're setting from a MockFileStream).

[ahendrickson, 2011/08/29 16:30:53]

Done.

+      static_cast<testing::MockFileStream *>(file_stream_.get());
+    mock_stream->set_forced_error(error);
+  }
+
+ protected:
+  // This version creates a |MockFileStream| instead of a |FileStream|.
+  virtual void CreateFileStream() {

[cbentzel, 2011/08/29 15:05:27]

OVERRIDE

[cbentzel, 2011/08/29 15:05:27]

Is this expected to be called? 

[ahendrickson, 2011/08/29 16:30:53]

Yes, from Open().

[ahendrickson, 2011/08/29 16:30:53]

Done.

[cbentzel, 2011/08/29 17:27:55]

Well, it could be called from Open(), but that only happens if file_stream_
isn't set. In this case, shouldn't it always be set?

[ahendrickson, 2011/08/29 17:51:37]

No, the rename operation does a Close() (which resets file_stream_ to NULL), and
then an Open(), which calls this.

+    file_stream_.reset(new testing::MockFileStream);
+  }
+};
+
 namespace {
 
 const struct {
@@ -384,13 +442,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 +540,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.
+  testing::MockFileStream* mock_stream = new 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 can use a stack variable.

[cbentzel, 2011/08/29 15:05:27]

Looks like other tests use scoped_ptr. I'd either convert this to scoped_ptr or
change the others to be stack variables to be consistent.

By the way, what function normally deletes DownloadCreateInfo? This seems a bit
fragile?

[ahendrickson, 2011/08/29 16:30:53]

DownloadFileManager::CreateDownloadFile() normally deletes DownloadCreateInfo.

I was using a scoped_ptr here, but Randy asked me to change it.

[Randy Smith (Not in Mondays), 2011/08/29 16:57:57]

I'm cool with shifting back to scoped_ptr; I agree with Chris that consistency
is more important.

[ahendrickson, 2011/08/29 17:51:37]

Done.

+  DownloadCreateInfo info;
+  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, download_manager_, mock_stream));
+  AddDownloadToFileManager(id, download_file);
+
+  // |download_file| is owned by DownloadFileManager.
+  ::testing::Mock::AllowLeak(download_file);

[Randy Smith (Not in Mondays), 2011/08/28 22:14:03]

Why does the leak occur?  Shouldn't it be cleaned up when we clean up
DownloadFileManager?  (More generally, any way to write the test so that we
cleanup as opposed to leaking?)

[ahendrickson, 2011/08/29 16:30:53]

Hmm, thinking about it, due to recent refactorings it should clean up the
DownloadFile as a result of the error.

Removed this line.

+  download_manager_->CreateDownloadItem(&info);
+
+  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);
+}
+
 TEST_F(DownloadManagerTest, DownloadCancelTest) {
   using ::testing::_;
   using ::testing::CreateFunctor;
@@ -609,7 +752,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 +828,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.