[CacheStorage] Give cache directories unique names

content/browser/cache_storage/cache_storage.cc

Index: content/browser/cache_storage/cache_storage.cc
diff --git a/content/browser/cache_storage/cache_storage.cc b/content/browser/cache_storage/cache_storage.cc
index 42f0215b8477c7a4afac97353e844b331c97e3f4..312e1165a0bd65e319e1a8924b43deeca10790e9 100644
--- a/content/browser/cache_storage/cache_storage.cc
+++ b/content/browser/cache_storage/cache_storage.cc
@@ -9,6 +9,7 @@
 #include "base/barrier_closure.h"
 #include "base/files/file_util.h"
 #include "base/files/memory_mapped_file.h"
+#include "base/guid.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
@@ -38,12 +39,19 @@ void CloseAllCachesDidCloseCache(const scoped_refptr<CacheStorageCache>& cache,
   barrier_closure.Run();
 }
 
+std::string ReadFile(const base::FilePath& path) {
+  std::string body;
+  base::ReadFileToString(path, &body);
+  return body;
+}
+
 }  // namespace
 
+class LegacyCacheDirectoryNameTest;
+
 const char CacheStorage::kIndexFileName[] = "index.txt";
 
-// Handles the loading and clean up of CacheStorageCache objects. The
-// callback of every public method is guaranteed to be called.
+// Handles the loading and clean up of CacheStorageCache objects.
 class CacheStorage::CacheLoader {
  public:
   typedef base::Callback<void(const scoped_refptr<CacheStorageCache>&)>
@@ -74,8 +82,8 @@ class CacheStorage::CacheLoader {
       const std::string& cache_name) = 0;
 
   // Deletes any pre-existing cache of the same name and then loads it.
-  virtual void CreateCache(const std::string& cache_name,
-                           const CacheCallback& callback) = 0;
+  virtual void PrepareNewCacheDestination(const std::string& cache_name,
+                                          const CacheCallback& callback) = 0;
 
   // After the backend has been deleted, do any extra house keeping such as
   // removing the cache's directory.
@@ -122,8 +130,8 @@ class CacheStorage::MemoryLoader : public CacheStorage::CacheLoader {
         origin_, request_context_getter_, quota_manager_proxy_, blob_context_);
   }
 
-  void CreateCache(const std::string& cache_name,
-                   const CacheCallback& callback) override {
+  void PrepareNewCacheDestination(const std::string& cache_name,
+                                  const CacheCallback& callback) override {
     scoped_refptr<CacheStorageCache> cache = CreateCache(cache_name);
     cache_refs_.insert(std::make_pair(cache_name, cache));
     callback.Run(cache);
@@ -177,56 +185,62 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
   scoped_refptr<CacheStorageCache> CreateCache(
       const std::string& cache_name) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    DCHECK(cache_name_to_cache_dir_.find(cache_name) !=

[jsbell, 2015/10/22 16:38:52]

Could use ContainsKey() from base/stl_util

[jkarlin, 2015/10/23 17:23:09]

Done, thanks! Forgot about that function.

+           cache_name_to_cache_dir_.end());
 
+    std::string cache_dir = cache_name_to_cache_dir_[cache_name];
+    base::FilePath cache_path = origin_path_.AppendASCII(cache_dir);
     return CacheStorageCache::CreatePersistentCache(
-        origin_, CreatePersistentCachePath(origin_path_, cache_name),
-        request_context_getter_, quota_manager_proxy_, blob_context_);
+        origin_, cache_path, request_context_getter_, quota_manager_proxy_,
+        blob_context_);
   }
 
-  void CreateCache(const std::string& cache_name,
-                   const CacheCallback& callback) override {
+  void PrepareNewCacheDestination(const std::string& cache_name,
+                                  const CacheCallback& callback) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-    // 1. Delete the cache's directory if it exists.
-    // (CreateCacheDeleteFilesInPool)
-    // 2. Load the cache. (LoadCreateDirectoryInPool)
-
-    base::FilePath cache_path =
-        CreatePersistentCachePath(origin_path_, cache_name);
-
     PostTaskAndReplyWithResult(
         cache_task_runner_.get(), FROM_HERE,
-        base::Bind(&SimpleCacheLoader::CreateCachePrepDirInPool, cache_path),
-        base::Bind(&SimpleCacheLoader::CreateCachePreppedDir, cache_name,
-                   callback, weak_ptr_factory_.GetWeakPtr()));
+        base::Bind(&SimpleCacheLoader::PrepareNewCacheDirectoryInPool,
+                   origin_path_),
+        base::Bind(&SimpleCacheLoader::PrepareNewCacheCreateCache,
+                   weak_ptr_factory_.GetWeakPtr(), cache_name, callback));
   }
 
-  static bool CreateCachePrepDirInPool(const base::FilePath& cache_path) {
-    if (base::PathExists(cache_path))
-      base::DeleteFile(cache_path, /* recursive */ true);
-    return base::CreateDirectory(cache_path);
+  static std::string PrepareNewCacheDirectoryInPool(

[jsbell, 2015/10/22 16:38:52]

Add comment that this is run on the cache_task_runner?

[jkarlin, 2015/10/23 17:23:09]

Done.

+      const base::FilePath& origin_path) {
+    std::string cache_dir;
+    base::FilePath cache_path;
+    do {
+      cache_dir = base::GenerateGUID();
+      cache_path = origin_path.AppendASCII(cache_dir);
+    } while (base::PathExists(cache_path));
+
+    return base::CreateDirectory(cache_path) ? cache_dir : "";
   }
 
-  static void CreateCachePreppedDir(const std::string& cache_name,
-                                    const CacheCallback& callback,
-                                    base::WeakPtr<SimpleCacheLoader> loader,
-                                    bool success) {
-    if (!success || !loader) {
+  void PrepareNewCacheCreateCache(const std::string& cache_name,
+                                  const CacheCallback& callback,
+                                  const std::string& cache_dir) {
+    if (cache_dir.empty()) {
       callback.Run(scoped_refptr<CacheStorageCache>());
       return;
     }
 
-    callback.Run(loader->CreateCache(cache_name));
+    cache_name_to_cache_dir_[cache_name] = cache_dir;
+    callback.Run(CreateCache(cache_name));
   }
 
   void CleanUpDeletedCache(const std::string& cache_name,
                            const BoolCallback& callback) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-    // 1. Delete the cache's directory. (CleanUpDeleteCacheDirInPool)
+    DCHECK(cache_name_to_cache_dir_.find(cache_name) !=

[jsbell, 2015/10/22 16:38:52]

Could use ContainsKey()

[jkarlin, 2015/10/23 17:23:09]

Done.

+           cache_name_to_cache_dir_.end());
 
     base::FilePath cache_path =
-        CreatePersistentCachePath(origin_path_, cache_name);
+        origin_path_.AppendASCII(cache_name_to_cache_dir_[cache_name]);
+    cache_name_to_cache_dir_.erase(cache_name);
+
     cache_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&SimpleCacheLoader::CleanUpDeleteCacheDirInPool, cache_path,
@@ -252,8 +266,12 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     index.set_origin(origin_.spec());
 
     for (size_t i = 0u, max = cache_names.size(); i < max; ++i) {
+      DCHECK(cache_name_to_cache_dir_.find(cache_names[i]) !=

[jsbell, 2015/10/22 16:38:52]

ContainsKey()

[jkarlin, 2015/10/23 17:23:09]

Done.

+             cache_name_to_cache_dir_.end());
+
       CacheStorageIndex::Cache* index_cache = index.add_cache();
       index_cache->set_name(cache_names[i]);
+      index_cache->set_cache_dir(cache_name_to_cache_dir_[cache_names[i]]);
     }
 
     std::string serialized;
@@ -297,28 +315,16 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     base::FilePath index_path =
         origin_path_.AppendASCII(CacheStorage::kIndexFileName);
 
-    cache_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&SimpleCacheLoader::LoadIndexReadFileInPool,
-                              index_path, base::Passed(names.Pass()), callback,
-                              base::ThreadTaskRunnerHandle::Get()));
-  }
-
-  static void LoadIndexReadFileInPool(
-      const base::FilePath& index_path,
-      scoped_ptr<std::vector<std::string>> names,
-      const StringVectorCallback& callback,
-      const scoped_refptr<base::SingleThreadTaskRunner>& original_task_runner) {
-    std::string body;
-    base::ReadFileToString(index_path, &body);
-
-    original_task_runner->PostTask(
-        FROM_HERE, base::Bind(&SimpleCacheLoader::LoadIndexDidReadFile,
-                              base::Passed(names.Pass()), callback, body));
+    PostTaskAndReplyWithResult(
+        cache_task_runner_.get(), FROM_HERE, base::Bind(&ReadFile, index_path),
+        base::Bind(&SimpleCacheLoader::LoadIndexDidReadFile,
+                   weak_ptr_factory_.GetWeakPtr(), base::Passed(&names),
+                   callback));
   }
 
-  static void LoadIndexDidReadFile(scoped_ptr<std::vector<std::string>> names,
-                                   const StringVectorCallback& callback,
-                                   const std::string& serialized) {
+  void LoadIndexDidReadFile(scoped_ptr<std::vector<std::string>> names,
+                            const StringVectorCallback& callback,
+                            const std::string& serialized) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
     CacheStorageIndex index;
@@ -326,6 +332,13 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
       for (int i = 0, max = index.cache_size(); i < max; ++i) {
         const CacheStorageIndex::Cache& cache = index.cache(i);
         names->push_back(cache.name());
+        // Before randomly generated cache directory names were used, the hexed
+        // hash of the cache name was used. For backwards compatibility, use the
+        // hexed hash as the directory name for protobufs that don't have a
+        // directory name.
+        std::string cache_dir =
+            cache.has_cache_dir() ? cache.cache_dir() : HexedHash(cache.name());
+        cache_name_to_cache_dir_[cache.name()] = cache_dir;
       }
     }
 
@@ -335,6 +348,7 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
   }
 
  private:
+  friend class LegacyCacheDirectoryNameTest;
   ~SimpleCacheLoader() override {}
 
   static std::string HexedHash(const std::string& value) {
@@ -344,13 +358,8 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     return valued_hexed_hash;
   }
 
-  static base::FilePath CreatePersistentCachePath(
-      const base::FilePath& origin_path,
-      const std::string& cache_name) {
-    return origin_path.AppendASCII(HexedHash(cache_name));
-  }
-
   const base::FilePath origin_path_;
+  std::map<std::string, std::string> cache_name_to_cache_dir_;
 
   base::WeakPtrFactory<SimpleCacheLoader> weak_ptr_factory_;
 };
@@ -569,7 +578,7 @@ void CacheStorage::OpenCacheImpl(const std::string& cache_name,
     return;
   }
 
-  cache_loader_->CreateCache(
+  cache_loader_->PrepareNewCacheDestination(
       cache_name, base::Bind(&CacheStorage::CreateCacheDidCreateCache,
                              weak_factory_.GetWeakPtr(), cache_name, callback));
 }