[Downloads] Retry renames after transient failures.

If the cause of a rename failure is suspected of being transient
(sharing violations, suspected race conditions in the file system), then
retry the rename with an exponential backoff.

BUG=368455

Review URL: https://codereview.chromium.org/319603003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278483 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/download/base_file.cc

@@ -159,15 +159,18 @@ DownloadInterruptReason BaseFile::Rename(const base::FilePath& new_path) {
   // permissions / security descriptors that makes sense in the new directory.
   rename_result = MoveFileAndAdjustPermissions(new_path);
 
-  if (rename_result == DOWNLOAD_INTERRUPT_REASON_NONE) {
+  if (rename_result == DOWNLOAD_INTERRUPT_REASON_NONE)
     full_path_ = new_path;
-    // Re-open the file if we were still using it.
+
+  // Re-open the file if we were still using it regardless of the interrupt
+  // reason.
+  DownloadInterruptReason open_result = DOWNLOAD_INTERRUPT_REASON_NONE;
   if (was_in_progress)
-      rename_result = Open();
-  }
+    open_result = Open();
 
   bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_RENAMED);
-  return rename_result;
+  return rename_result == DOWNLOAD_INTERRUPT_REASON_NONE ? open_result
+                                                         : rename_result;
 }
 
 void BaseFile::Detach() {
@@ -326,11 +329,10 @@ DownloadInterruptReason BaseFile::LogSystemError(
     const char* operation,
     logging::SystemErrorCode os_error) {
   // There's no direct conversion from a system error to an interrupt reason.
-  net::Error net_error = net::MapSystemError(os_error);
+  base::File::Error file_error = base::File::OSErrorToFileError(os_error);
   return LogInterruptReason(
       operation, os_error,
-      ConvertNetErrorToInterruptReason(
-          net_error, DOWNLOAD_INTERRUPT_FROM_DISK));
+      ConvertFileErrorToInterruptReason(file_error));
 }
 
 DownloadInterruptReason BaseFile::LogInterruptReason(

content/browser/download/base_file.h

@@ -56,7 +56,10 @@ class CONTENT_EXPORT BaseFile {
   DownloadInterruptReason AppendDataToFile(const char* data, size_t data_len);
 
   // Rename the download file. Returns a DownloadInterruptReason indicating the
-  // result of the operation.
+  // result of the operation. A return code of NONE indicates that the rename
+  // was successful. After a failure, the full_path() and in_progress() can be
+  // used to determine the last known filename and whether the file is available
+  // for writing or retrying the rename.
   virtual DownloadInterruptReason Rename(const base::FilePath& full_path);
 
   // Detach the file so it is not deleted on destruction.
@@ -79,8 +82,15 @@ class CONTENT_EXPORT BaseFile {
   // Windows to ensure the correct app client ID is available.
   DownloadInterruptReason AnnotateWithSourceInformation();
 
-  base::FilePath full_path() const { return full_path_; }
+  // Returns the last known path to the download file. Can be empty if there's
+  // no file.
+  const base::FilePath& full_path() const { return full_path_; }
+
+  // Returns true if the file is open. If true, the file can be written to or
+  // renamed.
   bool in_progress() const { return file_.IsValid(); }
+
+  // Returns the number of bytes in the file pointed to by full_path().
   int64 bytes_so_far() const { return bytes_so_far_; }
 
   // Fills |hash| with the hash digest for the file.

content/browser/download/base_file_posix.cc

@@ -27,8 +27,6 @@ DownloadInterruptReason BaseFile::MoveFileAndAdjustPermissions(
   if (!stat_succeeded)
     LogSystemError("stat", errno);
 
-  // TODO(estade): Move() falls back to copying and deleting when a simple
-  // rename fails. Copying sucks for large downloads. crbug.com/8737
   if (!base::Move(full_path_, new_path))
     return LogSystemError("Move", errno);
 

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);
+}
+
 // Test that a failed write reports an error.
 TEST_F(BaseFileTest, WriteWithError) {
   base::FilePath path;

content/browser/download/base_file_win.cc

@@ -10,6 +10,7 @@
 #include <shellapi.h>
 
 #include "base/file_util.h"
+#include "base/files/file.h"
 #include "base/guid.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
@@ -25,6 +26,8 @@ namespace {
 const int kAllSpecialShFileOperationCodes[] = {
   // Should be kept in sync with the case statement below.
   ERROR_ACCESS_DENIED,
+  ERROR_SHARING_VIOLATION,
+  ERROR_INVALID_PARAMETER,
   0x71,
   0x72,
   0x73,
@@ -69,10 +72,25 @@ DownloadInterruptReason MapShFileOperationCodes(int code) {
   // above.
   switch (code) {
     // Not a pre-Win32 error code; here so that this particular
-    // case shows up in our histograms.  This is redundant with the
-    // mapping function net::MapSystemError used later.
+    // case shows up in our histograms.
     case ERROR_ACCESS_DENIED:  // Access is denied.
-      result = DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
+      result = DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+      break;
+
+    // This isn't documented but returned from SHFileOperation. Sharing
+    // violations indicate that another process had the file open while we were
+    // trying to rename. Anti-virus is believed to be the cause of this error in
+    // the wild. Treated as a transient error on the assumption that the file
+    // will be made available for renaming at a later time.
+    case ERROR_SHARING_VIOLATION:
+      result = DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+      break;
+
+    // This is also not a documented return value of SHFileOperation, but has
+    // been observed in the wild. We are treating it as a transient error based
+    // on the cases we have seen so far.  See http://crbug.com/368455.
+    case ERROR_INVALID_PARAMETER:
+      result = DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
       break;
 
     // The source and destination files are the same file.
@@ -250,12 +268,20 @@ DownloadInterruptReason MapShFileOperationCodes(int code) {
             arraysize(kAllSpecialShFileOperationCodes)));
   }
 
+  if (result == DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+        "Download.MapWinShErrorTransientError", code,
+        base::CustomHistogram::ArrayToCustomRanges(
+            kAllSpecialShFileOperationCodes,
+            arraysize(kAllSpecialShFileOperationCodes)));
+  }
+
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE)
     return result;
 
   // If not one of the above codes, it should be a standard Windows error code.
-  return ConvertNetErrorToInterruptReason(
-      net::MapSystemError(code), DOWNLOAD_INTERRUPT_FROM_DISK);
+  return ConvertFileErrorToInterruptReason(
+      base::File::OSErrorToFileError(code));
 }
 
 // Maps a return code from ScanAndSaveDownloadedFile() to a

content/browser/download/download_file_impl.cc

@@ -25,6 +25,12 @@ namespace content {
 const int kUpdatePeriodMs = 500;
 const int kMaxTimeBlockingFileThreadMs = 1000;
 
+// These constants control the default retry behavior for failing renames. Each
+// retry is performed after a delay that is twice the previous delay. The
+// initial delay is specified by kInitialRenameRetryDelayMs.
+const int kMaxRenameRetries = 3;
+const int kInitialRenameRetryDelayMs = 200;
+
 int DownloadFile::number_active_objects_ = 0;
 
 DownloadFileImpl::DownloadFileImpl(
@@ -103,47 +109,84 @@ DownloadInterruptReason DownloadFileImpl::AppendDataToFile(
 void DownloadFileImpl::RenameAndUniquify(
     const base::FilePath& full_path,
     const RenameCompletionCallback& callback) {
+  RenameWithRetryInternal(
+      full_path, UNIQUIFY, kMaxRenameRetries, base::TimeTicks(), callback);
+}
+
+void DownloadFileImpl::RenameAndAnnotate(
+    const base::FilePath& full_path,
+    const RenameCompletionCallback& callback) {
+  RenameWithRetryInternal(full_path,
+                          ANNOTATE_WITH_SOURCE_INFORMATION,
+                          kMaxRenameRetries,
+                          base::TimeTicks(),
+                          callback);
+}
+
+base::TimeDelta DownloadFileImpl::GetRetryDelayForFailedRename(
+    int attempt_number) {
+  DCHECK_GE(attempt_number, 0);
+  // |delay| starts at kInitialRenameRetryDelayMs and increases by a factor of
+  // 2 at each subsequent retry. Assumes that |retries_left| starts at
+  // kMaxRenameRetries. Also assumes that kMaxRenameRetries is less than the
+  // number of bits in an int.
+  return base::TimeDelta::FromMilliseconds(kInitialRenameRetryDelayMs) *
+         (1 << attempt_number);
+}
+
+bool DownloadFileImpl::ShouldRetryFailedRename(DownloadInterruptReason reason) {
+  return reason == DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+}
+
+void DownloadFileImpl::RenameWithRetryInternal(
+    const base::FilePath& full_path,
+    RenameOption option,
+    int retries_left,
+    base::TimeTicks time_of_first_failure,
+    const RenameCompletionCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   base::FilePath new_path(full_path);
 
-  int uniquifier = base::GetUniquePathNumber(
-      new_path, base::FilePath::StringType());
-  if (uniquifier > 0) {
+  if ((option & UNIQUIFY) && full_path != file_.full_path()) {
+    int uniquifier =
+        base::GetUniquePathNumber(new_path, base::FilePath::StringType());
+    if (uniquifier > 0)
       new_path = new_path.InsertBeforeExtensionASCII(
           base::StringPrintf(" (%d)", uniquifier));
   }
 
   DownloadInterruptReason reason = file_.Rename(new_path);
-  if (reason != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    // Make sure our information is updated, since we're about to
-    // error out.
-    SendUpdate();
 
-    // Null out callback so that we don't do any more stream processing.
-    stream_reader_->RegisterCallback(base::Closure());
-
-    new_path.clear();
+  // Attempt to retry the rename if possible. If the rename failed and the
+  // subsequent open also failed, then in_progress() would be false. We don't
+  // try to retry renames if the in_progress() was false to begin with since we
+  // have less assurance that the file at file_.full_path() was the one we were
+  // working with.
+  if (ShouldRetryFailedRename(reason) && file_.in_progress() &&
+      retries_left > 0) {
+    int attempt_number = kMaxRenameRetries - retries_left;
+    BrowserThread::PostDelayedTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&DownloadFileImpl::RenameWithRetryInternal,
+                   weak_factory_.GetWeakPtr(),
+                   full_path,
+                   option,
+                   --retries_left,
+                   time_of_first_failure.is_null() ? base::TimeTicks::Now()
+                                                   : time_of_first_failure,
+                   callback),
+        GetRetryDelayForFailedRename(attempt_number));
+    return;
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(callback, reason, new_path));
-}
-
-void DownloadFileImpl::RenameAndAnnotate(
-    const base::FilePath& full_path,
-    const RenameCompletionCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  base::FilePath new_path(full_path);
-
-  DownloadInterruptReason reason = DOWNLOAD_INTERRUPT_REASON_NONE;
-  // Short circuit null rename.
-  if (full_path != file_.full_path())
-    reason = file_.Rename(new_path);
+  if (!time_of_first_failure.is_null())
+    RecordDownloadFileRenameResultAfterRetry(
+        base::TimeTicks::Now() - time_of_first_failure, reason);
 
-  if (reason == DOWNLOAD_INTERRUPT_REASON_NONE) {
+  if (reason == DOWNLOAD_INTERRUPT_REASON_NONE &&
+      (option & ANNOTATE_WITH_SOURCE_INFORMATION)) {
     // Doing the annotation after the rename rather than before leaves
     // a very small window during which the file has the final name but
     // hasn't been marked with the Mark Of The Web.  However, it allows

content/browser/download/download_file_impl.h

@@ -68,7 +68,36 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public DownloadFile {
   virtual DownloadInterruptReason AppendDataToFile(
       const char* data, size_t data_len);
 
+  virtual base::TimeDelta GetRetryDelayForFailedRename(int attempt_number);
+
+  virtual bool ShouldRetryFailedRename(DownloadInterruptReason reason);
+
  private:
+  friend class DownloadFileTest;
+
+  // Options for RenameWithRetryInternal.
+  enum RenameOption {
+    UNIQUIFY = 1 << 0,  // If there's already a file on disk that conflicts with
+                        // |new_path|, try to create a unique file by appending
+                        // a uniquifier.
+    ANNOTATE_WITH_SOURCE_INFORMATION = 1 << 1
+  };
+
+  // Rename file_ to |new_path|.
+  // |option| specifies additional operations to be performed during the rename.
+  //     See RenameOption above.
+  // |retries_left| indicates how many times to retry the operation if the
+  //     rename fails with a transient error.
+  // |time_of_first_failure| Set to an empty base::TimeTicks during the first
+  //     call. Once the first failure is seen, subsequent calls of
+  //     RenameWithRetryInternal will have a non-empty value keeping track of
+  //     the time of first observed failure.  Used for UMA.
+  void RenameWithRetryInternal(const base::FilePath& new_path,
+                               RenameOption option,
+                               int retries_left,
+                               base::TimeTicks time_of_first_failure,
+                               const RenameCompletionCallback& callback);
+
   // Send an update on our progress.
   void SendUpdate();
 

content/browser/download/download_file_unittest.cc

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "base/file_util.h"
+#include "base/files/file.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/test_file_util.h"
 #include "content/browser/browser_thread_impl.h"
@@ -59,6 +61,48 @@ class MockDownloadDestinationObserver : public DownloadDestinationObserver {
 
 MATCHER(IsNullCallback, "") { return (arg.is_null()); }
 
+typedef void (DownloadFile::*DownloadFileRenameMethodType)(
+    const base::FilePath&,
+    const DownloadFile::RenameCompletionCallback&);
+
+// This is a test DownloadFileImpl that has no retry delay and, on Posix,
+// retries renames failed due to ACCESS_DENIED.
+class TestDownloadFileImpl : public DownloadFileImpl {
+ public:
+  TestDownloadFileImpl(scoped_ptr<DownloadSaveInfo> save_info,
+                       const base::FilePath& default_downloads_directory,
+                       const GURL& url,
+                       const GURL& referrer_url,
+                       bool calculate_hash,
+                       scoped_ptr<ByteStreamReader> stream,
+                       const net::BoundNetLog& bound_net_log,
+                       base::WeakPtr<DownloadDestinationObserver> observer)
+      : DownloadFileImpl(save_info.Pass(),
+                         default_downloads_directory,
+                         url,
+                         referrer_url,
+                         calculate_hash,
+                         stream.Pass(),
+                         bound_net_log,
+                         observer) {}
+
+ protected:
+  virtual base::TimeDelta GetRetryDelayForFailedRename(
+      int attempt_count) OVERRIDE {
+    return base::TimeDelta::FromMilliseconds(0);
+  }
+
+#if !defined(OS_WIN)
+  // On Posix, we don't encounter transient errors during renames, except
+  // possibly EAGAIN, which is difficult to replicate reliably. So we resort to
+  // simulating a transient error using ACCESS_DENIED instead.
+  virtual bool ShouldRetryFailedRename(
+      DownloadInterruptReason reason) OVERRIDE {
+    return reason == DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
+  }
+#endif
+};
+
 }  // namespace
 
 class DownloadFileTest : public testing::Test {
@@ -104,15 +148,15 @@ class DownloadFileTest : public testing::Test {
   }
 
   // Mock calls to this function are forwarded here.
-  void RegisterCallback(base::Closure sink_callback) {
+  void RegisterCallback(const base::Closure& sink_callback) {
     sink_callback_ = sink_callback;
   }
 
-  void SetInterruptReasonCallback(bool* was_called,
+  void SetInterruptReasonCallback(const base::Closure& closure,
                                   DownloadInterruptReason* reason_p,
                                   DownloadInterruptReason reason) {
-    *was_called = true;
     *reason_p = reason;
+    closure.Run();
   }
 
   bool CreateDownloadFile(int offset, bool calculate_hash) {
@@ -128,8 +172,8 @@ class DownloadFileTest : public testing::Test {
         .RetiresOnSaturation();
 
     scoped_ptr<DownloadSaveInfo> save_info(new DownloadSaveInfo());
-    download_file_.reset(
-        new DownloadFileImpl(save_info.Pass(),
+    scoped_ptr<TestDownloadFileImpl> download_file_impl(
+        new TestDownloadFileImpl(save_info.Pass(),
                                  base::FilePath(),
                                  GURL(),  // Source
                                  GURL(),  // Referrer
@@ -137,21 +181,20 @@ class DownloadFileTest : public testing::Test {
                                  scoped_ptr<ByteStreamReader>(input_stream_),
                                  net::BoundNetLog(),
                                  observer_factory_.GetWeakPtr()));
-    download_file_->SetClientGuid(
-        "12345678-ABCD-1234-DCBA-123456789ABC");
+    download_file_impl->SetClientGuid("12345678-ABCD-1234-DCBA-123456789ABC");
+    download_file_ = download_file_impl.PassAs<DownloadFile>();
 
     EXPECT_CALL(*input_stream_, Read(_, _))
         .WillOnce(Return(ByteStreamReader::STREAM_EMPTY))
         .RetiresOnSaturation();
 
     base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this);
-    bool called = false;
-    DownloadInterruptReason result;
+    DownloadInterruptReason result = DOWNLOAD_INTERRUPT_REASON_NONE;
+    base::RunLoop loop_runner;
     download_file_->Initialize(base::Bind(
         &DownloadFileTest::SetInterruptReasonCallback,
-        weak_ptr_factory.GetWeakPtr(), &called, &result));
-    loop_.RunUntilIdle();
-    EXPECT_TRUE(called);
+        weak_ptr_factory.GetWeakPtr(), loop_runner.QuitClosure(), &result));
+    loop_runner.Run();
 
     ::testing::Mock::VerifyAndClearExpectations(input_stream_);
     return result == DOWNLOAD_INTERRUPT_REASON_NONE;
@@ -243,42 +286,36 @@ class DownloadFileTest : public testing::Test {
   DownloadInterruptReason RenameAndUniquify(
       const base::FilePath& full_path,
       base::FilePath* result_path_p) {
-    base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this);
-    DownloadInterruptReason result_reason(DOWNLOAD_INTERRUPT_REASON_NONE);
-    bool callback_was_called(false);
-    base::FilePath result_path;
-
-    download_file_->RenameAndUniquify(
-        full_path, base::Bind(&DownloadFileTest::SetRenameResult,
-                              weak_ptr_factory.GetWeakPtr(),
-                              &callback_was_called,
-                              &result_reason, result_path_p));
-    loop_.RunUntilIdle();
-
-    EXPECT_TRUE(callback_was_called);
-    return result_reason;
+    return InvokeRenameMethodAndWaitForCallback(
+        &DownloadFile::RenameAndUniquify, full_path, result_path_p);
   }
 
   DownloadInterruptReason RenameAndAnnotate(
       const base::FilePath& full_path,
       base::FilePath* result_path_p) {
-    base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this);
+    return InvokeRenameMethodAndWaitForCallback(
+        &DownloadFile::RenameAndAnnotate, full_path, result_path_p);
+  }
+
+ protected:
+  DownloadInterruptReason InvokeRenameMethodAndWaitForCallback(
+      DownloadFileRenameMethodType method,
+      const base::FilePath& full_path,
+      base::FilePath* result_path_p) {
     DownloadInterruptReason result_reason(DOWNLOAD_INTERRUPT_REASON_NONE);
-    bool callback_was_called(false);
     base::FilePath result_path;
 
-    download_file_->RenameAndAnnotate(
-        full_path, base::Bind(&DownloadFileTest::SetRenameResult,
-                              weak_ptr_factory.GetWeakPtr(),
-                              &callback_was_called,
-                              &result_reason, result_path_p));
-    loop_.RunUntilIdle();
-
-    EXPECT_TRUE(callback_was_called);
+    base::RunLoop loop_runner;
+    ((*download_file_).*method)(full_path,
+                                base::Bind(&DownloadFileTest::SetRenameResult,
+                                           base::Unretained(this),
+                                           loop_runner.QuitClosure(),
+                                           &result_reason,
+                                           result_path_p));
+    loop_runner.Run();
     return result_reason;
   }
 
- protected:
   scoped_ptr<StrictMock<MockDownloadDestinationObserver> > observer_;
   base::WeakPtrFactory<DownloadDestinationObserver> observer_factory_;
 
@@ -300,17 +337,16 @@ class DownloadFileTest : public testing::Test {
   base::MessageLoop loop_;
 
  private:
-  void SetRenameResult(bool* called_p,
+  void SetRenameResult(const base::Closure& closure,
                        DownloadInterruptReason* reason_p,
                        base::FilePath* result_path_p,
                        DownloadInterruptReason reason,
                        const base::FilePath& result_path) {
-    if (called_p)
-      *called_p = true;
     if (reason_p)
       *reason_p = reason;
     if (result_path_p)
       *result_path_p = result_path;
+    closure.Run();
   }
 
   // UI thread.
@@ -322,6 +358,33 @@ class DownloadFileTest : public testing::Test {
   std::string expected_data_;
 };
 
+// DownloadFile::RenameAndAnnotate and DownloadFile::RenameAndUniquify have a
+// considerable amount of functional overlap. In order to re-use test logic, we
+// are going to introduce this value parameterized test fixture. It will take a
+// DownloadFileRenameMethodType value which can be either of the two rename
+// methods.
+class DownloadFileTestWithRename
+    : public DownloadFileTest,
+      public ::testing::WithParamInterface<DownloadFileRenameMethodType> {
+ protected:
+  DownloadInterruptReason InvokeSelectedRenameMethod(
+      const base::FilePath& full_path,
+      base::FilePath* result_path_p) {
+    return InvokeRenameMethodAndWaitForCallback(
+        GetParam(), full_path, result_path_p);
+  }
+};
+
+// And now instantiate all DownloadFileTestWithRename tests using both
+// DownloadFile rename methods. Each test of the form
+// DownloadFileTestWithRename.<FooTest> will be instantiated once with
+// RenameAndAnnotate as the value parameter and once with RenameAndUniquify as
+// the value parameter.
+INSTANTIATE_TEST_CASE_P(DownloadFile,
+                        DownloadFileTestWithRename,
+                        ::testing::Values(&DownloadFile::RenameAndAnnotate,
+                                          &DownloadFile::RenameAndUniquify));
+
 const char* DownloadFileTest::kTestData1 =
     "Let's write some data to the file!\n";
 const char* DownloadFileTest::kTestData2 = "Writing more data.\n";
@@ -335,7 +398,7 @@ const int DownloadFileTest::kDummyRequestId = 67;
 
 // Rename the file before any data is downloaded, after some has, after it all
 // has, and after it's closed.
-TEST_F(DownloadFileTest, RenameFileFinal) {
+TEST_P(DownloadFileTestWithRename, RenameFileFinal) {
   ASSERT_TRUE(CreateDownloadFile(0, true));
   base::FilePath initial_path(download_file_->FullPath());
   EXPECT_TRUE(base::PathExists(initial_path));
@@ -343,12 +406,11 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   base::FilePath path_2(initial_path.InsertBeforeExtensionASCII("_2"));
   base::FilePath path_3(initial_path.InsertBeforeExtensionASCII("_3"));
   base::FilePath path_4(initial_path.InsertBeforeExtensionASCII("_4"));
-  base::FilePath path_5(initial_path.InsertBeforeExtensionASCII("_5"));
   base::FilePath output_path;
 
   // Rename the file before downloading any data.
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
-            RenameAndUniquify(path_1, &output_path));
+            InvokeSelectedRenameMethod(path_1, &output_path));
   base::FilePath renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_1, renamed_path);
   EXPECT_EQ(path_1, output_path);
@@ -363,7 +425,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // Rename the file after downloading some data.
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
-            RenameAndUniquify(path_2, &output_path));
+            InvokeSelectedRenameMethod(path_2, &output_path));
   renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_2, renamed_path);
   EXPECT_EQ(path_2, output_path);
@@ -377,7 +439,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // Rename the file after downloading all the data.
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
-            RenameAndUniquify(path_3, &output_path));
+            InvokeSelectedRenameMethod(path_3, &output_path));
   renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_3, renamed_path);
   EXPECT_EQ(path_3, output_path);
@@ -394,7 +456,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // Rename the file after downloading all the data and closing the file.
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
-            RenameAndUniquify(path_4, &output_path));
+            InvokeSelectedRenameMethod(path_4, &output_path));
   renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_4, renamed_path);
   EXPECT_EQ(path_4, output_path);
@@ -407,29 +469,42 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   EXPECT_TRUE(download_file_->GetHash(&hash));
   EXPECT_EQ(kDataHash, base::HexEncode(hash.data(), hash.size()));
 
-  // Check that a rename with overwrite to an existing file succeeds.
-  std::string file_contents;
-  ASSERT_FALSE(base::PathExists(path_5));
+  DestroyDownloadFile(0);
+}
+
+// Test to make sure the rename overwrites when requested. This is separate from
+// the above test because it only applies to RenameAndAnnotate().
+// RenameAndUniquify() doesn't overwrite by design.
+TEST_F(DownloadFileTest, RenameOverwrites) {
+  ASSERT_TRUE(CreateDownloadFile(0, true));
+  base::FilePath initial_path(download_file_->FullPath());
+  EXPECT_TRUE(base::PathExists(initial_path));
+  base::FilePath path_1(initial_path.InsertBeforeExtensionASCII("_1"));
+
+  ASSERT_FALSE(base::PathExists(path_1));
   static const char file_data[] = "xyzzy";
-  ASSERT_EQ(static_cast<int>(sizeof(file_data) - 1),
-            base::WriteFile(path_5, file_data, sizeof(file_data) - 1));
-  ASSERT_TRUE(base::PathExists(path_5));
-  EXPECT_TRUE(base::ReadFileToString(path_5, &file_contents));
-  EXPECT_EQ(std::string(file_data), file_contents);
+  ASSERT_EQ(static_cast<int>(sizeof(file_data)),
+            base::WriteFile(path_1, file_data, sizeof(file_data)));
+  ASSERT_TRUE(base::PathExists(path_1));
 
+  base::FilePath new_path;
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
-            RenameAndAnnotate(path_5, &output_path));
-  EXPECT_EQ(path_5, output_path);
+            RenameAndAnnotate(path_1, &new_path));
+  EXPECT_EQ(path_1.value(), new_path.value());
 
-  file_contents = "";
-  EXPECT_TRUE(base::ReadFileToString(path_5, &file_contents));
+  std::string file_contents;
+  ASSERT_TRUE(base::ReadFileToString(new_path, &file_contents));
   EXPECT_NE(std::string(file_data), file_contents);
 
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  loop_.RunUntilIdle();
   DestroyDownloadFile(0);
 }
 
 // Test to make sure the rename uniquifies if we aren't overwriting
-// and there's a file where we're aiming.
+// and there's a file where we're aiming. As above, not a
+// DownloadFileTestWithRename test because this only applies to
+// RenameAndUniquify().
 TEST_F(DownloadFileTest, RenameUniquifies) {
   ASSERT_TRUE(CreateDownloadFile(0, true));
   base::FilePath initial_path(download_file_->FullPath());
@@ -451,16 +526,35 @@ TEST_F(DownloadFileTest, RenameUniquifies) {
   DestroyDownloadFile(0);
 }
 
+// Test that RenameAndUniquify doesn't try to uniquify in the case where the
+// target filename is the same as the current filename.
+TEST_F(DownloadFileTest, RenameRecognizesSelfConflict) {
+  ASSERT_TRUE(CreateDownloadFile(0, true));
+  base::FilePath initial_path(download_file_->FullPath());
+  EXPECT_TRUE(base::PathExists(initial_path));
+
+  base::FilePath new_path;
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
+            RenameAndUniquify(initial_path, &new_path));
+  EXPECT_TRUE(base::PathExists(initial_path));
+
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  loop_.RunUntilIdle();
+  DestroyDownloadFile(0);
+  EXPECT_EQ(initial_path.value(), new_path.value());
+}
+
 // Test to make sure we get the proper error on failure.
-TEST_F(DownloadFileTest, RenameError) {
+TEST_P(DownloadFileTestWithRename, RenameError) {
   ASSERT_TRUE(CreateDownloadFile(0, true));
   base::FilePath initial_path(download_file_->FullPath());
 
   // Create a subdirectory.
-  base::FilePath tempdir(
-      initial_path.DirName().Append(FILE_PATH_LITERAL("tempdir")));
-  ASSERT_TRUE(base::CreateDirectory(tempdir));
-  base::FilePath target_path(tempdir.Append(initial_path.BaseName()));
+  base::FilePath target_dir(
+      initial_path.DirName().Append(FILE_PATH_LITERAL("TargetDir")));
+  ASSERT_FALSE(base::DirectoryExists(target_dir));
+  ASSERT_TRUE(base::CreateDirectory(target_dir));
+  base::FilePath target_path(target_dir.Append(initial_path.BaseName()));
 
   // Targets
   base::FilePath target_path_suffixed(
@@ -470,13 +564,13 @@ TEST_F(DownloadFileTest, RenameError) {
 
   // Make the directory unwritable and try to rename within it.
   {
-    file_util::PermissionRestorer restorer(tempdir);
-    ASSERT_TRUE(file_util::MakeFileUnwritable(tempdir));
+    file_util::PermissionRestorer restorer(target_dir);
+    ASSERT_TRUE(file_util::MakeFileUnwritable(target_dir));
 
     // Expect nulling out of further processing.
     EXPECT_CALL(*input_stream_, RegisterCallback(IsNullCallback()));
     EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
-              RenameAndAnnotate(target_path, NULL));
+              InvokeSelectedRenameMethod(target_path, NULL));
     EXPECT_FALSE(base::PathExists(target_path_suffixed));
   }
 
@@ -485,6 +579,98 @@ TEST_F(DownloadFileTest, RenameError) {
   DestroyDownloadFile(0);
 }
 
+namespace {
+
+void TestRenameCompletionCallback(const base::Closure& closure,
+                                  bool* did_run_callback,
+                                  DownloadInterruptReason interrupt_reason,
+                                  const base::FilePath& new_path) {
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
+  *did_run_callback = true;
+  closure.Run();
+}
+
+}  // namespace
+
+// Test that the retry logic works. This test assumes that DownloadFileImpl will
+// post tasks to the current message loop (acting as the FILE thread)
+// asynchronously to retry the renames. We will stuff RunLoop::QuitClosures()
+// in between the retry tasks to stagger them and then allow the rename to
+// succeed.
+//
+// Note that there is only one queue of tasks to run, and that is in the tests'
+// base::MessageLoop::current(). Each RunLoop processes that queue until it sees
+// a QuitClosure() targeted at itself, at which point it stops processing.
+TEST_P(DownloadFileTestWithRename, RenameWithErrorRetry) {
+  ASSERT_TRUE(CreateDownloadFile(0, true));
+  base::FilePath initial_path(download_file_->FullPath());
+
+  // Create a subdirectory.
+  base::FilePath target_dir(
+      initial_path.DirName().Append(FILE_PATH_LITERAL("TargetDir")));
+  ASSERT_FALSE(base::DirectoryExists(target_dir));
+  ASSERT_TRUE(base::CreateDirectory(target_dir));
+  base::FilePath target_path(target_dir.Append(initial_path.BaseName()));
+
+  bool did_run_callback = false;
+
+  // Each RunLoop can be used the run the MessageLoop until the corresponding
+  // QuitClosure() is run. This one is used to produce the QuitClosure() that
+  // will be run when the entire rename operation is complete.
+  base::RunLoop succeeding_run;
+  {
+    // (Scope for the base::File or file_util::PermissionRestorer below.)
+#if defined(OS_WIN)
+    // On Windows we test with an actual transient error, a sharing violation.
+    // The rename will fail because we are holding the file open for READ. On
+    // Posix this doesn't cause a failure.
+    base::File locked_file(initial_path,
+                           base::File::FLAG_OPEN | base::File::FLAG_READ);
+    ASSERT_TRUE(locked_file.IsValid());
+#else
+    // Simulate a transient failure by revoking write permission for target_dir.
+    // The TestDownloadFileImpl class treats this error as transient even though
+    // DownloadFileImpl itself doesn't.
+    file_util::PermissionRestorer restore_permissions_for(target_dir);
+    ASSERT_TRUE(file_util::MakeFileUnwritable(target_dir));
+#endif
+
+    // The Rename() should fail here and enqueue a retry task without invoking
+    // the completion callback.
+    ((*download_file_).*GetParam())(target_path,
+                                    base::Bind(&TestRenameCompletionCallback,
+                                               succeeding_run.QuitClosure(),
+                                               &did_run_callback));
+    EXPECT_FALSE(did_run_callback);
+
+    base::RunLoop first_failing_run;
+    // Queue the QuitClosure() on the MessageLoop now. Any tasks queued by the
+    // Rename() will be in front of the QuitClosure(). Running the message loop
+    // now causes the just the first retry task to be run. The rename still
+    // fails, so another retry task would get queued behind the QuitClosure().
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           first_failing_run.QuitClosure());
+    first_failing_run.Run();
+    EXPECT_FALSE(did_run_callback);
+
+    // Running another loop should have the same effect as the above as long as
+    // kMaxRenameRetries is greater than 2.
+    base::RunLoop second_failing_run;
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           second_failing_run.QuitClosure());
+    second_failing_run.Run();
+    EXPECT_FALSE(did_run_callback);
+  }
+
+  // This time the QuitClosure from succeeding_run should get executed.
+  succeeding_run.Run();
+  EXPECT_TRUE(did_run_callback);
+
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  loop_.RunUntilIdle();
+  DestroyDownloadFile(0);
+}
+
 // Various tests of the StreamActive method.
 TEST_F(DownloadFileTest, StreamEmptySuccess) {
   ASSERT_TRUE(CreateDownloadFile(0, true));

content/browser/download/download_interrupt_reasons_impl.cc

@@ -8,6 +8,35 @@
 
 namespace content {
 
+DownloadInterruptReason ConvertFileErrorToInterruptReason(
+    base::File::Error file_error) {
+  switch (file_error) {
+    case base::File::FILE_OK:
+      return DOWNLOAD_INTERRUPT_REASON_NONE;
+
+    case base::File::FILE_ERROR_IN_USE:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+
+    case base::File::FILE_ERROR_ACCESS_DENIED:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
+
+    case base::File::FILE_ERROR_TOO_MANY_OPENED:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+
+    case base::File::FILE_ERROR_NO_MEMORY:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+
+    case base::File::FILE_ERROR_NO_SPACE:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE;
+
+    case base::File::FILE_ERROR_SECURITY:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
+
+    default:
+      return DOWNLOAD_INTERRUPT_REASON_FILE_FAILED;
+  }
+}
+
 DownloadInterruptReason ConvertNetErrorToInterruptReason(
     net::Error net_error, DownloadInterruptSource source) {
   switch (net_error) {

content/browser/download/download_interrupt_reasons_impl.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_INTERRUPT_REASONS_IMPL_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_INTERRUPT_REASONS_IMPL_H_
 
+#include "base/files/file.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "net/base/net_errors.h"
 
@@ -22,6 +23,10 @@ enum DownloadInterruptSource {
 DownloadInterruptReason CONTENT_EXPORT ConvertNetErrorToInterruptReason(
     net::Error file_error, DownloadInterruptSource source);
 
+// Safe to call from any thread.
+DownloadInterruptReason CONTENT_EXPORT ConvertFileErrorToInterruptReason(
+    base::File::Error file_error);
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_INTERRUPT_REASONS_IMPL_H_

content/browser/download/download_stats.cc

@@ -611,6 +611,18 @@ void RecordFileBandwidth(size_t length,
                            disk_write_time_ms * 100 / elapsed_time_ms);
 }
 
+void RecordDownloadFileRenameResultAfterRetry(
+    base::TimeDelta time_since_first_failure,
+    DownloadInterruptReason interrupt_reason) {
+  if (interrupt_reason == DOWNLOAD_INTERRUPT_REASON_NONE) {
+    UMA_HISTOGRAM_TIMES("Download.TimeToRenameSuccessAfterInitialFailure",
+                        time_since_first_failure);
+  } else {
+    UMA_HISTOGRAM_TIMES("Download.TimeToRenameFailureAfterInitialFailure",
+                        time_since_first_failure);
+  }
+}
+
 void RecordSavePackageEvent(SavePackageEvent event) {
   UMA_HISTOGRAM_ENUMERATION("Download.SavePackage",
                             event,

content/browser/download/download_stats.h

@@ -200,6 +200,11 @@ void RecordFileBandwidth(size_t length,
                          base::TimeDelta disk_write_time,
                          base::TimeDelta elapsed_time);
 
+// Record the result of a download file rename.
+void RecordDownloadFileRenameResultAfterRetry(
+    base::TimeDelta time_since_first_failure,
+    DownloadInterruptReason interrupt_reason);
+
 enum SavePackageEvent {
   // The user has started to save a page as a package.
   SAVE_PACKAGE_STARTED,

content/public/browser/download_interrupt_reason_values.h

@@ -14,7 +14,6 @@
 INTERRUPT_REASON(FILE_FAILED, 1)
 
 // The file cannot be accessed due to security restrictions.
-// The file cannot be accessed.
 // "Access Denied".
 INTERRUPT_REASON(FILE_ACCESS_DENIED, 2)
 

tools/metrics/histograms/histograms.xml

@@ -4736,6 +4736,15 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   </summary>
 </histogram>
 
+<histogram name="Download.MapWinShErrorTransientError"
+    enum="SpecialShFileOperationCodes">
+  <owner>asanka@chromium.org</owner>
+  <summary>
+    Windows error that produced a DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR
+    result in MapShFileOperationCodes().
+  </summary>
+</histogram>
+
 <histogram name="Download.OnChanged">
   <owner>asanka@chromium.org</owner>
   <summary>
@@ -4869,6 +4878,24 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <summary>Time between the start of a download and its completion.</summary>
 </histogram>
 
+<histogram name="Download.TimeToRenameFailureAfterInitialFailure"
+    units="milliseconds">
+  <owner>asanka@chromium.org</owner>
+  <summary>
+    Time elapsed until a retried download file rename operation failed for the
+    last time after the initial rename failed.
+  </summary>
+</histogram>
+
+<histogram name="Download.TimeToRenameSuccessAfterInitialFailure"
+    units="milliseconds">
+  <owner>asanka@chromium.org</owner>
+  <summary>
+    Time elapsed until a retried download file rename operation succeeded after
+    the initial rename failed.
+  </summary>
+</histogram>
+
 <histogram name="Download.UserDiscard" enum="DownloadItem.DangerType">
   <owner>asanka@chromium.org</owner>
   <owner>felt@chromium.org</owner>
@@ -44086,7 +44113,9 @@ Therefore, the affected-histogram name has to have at least one dot in it.
 
 <enum name="SpecialShFileOperationCodes" type="int">
   <summary>Legacy error codes still returned by |ShFileOperation()|</summary>
-  <int value="5" label="Access denied"/>
+  <int value="5" label="Access denied (Win32)"/>
+  <int value="32" label="Sharing violation (Win32)"/>
+  <int value="87" label="Invalid parameter (Win32)"/>
   <int value="113" label="Source and Destination are same file"/>
   <int value="114" label="Multiple source mapped to single destination"/>
   <int value="115" label="Rename to different directory"/>