Bug 1173320 - patch 3/8 - Improve the Windows path management, r=smaug
☠☠ backed out by bd8284e36c7c ☠ ☠
authorAndrea Marchesini <amarchesini@mozilla.com>
Sat, 19 Mar 2016 14:33:27 +0100
changeset 289461 401c1e7df8eaef6c5801a84d1abf05fdf0e3d2be
parent 289460 bf65b38f759ede2f241278b83f3d46213c2ad302
child 289462 e94ced2f8e3cf5e639924b1800f93f1e79f4b1a9
push id30102
push userryanvm@gmail.com
push dateSat, 19 Mar 2016 15:23:17 +0000
treeherdermozilla-central@720fb3d55e28 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1173320
milestone48.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 1173320 - patch 3/8 - Improve the Windows path management, r=smaug
dom/devicestorage/test/test_fs_createDirectory.html
dom/filesystem/Directory.cpp
dom/filesystem/Directory.h
dom/filesystem/FileSystemBase.cpp
dom/filesystem/FileSystemBase.h
dom/filesystem/FileSystemUtils.cpp
dom/filesystem/FileSystemUtils.h
dom/filesystem/GetDirectoryListingTask.cpp
dom/filesystem/OSFileSystem.cpp
--- a/dom/devicestorage/test/test_fs_createDirectory.html
+++ b/dom/devicestorage/test/test_fs_createDirectory.html
@@ -29,24 +29,24 @@ var gTestCount = 0;
 var gPath = '';
 var gName = '';
 
 function testCreateDirectory(rootDir, path) {
   rootDir.createDirectory(path).then(createDirectorySuccess, cbError);
 }
 
 function createDirectorySuccess(d) {
-  ok(d.name === gName, "Failed to create directory: name mismatch.");
+  is(d.name, gName, "Failed to create directory: name mismatch.");
 
   // Get the new created directory from the root.
   gRoot.get(gPath).then(getSuccess, cbError);
 }
 
 function getSuccess(d) {
-  ok(d.name === gName, "Should get directory - " + (gPath || "[root]") + ".");
+  is(d.name, gName, "Should get directory - " + (gPath || "[root]") + ".");
   switch (gTestCount) {
     case 0:
       gRoot = d;
       // Create a new directory under the root.
       gName = gPath = randomFilename(12);
       testCreateDirectory(gRoot, gName);
       break;
     case 1:
--- a/dom/filesystem/Directory.cpp
+++ b/dom/filesystem/Directory.cpp
@@ -30,16 +30,56 @@
 // by Directory#CreateFileW.
 #ifdef CreateFile
 #undef CreateFile
 #endif
 
 namespace mozilla {
 namespace dom {
 
+namespace {
+
+bool
+IsValidRelativeDOMPath(const nsString& aPath, nsTArray<nsString>& aParts)
+{
+  // We don't allow empty relative path to access the root.
+  if (aPath.IsEmpty()) {
+    return false;
+  }
+
+  // Leading and trailing "/" are not allowed.
+  if (aPath.First() == FILESYSTEM_DOM_PATH_SEPARATOR_CHAR ||
+      aPath.Last() == FILESYSTEM_DOM_PATH_SEPARATOR_CHAR) {
+    return false;
+  }
+
+  NS_NAMED_LITERAL_STRING(kCurrentDir, ".");
+  NS_NAMED_LITERAL_STRING(kParentDir, "..");
+
+  // Split path and check each path component.
+  nsCharSeparatedTokenizer tokenizer(aPath, FILESYSTEM_DOM_PATH_SEPARATOR_CHAR);
+  while (tokenizer.hasMoreTokens()) {
+    nsDependentSubstring pathComponent = tokenizer.nextToken();
+    // The path containing empty components, such as "foo//bar", is invalid.
+    // We don't allow paths, such as "../foo", "foo/./bar" and "foo/../bar",
+    // to walk up the directory.
+    if (pathComponent.IsEmpty() ||
+        pathComponent.Equals(kCurrentDir) ||
+        pathComponent.Equals(kParentDir)) {
+      return false;
+    }
+
+    aParts.AppendElement(pathComponent);
+  }
+
+  return true;
+}
+
+} // anonymous namespace
+
 NS_IMPL_CYCLE_COLLECTION_CLASS(Directory)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Directory)
   if (tmp->mFileSystem) {
     tmp->mFileSystem->Unlink();
     tmp->mFileSystem = nullptr;
   }
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWindow)
@@ -297,17 +337,17 @@ Directory::RemoveInternal(const StringOr
   FileSystemPermissionRequest::RequestForTask(task);
   return task->GetPromise();
 }
 
 void
 Directory::GetPath(nsAString& aRetval, ErrorResult& aRv)
 {
   if (mType == eDOMRootDirectory) {
-    aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
+    aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
   } else {
     // TODO: this should be a bit different...
     GetName(aRetval, aRv);
   }
 }
 
 nsresult
 Directory::GetFullRealPath(nsAString& aPath)
@@ -379,64 +419,32 @@ Directory::DOMPathToRealPath(const nsASt
 {
   nsString relativePath;
   relativePath = aPath;
 
   // Trim white spaces.
   static const char kWhitespace[] = "\b\t\r\n ";
   relativePath.Trim(kWhitespace);
 
-  if (!IsValidRelativePath(relativePath)) {
+  nsTArray<nsString> parts;
+  if (!IsValidRelativeDOMPath(relativePath, parts)) {
     return NS_ERROR_DOM_FILESYSTEM_INVALID_PATH_ERR;
   }
 
   nsCOMPtr<nsIFile> file;
   nsresult rv = mFile->Clone(getter_AddRefs(file));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
-  rv = file->AppendRelativePath(relativePath);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
+  for (uint32_t i = 0; i < parts.Length(); ++i) {
+    rv = file->AppendRelativePath(parts[i]);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
   }
 
   file.forget(aFile);
   return NS_OK;
 }
 
-// static
-bool
-Directory::IsValidRelativePath(const nsString& aPath)
-{
-  // We don't allow empty relative path to access the root.
-  if (aPath.IsEmpty()) {
-    return false;
-  }
-
-  // Leading and trailing "/" are not allowed.
-  if (aPath.First() == FileSystemUtils::kSeparatorChar ||
-      aPath.Last() == FileSystemUtils::kSeparatorChar) {
-    return false;
-  }
-
-  NS_NAMED_LITERAL_STRING(kCurrentDir, ".");
-  NS_NAMED_LITERAL_STRING(kParentDir, "..");
-
-  // Split path and check each path component.
-  nsCharSeparatedTokenizer tokenizer(aPath, FileSystemUtils::kSeparatorChar);
-  while (tokenizer.hasMoreTokens()) {
-    nsDependentSubstring pathComponent = tokenizer.nextToken();
-    // The path containing empty components, such as "foo//bar", is invalid.
-    // We don't allow paths, such as "../foo", "foo/./bar" and "foo/../bar",
-    // to walk up the directory.
-    if (pathComponent.IsEmpty() ||
-        pathComponent.Equals(kCurrentDir) ||
-        pathComponent.Equals(kParentDir)) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 } // namespace dom
 } // namespace mozilla
--- a/dom/filesystem/Directory.h
+++ b/dom/filesystem/Directory.h
@@ -142,19 +142,16 @@ public:
   GetFileSystem(ErrorResult& aRv);
 
 private:
   Directory(nsPIDOMWindowInner* aWindow,
             nsIFile* aFile, DirectoryType aType,
             FileSystemBase* aFileSystem = nullptr);
   ~Directory();
 
-  static bool
-  IsValidRelativePath(const nsString& aPath);
-
   /*
    * Convert relative DOM path to the absolute real path.
    */
   nsresult
   DOMPathToRealPath(const nsAString& aPath, nsIFile** aFile) const;
 
   already_AddRefed<Promise>
   RemoveInternal(const StringOrFileOrDirectory& aPath, bool aRecursive,
--- a/dom/filesystem/FileSystemBase.cpp
+++ b/dom/filesystem/FileSystemBase.cpp
@@ -59,49 +59,16 @@ FileSystemBase::Shutdown()
 }
 
 nsPIDOMWindowInner*
 FileSystemBase::GetWindow() const
 {
   return nullptr;
 }
 
-already_AddRefed<nsIFile>
-FileSystemBase::GetLocalFile(const nsAString& aRealPath) const
-{
-  MOZ_ASSERT(XRE_IsParentProcess(),
-             "Should be on parent process!");
-
-  // Let's convert the input path to /path
-  nsAutoString localPath;
-  if (!aRealPath.IsEmpty() &&
-      !StringBeginsWith(aRealPath,
-                        NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR))) {
-    localPath.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
-  }
-  localPath.Append(aRealPath);
-
-  // We have to normalize the path string in order to follow the separator
-  // schema of this OS.
-  nsAutoString normalizedPath;
-  FileSystemUtils::NormalizedPathToLocalPath(localPath, normalizedPath);
-
-  // The full path is mLocalRootPath + normalizedPath.
-  nsAutoString path(mLocalRootPath);
-  path.Append(normalizedPath);
-
-  nsCOMPtr<nsIFile> file;
-  nsresult rv = NS_NewLocalFile(path, false, getter_AddRefs(file));
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return nullptr;
-  }
-
-  return file.forget();
-}
-
 bool
 FileSystemBase::GetRealPath(BlobImpl* aFile, nsIFile** aPath) const
 {
   MOZ_ASSERT(XRE_IsParentProcess(),
              "Should be on parent process!");
   MOZ_ASSERT(aFile, "aFile Should not be null.");
 
   nsAutoString filePath;
--- a/dom/filesystem/FileSystemBase.h
+++ b/dom/filesystem/FileSystemBase.h
@@ -34,22 +34,16 @@ public:
 
   // SerializeDOMPath the FileSystem to string.
   virtual void
   SerializeDOMPath(nsAString& aOutput) const = 0;
 
   virtual nsPIDOMWindowInner*
   GetWindow() const;
 
-  /**
-   * Create nsIFile object from the given real path (absolute DOM path).
-   */
-  already_AddRefed<nsIFile>
-  GetLocalFile(const nsAString& aRealPath) const;
-
   /*
    * Get the virtual name of the root directory. This name will be exposed to
    * the content page.
    */
   virtual void
   GetRootName(nsAString& aRetval) const = 0;
 
   const nsAString&
--- a/dom/filesystem/FileSystemUtils.cpp
+++ b/dom/filesystem/FileSystemUtils.cpp
@@ -49,17 +49,17 @@ FileSystemUtils::NormalizedPathToLocalPa
 
 // static
 bool
 FileSystemUtils::IsDescendantPath(const nsAString& aPath,
                                   const nsAString& aDescendantPath)
 {
   // The descendant path should begin with its ancestor path.
   nsAutoString prefix;
-  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR);
+  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
 
   // Check the sub-directory path to see if it has the parent path as prefix.
   if (aDescendantPath.Length() < prefix.Length() ||
       !StringBeginsWith(aDescendantPath, prefix)) {
     return false;
   }
 
   return true;
--- a/dom/filesystem/FileSystemUtils.h
+++ b/dom/filesystem/FileSystemUtils.h
@@ -7,17 +7,18 @@
 #ifndef mozilla_dom_FileSystemUtils_h
 #define mozilla_dom_FileSystemUtils_h
 
 #include "nsString.h"
 
 namespace mozilla {
 namespace dom {
 
-#define FILESYSTEM_DOM_PATH_SEPARATOR "/"
+#define FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL "/"
+#define FILESYSTEM_DOM_PATH_SEPARATOR_CHAR '/'
 
 /*
  * This class is for error handling.
  * All methods in this class are static.
  */
 class FileSystemUtils
 {
 public:
@@ -41,16 +42,14 @@ public:
   IsDescendantPath(nsIFile* aPath, nsIFile* aDescendantPath);
 
   /*
    * Return true if aDescendantPath is a descendant of aPath. Both aPath and
    * aDescendantPath are absolute DOM path.
    */
   static bool
   IsDescendantPath(const nsAString& aPath, const nsAString& aDescendantPath);
-
-  static const char16_t kSeparatorChar = char16_t('/');
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_FileSystemUtils_h
--- a/dom/filesystem/GetDirectoryListingTask.cpp
+++ b/dom/filesystem/GetDirectoryListingTask.cpp
@@ -338,61 +338,39 @@ GetDirectoryListingTask::HandlerCallback
   if (!listing.SetLength(count, mozilla::fallible_t())) {
     mPromise->MaybeReject(NS_ERROR_FAILURE);
     mPromise = nullptr;
     return;
   }
 
   for (unsigned i = 0; i < count; i++) {
     if (mTargetData[i].mType == Directory::BlobImplOrDirectoryPath::eDirectoryPath) {
-#ifdef DEBUG
-      if (XRE_IsParentProcess()) {
-        nsCOMPtr<nsIFile> file;
-        nsresult rv = NS_NewLocalFile(mTargetData[i].mDirectoryPath, false,
-                                      getter_AddRefs(file));
-        if (NS_WARN_IF(NS_FAILED(rv))) {
-          mPromise->MaybeReject(rv);
-          mPromise = nullptr;
-          return;
-        }
-
-        bool exist;
-        rv = file->Exists(&exist);
-        if (NS_WARN_IF(NS_FAILED(rv))) {
-          mPromise->MaybeReject(rv);
-          mPromise = nullptr;
-          return;
-        }
-
-        // We cannot assert here because the file can be done in the meantime.
-        NS_ASSERTION(exist, "The file doesn't exist anymore?!?");
-
-        nsAutoString normalizedLocalRootPath;
-        FileSystemUtils::NormalizedPathToLocalPath(mFileSystem->GetLocalRootPath(),
-                                                   normalizedLocalRootPath);
-
-        nsAutoString directoryPath;
-        FileSystemUtils::NormalizedPathToLocalPath(mTargetData[i].mDirectoryPath,
-                                                   directoryPath);
-
-        MOZ_ASSERT(FileSystemUtils::IsDescendantPath(normalizedLocalRootPath,
-                                                     directoryPath));
-      }
-#endif
-
       nsCOMPtr<nsIFile> directoryPath;
       NS_ConvertUTF16toUTF8 path(mTargetData[i].mDirectoryPath);
       nsresult rv = NS_NewNativeLocalFile(path, true,
                                           getter_AddRefs(directoryPath));
       if (NS_WARN_IF(NS_FAILED(rv))) {
         mPromise->MaybeReject(rv);
         mPromise = nullptr;
         return;
       }
 
+#ifdef DEBUG
+      nsCOMPtr<nsIFile> rootPath;
+      rv = NS_NewLocalFile(mFileSystem->GetLocalRootPath(), false,
+                           getter_AddRefs(rootPath));
+      if (NS_WARN_IF(NS_FAILED(rv))) {
+        mPromise->MaybeReject(rv);
+        mPromise = nullptr;
+        return;
+      }
+
+      MOZ_ASSERT(FileSystemUtils::IsDescendantPath(rootPath, directoryPath));
+#endif
+
       RefPtr<Directory> directory = Directory::Create(mFileSystem->GetWindow(),
                                                       directoryPath,
                                                       Directory::eNotDOMRootDirectory,
                                                       mFileSystem);
       MOZ_ASSERT(directory);
 
       // Propogate mFilter onto sub-Directory object:
       directory->SetContentFilters(mFilters);
--- a/dom/filesystem/OSFileSystem.cpp
+++ b/dom/filesystem/OSFileSystem.cpp
@@ -46,17 +46,17 @@ OSFileSystem::GetWindow() const
 {
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
   return mWindow;
 }
 
 void
 OSFileSystem::GetRootName(nsAString& aRetval) const
 {
-  return aRetval.AssignLiteral("/");
+  aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
 }
 
 bool
 OSFileSystem::IsSafeFile(nsIFile* aFile) const
 {
   // The concept of "safe files" is specific to the Device Storage API where
   // files are only "safe" if they're of a type that is appropriate for the
   // area of device storage that is being used.