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 147 matching lines @@
 }
 
 // static
 bool PathService::GetFromOverrides(int key, FilePath* result) {
   PathData* path_data = GetPathData();
   base::AutoLock scoped_lock(path_data->lock);
 
   // check for an overridden version.
   PathMap::const_iterator it = path_data->overrides.find(key);
   if (it != path_data->overrides.end()) {
     *result = it->second;

[jar (doing other things), 2012/09/15 02:01:20]

This is (I think) the one place they *could* (should?) have put an override into
the cache.... but it does not get done.

It might be simpler to never use the cache for overridden values.

To be cleaner, it is then probably nicer to check for the override while we have
the lock and are checking for the cache, rather than acquiring the lock twice. 
This way we don't have to copy the override into a second map.

[pastarmovj, 2012/09/17 16:30:40]

Absolutely. What happens now seems to be that only the last override ever stays
in the cache and all others are always taken out of the overrides array.
I have uploaded now a heavily rewritten version that actually hides GetFrom* +
AddToCache in the internal implementation portion of this file and changes their
contract to make them expect the structre is locked from the caller. This allows
us to make much less locks than needed and properly cache all paths.
Given that this is bigger change I decided to ask Brett to have a look at it too
because I think he wrote big portions of this code or will know who to delegate
this.

     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.
@@ skipping 18 matching lines @@
     return true;
 
   if (GetFromOverrides(key, result))
     return true;
 
   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;

[jar (doing other things), 2012/09/15 02:01:20]

This is probably racy, as the contents of path_data->provider may arrive
asynchronously, and worse yet, there are no acquire and release semantics.  As a
result, we might get the pointer intact... but what it points to might not be
intact.  

The acquisition of the start of this list should be done under a lock, although
it is true that we can iterate over the list without a lock. 

The corresponding lock is present at the end of the file (where we do the
prepend).

[pastarmovj, 2012/09/17 16:30:40]

Theoretically you are right but practically the list is rather static. If new
providers should arrive it only happens in the initialization path which happens
before we spawn the threads. Only overrides might happen later in the lifetime
of the process. And even that happens mostly very early at start up because as
the comment says changes to the overrides might be very unexpected to the
clients and should not be considered lightly. That said I would be confortable
leaving this assignment without a lock.

[jar (doing other things), 2012/09/17 17:06:08]

The comment is wrong.  The fact that we use a lock to pre-pend makes it clear we
are potentially mutli-threaded when doing so, and hence we should use a lock to
read this to start an iterator.

Unless you can argue that this is all done while single threaded (which is not
the argument in the comment), this should be fixed.

[pastarmovj, 2012/09/18 14:27:13]

Done.

   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;
   return true;
 }
 
+// static

[jar (doing other things), 2012/09/15 02:01:20]

I personally like this style... but it is a gratuitous style change to this
file.  You should just leave the existing style intact.

[pastarmovj, 2012/09/17 16:30:40]

Actually the file is already mixed style (see line 185). I think in that case
unifying the style is still a step in the right direction :)

 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;

[jar (doing other things), 2012/09/15 02:01:20]

It is interesting that the directly implent into the cache when they establish
the override.  This is a little suspicious, but could have just been premature
optimization.
Note that the cache.clear() above would have undone other "premature
optimization" done for other overrides.

Given the odd performance, it is good to check that this code can properly
handle two overrides.  Is that in the unit test?

[pastarmovj, 2012/09/17 16:30:40]

I agree. I added a new test to verify that. 

   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
   path_data->providers = p;

[jar (doing other things), 2012/09/15 02:01:20]

This push operation is correctly locked, but it would have been better if the
lock was not used during the construction of the new Provider (locks should be
held for the least possible period of time... and try not to call other
functions  while holding a lock).

[pastarmovj, 2012/09/17 16:30:40]

Done.

 }