[CacheStorage] Deleting a cache could delete the wrong directory

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 bb5269bc3e155586ad693ee1b4e4703e522efda7..5afa3541e82a99261f69bf04c19bdc35c121ed30 100644
--- a/content/browser/cache_storage/cache_storage.cc
+++ b/content/browser/cache_storage/cache_storage.cc
@@ -113,7 +113,7 @@ class CacheStorage::CacheLoader {
 
   // After the backend has been deleted, do any extra house keeping such as
   // removing the cache's directory.
-  virtual void CleanUpDeletedCache(const std::string& key) = 0;
+  virtual void CleanUpDeletedCache(CacheStorageCache* cache) = 0;
 
   // Writes the cache names (and sizes) to disk if applicable.
   virtual void WriteIndex(const StringVector& cache_names,
@@ -129,10 +129,10 @@ class CacheStorage::CacheLoader {
       const std::string& cache_name,
       std::unique_ptr<CacheStorageCacheHandle> cache_handle){};

[jsbell, 2016/07/26 17:43:01]

nit: space between () and {}

[jkarlin, 2016/07/26 17:50:21]

Looks like a clang-format bug. Filed https://crbug.com/631531 and will override
clang-format for now.

 
-  // Notification that a cache has been doomed and will be deleted once the last
-  // cache handle has been dropped. If the loader is holding a handle to the
-  // cache, it should drop it now.
-  virtual void NotifyCacheDoomed(const std::string& cache_name){};
+  // Notification that the cache for |cache_handle| has been doomed. If the
+  // loader is holding a handle to the cache, it should drop it now.
+  virtual void NotifyCacheDoomed(
+      std::unique_ptr<CacheStorageCacheHandle> cache_handle){};

[jsbell, 2016/07/26 17:43:01]

ditto

[jkarlin, 2016/07/26 17:50:21]

Done.

 
  protected:
   scoped_refptr<base::SequencedTaskRunner> cache_task_runner_;
@@ -181,8 +181,7 @@ class CacheStorage::MemoryLoader : public CacheStorage::CacheLoader {
     callback.Run(std::move(cache));
   }
 
-  void CleanUpDeletedCache(const std::string& cache_name) override {
-  }
+  void CleanUpDeletedCache(CacheStorageCache* cache) override {}
 
   void WriteIndex(const StringVector& cache_names,
                   const BoolCallback& callback) override {
@@ -201,9 +200,10 @@ class CacheStorage::MemoryLoader : public CacheStorage::CacheLoader {
     cache_handles_.insert(std::make_pair(cache_name, std::move(cache_handle)));
   };
 
-  void NotifyCacheDoomed(const std::string& cache_name) override {
-    DCHECK(ContainsKey(cache_handles_, cache_name));
-    cache_handles_.erase(cache_name);
+  void NotifyCacheDoomed(
+      std::unique_ptr<CacheStorageCacheHandle> cache_handle) override {
+    DCHECK(ContainsKey(cache_handles_, cache_handle->value()->cache_name()));
+    cache_handles_.erase(cache_handle->value()->cache_name());
   };
 
  private:
@@ -284,13 +284,13 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     callback.Run(CreateCache(cache_name));
   }
 
-  void CleanUpDeletedCache(const std::string& cache_name) override {
+  void CleanUpDeletedCache(CacheStorageCache* cache) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
-    DCHECK(ContainsKey(cache_name_to_cache_dir_, cache_name));
+    DCHECK(ContainsKey(doomed_cache_to_path_, cache));
 
     base::FilePath cache_path =
-        origin_path_.AppendASCII(cache_name_to_cache_dir_[cache_name]);
-    cache_name_to_cache_dir_.erase(cache_name);
+        origin_path_.AppendASCII(doomed_cache_to_path_[cache]);
+    doomed_cache_to_path_.erase(cache);
 
     cache_task_runner_->PostTask(
         FROM_HERE, base::Bind(&SimpleCacheLoader::CleanUpDeleteCacheDirInPool,
@@ -390,6 +390,16 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     callback.Run(std::move(names));
   }
 
+  void NotifyCacheDoomed(
+      std::unique_ptr<CacheStorageCacheHandle> cache_handle) override {
+    DCHECK(ContainsKey(cache_name_to_cache_dir_,
+                       cache_handle->value()->cache_name()));
+    auto iter =
+        cache_name_to_cache_dir_.find(cache_handle->value()->cache_name());
+    doomed_cache_to_path_[cache_handle->value()] = iter->second;
+    cache_name_to_cache_dir_.erase(iter);
+  };
+
  private:
   friend class MigratedLegacyCacheDirectoryNameTest;
   ~SimpleCacheLoader() override {}
@@ -475,6 +485,7 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
 
   const base::FilePath origin_path_;
   std::map<std::string, std::string> cache_name_to_cache_dir_;
+  std::map<CacheStorageCache*, std::string> doomed_cache_to_path_;
 
   base::WeakPtrFactory<SimpleCacheLoader> weak_ptr_factory_;
 };
@@ -784,12 +795,17 @@ void CacheStorage::DeleteCacheDidWriteIndex(
     return;
   }
 
+  // Make sure that a cache handle exists for the doomed cache to ensure that
+  // DeleteCacheFinalize is called.
+  std::unique_ptr<CacheStorageCacheHandle> cache_handle =
+      GetLoadedCache(cache_name);
+
   CacheMap::iterator map_iter = cache_map_.find(cache_name);
   doomed_caches_.insert(
       std::make_pair(map_iter->second.get(), std::move(map_iter->second)));
   cache_map_.erase(map_iter);
 
-  cache_loader_->NotifyCacheDoomed(cache_name);
+  cache_loader_->NotifyCacheDoomed(std::move(cache_handle));
 
   callback.Run(true, CACHE_STORAGE_OK);
 }
@@ -812,7 +828,7 @@ void CacheStorage::DeleteCacheDidGetSize(
       storage::QuotaClient::kServiceWorkerCache, origin_,
       storage::kStorageTypeTemporary, -1 * cache_size);
 
-  cache_loader_->CleanUpDeletedCache(cache->cache_name());
+  cache_loader_->CleanUpDeletedCache(cache.get());
 }
 
 void CacheStorage::EnumerateCachesImpl(