Bug 1544009 - stop doing remote drive and directory exist/creation checks for renames/moves on Windows, esp. if in the same directory, r=froydnj
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 01 May 2019 00:17:22 +0000
changeset 472057 8ccd4b2a5fbaee6cd2c1326521082fe32c07556a
parent 472056 e1993a1f09ac53cd1a04fdf6a87f8cad8e44f73e
child 472058 6a3c75d8e68ad9e84c8f41f1accae69e13dd1210
push id35946
push userapavel@mozilla.com
push dateWed, 01 May 2019 15:54:31 +0000
treeherdermozilla-central@a027a998b8b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1544009
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1544009 - stop doing remote drive and directory exist/creation checks for renames/moves on Windows, esp. if in the same directory, r=froydnj Differential Revision: https://phabricator.services.mozilla.com/D29222
xpcom/io/nsLocalFileWin.cpp
--- a/xpcom/io/nsLocalFileWin.cpp
+++ b/xpcom/io/nsLocalFileWin.cpp
@@ -1671,56 +1671,54 @@ nsresult nsLocalFile::CopySingleFile(nsI
   } else {
     rv = aSourceFile->GetPath(filePath);
   }
 
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  // Pass the flag COPY_FILE_NO_BUFFERING to CopyFileEx as we may be copying
-  // to a SMBV2 remote drive. Without this parameter subsequent append mode
-  // file writes can cause the resultant file to become corrupt. We only need to
-  // do this if the major version of Windows is > 5(Only Windows Vista and above
-  // can support SMBV2).  With a 7200RPM hard drive:
-  // Copying a 1KB file with COPY_FILE_NO_BUFFERING takes about 30-60ms.
-  // Copying a 1KB file without COPY_FILE_NO_BUFFERING takes < 1ms.
-  // So we only use COPY_FILE_NO_BUFFERING when we have a remote drive.
-  int copyOK;
-  DWORD dwCopyFlags = COPY_FILE_ALLOW_DECRYPTED_DESTINATION;
-  bool path1Remote, path2Remote;
-  if (!IsRemoteFilePath(filePath.get(), path1Remote) ||
-      !IsRemoteFilePath(destPath.get(), path2Remote) || path1Remote ||
-      path2Remote) {
-    dwCopyFlags |= COPY_FILE_NO_BUFFERING;
-  }
-
   if (FilePreferences::IsBlockedUNCPath(destPath)) {
     return NS_ERROR_FILE_ACCESS_DENIED;
   }
 
-  if (!move) {
-    copyOK = ::CopyFileExW(filePath.get(), destPath.get(), nullptr, nullptr,
-                           nullptr, dwCopyFlags);
-  } else {
+  int copyOK = 0;
+  if (move) {
     copyOK = ::MoveFileExW(filePath.get(), destPath.get(),
                            MOVEFILE_REPLACE_EXISTING);
-
-    // Check if copying the source file to a different volume,
-    // as this could be an SMBV2 mapped drive.
-    if (!copyOK && GetLastError() == ERROR_NOT_SAME_DEVICE) {
-      if (aOptions & Rename) {
-        return NS_ERROR_FILE_ACCESS_DENIED;
-      }
-      copyOK = CopyFileExW(filePath.get(), destPath.get(), nullptr, nullptr,
+  }
+
+  // If we either failed to move the file, or this is a copy, try copying:
+  if (!copyOK && (!move || GetLastError() == ERROR_NOT_SAME_DEVICE)) {
+    // Failed renames here should just return access denied.
+    if (move && (aOptions & Rename)) {
+      return NS_ERROR_FILE_ACCESS_DENIED;
+    }
+
+    // Pass the flag COPY_FILE_NO_BUFFERING to CopyFileEx as we may be copying
+    // to a SMBV2 remote drive. Without this parameter subsequent append mode
+    // file writes can cause the resultant file to become corrupt. We only need to
+    // do this if the major version of Windows is > 5(Only Windows Vista and above
+    // can support SMBV2).  With a 7200RPM hard drive:
+    // Copying a 1KB file with COPY_FILE_NO_BUFFERING takes about 30-60ms.
+    // Copying a 1KB file without COPY_FILE_NO_BUFFERING takes < 1ms.
+    // So we only use COPY_FILE_NO_BUFFERING when we have a remote drive.
+    DWORD dwCopyFlags = COPY_FILE_ALLOW_DECRYPTED_DESTINATION;
+    bool path1Remote, path2Remote;
+    if (!IsRemoteFilePath(filePath.get(), path1Remote) ||
+        !IsRemoteFilePath(destPath.get(), path2Remote) || path1Remote ||
+        path2Remote) {
+      dwCopyFlags |= COPY_FILE_NO_BUFFERING;
+    }
+
+    copyOK = ::CopyFileExW(filePath.get(), destPath.get(), nullptr, nullptr,
                            nullptr, dwCopyFlags);
 
-      if (copyOK) {
-        DeleteFileW(filePath.get());
-      }
+    if (move && copyOK) {
+      DeleteFileW(filePath.get());
     }
   }
 
   if (!copyOK) {  // CopyFileEx and MoveFileEx return zero at failure.
     rv = ConvertWinError(GetLastError());
   } else if (move && !(aOptions & SkipNtfsAclReset)) {
     // Set security permissions to inherit from parent.
     // Note: propagates to all children: slow for big file trees
@@ -1741,16 +1739,20 @@ nsresult nsLocalFile::CopySingleFile(nsI
 
   return rv;
 }
 
 nsresult nsLocalFile::CopyMove(nsIFile* aParentDir, const nsAString& aNewName,
                                uint32_t aOptions) {
   bool move = aOptions & (Move | Rename);
   bool followSymlinks = aOptions & FollowSymlinks;
+  // If we're not provided with a new parent, we're copying or moving to
+  // another file in the same directory and can safely skip checking if the
+  // destination directory exists:
+  bool targetInSameDirectory = !aParentDir;
 
   nsCOMPtr<nsIFile> newParentDir = aParentDir;
   // check to see if this exists, otherwise return an error.
   // we will check this by resolving.  If the user wants us
   // to follow links, then we are talking about the target,
   // hence we can use the |FollowSymlinks| option.
   nsresult rv = ResolveAndStat();
   if (NS_FAILED(rv)) {
@@ -1768,61 +1770,63 @@ nsresult nsLocalFile::CopyMove(nsIFile* 
       return rv;
     }
   }
 
   if (!newParentDir) {
     return NS_ERROR_FILE_DESTINATION_NOT_DIR;
   }
 
-  // make sure it exists and is a directory.  Create it if not there.
-  bool exists = false;
-  rv = newParentDir->Exists(&exists);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
-  if (!exists) {
-    rv = newParentDir->Create(DIRECTORY_TYPE,
-                              0644);  // TODO, what permissions should we use
-    if (NS_FAILED(rv)) {
-      return rv;
-    }
-  } else {
-    bool isDir = false;
-    rv = newParentDir->IsDirectory(&isDir);
+  if (!targetInSameDirectory) {
+    // make sure it exists and is a directory.  Create it if not there.
+    bool exists = false;
+    rv = newParentDir->Exists(&exists);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
-    if (!isDir) {
-      if (followSymlinks) {
-        bool isLink = false;
-        rv = newParentDir->IsSymlink(&isLink);
-        if (NS_FAILED(rv)) {
-          return rv;
-        }
-
-        if (isLink) {
-          nsAutoString target;
-          rv = newParentDir->GetTarget(target);
+    if (!exists) {
+      rv = newParentDir->Create(DIRECTORY_TYPE,
+                                0644);  // TODO, what permissions should we use
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+    } else {
+      bool isDir = false;
+      rv = newParentDir->IsDirectory(&isDir);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+
+      if (!isDir) {
+        if (followSymlinks) {
+          bool isLink = false;
+          rv = newParentDir->IsSymlink(&isLink);
           if (NS_FAILED(rv)) {
             return rv;
           }
 
-          nsCOMPtr<nsIFile> realDest = new nsLocalFile();
-          rv = realDest->InitWithPath(target);
-          if (NS_FAILED(rv)) {
-            return rv;
+          if (isLink) {
+            nsAutoString target;
+            rv = newParentDir->GetTarget(target);
+            if (NS_FAILED(rv)) {
+              return rv;
+            }
+
+            nsCOMPtr<nsIFile> realDest = new nsLocalFile();
+            rv = realDest->InitWithPath(target);
+            if (NS_FAILED(rv)) {
+              return rv;
+            }
+
+            return CopyMove(realDest, aNewName, aOptions);
           }
-
-          return CopyMove(realDest, aNewName, aOptions);
+        } else {
+          return NS_ERROR_FILE_DESTINATION_NOT_DIR;
         }
-      } else {
-        return NS_ERROR_FILE_DESTINATION_NOT_DIR;
       }
     }
   }
 
   // Try different ways to move/copy files/directories
   bool done = false;
 
   bool isDir = false;
@@ -1892,16 +1896,17 @@ nsresult nsLocalFile::CopyMove(nsIFile* 
 
     rv = target->Append(allocatedNewName);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     allocatedNewName.Truncate();
 
+    bool exists = false;
     // check if the destination directory already exists
     rv = target->Exists(&exists);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     if (!exists) {
       // if the destination directory cannot be created, return an error
@@ -2031,16 +2036,21 @@ nsLocalFile::CopyToFollowingLinks(nsIFil
 
 NS_IMETHODIMP
 nsLocalFile::MoveTo(nsIFile* aNewParentDir, const nsAString& aNewName) {
   return CopyMove(aNewParentDir, aNewName, Move);
 }
 
 NS_IMETHODIMP
 nsLocalFile::RenameTo(nsIFile* aNewParentDir, const nsAString& aNewName) {
+  // If we're not provided with a new parent, we're renaming inside one and
+  // the same directory and can safely skip checking if the destination
+  // directory exists:
+  bool targetInSameDirectory = !aNewParentDir;
+
   nsCOMPtr<nsIFile> targetParentDir = aNewParentDir;
   // check to see if this exists, otherwise return an error.
   // we will check this by resolving.  If the user wants us
   // to follow links, then we are talking about the target,
   // hence we can use the |followSymlinks| parameter.
   nsresult rv = ResolveAndStat();
   if (NS_FAILED(rv)) {
     return rv;
@@ -2056,36 +2066,38 @@ nsLocalFile::RenameTo(nsIFile* aNewParen
       return rv;
     }
   }
 
   if (!targetParentDir) {
     return NS_ERROR_FILE_DESTINATION_NOT_DIR;
   }
 
-  // make sure it exists and is a directory.  Create it if not there.
-  bool exists = false;
-  rv = targetParentDir->Exists(&exists);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
-  if (!exists) {
-    rv = targetParentDir->Create(DIRECTORY_TYPE, 0644);
+  if (!targetInSameDirectory) {
+    // make sure it exists and is a directory.  Create it if not there.
+    bool exists = false;
+    rv = targetParentDir->Exists(&exists);
     if (NS_FAILED(rv)) {
       return rv;
     }
-  } else {
-    bool isDir = false;
-    rv = targetParentDir->IsDirectory(&isDir);
-    if (NS_FAILED(rv)) {
-      return rv;
-    }
-    if (!isDir) {
-      return NS_ERROR_FILE_DESTINATION_NOT_DIR;
+
+    if (!exists) {
+      rv = targetParentDir->Create(DIRECTORY_TYPE, 0644);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+    } else {
+      bool isDir = false;
+      rv = targetParentDir->IsDirectory(&isDir);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+      if (!isDir) {
+        return NS_ERROR_FILE_DESTINATION_NOT_DIR;
+      }
     }
   }
 
   uint32_t options = Rename;
   if (!aNewParentDir) {
     options |= SkipNtfsAclReset;
   }
   // Move single file, or move a directory