[Downloads] Retry renames after transient failures.

content/browser/download/base_file_win.cc

Index: content/browser/download/base_file_win.cc
diff --git a/content/browser/download/base_file_win.cc b/content/browser/download/base_file_win.cc
index 252f0495d7da5ba1446d1a9a766849e873fc37dc..42162f132b1e90ebef5b344aa312c8d7c7919705 100644
--- a/content/browser/download/base_file_win.cc
+++ b/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 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:

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

nit: Just because it's the kind of a-r person I am, is it reasonable to order
the case statements the same as the above kAllSpecialShFileOperationCodes[]
array?

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

Haha. Done. That was actually the intent. The constants in the
kAllSpecialShFileOperationCodes are sorted the value of the constant. I forgot
to reorder this list after I looked up values.

+      result = DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+      break;
+
+    // This is also undocumented but returned from SHFileOperation. Treated as a
+    // transient error since noone else should be holding on to our file while
+    // we are using it and whomever is holding on may let go momentarily. Value

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

This comment makes me a little uncomfortable, since it's effectively saying
"This should never happen, but if it does it probably won't last very long, but
we won't provide any reasoning as to why it won't last very long."  My personal
(uninformed) opinion is that if this happens it's probably anti-virus, which is
a reasonable reason to think it won't last very long--worth putting that theory
in the comment?

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

Mentioned AV. But in general sharing violations are considered transient since
it represents an attempt to access the same resource in a conflicting manner by
two or more running processes. ACCESS_DENIED due to permission issues, on the
other hand, isn't transient and usually require user intervention.

+    // is converted here so that it shows up on UMA.
+    case ERROR_SHARING_VIOLATION:
+      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