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 36849eb252997f48d73fb1d10de4ddc38ba7a659..94d94ffed7a34c454cdad4e07dfad0dabfa03bd2 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -31,6 +31,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"
@@ -68,8 +70,9 @@ class DownloadManagerTest : public TestingBrowserProcessTest {
     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) {
@@ -80,6 +83,31 @@ class DownloadManagerTest : public TestingBrowserProcessTest {
     download_manager_->ContinueDownloadWithPath(download, path);
   }
 
+  void UpdateData(int32 id, const void* 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 io_buffer is not a |scoped_refptr|, and we
+    // will 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);
   }
@@ -100,6 +128,7 @@ class DownloadManagerTest : public TestingBrowserProcessTest {
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
+  DownloadBuffer download_buffer_;
 
   DownloadFileManager* file_manager() {
     if (!file_manager_) {
@@ -127,6 +156,31 @@ 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)
+      : DownloadFile(info, manager) {
+  }
+  virtual ~DownloadFileWithMockStream() {
+  }
+
+  void SetForcedError(int error)
+  {
+    testing::MockFileStream* mock_stream =
+      static_cast<testing::MockFileStream *>(file_stream_.get());

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

This kind of upcasting is nervous making and should be avoided unless you need
to do it, and in this case I don't think you need to.  You're creating the
mock_stream; can't you just keep a reference to it and call set_forced_error on
the stream instead of on the DownloadFile at the appropriate time?

[ahendrickson, 2011/08/26 21:06:54]

Leaving it in, per our conversation, but adding an extra MockFileStream
parameter to the constructor.  The info parameter is required to have a NULL
file_stream pointer.

+    mock_stream->set_forced_error(error);
+  }
+
+ protected:
+  // This version creates a |MockFileStream| instead of a |FileStream|.
+  virtual void CreateFileStream() {
+    file_stream_.reset(new testing::MockFileStream);
+  }
+};
+
 namespace {
 
 const struct {
@@ -385,13 +439,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();
@@ -483,6 +537,95 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_EQ(download->total_bytes(), static_cast<int64>(kTestDataLen));
 }
 
+TEST_F(DownloadManagerTest, DownloadFileErrorTest) {

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

There's enough "stuff" in this test that I think it'd be useful to put in a
couple (3-5?) comments indicating what each section of the test is doing; it'll
aid people in reading the function.

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

Comment before the function saying what functionality it's testing.

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

I don't think this belongs under DownloadManagerTest; maybe 
download_file_unittest.cc ?  (ETA: It's fuzzier for the full set of
functionality you're testing, but in an ideal world, functionality that was
primarily around DownloadFile would be someplace else.  But it's ok to keep this
here given what you're actually testing.)

[ahendrickson, 2011/08/26 21:06:54]

Done.

[ahendrickson, 2011/08/26 21:06:54]

Done.

[ahendrickson, 2011/08/26 21:06:54]

OK.  Keeping.

+  using ::testing::_;
+  using ::testing::CreateFunctor;

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

This has the look of a pattern copied from elsewhere in the code, and if it's a
common pattern used in the chrome code base, I'm good with it.  But if it's not:
I'm specifically concerned with "using ::testing::_", as "_" is a heck of a name
to have have a specific meaning in a file.  I think readers will have a hard
time figuring out what that's about.

[ahendrickson, 2011/08/26 21:06:54]

Yes, it was copied.  The pattern is from using the gmock testing code, but I'm
not using it here so I've deleted this section.

+  using ::testing::Invoke;
+  using ::testing::Return;
+
+  // 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);

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

Any reason to make it a scoped_ptr rather than just declare it inline in the
stack?

[ahendrickson, 2011/08/26 21:06:54]

Not really, other than as an example of how it is normally used.

Changed.

+  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);
+  FilePath path;
+  ASSERT_TRUE(file_util::CreateTemporaryFile(&path));
+  testing::MockFileStream* mock_stream = new testing::MockFileStream;
+  linked_ptr<net::FileStream> stream(mock_stream);

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

Why a linked_ptr?  They're rare enough that I think of them as only being used
for special purposes; what's the purpose here?  If there's a reason for this,
please put a comment in explaining it; if not, maybe just use a scoped_ptr?

[ahendrickson, 2011/08/26 21:06:54]

This was necessary in order to assign it to info->save_info.file_stream. 
However, with the change to the constructor of DownloadFileWithMockStream I
don't need it any more.

Removed.

+  ASSERT_EQ(0, mock_stream->Open(
+      path,
+      base::PLATFORM_FILE_OPEN_ALWAYS | base::PLATFORM_FILE_WRITE));
+  info->save_info.file_path = path;
+  info->save_info.file_stream = stream;

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

I'd suggest moving the stream initialization to before the info initialization;
that way each initialization is consistent and reading through you don't have to
jump back and forth between them.

[ahendrickson, 2011/08/26 21:06:54]

Done.

+
+  DownloadFileWithMockStream* download_file(new DownloadFileWithMockStream(
+      info.get(), download_manager_));
+  AddDownloadToFileManager(id, download_file);
+
+  // |download_file| is owned by DownloadFileManager.
+  ::testing::Mock::AllowLeak(download_file);
+  download_manager_->CreateDownloadItem(info.get());
+
+  DownloadItem* download = GetActiveDownloadItem(0);
+  ASSERT_TRUE(download != NULL);
+  scoped_ptr<DownloadItemModel> download_item_model(
+      new DownloadItemModel(download));
+
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
+  scoped_ptr<ItemObserver> observer(new ItemObserver(download));
+
+  UpdateData(id, kTestData, kTestDataLen);
+
+  ContinueDownloadWithPath(download, path);
+  message_loop_.RunAllPending();
+  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
+
+  UpdateData(id, kTestData, kTestDataLen);
+
+  download_file->SetForcedError(net::ERR_FAILED);
+  UpdateData(id, kTestData, kTestDataLen);
+
+  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());
+
+  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());
+
+  download->Cancel(true);

[Randy Smith (Not in Mondays), 2011/08/26 18:51:40]

Everything from here on in doesn't test any error detection functionality, does
it?  If this functionality is tested elsewhere in the file, I'd eliminate it
here, to keep this test simple.

[ahendrickson, 2011/08/26 21:06:54]

Done.

+
+  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());
+  EXPECT_EQ(static_cast<int64>(error_size), download->received_bytes());
+  EXPECT_EQ(static_cast<int64>(kTestDataLen) * 3, download->total_bytes());
+}
+
 TEST_F(DownloadManagerTest, DownloadCancelTest) {
   using ::testing::_;
   using ::testing::CreateFunctor;
@@ -610,7 +753,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.
@@ -686,7 +829,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.