Bug 1488452 - Remove incorrect use of PromiseFlatString in FileUtilsWin. r=froydnj
authorHenri Sivonen <hsivonen@hsivonen.fi>
Fri, 07 Sep 2018 14:48:36 +0000
changeset 490957 36ee80fc14aefdd00385da7fc073f28e78dfbd40
parent 490956 5b5f54aa78d77f0df5ae455265c4b03323e8c731
child 490965 3026c40acec365f6606c39234bba5f4e99c83dac
child 490966 70224e316a2cbf815e6916029c98f19989e78c03
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1488452
milestone64.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 1488452 - Remove incorrect use of PromiseFlatString in FileUtilsWin. r=froydnj Also rewrote some code around the actual problem to be more obviously correct and comprehensible. MozReview-Commit-ID: FF2hSjQ4U1x Differential Revision: https://phabricator.services.mozilla.com/D5154
xpcom/io/FileUtilsWin.h
--- a/xpcom/io/FileUtilsWin.h
+++ b/xpcom/io/FileUtilsWin.h
@@ -12,45 +12,35 @@
 #include "mozilla/Scoped.h"
 #include "nsString.h"
 
 namespace mozilla {
 
 inline bool
 EnsureLongPath(nsAString& aDosPath)
 {
-  uint32_t aDosPathOriginalLen = aDosPath.Length();
-  auto inputPath = PromiseFlatString(aDosPath);
-  // Try to get the long path, or else get the required length of the long path
-  DWORD longPathLen = GetLongPathNameW(inputPath.get(),
-                                       reinterpret_cast<wchar_t*>(aDosPath.BeginWriting()),
-                                       aDosPathOriginalLen);
-  if (longPathLen == 0) {
-    return false;
-  }
-  aDosPath.SetLength(longPathLen);
-  if (longPathLen <= aDosPathOriginalLen) {
-    // Our string happened to be long enough for the first call to succeed.
-    return true;
+  nsAutoString inputPath(aDosPath);
+  while (true) {
+    DWORD requiredLength = GetLongPathNameW(inputPath.get(),
+                                            reinterpret_cast<wchar_t*>(aDosPath.BeginWriting()),
+                                            aDosPath.Length());
+    if (!requiredLength) {
+      return false;
+    }
+    if (requiredLength < aDosPath.Length()) {
+      // When GetLongPathNameW deems the last argument too small,
+      // it returns a value, but when you pass that value back, it's
+      // satisfied and returns a number that's one smaller. If the above
+      // check was == instead of <, the loop would go on forever with
+      // GetLongPathNameW returning oscillating values!
+      aDosPath.Truncate(requiredLength);
+      return true;
+    }
+    aDosPath.SetLength(requiredLength);
   }
-  // Now we have a large enough buffer, get the actual string
-  longPathLen = GetLongPathNameW(inputPath.get(),
-                                 reinterpret_cast<wchar_t*>(aDosPath.BeginWriting()), aDosPath.Length());
-  if (longPathLen == 0) {
-    return false;
-  }
-  // This success check should always be less-than because longPathLen excludes
-  // the null terminator on success, but includes it in the first call that
-  // returned the required size.
-  if (longPathLen < aDosPath.Length()) {
-    aDosPath.SetLength(longPathLen);
-    return true;
-  }
-  // We shouldn't reach this, but if we do then it's a failure!
-  return false;
 }
 
 inline bool
 NtPathToDosPath(const nsAString& aNtPath, nsAString& aDosPath)
 {
   aDosPath.Truncate();
   if (aNtPath.IsEmpty()) {
     return true;
@@ -62,34 +52,42 @@ NtPathToDosPath(const nsAString& aNtPath
       ntPathLen >= symLinkPrefixLen &&
       Substring(aNtPath, 0, symLinkPrefixLen).Equals(symLinkPrefix)) {
     // Symbolic link for DOS device. Just strip off the prefix.
     aDosPath = aNtPath;
     aDosPath.Cut(0, 4);
     return true;
   }
   nsAutoString logicalDrives;
-  DWORD len = 0;
   while (true) {
-    len = GetLogicalDriveStringsW(
-      len, reinterpret_cast<wchar_t*>(logicalDrives.BeginWriting()));
-    if (!len) {
+    DWORD requiredLength = GetLogicalDriveStringsW(
+      logicalDrives.Length(), reinterpret_cast<wchar_t*>(logicalDrives.BeginWriting()));
+    if (!requiredLength) {
       return false;
-    } else if (len > logicalDrives.Length()) {
-      logicalDrives.SetLength(len);
-    } else {
+    }
+    if (requiredLength < logicalDrives.Length()) {
+      // When GetLogicalDriveStringsW deems the first argument too small,
+      // it returns a value, but when you pass that value back, it's
+      // satisfied and returns a number that's one smaller. If the above
+      // check was == instead of <, the loop would go on forever with
+      // GetLogicalDriveStringsW returning oscillating values!
+      logicalDrives.Truncate(requiredLength);
+      // logicalDrives now has the format "C:\\\0D:\\\0Z:\\\0". That is,
+      // the sequence drive letter, colon, backslash, U+0000 repeats.
       break;
     }
+    logicalDrives.SetLength(requiredLength);
   }
+
   const char16_t* cur = logicalDrives.BeginReading();
   const char16_t* end = logicalDrives.EndReading();
   nsString targetPath;
   targetPath.SetLength(MAX_PATH);
   wchar_t driveTemplate[] = L" :";
-  do {
+  while (cur < end) {
     // Unfortunately QueryDosDevice doesn't support the idiom for querying the
     // output buffer size, so it may require retries.
     driveTemplate[0] = *cur;
     DWORD targetPathLen = 0;
     SetLastError(ERROR_SUCCESS);
     while (true) {
       targetPathLen = QueryDosDeviceW(driveTemplate,
                                       reinterpret_cast<wchar_t*>(targetPath.BeginWriting()),
@@ -108,19 +106,26 @@ NtPathToDosPath(const nsAString& aNtPath
                              firstTargetPathLen) == 0 &&
                    *pathComponent == L'\\';
       if (found) {
         aDosPath = driveTemplate;
         aDosPath += pathComponent;
         return EnsureLongPath(aDosPath);
       }
     }
-    // Advance to the next NUL character in logicalDrives
-    while (*cur++);
-  } while (cur != end);
+    // Find the next U+0000 within the logical string
+    while (*cur) {
+      // This loop skips the drive letter, the colon
+      // and the backslash.
+      cur++;
+    }
+    // Skip over the U+0000 that ends a drive entry
+    // within the logical string
+    cur++;
+  }
   // Try to handle UNC paths. NB: This must happen after we've checked drive
   // mappings in case a UNC path is mapped to a drive!
   NS_NAMED_LITERAL_STRING(uncPrefix, "\\\\");
   NS_NAMED_LITERAL_STRING(deviceMupPrefix, "\\Device\\Mup\\");
   if (StringBeginsWith(aNtPath, deviceMupPrefix)) {
     aDosPath = uncPrefix;
     aDosPath += Substring(aNtPath, deviceMupPrefix.Length());
     return true;