[CacheStorage] Deleting a cache could delete the wrong directory

content/browser/cache_storage/cache_storage.cc

 // Copyright 2014 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 "content/browser/cache_storage/cache_storage.h"
 
 #include <stddef.h>
 
 #include <set>
 #include <string>
@@ skipping 95 matching lines @@
   // load the backend, that happens lazily when the cache is used.
   virtual std::unique_ptr<CacheStorageCache> CreateCache(
       const std::string& cache_name) = 0;
 
   // Deletes any pre-existing cache of the same name and then loads it.
   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.
-  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,
                           const BoolCallback& callback) = 0;
 
   // Loads the cache names from disk if applicable.
   virtual void LoadIndex(std::unique_ptr<std::vector<std::string>> cache_names,
                          const StringVectorCallback& callback) = 0;
 
   // Called when CacheStorage has created a cache. Used to hold onto a handle to
   // the cache if necessary.
   virtual void NotifyCacheCreated(
       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_;
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
 
   // Owned by CacheStorage which owns this.
   storage::QuotaManagerProxy* quota_manager_proxy_;
 
   base::WeakPtr<storage::BlobStorageContext> blob_context_;
 
@@ skipping 28 matching lines @@
         origin_, cache_name, cache_storage_, request_context_getter_,
         quota_manager_proxy_, blob_context_);
   }
 
   void PrepareNewCacheDestination(const std::string& cache_name,
                                   const CacheCallback& callback) override {
     std::unique_ptr<CacheStorageCache> cache = CreateCache(cache_name);
     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 {
     callback.Run(true);
   }
 
   void LoadIndex(std::unique_ptr<std::vector<std::string>> cache_names,
                  const StringVectorCallback& callback) override {
     callback.Run(std::move(cache_names));
   }
 
   void NotifyCacheCreated(
       const std::string& cache_name,
       std::unique_ptr<CacheStorageCacheHandle> cache_handle) override {
     DCHECK(!ContainsKey(cache_handles_, cache_name));
     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:
   typedef std::map<std::string, std::unique_ptr<CacheStorageCacheHandle>>
       CacheHandles;
   ~MemoryLoader() override {}
 
   // Keep a reference to each cache to ensure that it's not freed before the
   // client calls CacheStorage::Delete or the CacheStorage is
   // freed.
@@ skipping 60 matching lines @@
                                   const std::string& cache_dir) {
     if (cache_dir.empty()) {
       callback.Run(std::unique_ptr<CacheStorageCache>());
       return;
     }
 
     cache_name_to_cache_dir_[cache_name] = cache_dir;
     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,
                               cache_path));
   }
 
   static void CleanUpDeleteCacheDirInPool(const base::FilePath& cache_path) {
     base::DeleteFile(cache_path, true /* recursive */);
   }
 
@@ skipping 79 matching lines @@
         cache_dirs->insert(cache.cache_dir());
       }
     }
 
     cache_task_runner_->PostTask(
         FROM_HERE, base::Bind(&DeleteUnreferencedCachesInPool, origin_path_,
                               base::Passed(&cache_dirs)));
     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 {}
 
   // Iterates over the caches and deletes any directory not found in
   // |cache_dirs|. Runs on cache_task_runner_
   static void DeleteUnreferencedCachesInPool(
       const base::FilePath& cache_base_dir,
       std::unique_ptr<std::set<std::string>> cache_dirs) {
     base::FileEnumerator file_enum(cache_base_dir, false /* recursive */,
@@ skipping 65 matching lines @@
   static std::string ReadAndMigrateIndexInPool(
       const base::FilePath& index_path) {
     std::string body;
     base::ReadFileToString(index_path, &body);
 
     return MigrateCachesIfNecessaryInPool(body, index_path);
   }
 
   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_;
 };
 
 CacheStorage::CacheStorage(
     const base::FilePath& path,
     bool memory_only,
     base::SequencedTaskRunner* cache_task_runner,
     scoped_refptr<net::URLRequestContextGetter> request_context,
     scoped_refptr<storage::QuotaManagerProxy> quota_manager_proxy,
@@ skipping 289 matching lines @@
     bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!success) {
     // Undo any changes if the change couldn't be written to disk.
     ordered_cache_names_ = original_ordered_cache_names;
     callback.Run(false, CACHE_STORAGE_ERROR_STORAGE);
     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);
 }
 
 // Call this once the last handle to a doomed cache is gone. It's okay if this
 // doesn't get to complete before shutdown, the cache will be removed from disk
 // on next startup in that case.
 void CacheStorage::DeleteCacheFinalize(
     std::unique_ptr<CacheStorageCache> doomed_cache) {
   CacheStorageCache* cache = doomed_cache.get();
   cache->Size(base::Bind(&CacheStorage::DeleteCacheDidGetSize,
                          weak_factory_.GetWeakPtr(),
                          base::Passed(std::move(doomed_cache))));
 }
 
 void CacheStorage::DeleteCacheDidGetSize(
     std::unique_ptr<CacheStorageCache> cache,
     int64_t cache_size) {
   quota_manager_proxy_->NotifyStorageModified(
       storage::QuotaClient::kServiceWorkerCache, origin_,
       storage::kStorageTypeTemporary, -1 * cache_size);
 
-  cache_loader_->CleanUpDeletedCache(cache->cache_name());
+  cache_loader_->CleanUpDeletedCache(cache.get());
 }
 
 void CacheStorage::EnumerateCachesImpl(
     const StringsAndErrorCallback& callback) {
   callback.Run(ordered_cache_names_, CACHE_STORAGE_OK);
 }
 
 void CacheStorage::MatchCacheImpl(
     const std::string& cache_name,
     std::unique_ptr<ServiceWorkerFetchRequest> request,
@@ skipping 185 matching lines @@
     std::unique_ptr<CacheStorageCacheHandle> cache_handle =
         GetLoadedCache(cache_name);
     CacheStorageCache* cache = cache_handle->value();
     cache->Size(base::Bind(&SizeRetrievedFromCache,
                            base::Passed(std::move(cache_handle)),
                            barrier_closure, accumulator_ptr));
   }
 }
 
 }  // namespace content