[Downloads] Retry renames after transient failures.

content/browser/download/base_file.cc

 // Copyright (c) 2012 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 "content/browser/download/base_file.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/file.h"
 #include "base/format_macros.h"
@@ skipping 141 matching lines @@
   bound_net_log_.BeginEvent(
       net::NetLog::TYPE_DOWNLOAD_FILE_RENAMED,
       base::Bind(&FileRenamedNetLogCallback, &full_path_, &new_path));
   Close();
   base::CreateDirectory(new_path.DirName());
 
   // A simple rename wouldn't work here since we want the file to have
   // 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.
-    if (was_in_progress)
-      rename_result = Open();
-  }
+
+  // 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)
+    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;

[Randy Smith (Not in Mondays), 2014/06/11 18:07:29]

This feels a bit funny, though maybe it's ok.  I'm worried about the state the
BaseFile can be in after this call; it seems like it can be:
* Success: new name && opened
* Rename failure: old name && opened
* Open failure: new name && unopened
* Both failed: old name && unopened

Can the difference between all of these be detected from the result?  Does the
calling code properly handle all of these cases?  Should we break Rename into
two calls (Reopen) or return separate errors so that the calling code can at
least handle these all properly if it wants to?

[asanka, 2014/06/12 20:02:37]

The invariants are:
* full_path() always returns the last known name of the file.
* in_progress() is true if the file is opened and ready to be written to or
renamed.
* The return value is NONE only if all intermediate steps succeeded. This isn't
currently true [1], but well...

So, for the cases above:
* Return code is REASON_NONE.
* Return code != NONE. in_progress() is true. full_path() returns old name.
* Return code != NONE. in_progress() is false. full_path() returns new name.
* Return code != NONE. in_progress() is false. full_path() returns old name.

The caller, at the moment, is only interested in:
* Did it work? (check return value)
* If not, can I try again? or should I just give up? (check in_progress())
* If it failed, where can I find what's left of the file? (check full_path())

[1]: Close() is an important failure point even though we're ignoring it.
Depending on the file system, close() could be where much of the fallible
activity happens. write()s can just write to a local cache or buffer. close()
doesn't have such a luxury and usually forces the underlying file system to do
all the actual writes (to disk or over the network).

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

Ah, that makes sense.  Thanks for taking me through it.  

(Part of the reaction I'm having is that this doesn't seem like a solid,
bullet-proof API, and that that's well well beyond the scope of this CL to fix
:-}.  But I'm happier thinking the consumer can figure out the state if they
need to.)

 }
 
 void BaseFile::Detach() {
   detached_ = true;
   bound_net_log_.AddEvent(net::NetLog::TYPE_DOWNLOAD_FILE_DETACHED);
 }
 
 void BaseFile::Cancel() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(!detached_);
@@ skipping 138 matching lines @@
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_FILE_ERROR,
       base::Bind(&FileErrorNetLogCallback, operation, error));
   return ConvertNetErrorToInterruptReason(error, DOWNLOAD_INTERRUPT_FROM_DISK);
 }
 
 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(
     const char* operation,
     int os_error,
     DownloadInterruptReason reason) {
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_FILE_ERROR,
       base::Bind(&FileInterruptedNetLogCallback, operation, os_error, reason));
   return reason;
 }
 
 }  // namespace content