[Downloads] Retry renames after transient failures.

content/browser/download/base_file_win.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 <windows.h>
 #include <cguid.h>
 #include <objbase.h>
 #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"
 #include "base/threading/thread_restrictions.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/safe_util_win.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace content {
 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,
   0x74,
   0x75,
   0x76,
   0x78,
   0x79,
   0x7A,
   0x7C,
@@ skipping 24 matching lines @@
 // See http://msdn.microsoft.com/en-us/library/bb762164(VS.85).aspx.
 DownloadInterruptReason MapShFileOperationCodes(int code) {
   DownloadInterruptReason result = DOWNLOAD_INTERRUPT_REASON_NONE;
 
   // Check these pre-Win32 error codes first, then check for matches
   // in Winerror.h.
   // This switch statement should be kept in sync with the list of codes
   // 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.
     // DE_SAMEFILE == 0x71
     case 0x71:
       result = DOWNLOAD_INTERRUPT_REASON_FILE_FAILED;
       break;
 
     // The operation was canceled by the user, or silently canceled if the
     // appropriate flags were supplied to SHFileOperation.
@@ skipping 157 matching lines @@
   }
 
   if (result == DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED) {
     UMA_HISTOGRAM_CUSTOM_ENUMERATION(
         "Download.MapWinShErrorAccessDenied", code,
         base::CustomHistogram::ArrayToCustomRanges(
             kAllSpecialShFileOperationCodes,
             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
 // DownloadInterruptReason. The return code in |result| is usually from the
 // final IAttachmentExecute::Save() call.
 DownloadInterruptReason MapScanAndSaveErrorCodeToInterruptReason(
     HRESULT result) {
   if (SUCCEEDED(result))
     return DOWNLOAD_INTERRUPT_REASON_NONE;
 
@@ skipping 89 matching lines @@
       RecordDownloadCount(FILE_MISSING_AFTER_SUCCESSFUL_SCAN_COUNT);
       result = DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED;
     }
     LogInterruptReason("ScanAndSaveDownloadedFile", hr, result);
   }
   bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED);
   return result;
 }
 
 }  // namespace content