Get rid of FilePath::AppendAndResolveRelative().

To resolve the problem of '..' parent references as well as symbolic links on POSIX platforms, we can simply use the file_util::AbsolutePath() function.

This has the drawback of having a different behavior on Windows and POSIX platforms, in the way that it can return a canonical path that doesn't exists when ran on Windows, but it will return an empty path (or false) when run on a POSIX platform.

So we need to add an extra PathExists() call to unify the behavior.

BUG=25681,25131 

Review URL: http://codereview.chromium.org/343003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30334 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 34 insertions, 190 deletions

diff --git a/base/file_path.cc b/base/file_path.cc
index 2d9d26e..dde8ae1 100644
--- a/base/file_path.cc
+++ b/base/file_path.cc
@@ -171,75 +171,6 @@ bool FilePath::operator!=(const FilePath& that) const {
 #endif  // defined(FILE_PATH_USES_DRIVE_LETTERS)
 }
 
-bool FilePath::AppendAndResolveRelative(const FilePath& relative_path,
-                                        FilePath* path) const {
-  DCHECK(path);
-  if (!path || relative_path.IsAbsolute())
-    return false;
-
-  FilePath full_path = Append(relative_path);
-  // Is it worth looking for parent references?
-  if (!full_path.ReferencesParent()) {
-    *path = full_path;
-    return true;
-  }
-
-  // If the parent has a drive letter, then we must not remove the first
-  // component, which is the drive letter.
-  bool drive_letter = (FindDriveLetter(full_path.path_) !=
-                       FilePath::StringType::npos);
-
-  std::vector<FilePath::StringType> components;
-  full_path.GetComponents(&components);
-  std::vector<FilePath::StringType>::iterator it = components.begin();
-  // Start by removing any kCurrentDirectory component, since they may
-  // fool us into not going back to the appropriate parent level.
-  for (; it != components.end(); ++it) {
-    if (*it == kCurrentDirectory) {
-      // erase returns an iterator to the next component.
-      it = components.erase(it);
-      // So now, go back to previous iterator,
-      // so that we can appropriately process the next one as we loop.
-      --it;
-    }
-  }
-
-  // Now parse the component looking for kParentDirectory and remove them as
-  // well as the previous component.
-  it = components.begin();
-  for (; it != components.end(); ++it) {
-    if (*it == kParentDirectory) {
-      // Did we reach the beginning?
-      if (it == components.begin() ||
-          (drive_letter && (it - 1) == components.begin())) {
-        return false;
-      }
-      // Remove the previous component, as well as the current one.
-      std::vector<FilePath::StringType>::iterator previous = it - 1;
-      // Unless the previous is at the beginning.
-      if (previous == components.begin() ||
-          (drive_letter && (previous - 1) == components.begin())) {
-        return false;
-      }
-      // vector::erase doesn't erase _Last, it erases [_First, _Last[,
-      // so we must increment current which we want erased.
-      it = components.erase(previous, it + 1);
-      // And go back to previous so that we can process the next one as we loop.
-      --it;
-    }
-  }
-
-  // Now reconstruct the path with the components that were left in.
-  it = components.begin();
-  // We start with the first component, in case it is absolute
-  // and absolute paths can't be appended.
-  *path = FilePath(*it);
-  for (++it; it != components.end(); ++it)
-    *path = path->Append(*it);
-
-  return true;
-}
-
 bool FilePath::IsParent(const FilePath& child) const {
   return AppendRelativePath(child, NULL);
 }
diff --git a/base/file_path.h b/base/file_path.h
index 7900922..0d0bdad 100644
--- a/base/file_path.h
+++ b/base/file_path.h
@@ -178,18 +178,6 @@ class FilePath {
   // and BaseName().value() on each child component.
   void GetComponents(std::vector<FilePath::StringType>* components) const;
 
-  // Returns true, and sets *path to resulting full path, if relative_path can
-  // be applied to current path by resolving any '..' it may contain. Returns
-  // false otherwise, e.g., if relative path is absolute, or if it climbs back
-  // up the hierarchy too far (i.e., beyond the root of current path).
-  //
-  // Note that if the current path ends with a file name, we won't try to
-  // figure it out (so this method doesn't go to the disk) and we will blindly
-  // append relative_path at the end of the current path, including the file
-  // name in the current path (if any).
-  bool AppendAndResolveRelative(const FilePath& relative_path,
-                                FilePath* path) const;
-
   // Returns true if this FilePath is a strict parent of the |child|. Absolute
   // and relative paths are accepted i.e. is /foo parent to /foo/bar and
   // is foo parent to foo/bar. Does not convert paths to absolute, follow
diff --git a/base/file_path_unittest.cc b/base/file_path_unittest.cc
index 2673220..92659f1 100644
--- a/base/file_path_unittest.cc
+++ b/base/file_path_unittest.cc
@@ -502,98 +502,6 @@ TEST_F(FilePathTest, PathComponentsTest) {
   }
 }
 
-TEST_F(FilePathTest, AppendAndResolveRelativeTest) {
-  const struct BinaryTestData cases[] = {
-#if defined(FILE_PATH_USES_DRIVE_LETTERS)
-    { { FPL("c:/"),              FPL("foo") },        FPL("c:/foo") },
-    { { FPL("f:/foo/bar"),       FPL("..") },         FPL("f:/foo") },
-    { { FPL("f:/foo.bar"),       FPL("..") },         FPL("f:/") },
-    { { FPL("F:/foo/.."),        FPL("./bar/.") },    FPL("F:/bar") },
-    { { FPL("E:/Foo/bar"),       FPL("../..") },      FPL("E:/") },
-    { { FPL("E:/Foo/bar/."),     FPL("../..") },      FPL("E:/") },
-    { { FPL("e:/foo/.."),        FPL("bar/..") },     FPL("e:/") },
-    { { FPL("c:/foo/./bar/.."),  FPL("../baz") },     FPL("c:/baz") },
-    { { FPL("E:/./foo/bar/.."),  FPL("../baz/..") },  FPL("E:/") },
-    { { FPL("x:/foo/../bar/.."), FPL("baz/../boo") }, FPL("x:/boo") },
-    { { FPL("E:/foo.bar/.."),    FPL("../baz/..") },  FPL("") },
-    { { FPL("Z:/foo"),           FPL("../..") },      FPL("") },
-    { { FPL("y:/"),              FPL("..") },         FPL("") },
-    { { FPL("B:/.."),            FPL("bar/.") },      FPL("") },
-    { { FPL("a:/foo/.."),        FPL("..") },         FPL("") },
-    { { FPL("r:/.."),            FPL("..") },         FPL("") },
-    { { FPL("F:/foo/.."),        FPL("../..") },      FPL("") },
-    { { FPL("O:/foo/bar/.."),    FPL("../..") },      FPL("") },
-#endif  // FILE_PATH_USES_DRIVE_LETTERS
-#if defined(FILE_PATH_USES_WIN_SEPARATORS)
-    { { FPL("\\\\"),                FPL("foo") },         FPL("\\\\foo") },
-    { { FPL("\\\\foo"),             FPL("bar") },         FPL("\\\\foo\\bar") },
-    { { FPL("\\\\foo\\bar"),        FPL("..") },          FPL("\\\\foo") },
-    { { FPL("\\\\foo.bar"),         FPL("..") },          FPL("\\\\") },
-    { { FPL("\\\\Foo\\bar"),        FPL("..\\..") },      FPL("\\\\") },
-    { { FPL("\\\\Foo\\bar\\."),     FPL("..\\..") },      FPL("\\\\") },
-    { { FPL("\\\\foo\\bar"), FPL("foo\\..\\baz") }, FPL("\\\\foo\\bar\\baz") },
-    { { FPL("\\\\foo\\.\\bar"),     FPL("..\\baz\\.") },  FPL("\\\\foo\\baz") },
-    { { FPL("\\\\.\\foo\\.."),      FPL("bar") },         FPL("\\\\bar") },
-    { { FPL("\\\\foo\\.."),         FPL(".\\bar\\..") },  FPL("\\\\") },
-    { { FPL("\\\\foo\\bar\\.."),    FPL("..\\baz") },     FPL("\\\\baz") },
-    { { FPL("\\\\foo\\bar\\.."),    FPL("..\\baz\\..") }, FPL("\\\\") },
-    { { FPL("\\\\foo\\..\\bar\\.."), FPL("baz\\..\\boo") }, FPL("\\\\boo"), },
-    { { FPL("\\\\foo.bar\\.."),     FPL("..\\baz\\..") }, FPL("") },
-    { { FPL("\\\\foo"),             FPL("..\\..") },      FPL("") },
-    { { FPL("\\\\"),                FPL("..") },          FPL("") },
-    { { FPL("\\\\.."),              FPL("bar\\.") },      FPL("") },
-    { { FPL("\\\\foo\\.."),         FPL("..") },          FPL("") },
-    { { FPL("\\\\.."),              FPL("..") },          FPL("") },
-    { { FPL("\\\\foo\\.."),         FPL("..\\..") },      FPL("") },
-    { { FPL("\\\\foo\\bar\\.."),    FPL("..\\..") },      FPL("") },
-#if defined(FILE_PATH_USES_DRIVE_LETTERS)
-    { { FPL("E:/foo"),           FPL("bar") },        FPL("E:/foo\\bar") },
-    { { FPL("C:/foo/bar"),       FPL("foo/../baz") }, FPL("C:/foo\\bar\\baz") },
-    { { FPL("e:/foo/bar"),       FPL("../baz") },     FPL("e:/foo\\baz") },
-#endif
-#else  // FILE_PATH_USES_WIN_SEPARAORS
-    { { FPL("/"),              FPL("foo") },        FPL("/foo") },
-    { { FPL("/foo"),           FPL("bar") },        FPL("/foo/bar") },
-    { { FPL("/foo/bar/"),      FPL("..") },         FPL("/foo") },
-    { { FPL("/foo.bar"),       FPL("..") },         FPL("/") },
-    { { FPL("//foo"),          FPL("..") },         FPL("//") },
-    { { FPL("/foo/./bar"),     FPL("../..") },      FPL("/") },
-    { { FPL("/foo/bar/."),     FPL("foo/../baz") }, FPL("/foo/bar/baz") },
-    { { FPL("/./foo/bar"),     FPL("../baz/.") },   FPL("/foo/baz") },
-    { { FPL("/foo/.."),        FPL("./bar") },      FPL("/bar") },
-    { { FPL("/foo/.."),        FPL("bar/..") },     FPL("/") },
-    { { FPL("//foo/bar/.."),   FPL("../baz") },     FPL("//baz") },
-    { { FPL("/foo/bar/.."),    FPL("../baz/..") },  FPL("/") },
-    { { FPL("/foo/../bar/.."), FPL("baz/../boo") }, FPL("/boo") },
-    { { FPL("//foo.bar/.."),   FPL("../baz") },     FPL("") },
-    { { FPL("/foo"),           FPL("../..") },      FPL("") },
-    { { FPL("//"),             FPL("..") },         FPL("") },
-    { { FPL("/.."),            FPL("./bar") },      FPL("") },
-    { { FPL("/foo/.."),        FPL("..") },         FPL("") },
-    { { FPL("/.."),            FPL("..") },         FPL("") },
-    { { FPL("/foo/.."),        FPL("../..") },      FPL("") },
-    { { FPL("/foo/bar/.."),    FPL("../..") },      FPL("") },
-#if defined(FILE_PATH_USES_DRIVE_LETTERS)
-    { { FPL("E:/foo"),           FPL("bar") },        FPL("E:/foo/bar") },
-    { { FPL("C:/foo/bar"),       FPL("foo/../baz") }, FPL("C:/foo/bar/baz") },
-    { { FPL("e:/foo/bar"),       FPL("../baz") },     FPL("e:/foo/baz") },
-#endif
-#endif  // FILE_PATH_USES_WIN_SEPARAORS
-  };
-
-  for (size_t i = 0; i < arraysize(cases); ++i) {
-    FilePath parent(cases[i].inputs[0]);
-    FilePath child(cases[i].inputs[1]);
-
-    FilePath result;
-    EXPECT_EQ(cases[i].expected[0] != '\0',
-              parent.AppendAndResolveRelative(child, &result)) <<
-        "i: " << i << ", parent: " << parent.value() << ", child: " <<
-        child.value();
-    EXPECT_STREQ(cases[i].expected, result.value().c_str());
-  }
-}
-
 TEST_F(FilePathTest, IsParentTest) {
   const struct BinaryBooleanTestData cases[] = {
     { { FPL("/"),             FPL("/foo/bar/baz") },      true},
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 45cad68..e6ed987 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -542,19 +542,23 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   EXPECT_EQ(2u, scripts[0].js_scripts().size());
   ExtensionResource resource00(scripts[0].js_scripts()[0].extension_root(),
                                scripts[0].js_scripts()[0].relative_path());
-  EXPECT_TRUE(resource00.ComparePathWithDefault(
-      extension->path().AppendASCII("script1.js")));
+  FilePath expected_path(extension->path().AppendASCII("script1.js"));
+  ASSERT_TRUE(file_util::AbsolutePath(&expected_path));
+  EXPECT_TRUE(resource00.ComparePathWithDefault(expected_path));
   ExtensionResource resource01(scripts[0].js_scripts()[1].extension_root(),
                                scripts[0].js_scripts()[1].relative_path());
-  EXPECT_TRUE(resource01.ComparePathWithDefault(
-      extension->path().AppendASCII("script2.js")));
+  expected_path = extension->path().AppendASCII("script2.js");
+  ASSERT_TRUE(file_util::AbsolutePath(&expected_path));
+  EXPECT_TRUE(resource01.ComparePathWithDefault(expected_path));
   EXPECT_TRUE(extension->plugins().empty());
   EXPECT_EQ(1u, scripts[1].url_patterns().size());
   EXPECT_EQ("http://*.news.com/*", scripts[1].url_patterns()[0].GetAsString());
   ExtensionResource resource10(scripts[1].js_scripts()[0].extension_root(),
                                scripts[1].js_scripts()[0].relative_path());
-  EXPECT_TRUE(resource10.ComparePathWithDefault(
-      extension->path().AppendASCII("js_files").AppendASCII("script3.js")));
+  expected_path =
+      extension->path().AppendASCII("js_files").AppendASCII("script3.js");
+  ASSERT_TRUE(file_util::AbsolutePath(&expected_path));
+  EXPECT_TRUE(resource10.ComparePathWithDefault(expected_path));
   const std::vector<URLPattern> permissions = extension->host_permissions();
   ASSERT_EQ(2u, permissions.size());
   EXPECT_EQ("http://*.google.com/*", permissions[0].GetAsString());
diff --git a/chrome/common/extensions/extension_resource.cc b/chrome/common/extensions/extension_resource.cc
index 7b2a833..6c1cff6 100644
--- a/chrome/common/extensions/extension_resource.cc
+++ b/chrome/common/extensions/extension_resource.cc
@@ -41,21 +41,35 @@ FilePath ExtensionResource::GetFilePath(const FilePath& extension_root,
   extension_l10n_util::GetL10nRelativePaths(relative_path,
                                             &l10n_relative_paths);
 
+  // We need to resolve the parent references in the extension_root
+  // path on its own because IsParent doesn't like parent references.
+  FilePath clean_extension_root(extension_root);
+  if (!file_util::AbsolutePath(&clean_extension_root))
+    return FilePath();
+
   // Stat l10n file(s), and return new path if it exists.
   for (size_t i = 0; i < l10n_relative_paths.size(); ++i) {
-    FilePath full_path;
-    if (extension_root.AppendAndResolveRelative(l10n_relative_paths[i],
-                                                &full_path) &&
-        extension_root.IsParent(full_path) &&
+    FilePath full_path = clean_extension_root.Append(l10n_relative_paths[i]);
+    if (file_util::AbsolutePath(&full_path) &&
+        clean_extension_root.IsParent(full_path) &&
         file_util::PathExists(full_path)) {
       return full_path;
     }
   }
 
   // Fall back to root resource.
-  FilePath full_path;
-  if (extension_root.AppendAndResolveRelative(relative_path, &full_path) &&
-      extension_root.IsParent(full_path)) {
+  FilePath full_path = clean_extension_root.Append(relative_path);
+
+  // We must resolve the absolute path of the combined path when
+  // the relative path contains references to a parent folder (i.e., '..').
+  // We also check if the path exists because the posix version of AbsolutePath
+  // will fail if the path doesn't exist, and we want the same behavior on
+  // Windows... So until the posix and Windows version of AbsolutePath are
+  // unified, we need an extra call to PathExists, unfortunately.
+  // TODO(mad): Fix this once AbsolutePath is unified.
+  if (file_util::AbsolutePath(&full_path) &&
+      file_util::PathExists(full_path) &&
+      clean_extension_root.IsParent(full_path)) {
     return full_path;
   }
 
diff --git a/chrome/common/extensions/extension_resource_unittest.cc b/chrome/common/extensions/extension_resource_unittest.cc
index f467e47..807452e 100644
--- a/chrome/common/extensions/extension_resource_unittest.cc
+++ b/chrome/common/extensions/extension_resource_unittest.cc
@@ -35,12 +35,10 @@ TEST(ExtensionResourceTest, CreateWithMissingResourceOnDisk) {
   relative_path = relative_path.AppendASCII("cira.js");
   ExtensionResource resource(root_path, relative_path);
 
+  // The path doesn't exist on disk, we will be returned an empty path.
   EXPECT_EQ(root_path.value(), resource.extension_root().value());
   EXPECT_EQ(relative_path.value(), resource.relative_path().value());
-  EXPECT_EQ(ToLower(root_path.Append(relative_path).value()),
-    ToLower(resource.GetFilePath().value()));
-
-  EXPECT_FALSE(resource.GetFilePath().empty());
+  EXPECT_TRUE(resource.GetFilePath().empty());
 }
 
 TEST(ExtensionResourceTest, CreateWithAllResourcesOnDisk) {
@@ -76,6 +74,7 @@ TEST(ExtensionResourceTest, CreateWithAllResourcesOnDisk) {
   ASSERT_FALSE(locales.empty());
   FilePath expected_path;
   expected_path = l10n_path.AppendASCII(locales[0]).AppendASCII(filename);
+  ASSERT_TRUE(file_util::AbsolutePath(&expected_path));
 
   EXPECT_EQ(ToLower(expected_path.value()), ToLower(resolved_path.value()));
   EXPECT_EQ(ToLower(temp.path().value()),