Add PathService::RemoveOverride to clear path overrides.

base/path_service.cc

Index: base/path_service.cc
diff --git a/base/path_service.cc b/base/path_service.cc
index a3b882c2a9e8f76f668ed71b8549706059aecf51..8b59cb78ca4e8aeb79ec1855629b457ac06ec8b9 100644
--- a/base/path_service.cc
+++ b/base/path_service.cc
@@ -140,14 +140,14 @@ 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);
+// Adds a value to the cache map. |path_data| should be locked by the caller!
+void LockedAddToCache(int key, const FilePath& path, PathData* path_data) {
+  // Save the computed path in our cache.
+  path_data->cache[key] = path;

[jar (doing other things), 2012/09/18 16:47:17]

This line of code (which is the whole function), seems small enough and clear
enough that it should be inlined explicitly (no need for a big comment about it,
etc.).

[pastarmovj, 2012/09/19 11:24:28]

Done.

+}
 
+// 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()) {
@@ -157,27 +157,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()) {
+    LockedAddToCache(key, it->second, path_data);
     *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
@@ -189,22 +182,28 @@ bool PathService::Get(int key, FilePath* result) {
   DCHECK(result);
   DCHECK_GE(key, base::DIR_CURRENT);
 
+  Provider* provider = NULL;
   // special case the current directory because it can never be cached
   if (key == base::DIR_CURRENT)
     return file_util::GetCurrentDirectory(result);
 
-  if (GetFromCache(key, result))
-    return true;
+  {
+    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;
   while (provider) {
     if (provider->func(key, &path))
       break;
@@ -215,17 +214,22 @@ bool PathService::Get(int key, FilePath* result) {
   if (path.empty())
     return false;
 
-  AddToCache(key, path);
+  {
+    base::AutoLock scoped_lock(path_data->lock);
+    LockedAddToCache(key, path, path_data);

[jar (doing other things), 2012/09/18 16:47:17]

nit: Move these two line down to between lines 222 and 223, and there will be no
need for curlies.

[pastarmovj, 2012/09/19 11:24:28]

Done.

+  }
 
   *result = 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) {
@@ -258,38 +262,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;
 }