Add PathService::RemoveOverride to clear path overrides.

This is especially useful for unit tests because they live in the same process
and share all the singletons including PathService. One can of course override
the overrides but it is much cleaner and sometimes the only good solution to
return to the original PathProvider instead.

BUG=149161
TEST=base_unittests:PathServiceTest.*


Review URL: https://chromiumcodereview.appspot.com/10909228

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

3 files changed, 112 insertions, 46 deletions

diff --git a/base/path_service.cc b/base/path_service.cc
index 7a696e9..2697653 100644
--- a/base/path_service.cc
+++ b/base/path_service.cc
@@ -142,14 +142,8 @@ static PathData* GetPathData() {
   return g_path_data.Pointer();
 }
 
-}  // namespace
-
-
-// static
-bool PathService::GetFromCache(int key, FilePath* result) {
-  PathData* path_data = GetPathData();
-  base::AutoLock scoped_lock(path_data->lock);
-
+// Tries to find |key| in the cache. |path_data| should be locked by the caller!
+bool LockedGetFromCache(int key, const PathData* path_data, FilePath* result) {
   // check for a cached version
   PathMap::const_iterator it = path_data->cache.find(key);
   if (it != path_data->cache.end()) {
@@ -159,27 +153,20 @@ bool PathService::GetFromCache(int key, FilePath* result) {
   return false;
 }
 
-// static
-bool PathService::GetFromOverrides(int key, FilePath* result) {
-  PathData* path_data = GetPathData();
-  base::AutoLock scoped_lock(path_data->lock);
-
+// Tries to find |key| in the overrides map. |path_data| should be locked by the
+// caller!
+bool LockedGetFromOverrides(int key, PathData* path_data, FilePath* result) {
   // check for an overridden version.
   PathMap::const_iterator it = path_data->overrides.find(key);
   if (it != path_data->overrides.end()) {
+    path_data->cache[key] = it->second;
     *result = it->second;
     return true;
   }
   return false;
 }
 
-// static
-void PathService::AddToCache(int key, const FilePath& path) {
-  PathData* path_data = GetPathData();
-  base::AutoLock scoped_lock(path_data->lock);
-  // Save the computed path in our cache.
-  path_data->cache[key] = path;
-}
+}  // namespace
 
 // TODO(brettw): this function does not handle long paths (filename > MAX_PATH)
 // characters). This isn't supported very well by Windows right now, so it is
@@ -195,18 +182,23 @@ bool PathService::Get(int key, FilePath* result) {
   if (key == base::DIR_CURRENT)
     return file_util::GetCurrentDirectory(result);
 
-  if (GetFromCache(key, result))
-    return true;
+  Provider* provider = NULL;
+  {
+    base::AutoLock scoped_lock(path_data->lock);
+    if (LockedGetFromCache(key, path_data, result))
+      return true;
 
-  if (GetFromOverrides(key, result))
-    return true;
+    if (LockedGetFromOverrides(key, path_data, result))
+      return true;
+
+    // Get the beginning of the list while it is still locked.
+    provider = path_data->providers;
+  }
 
   FilePath path;
 
-  // search providers for the requested path
-  // NOTE: it should be safe to iterate here without the lock
-  // since RegisterProvider always prepends.
-  Provider* provider = path_data->providers;
+  // Iterating does not need the lock because only the list head might be
+  // modified on another thread.
   while (provider) {
     if (provider->func(key, &path))
       break;
@@ -217,17 +209,21 @@ bool PathService::Get(int key, FilePath* result) {
   if (path.empty())
     return false;
 
-  AddToCache(key, path);
-
   *result = path;
+
+  base::AutoLock scoped_lock(path_data->lock);
+  path_data->cache[key] = path;
+
   return true;
 }
 
+// static
 bool PathService::Override(int key, const FilePath& path) {
   // Just call the full function with true for the value of |create|.
   return OverrideAndCreateIfNeeded(key, path, true);
 }
 
+// static
 bool PathService::OverrideAndCreateIfNeeded(int key,
                                             const FilePath& path,
                                             bool create) {
@@ -260,38 +256,58 @@ bool PathService::OverrideAndCreateIfNeeded(int key,
   // on the value we are overriding, and are now out of sync with reality.
   path_data->cache.clear();
 
-  path_data->cache[key] = file_path;
   path_data->overrides[key] = file_path;
 
   return true;
 }
 
+// static
+bool PathService::RemoveOverride(int key) {
+  PathData* path_data = GetPathData();
+  DCHECK(path_data);
+
+  base::AutoLock scoped_lock(path_data->lock);
+
+  if (path_data->overrides.find(key) == path_data->overrides.end())
+    return false;
+
+  // Clear the cache now. Some of its entries could have depended on the value
+  // we are going to remove, and are now out of sync.
+  path_data->cache.clear();
+
+  path_data->overrides.erase(key);
+
+  return true;
+}
+
+// static
 void PathService::RegisterProvider(ProviderFunc func, int key_start,
                                    int key_end) {
   PathData* path_data = GetPathData();
   DCHECK(path_data);
   DCHECK_GT(key_end, key_start);
 
-  base::AutoLock scoped_lock(path_data->lock);
-
   Provider* p;
 
-#ifndef NDEBUG
-  p = path_data->providers;
-  while (p) {
-    DCHECK(key_start >= p->key_end || key_end <= p->key_start) <<
-      "path provider collision";
-    p = p->next;
-  }
-#endif
-
   p = new Provider;
   p->is_static = false;
   p->func = func;
-  p->next = path_data->providers;
 #ifndef NDEBUG
   p->key_start = key_start;
   p->key_end = key_end;
 #endif
+
+  base::AutoLock scoped_lock(path_data->lock);
+
+#ifndef NDEBUG
+  Provider *iter = path_data->providers;
+  while (iter) {
+    DCHECK(key_start >= iter->key_end || key_end <= iter->key_start) <<
+      "path provider collision";
+    iter = iter->next;
+  }
+#endif
+
+  p->next = path_data->providers;
   path_data->providers = p;
 }
diff --git a/base/path_service.h b/base/path_service.h
index e835a1f..40da55a 100644
--- a/base/path_service.h
+++ b/base/path_service.h
@@ -9,6 +9,7 @@
 
 #include "base/base_export.h"
 #include "base/base_paths.h"
+#include "base/gtest_prod_util.h"
 #include "build/build_config.h"
 
 class FilePath;
@@ -61,10 +62,14 @@ class BASE_EXPORT PathService {
   static void RegisterProvider(ProviderFunc provider,
                                int key_start,
                                int key_end);
+
  private:
-  static bool GetFromCache(int key, FilePath* path);
-  static bool GetFromOverrides(int key, FilePath* path);
-  static void AddToCache(int key, const FilePath& path);
+  FRIEND_TEST_ALL_PREFIXES(PathServiceTest, RemoveOverride);
+
+  // Removes an override for a special directory or file. Returns true if there
+  // was an override to remove or false if none was present.
+  // NOTE: This function is intended to be used by tests only!
+  static bool RemoveOverride(int key);
 };
 
 #endif  // BASE_PATH_SERVICE_H_
diff --git a/base/path_service_unittest.cc b/base/path_service_unittest.cc
index e4bda0c..d3110f9 100644
--- a/base/path_service_unittest.cc
+++ b/base/path_service_unittest.cc
@@ -149,3 +149,48 @@ TEST_F(PathServiceTest, Override) {
                                                      true));
   EXPECT_TRUE(file_util::PathExists(fake_cache_dir2));
 }
+
+// Check if multiple overrides can co-exist.
+TEST_F(PathServiceTest, OverrideMultiple) {
+  int my_special_key = 666;
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FilePath fake_cache_dir1(temp_dir.path().AppendASCII("1"));
+  EXPECT_TRUE(PathService::Override(my_special_key, fake_cache_dir1));
+  EXPECT_TRUE(file_util::PathExists(fake_cache_dir1));
+  ASSERT_EQ(1, file_util::WriteFile(fake_cache_dir1.AppendASCII("t1"), ".", 1));
+
+  FilePath fake_cache_dir2(temp_dir.path().AppendASCII("2"));
+  EXPECT_TRUE(PathService::Override(my_special_key + 1, fake_cache_dir2));
+  EXPECT_TRUE(file_util::PathExists(fake_cache_dir2));
+  ASSERT_EQ(1, file_util::WriteFile(fake_cache_dir2.AppendASCII("t2"), ".", 1));
+
+  FilePath result;
+  EXPECT_TRUE(PathService::Get(my_special_key, &result));
+  // Override might have changed the path representation but our test file
+  // should be still there.
+  EXPECT_TRUE(file_util::PathExists(result.AppendASCII("t1")));
+  EXPECT_TRUE(PathService::Get(my_special_key + 1, &result));
+  EXPECT_TRUE(file_util::PathExists(result.AppendASCII("t2")));
+}
+
+TEST_F(PathServiceTest, RemoveOverride) {
+  // Before we start the test we have to call RemoveOverride at least once to
+  // clear any overrides that might have been left from other tests.
+  PathService::RemoveOverride(base::DIR_TEMP);
+
+  FilePath original_user_data_dir;
+  EXPECT_TRUE(PathService::Get(base::DIR_TEMP, &original_user_data_dir));
+  EXPECT_FALSE(PathService::RemoveOverride(base::DIR_TEMP));
+
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  EXPECT_TRUE(PathService::Override(base::DIR_TEMP, temp_dir.path()));
+  FilePath new_user_data_dir;
+  EXPECT_TRUE(PathService::Get(base::DIR_TEMP, &new_user_data_dir));
+  EXPECT_NE(original_user_data_dir, new_user_data_dir);
+
+  EXPECT_TRUE(PathService::RemoveOverride(base::DIR_TEMP));
+  EXPECT_TRUE(PathService::Get(base::DIR_TEMP, &new_user_data_dir));
+  EXPECT_EQ(original_user_data_dir, new_user_data_dir);
+}