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 12 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 testing::Test {
  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) {

[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);
   }
 
   // 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.

[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 {
   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 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;
   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));
 }