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

RESOLVED FIXED in Firefox 68

Status

()

defect
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 2 bugs, {main-thread-io, perf})

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [fxperf:p2] [fxperfsize:S])

Attachments

(1 attachment)

Assignee

Description

2 months ago

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.
Assignee

Updated

2 months ago
See Also: → 1471668
Assignee

Updated

2 months ago
Blocks: 1544019
Whiteboard: [fxperf] → [fxperf:p2]
Assignee

Updated

2 months ago
Whiteboard: [fxperf:p2] → [fxperf:p2] [fxperfsize:S]
Assignee

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Comment 2

2 months ago
Pushed by gijskruitbosch@gmail.com:
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

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.