Detect file system errors during downloads.

chrome/browser/download/download_manager_unittest.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 <string>
 #include <set>
 
 #include "base/file_util.h"
 #include "base/i18n/number_formatting.h"
 #include "base/i18n/rtl.h"
@@ skipping 13 matching lines @@
 #include "chrome/test/base/testing_profile.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_manager.h"
 #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"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/text/bytes_formatting.h"
 
 class DownloadManagerTest : public TestingBrowserProcessTest {
  public:
   static const char* kTestData;
   static const size_t kTestDataLen;
@@ skipping 17 matching lines @@
     download_manager_ = NULL;
     download_manager_delegate_ = NULL;
     profile_.reset(NULL);
     message_loop_.RunAllPending();
   }
 
   void AddDownloadToFileManager(int id, DownloadFile* download_file) {
     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) {
     download_manager_->FileSelected(path, params);
   }
 
   void ContinueDownloadWithPath(DownloadItem* download, const FilePath& path) {
     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);
   }
 
   // Get the download item with ID |id|.
   DownloadItem* GetActiveDownloadItem(int32 id) {
     if (ContainsKey(download_manager_->active_downloads_, id))
       return download_manager_->active_downloads_[id];
     return NULL;
   }
 
  protected:
   DownloadStatusUpdater download_status_updater_;
   scoped_ptr<TestingProfile> profile_;
   scoped_refptr<ChromeDownloadManagerDelegate> download_manager_delegate_;
   scoped_refptr<DownloadManager> download_manager_;
   scoped_refptr<DownloadFileManager> file_manager_;
   MessageLoopForUI message_loop_;
   BrowserThread ui_thread_;
   BrowserThread file_thread_;
+  DownloadBuffer download_buffer_;
 
   DownloadFileManager* file_manager() {
     if (!file_manager_) {
       file_manager_ = new DownloadFileManager(NULL);
       download_manager_->file_manager_ = file_manager_;
     }
     return file_manager_;
   }
 
   // Make sure download item |id| was set with correct safety state for
   // given |is_dangerous_file| and |is_dangerous_url|.
   bool VerifySafetyState(bool is_dangerous_file,
                          bool is_dangerous_url,
                          int id) {
     DownloadItem::SafetyState safety_state =
         download_manager_->GetDownloadItem(id)->safety_state();
     return (is_dangerous_file || is_dangerous_url) ?
         safety_state != DownloadItem::SAFE : safety_state == DownloadItem::SAFE;
   }
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerTest);
 };
 
 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 {
   const char* url;
   const char* mime_type;
   bool save_as;
   bool prompt_for_download;
   bool expected_save_as;
 } kStartDownloadCases[] = {
   { "http://www.foo.com/dont-open.html",
@@ skipping 238 matching lines @@
     DownloadItem* download = GetActiveDownloadItem(i);
     ASSERT_TRUE(download != NULL);
     if (kDownloadRenameCases[i].is_dangerous_file)
       download->MarkFileDangerous();
     if (kDownloadRenameCases[i].is_dangerous_url)
       download->MarkUrlDangerous();
 
     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();
     EXPECT_TRUE(VerifySafetyState(kDownloadRenameCases[i].is_dangerous_file,
                                   kDownloadRenameCases[i].is_dangerous_url,
                                   i));
   }
 }
 
 TEST_F(DownloadManagerTest, DownloadInterruptTest) {
@@ skipping 71 matching lines @@
   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(download->received_bytes(), error_size);
   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;
   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);
@@ skipping 107 matching lines @@
   // |download_file| is owned by DownloadFileManager.
   AddDownloadToFileManager(info->download_id, download_file);
 
   ContinueDownloadWithPath(download, new_path);
   message_loop_.RunAllPending();
   EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
 
   download_file->AppendDataToFile(kTestData, kTestDataLen);
 
   // Finish the download.
-  OnAllDataSaved(0, kTestDataLen, "");
+  OnResponseCompleted(0, kTestDataLen, "");
   message_loop_.RunAllPending();
 
   // Download is complete.
   EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
   EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
   EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
   EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
@@ skipping 55 matching lines @@
   // |download_file| is owned by DownloadFileManager.
   AddDownloadToFileManager(info->download_id, download_file);
 
   ContinueDownloadWithPath(download, new_path);
   message_loop_.RunAllPending();
   EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
 
   download_file->AppendDataToFile(kTestData, kTestDataLen);
 
   // Finish the download.
-  OnAllDataSaved(0, kTestDataLen, "");
+  OnResponseCompleted(0, kTestDataLen, "");
   message_loop_.RunAllPending();
 
   // Download is complete.
   EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
   EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
   EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
   EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
@@ skipping 18 matching lines @@
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
   EXPECT_FALSE(observer->was_opened());
   EXPECT_TRUE(download->file_externally_removed());
   EXPECT_EQ(DownloadItem::COMPLETE, download->state());
   EXPECT_EQ(download_item_model->GetStatusText(),
             l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_REMOVED));
 
   EXPECT_FALSE(file_util::PathExists(new_path));
 }