Add PathService::RemoveOverride to clear path overrides.

base/path_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/path_service.h"
 
 #ifdef OS_WIN
 #include <windows.h>
 #include <shellapi.h>
 #include <shlobj.h>
@@ skipping 122 matching lines @@
     }
   }
 };
 
 static base::LazyInstance<PathData> g_path_data = LAZY_INSTANCE_INITIALIZER;
 
 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()) {
     *result = it->second;
     return true;
   }
   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
 // moot, but we should keep this in mind for the future.
 // static
 bool PathService::Get(int key, FilePath* result) {
   PathData* path_data = GetPathData();
   DCHECK(path_data);
   DCHECK(result);
   DCHECK_GE(key, base::DIR_CURRENT);
 
+  Provider* provider = NULL;

[jar (doing other things), 2012/09/20 01:51:19]

nit: move to in front of line 184, closest to first use that we can be.

[pastarmovj, 2012/09/20 09:32:19]

Done.

   // 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

[jar (doing other things), 2012/09/20 01:51:19]

nit: Probably delete this, but you could put it near line 193 if you wanted.

[pastarmovj, 2012/09/20 09:32:19]

Done.

   // NOTE: it should be safe to iterate here without the lock

[jar (doing other things), 2012/09/20 01:51:19]

Delete the outdated comments on line 199 and 200.

[pastarmovj, 2012/09/20 09:32:19]

I think it is good to have a comment why this is not protected by the lock. So I
slightly modified the comment but left it in.

[jar (doing other things), 2012/09/20 16:30:32]

Good point.  I'm fine with that.

   // since RegisterProvider always prepends.
-  Provider* provider = path_data->providers;
   while (provider) {
     if (provider->func(key, &path))
       break;
     DCHECK(path.empty()) << "provider should not have modified path";
     provider = provider->next;
   }
 
   if (path.empty())
     return false;
 
-  AddToCache(key, path);
+  *result = 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) {
   PathData* path_data = GetPathData();
   DCHECK(path_data);
   DCHECK_GT(key, base::DIR_CURRENT) << "invalid path key";
 
   FilePath file_path = path;
 
   // For some locations this will fail if called from inside the sandbox there-
@@ skipping 12 matching lines @@
   // relative path.
   if (!file_util::AbsolutePath(&file_path))
     return false;
 
   base::AutoLock scoped_lock(path_data->lock);
 
   // Clear the cache now. Some of its entries could have depended
   // 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;
 }