[Downloads] Retry renames after transient failures.

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 8b45ce9ca0f672b5624f41210b204403e20e2367..da2b52d0f8c07b85b425b96370c3c03e2b2ec0ba 100644
--- a/content/browser/download/base_file_unittest.cc
+++ b/content/browser/download/base_file_unittest.cc
@@ -476,6 +476,58 @@ TEST_F(BaseFileTest, RenameWithError) {
   base_file_->Finish();
 }
 
+// Test that if a rename fails for an in-progress BaseFile, it remains writeable
+// and renameable.
+TEST_F(BaseFileTest, RenameWithErrorInProgress) {
+  ASSERT_TRUE(InitializeFile());
+
+  base::FilePath test_dir(temp_dir_.path().AppendASCII("TestDir"));
+  ASSERT_TRUE(base::CreateDirectory(test_dir));
+
+  base::FilePath new_path(test_dir.AppendASCII("TestFile"));
+  EXPECT_FALSE(base::PathExists(new_path));
+
+  // Write some data to start with.
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
+  ASSERT_TRUE(base_file_->in_progress());
+
+  base::FilePath old_path = base_file_->full_path();
+
+  {
+    file_util::PermissionRestorer restore_permissions_for(test_dir);
+    ASSERT_TRUE(file_util::MakeFileUnwritable(test_dir));
+    EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
+              base_file_->Rename(new_path));
+
+    // The file should still be open and we should be able to continue writing
+    // to it.
+    ASSERT_TRUE(base_file_->in_progress());
+    ASSERT_TRUE(AppendDataToFile(kTestData2));
+    ASSERT_EQ(old_path.value(), base_file_->full_path().value());
+
+    // Try to rename again, just for kicks. It should still fail with
+    // ACCESS_DENIED.
+    EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
+              base_file_->Rename(new_path));
+  }
+
+  // Now that TestDir is writeable again, we should be able to successfully
+  // rename the file.
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, base_file_->Rename(new_path));
+  ASSERT_EQ(new_path.value(), base_file_->full_path().value());
+  ASSERT_TRUE(AppendDataToFile(kTestData3));
+
+  base_file_->Finish();
+
+  // The contents of the file should be intact.
+  std::string file_contents;
+  std::string expected_contents(kTestData1);
+  expected_contents += kTestData2;
+  expected_contents += kTestData3;
+  ASSERT_TRUE(base::ReadFileToString(new_path, &file_contents));
+  EXPECT_EQ(expected_contents, file_contents);
+}
+

[Randy Smith (Not in Mondays), 2014/06/12 22:10:30]

Should you also test the case where the rename succeed and the open failed? 
(Note: I don't offhand think of a way to do that, and if you can't either, don't
worry about it.)

[asanka, 2014/06/17 21:40:56]

I was thinking something along the lines of using dropbox ACLs (allow insert,
but not write, thus allowing a caller of create() to write() until close() but
then subsequent opens fail). But it's too file system dependent.

I think I'll let this one slide.

 // Test that a failed write reports an error.
 TEST_F(BaseFileTest, WriteWithError) {
   base::FilePath path;