1544009 - Windows file moving code does smbv2-related "is this a remote drive" checks even for what are effectively renames

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[:Gijs (he/him)]

https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/xpcom/io/nsLocalFileWin.cpp#1679-1720

Provided in full for your despair:

  // 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 {
    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,
                           nullptr, dwCopyFlags);

      if (copyOK) {
        DeleteFileW(filePath.get());
      }
    }
  }

Note, the IsRemoteFilePath checks call GetDriveType which does a create/open call on the parent directory. See also bug 1541593 - we should probably cache some of this information for common folders (like the profile dir, local profile dir, and app dirs).

Anyway, this code is inefficient, unfortunately not in a way that the compiler groks and optimizes for us. I propose doing the following:

• move the UNC check before the remote volume check to save work (hopefully the compiler does this already but it's pretty easy to just do ourselves, so...)
• invert the !move check and put the move code first
• either in an else branch, or in an if (!move || (!copyOK && ... )) branch, do the dwCopyFlags computation and do a copy, deleting the original if move.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1544009 - stop doing remote drive and directory exist/creation checks for renames/moves on Windows, esp. if in the same directory, r?froydnj

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ccd4b2a5fba
stop doing remote drive and directory exist/creation checks for renames/moves on Windows, esp. if in the same directory, r=froydnj

Comment 3

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8ccd4b2a5fba