Detect file system errors during downloads.

content/browser/download/base_file_unittest.cc

Index: content/browser/download/base_file_unittest.cc
diff --git a/content/browser/download/base_file_unittest.cc b/content/browser/download/base_file_unittest.cc
index ce8acb53c20a4c3d3e9c82847b32b87e49a7c27a..7eb6fb631661b3a3e61e20320c2ba44d51ad1fb4 100644
--- a/content/browser/download/base_file_unittest.cc
+++ b/content/browser/download/base_file_unittest.cc
@@ -6,17 +6,89 @@
 #include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
 #include "base/string_number_conversions.h"
+#include "chrome/browser/download/download_util.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/download/base_file.h"
 #include "net/base/file_stream.h"
+#include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
+class MockFileStream : public net::FileStream {
+ public:
+  MockFileStream() : forced_error_(0) {}
+  MockFileStream(base::PlatformFile file, int flags)
+      : net::FileStream(file, flags), forced_error_(0) {}
+
+  void set_forced_error(int error) { forced_error_ = error; }
+  void clear_forced_error() { forced_error_ = 0; }

[wtc, 2011/08/23 00:18:35]

Nit: it may be better to use net::OK instead of 0 with
the forced_error_ member of this class.

[cbentzel, 2011/08/23 01:34:01]

+1

[ahendrickson, 2011/08/25 21:55:15]

Done.

+
+  int Open(const FilePath& path, int open_flags) {
+    path_ = path;
+    return get_error(net::FileStream::Open(path, open_flags));
+  }
+
+  int64 Seek(net::Whence whence, int64 offset) {
+    return get_error64(net::FileStream::Seek(whence, offset));
+  }
+
+  int64 Available() {
+    return get_error64(net::FileStream::Available());
+  }
+
+  int Read(char* buf, int buf_len, net::CompletionCallback* callback) {
+    return get_error(net::FileStream::Read(buf, buf_len, callback));
+  }
+
+  int ReadUntilComplete(char *buf, int buf_len) {
+    return get_error(net::FileStream::ReadUntilComplete(buf, buf_len));
+  }
+
+  int Write(const char* buf, int buf_len, net::CompletionCallback* callback) {
+    return get_error(net::FileStream::Write(buf, buf_len, callback));
+  }

[wtc, 2011/08/23 00:18:35]

Nit: add a blank line after this line.

[ahendrickson, 2011/08/25 21:55:15]

Done.

+  int64 Truncate(int64 bytes) {
+    return get_error64(net::FileStream::Truncate(bytes));
+  }
+
+  int Flush() {
+    return get_error(net::FileStream::Flush());
+  }

[wtc, 2011/08/23 00:18:35]

All the net::FileStream virtual methods that you override
should have the "virtual" keyword.  You should also use the
OVERRIDE macro (defined in compiler_specific.h).  See
http://www.chromium.org/developers/coding-style#Method_Signatures

[ahendrickson, 2011/08/25 21:55:15]

Done.

+
+  const FilePath& get_path() const { return path_; }
+
+ private:
+  int get_error(int function_error) {

[cbentzel, 2011/08/23 01:34:01]

Perhaps GetError or even ReturnError() since this isn't a simple getter.

[ahendrickson, 2011/08/25 21:55:15]

Done.

+    if (forced_error_ != 0) {
+      int ret = forced_error_;
+      clear_forced_error();
+      return ret;
+    }
+
+    return function_error;
+  }
+
+  int64 get_error64(int64 function_error) {
+    if (forced_error_ != 0) {
+      int64 ret = forced_error_;
+      clear_forced_error();
+      return ret;
+    }
+
+    return function_error;
+  }
+
+   int forced_error_;
+   FilePath path_;
+};
+
 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 +123,48 @@ 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 (!download_util::CreateTemporaryFileForDownload(&path))
+      return false;
+
+    // Create a new file stream if it is not provided.

[cbentzel, 2011/08/23 01:34:01]

What does the "if it is not provided" mean?  When is it ever provided?

[ahendrickson, 2011/08/25 21:55:15]

Removed.

+    mock_file_stream_.reset(new MockFileStream);
+    if (mock_file_stream_->Open(
+        path,
+        base::PLATFORM_FILE_OPEN_ALWAYS | base::PLATFORM_FILE_WRITE) != 0) {
+      mock_file_stream_.reset();
+      return false;
+    }
+
+    // We may be re-opening the file after rename. Always make sure we're
+    // writing at the end of the file.
+    if (mock_file_stream_->Seek(net::FROM_END, 0) < 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<MockFileStream> mock_file_stream_;
 
   // BaseClass instance we are testing.
   scoped_ptr<BaseFile> base_file_;
@@ -227,4 +331,17 @@ TEST_F(BaseFileTest, RenameWhileInProgress) {
   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);

[cbentzel, 2011/08/23 01:34:01]

Should you use one of the more likely ERR's here? I suppose many of them will be
introduced in another CL.

[ahendrickson, 2011/08/25 21:55:15]

Yes, they will.

+  ASSERT_FALSE(AppendDataToFile(kTestData3));
+  std::string hash;
+  EXPECT_FALSE(base_file_->GetSha256Hash(&hash));
+  base_file_->Finish();
+}