Write out CacheStorageCache size to index file.

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 f7db68a4b8d94f5692c2de6d612d87db012729ba..8683a6f48ad1767fd7b78c520513cf1d0bc3a663 100644
--- a/content/browser/cache_storage/cache_storage.cc
+++ b/content/browser/cache_storage/cache_storage.cc
@@ -8,6 +8,7 @@
 
 #include <set>
 #include <string>
+#include <unordered_map>
 #include <utility>
 
 #include "base/barrier_closure.h"
@@ -47,24 +48,18 @@ std::string HexedHash(const std::string& value) {
   return valued_hexed_hash;
 }
 
-void SizeRetrievedFromCache(
-    std::unique_ptr<CacheStorageCacheHandle> cache_handle,
-    const base::Closure& closure,
-    int64_t* accumulator,
-    int64_t size) {
-  *accumulator += size;
-  closure.Run();
-}
-
 void SizeRetrievedFromAllCaches(std::unique_ptr<int64_t> accumulator,
                                 const CacheStorage::SizeCallback& callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback, *accumulator));
 }
 
+void DoNothingWithBool(bool success) {}
+
 }  // namespace
 
 const char CacheStorage::kIndexFileName[] = "index.txt";
+const int64_t CacheStorage::kSizeUnknown;

[jkarlin, 2016/11/11 18:24:57]

Needs to be assigned something

[cmumford, 2016/11/22 17:45:03]

It's initialized in the header, but because kSizeUnknown is ODR-use it needs to
be defined here to avoid a link error. See
http://en.cppreference.com/w/cpp/language/definition#ODR-use. Apparently
DCHECK_NE takes a reference to the parameter forcing this definition.

Let me know if you still want it initialized in the *.cc and I'll move it.

 
 struct CacheStorage::CacheMatchResponse {
   CacheMatchResponse() = default;
@@ -75,14 +70,126 @@ struct CacheStorage::CacheMatchResponse {
   std::unique_ptr<storage::BlobDataHandle> blob_data_handle;
 };
 
+class CacheStorage::CacheStorageIndex {
+ public:
+  CacheStorageIndex(const CacheStorageIndex& other) {
+    for (const auto& cache_metadata : other.ordered_cache_metadata_)
+      Insert(cache_metadata);
+  }
+
+  CacheStorageIndex() = default;
+
+  ~CacheStorageIndex() = default;
+
+  CacheStorageIndex& operator=(CacheStorageIndex&& rhs) {
+    ordered_cache_metadata_ = std::move(rhs.ordered_cache_metadata_);
+    cache_metadata_map_ = std::move(rhs.cache_metadata_map_);
+    storage_size_ = rhs.storage_size_;
+    storage_size_dirty_ = rhs.storage_size_dirty_;
+    return *this;
+  }
+
+  void Insert(const CacheMetadata& cache_metadata) {
+    DCHECK(cache_metadata_map_.find(cache_metadata.name) ==
+           cache_metadata_map_.end());
+    ordered_cache_metadata_.push_back(cache_metadata);
+    cache_metadata_map_[cache_metadata.name] = --ordered_cache_metadata_.end();
+    storage_size_dirty_ = true;
+  }
+
+  void Delete(const std::string& cache_name) {
+    auto it = cache_metadata_map_.find(cache_name);
+    DCHECK(it != cache_metadata_map_.end());
+    ordered_cache_metadata_.erase(it->second);
+    cache_metadata_map_.erase(it);
+    storage_size_dirty_ = true;
+  }
+
+  // Sets the cache size. Returns true if the new size is different than the
+  // current size else false.
+  bool SetCacheSize(const std::string& cache_name, int64_t size) {
+    auto it = cache_metadata_map_.find(cache_name);
+    if (it == cache_metadata_map_.end()) {
+      // Deleted (but still referenced) caches can still run.
+      return false;
+    }
+    if (it->second->size == size)
+      return false;
+    it->second->size = size;
+    storage_size_dirty_ = true;
+    return true;
+  }
+
+  // The cache was modified, increasing/decreasing cache size by |size_delta|.
+  void SetCacheSizeModified(const std::string& cache_name, int64_t size_delta) {
+    DCHECK_NE(size_delta, 0);
+
+    auto it = cache_metadata_map_.find(cache_name);
+    if (it == cache_metadata_map_.end()) {
+      // Deleted (but still referenced) caches can still run.
+      return;
+    }
+    DCHECK_NE(it->second->size, kSizeUnknown);
+    it->second->size += size_delta;
+    DCHECK_GE(it->second->size, 0U);
+    if (storage_size_dirty_ || storage_size_ == kSizeUnknown)
+      return;
+    DCHECK_NE(storage_size_, kSizeUnknown);
+    storage_size_ += size_delta;
+    DCHECK_GE(storage_size_, 0U);
+  }
+
+  int64_t GetCacheSize(const std::string& cache_name) const {
+    const auto& it = cache_metadata_map_.find(cache_name);
+    if (it == cache_metadata_map_.end())
+      return kSizeUnknown;
+    return it->second->size;
+  }
+
+  const std::list<CacheMetadata>& ordered_cache_metadata() const {
+    return ordered_cache_metadata_;
+  }
+
+  size_t num_entries() const { return ordered_cache_metadata_.size(); }
+
+  int64_t GetStorageSize() {
+    if (storage_size_dirty_)
+      UpdateStorageSize();
+    return storage_size_;
+  }
+
+ private:
+  void UpdateStorageSize() {
+    DCHECK(storage_size_dirty_);
+    storage_size_dirty_ = false;
+    int64_t storage_size = 0;
+    storage_size_ = kSizeUnknown;
+    for (const CacheMetadata& info : ordered_cache_metadata_) {
+      if (info.size == kSizeUnknown)
+        return;
+      storage_size += info.size;
+    }
+    storage_size_ = storage_size;
+  }
+  // Use a list to keep saved iterators valid during insert/erase.
+  // Note: ordered by cache creation.
+  std::list<CacheMetadata> ordered_cache_metadata_;
+  std::unordered_map<std::string, std::list<CacheMetadata>::iterator>
+      cache_metadata_map_;
+
+  // The total size of all caches in this store.
+  int64_t storage_size_ = CacheStorage::kSizeUnknown;
+  bool storage_size_dirty_ = true;
+};
+
 // Handles the loading and clean up of CacheStorageCache objects.
 class CacheStorage::CacheLoader {
  public:
   typedef base::Callback<void(std::unique_ptr<CacheStorageCache>)>
       CacheCallback;
   typedef base::Callback<void(bool)> BoolCallback;
-  typedef base::Callback<void(std::unique_ptr<std::vector<std::string>>)>
-      StringVectorCallback;
+  using CacheStorageIndexCallback =
+      base::Callback<void(std::unique_ptr<CacheStorageIndex>)>;
 
   CacheLoader(
       base::SequencedTaskRunner* cache_task_runner,
@@ -105,7 +212,8 @@ class CacheStorage::CacheLoader {
   // Creates a CacheStorageCache with the given name. It does not attempt to
   // load the backend, that happens lazily when the cache is used.
   virtual std::unique_ptr<CacheStorageCache> CreateCache(
-      const std::string& cache_name) = 0;
+      const std::string& cache_name,
+      int64_t cache_size) = 0;
 
   // Deletes any pre-existing cache of the same name and then loads it.
   virtual void PrepareNewCacheDestination(const std::string& cache_name,
@@ -115,13 +223,12 @@ class CacheStorage::CacheLoader {
   // removing the cache's directory.
   virtual void CleanUpDeletedCache(CacheStorageCache* cache) = 0;
 
-  // Writes the cache names (and sizes) to disk if applicable.
-  virtual void WriteIndex(const StringVector& cache_names,
+  // Writes the cache index to disk if applicable.
+  virtual void WriteIndex(const CacheStorageIndex& index,
                           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;
+  // Loads the cache index from disk if applicable.
+  virtual void LoadIndex(const CacheStorageIndexCallback& callback) = 0;
 
   // Called when CacheStorage has created a cache. Used to hold onto a handle to
   // the cache if necessary.
@@ -168,8 +275,8 @@ class CacheStorage::MemoryLoader : public CacheStorage::CacheLoader {
                     cache_storage,
                     origin) {}
 
-  std::unique_ptr<CacheStorageCache> CreateCache(
-      const std::string& cache_name) override {
+  std::unique_ptr<CacheStorageCache> CreateCache(const std::string& cache_name,
+                                                 int64_t cache_size) override {
     return CacheStorageCache::CreateMemoryCache(
         origin_, cache_name, cache_storage_, request_context_getter_,
         quota_manager_proxy_, blob_context_);
@@ -177,20 +284,20 @@ class CacheStorage::MemoryLoader : public CacheStorage::CacheLoader {
 
   void PrepareNewCacheDestination(const std::string& cache_name,
                                   const CacheCallback& callback) override {
-    std::unique_ptr<CacheStorageCache> cache = CreateCache(cache_name);
+    std::unique_ptr<CacheStorageCache> cache =
+        CreateCache(cache_name, 0 /*cache_size*/);
     callback.Run(std::move(cache));
   }
 
   void CleanUpDeletedCache(CacheStorageCache* cache) override {}
 
-  void WriteIndex(const StringVector& cache_names,
+  void WriteIndex(const CacheStorageIndex& index,
                   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 LoadIndex(const CacheStorageIndexCallback& callback) override {
+    callback.Run(base::MakeUnique<CacheStorageIndex>());
   }
 
   void NotifyCacheCreated(
@@ -236,8 +343,8 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
         origin_path_(origin_path),
         weak_ptr_factory_(this) {}
 
-  std::unique_ptr<CacheStorageCache> CreateCache(
-      const std::string& cache_name) override {
+  std::unique_ptr<CacheStorageCache> CreateCache(const std::string& cache_name,
+                                                 int64_t cache_size) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
     DCHECK(base::ContainsKey(cache_name_to_cache_dir_, cache_name));
 
@@ -245,7 +352,8 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     base::FilePath cache_path = origin_path_.AppendASCII(cache_dir);
     return CacheStorageCache::CreatePersistentCache(
         origin_, cache_name, cache_storage_, cache_path,
-        request_context_getter_, quota_manager_proxy_, blob_context_);
+        request_context_getter_, quota_manager_proxy_, blob_context_,
+        cache_size);
   }
 
   void PrepareNewCacheDestination(const std::string& cache_name,
@@ -282,7 +390,7 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     }
 
     cache_name_to_cache_dir_[cache_name] = cache_dir;
-    callback.Run(CreateCache(cache_name));
+    callback.Run(CreateCache(cache_name, CacheStorage::kSizeUnknown));
   }
 
   void CleanUpDeletedCache(CacheStorageCache* cache) override {
@@ -302,26 +410,30 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     base::DeleteFile(cache_path, true /* recursive */);
   }
 
-  void WriteIndex(const StringVector& cache_names,
+  void WriteIndex(const CacheStorageIndex& index,
                   const BoolCallback& callback) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
     // 1. Create the index file as a string. (WriteIndex)
     // 2. Write the file to disk. (WriteIndexWriteToFileInPool)
 
-    CacheStorageIndex index;
-    index.set_origin(origin_.spec());
+    content::CacheStorageIndex pb_index;

[jkarlin, 2016/11/11 18:24:56]

better to write it out, protobuf_index or proto_index

[cmumford, 2016/11/22 17:45:03]

Done. I went with proto_index.

+    pb_index.set_origin(origin_.spec());
 
-    for (size_t i = 0u, max = cache_names.size(); i < max; ++i) {
-      DCHECK(base::ContainsKey(cache_name_to_cache_dir_, cache_names[i]));
+    for (const auto& cache_metadata : index.ordered_cache_metadata()) {
+      DCHECK(base::ContainsKey(cache_name_to_cache_dir_, cache_metadata.name));
 
-      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]]);
+      content::CacheStorageIndex::Cache* index_cache = pb_index.add_cache();
+      index_cache->set_name(cache_metadata.name);
+      index_cache->set_cache_dir(cache_name_to_cache_dir_[cache_metadata.name]);
+      if (cache_metadata.size == CacheStorage::kSizeUnknown)
+        index_cache->clear_size();
+      else
+        index_cache->set_size(cache_metadata.size);
     }
 
     std::string serialized;
-    bool success = index.SerializeToString(&serialized);
+    bool success = pb_index.SerializeToString(&serialized);
     DCHECK(success);
 
     base::FilePath tmp_path = origin_path_.AppendASCII("index.txt.tmp");
@@ -348,47 +460,38 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
     return base::ReplaceFile(tmp_path, index_path, NULL);
   }
 
-  void LoadIndex(std::unique_ptr<std::vector<std::string>> names,
-                 const StringVectorCallback& callback) override {
+  void LoadIndex(const CacheStorageIndexCallback& callback) override {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-    // 1. Read the file from disk. (LoadIndexReadFileInPool)
-    // 2. Parse file and return the names of the caches (LoadIndexDidReadFile)
-
-    base::FilePath index_path =
-        origin_path_.AppendASCII(CacheStorage::kIndexFileName);
-
     PostTaskAndReplyWithResult(
         cache_task_runner_.get(), FROM_HERE,
-        base::Bind(&SimpleCacheLoader::ReadAndMigrateIndexInPool, index_path),
-        base::Bind(&SimpleCacheLoader::LoadIndexDidReadFile,
-                   weak_ptr_factory_.GetWeakPtr(), base::Passed(&names),
-                   callback));
+        base::Bind(&SimpleCacheLoader::ReadAndMigrateIndexInPool, origin_path_),
+        base::Bind(&SimpleCacheLoader::LoadIndexDidReadIndex,
+                   weak_ptr_factory_.GetWeakPtr(), callback));
   }
 
-  void LoadIndexDidReadFile(std::unique_ptr<std::vector<std::string>> names,
-                            const StringVectorCallback& callback,
-                            const std::string& serialized) {
+  void LoadIndexDidReadIndex(const CacheStorageIndexCallback& callback,
+                             content::CacheStorageIndex pb_index) {
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
     std::unique_ptr<std::set<std::string>> cache_dirs(
         new std::set<std::string>);
 
-    CacheStorageIndex index;
-    if (index.ParseFromString(serialized)) {
-      for (int i = 0, max = index.cache_size(); i < max; ++i) {
-        const CacheStorageIndex::Cache& cache = index.cache(i);
-        DCHECK(cache.has_cache_dir());
-        names->push_back(cache.name());
-        cache_name_to_cache_dir_[cache.name()] = cache.cache_dir();
-        cache_dirs->insert(cache.cache_dir());
-      }
+    auto index = base::MakeUnique<CacheStorageIndex>();
+    for (int i = 0, max = pb_index.cache_size(); i < max; ++i) {
+      const content::CacheStorageIndex::Cache& cache = pb_index.cache(i);
+      DCHECK(cache.has_cache_dir());
+      int64_t cache_size =
+          cache.has_size() ? cache.size() : CacheStorage::kSizeUnknown;
+      index->Insert(CacheMetadata(cache.name(), cache_size));
+      cache_name_to_cache_dir_[cache.name()] = cache.cache_dir();
+      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));
+    callback.Run(std::move(index));
   }
 
   void NotifyCacheDoomed(
@@ -424,22 +527,40 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
   }
 
   // Runs on cache_task_runner_
-  static std::string MigrateCachesIfNecessaryInPool(
-      const std::string& body,
-      const base::FilePath& index_path) {
-    CacheStorageIndex index;
-    if (!index.ParseFromString(body))
-      return body;
+  static content::CacheStorageIndex ReadAndMigrateIndexInPool(
+      const base::FilePath& origin_path) {
+    const base::FilePath index_path =
+        origin_path.AppendASCII(CacheStorage::kIndexFileName);
+
+    content::CacheStorageIndex index;
+    std::string body;
+    if (!base::ReadFileToString(index_path, &body) ||
+        !index.ParseFromString(body))
+      return content::CacheStorageIndex();
+    body.clear();
 
-    base::FilePath origin_path = index_path.DirName();
-    bool index_is_dirty = false;
-    const std::string kBadIndexState("");
+    base::File::Info file_info;
+    base::Time index_last_modified;
+    if (GetFileInfo(index_path, &file_info))
+      index_last_modified = file_info.last_modified;
+    bool index_modified = false;
 
     // Look for caches that have no cache_dir. Give any such caches a directory
     // with a random name and move them there. Then, rewrite the index file.

[jkarlin, 2016/11/11 18:24:56]

Need to update this comment.

[cmumford, 2016/11/22 17:45:03]

Done.

     for (int i = 0, max = index.cache_size(); i < max; ++i) {
-      const CacheStorageIndex::Cache& cache = index.cache(i);
-      if (!cache.has_cache_dir()) {
+      const content::CacheStorageIndex::Cache& cache = index.cache(i);
+      if (cache.has_cache_dir()) {
+        if (cache.has_size()) {
+          base::FilePath cache_dir = origin_path.AppendASCII(cache.cache_dir());
+          if (!GetFileInfo(cache_dir, &file_info) ||
+              index_last_modified <= file_info.last_modified) {

[jkarlin, 2016/11/11 18:24:56]

So we need to guarantee that the backend (SimpleCache) is done making changes
before we write the CacheStorage index. Can you write that in a comment
somewhere?

I do worry about races, but suspect that this approach is sufficient for quota
manager.

[cmumford, 2016/11/22 17:45:03]

Comment added to ScheduleWriteIndex.

+            // Index is older than this cache, so invalidate index entries that
+            // may change as a result of cache operations.
+            index.mutable_cache(i)->clear_size();
+            index_modified = true;

[jkarlin, 2016/11/11 18:24:56]

Why do we need to write this back to disk?

[cmumford, 2016/11/22 17:45:03]

Good point - clearing the size is sufficient. 

+          }
+        }
+      } else {
         // Find a new home for the cache.
         base::FilePath legacy_cache_path =
             origin_path.AppendASCII(HexedHash(cache.name()));
@@ -454,34 +575,24 @@ class CacheStorage::SimpleCacheLoader : public CacheStorage::CacheLoader {
           // If the move fails then the cache is in a bad state. Return an empty
           // index so that the CacheStorage can start fresh. The unreferenced
           // caches will be discarded later in initialization.
-          return kBadIndexState;
+          return content::CacheStorageIndex();
         }
 
         index.mutable_cache(i)->set_cache_dir(cache_dir);
-        index_is_dirty = true;
+        index.mutable_cache(i)->clear_size();
+        index_modified = true;
       }
     }
 
-    if (index_is_dirty) {
-      std::string new_body;
-      if (!index.SerializeToString(&new_body))
-        return kBadIndexState;
-      if (base::WriteFile(index_path, new_body.c_str(), new_body.size()) !=
-          base::checked_cast<int>(new_body.size()))
-        return kBadIndexState;
-      return new_body;
+    if (index_modified) {
+      if (!index.SerializeToString(&body))
+        return content::CacheStorageIndex();
+      if (base::WriteFile(index_path, body.c_str(), body.size()) !=
+          base::checked_cast<int>(body.size()))
+        return content::CacheStorageIndex();
     }
 
-    return body;
-  }
-
-  // Runs on cache_task_runner_
-  static std::string ReadAndMigrateIndexInPool(
-      const base::FilePath& index_path) {
-    std::string body;
-    base::ReadFileToString(index_path, &body);
-
-    return MigrateCachesIfNecessaryInPool(body, index_path);
+    return index;
   }
 
   const base::FilePath origin_path_;
@@ -504,6 +615,7 @@ CacheStorage::CacheStorage(
       memory_only_(memory_only),
       scheduler_(new CacheStorageScheduler(
           CacheStorageSchedulerClient::CLIENT_STORAGE)),
+      index_(base::MakeUnique<CacheStorageIndex>()),
       origin_path_(path),
       cache_task_runner_(cache_task_runner),
       quota_manager_proxy_(quota_manager_proxy),
@@ -570,7 +682,7 @@ void CacheStorage::DeleteCache(const std::string& cache_name,
                  cache_name, scheduler_->WrapCallbackToRunNext(callback)));
 }
 
-void CacheStorage::EnumerateCaches(const StringsCallback& callback) {
+void CacheStorage::EnumerateCaches(const CacheMetadataCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!initialized_)
@@ -646,6 +758,27 @@ void CacheStorage::Size(const CacheStorage::SizeCallback& callback) {
                  scheduler_->WrapCallbackToRunNext(callback)));
 }
 
+void CacheStorage::ScheduleWriteIndex() {
+  // TODO(cmumford): Write in a lazy fashion to minimize writes.
+  cache_loader_->WriteIndex(*index_, base::Bind(&DoNothingWithBool));
+}
+
+void CacheStorage::CacheSizeSet(const CacheStorageCache* cache,
+                                Whence whence,
+                                int64_t size) {
+  switch (whence) {
+    case Whence::SET:
+      index_->SetCacheSize(cache->cache_name(), size);
+      break;
+    case Whence::DELTA:
+      if (!size)
+        return;
+      index_->SetCacheSizeModified(cache->cache_name(), size);
+      break;
+  }
+  ScheduleWriteIndex();
+}
+
 void CacheStorage::StartAsyncOperationForTesting() {
   scheduler_->ScheduleOperation(base::Bind(&base::DoNothing));
 }
@@ -674,29 +807,27 @@ void CacheStorage::LazyInitImpl() {
   DCHECK(!initialized_);
   DCHECK(initializing_);
 
-  // 1. Get the list of cache names (async call)
+  // 1. Get the cache index (async call)
   // 2. For each cache name, load the cache (async call)
   // 3. Once each load is complete, update the map variables.
   // 4. Call the list of waiting callbacks.
 
-  std::unique_ptr<std::vector<std::string>> indexed_cache_names(
-      new std::vector<std::string>());
-
-  cache_loader_->LoadIndex(std::move(indexed_cache_names),
-                           base::Bind(&CacheStorage::LazyInitDidLoadIndex,
+  cache_loader_->LoadIndex(base::Bind(&CacheStorage::LazyInitDidLoadIndex,
                                       weak_factory_.GetWeakPtr()));
 }
 
 void CacheStorage::LazyInitDidLoadIndex(
-    std::unique_ptr<std::vector<std::string>> indexed_cache_names) {
+    std::unique_ptr<CacheStorageIndex> index) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  for (size_t i = 0u, max = indexed_cache_names->size(); i < max; ++i) {
-    cache_map_.insert(std::make_pair(indexed_cache_names->at(i),
+  DCHECK(cache_map_.empty());

[jkarlin, 2016/11/11 18:24:56]

Move this up a line with the other DCHECK

[cmumford, 2016/11/22 17:45:03]

Done.

+  for (const auto& cache_metadata : index->ordered_cache_metadata()) {
+    cache_map_.insert(std::make_pair(cache_metadata.name,
                                      std::unique_ptr<CacheStorageCache>()));
-    ordered_cache_names_.push_back(indexed_cache_names->at(i));
   }
 
+  index_.swap(index);
+
   initializing_ = false;
   initialized_ = true;
 
@@ -735,13 +866,12 @@ void CacheStorage::CreateCacheDidCreateCache(
   CacheStorageCache* cache_ptr = cache.get();
 
   cache_map_.insert(std::make_pair(cache_name, std::move(cache)));
-  ordered_cache_names_.push_back(cache_name);
+  index_->Insert(CacheMetadata(cache_name, cache_ptr->cache_size()));
 
   cache_loader_->WriteIndex(
-      ordered_cache_names_,
-      base::Bind(&CacheStorage::CreateCacheDidWriteIndex,
-                 weak_factory_.GetWeakPtr(), callback,
-                 base::Passed(CreateCacheHandle(cache_ptr))));
+      *index_, base::Bind(&CacheStorage::CreateCacheDidWriteIndex,
+                          weak_factory_.GetWeakPtr(), callback,
+                          base::Passed(CreateCacheHandle(cache_ptr))));
 
   cache_loader_->NotifyCacheCreated(cache_name, CreateCacheHandle(cache_ptr));
 }
@@ -772,29 +902,28 @@ void CacheStorage::DeleteCacheImpl(const std::string& cache_name,
     return;
   }
 
-  // Delete the name from ordered_cache_names_.
-  StringVector original_ordered_cache_names = ordered_cache_names_;
-  StringVector::iterator iter = std::find(
-      ordered_cache_names_.begin(), ordered_cache_names_.end(), cache_name);
-  DCHECK(iter != ordered_cache_names_.end());
-  ordered_cache_names_.erase(iter);
-
-  cache_loader_->WriteIndex(ordered_cache_names_,
-                            base::Bind(&CacheStorage::DeleteCacheDidWriteIndex,
-                                       weak_factory_.GetWeakPtr(), cache_name,
-                                       original_ordered_cache_names, callback));
+  // Save the current index so that it can be restored if the write fails.
+  // TODO(cmumford): Avoid creating a copy, and make CacheStorageCacheIndex
+  // DISALLOW_COPY_AND_ASSIGN.
+  auto prior_index = base::MakeUnique<CacheStorageIndex>(*index_);
+  index_->Delete(cache_name);
+
+  cache_loader_->WriteIndex(
+      *index_, base::Bind(&CacheStorage::DeleteCacheDidWriteIndex,
+                          weak_factory_.GetWeakPtr(), cache_name,
+                          base::Passed(std::move(prior_index)), callback));
 }
 
 void CacheStorage::DeleteCacheDidWriteIndex(
     const std::string& cache_name,
-    const StringVector& original_ordered_cache_names,
+    std::unique_ptr<CacheStorageIndex> index_before_delete,
     const BoolAndErrorCallback& callback,
     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;
+    index_.swap(index_before_delete);
     callback.Run(false, CACHE_STORAGE_ERROR_STORAGE);
     return;
   }
@@ -820,6 +949,7 @@ void CacheStorage::DeleteCacheDidWriteIndex(
 void CacheStorage::DeleteCacheFinalize(
     std::unique_ptr<CacheStorageCache> doomed_cache) {
   CacheStorageCache* cache = doomed_cache.get();
+  cache->SetObserver(nullptr);
   cache->Size(base::Bind(&CacheStorage::DeleteCacheDidGetSize,
                          weak_factory_.GetWeakPtr(),
                          base::Passed(std::move(doomed_cache))));
@@ -835,8 +965,8 @@ void CacheStorage::DeleteCacheDidGetSize(
   cache_loader_->CleanUpDeletedCache(cache.get());
 }
 
-void CacheStorage::EnumerateCachesImpl(const StringsCallback& callback) {
-  callback.Run(ordered_cache_names_);
+void CacheStorage::EnumerateCachesImpl(const CacheMetadataCallback& callback) {
+  callback.Run(index_->ordered_cache_metadata());
 }
 
 void CacheStorage::MatchCacheImpl(
@@ -877,17 +1007,18 @@ void CacheStorage::MatchAllCachesImpl(
     const CacheStorageCacheQueryParams& match_params,
     const CacheStorageCache::ResponseCallback& callback) {
   std::vector<CacheMatchResponse>* match_responses =
-      new std::vector<CacheMatchResponse>(ordered_cache_names_.size());
+      new std::vector<CacheMatchResponse>(index_->num_entries());
 
   base::Closure barrier_closure = base::BarrierClosure(
-      ordered_cache_names_.size(),
+      index_->num_entries(),
       base::Bind(&CacheStorage::MatchAllCachesDidMatchAll,
                  weak_factory_.GetWeakPtr(),
                  base::Passed(base::WrapUnique(match_responses)), callback));
 
-  for (size_t i = 0, max = ordered_cache_names_.size(); i < max; ++i) {
+  size_t idx = 0, max = index_->num_entries();

[jkarlin, 2016/11/11 18:24:56]

What's this about?  index_->num_entries() ==
index_->ordered_cache_metadata().size() right? DCHECK_EQ perhaps?

[cmumford, 2016/11/22 17:45:03]

I'm assuming you were asking about using |max| and the early break if idx > max?

I was just preserving the initial logic. I wasn't sure of the current
implementation is using |max| to avoid calling size(), or if
ordered_cache_names_ could grow during this for-loop - which I wouldn't expect,
but I wanted to be cautious and stick with the initial logic.

I'll remove the idx>max check, and let me know if you want it back (or if I've
misunderstood your question).

+  for (const auto& cache_metadata : index_->ordered_cache_metadata()) {
     std::unique_ptr<CacheStorageCacheHandle> cache_handle =
-        GetLoadedCache(ordered_cache_names_[i]);
+        GetLoadedCache(cache_metadata.name);
     DCHECK(cache_handle);
 
     CacheStorageCache* cache_ptr = cache_handle->value();
@@ -896,7 +1027,9 @@ void CacheStorage::MatchAllCachesImpl(
                      base::Bind(&CacheStorage::MatchAllCachesDidMatch,
                                 weak_factory_.GetWeakPtr(),
                                 base::Passed(std::move(cache_handle)),
-                                &match_responses->at(i), barrier_closure));
+                                &match_responses->at(idx), barrier_closure));
+    if (++idx > max)
+      break;
   }
 }
 
@@ -984,8 +1117,9 @@ std::unique_ptr<CacheStorageCacheHandle> CacheStorage::GetLoadedCache(
   CacheStorageCache* cache = map_iter->second.get();
 
   if (!cache) {
-    std::unique_ptr<CacheStorageCache> new_cache =
-        cache_loader_->CreateCache(cache_name);
+    DCHECK(initialized_);

[jkarlin, 2016/11/11 18:24:56]

This is a duplicate of a DCHECK a few lines up

[cmumford, 2016/11/22 17:45:03]

Done.

+    std::unique_ptr<CacheStorageCache> new_cache = cache_loader_->CreateCache(
+        cache_name, index_->GetCacheSize(cache_name));
     CacheStorageCache* cache_ptr = new_cache.get();
     map_iter->second = std::move(new_cache);
 
@@ -995,6 +1129,27 @@ std::unique_ptr<CacheStorageCacheHandle> CacheStorage::GetLoadedCache(
   return CreateCacheHandle(cache);
 }
 
+void CacheStorage::SizeRetrievedFromCache(
+    std::unique_ptr<CacheStorageCacheHandle> cache_handle,
+    const base::Closure& closure,
+    int64_t* accumulator,
+    int64_t size) {
+  index_->SetCacheSize(cache_handle->value()->cache_name(), size);
+  *accumulator += size;
+  closure.Run();
+}
+
+void CacheStorage::CloseAllCaches() {

[jkarlin, 2016/11/11 18:24:56]

Who calls this?

[cmumford, 2016/11/22 17:45:03]

Woops - I called this in CacheStorage::GetSizeThenCloseAllCachesImpl, but
returned that to it's initial implementation, and forgot to delete this uncalled
function.

+  for (const CacheMetadata& cache_metadata : index_->ordered_cache_metadata()) {
+    auto map_iter = cache_map_.find(cache_metadata.name);

[jkarlin, 2016/11/11 18:24:56]

Why iterate over ordered_cache_metadata and then look it up in cache_map_? Why
not just iterate over cache_map_ to begin with?

+    if (map_iter != cache_map_.end()) {
+      CacheStorageCache* cache = map_iter->second.get();
+      if (cache)
+        cache->Close(base::Bind(&base::DoNothing));
+    }
+  }
+}
+
 void CacheStorage::GetSizeThenCloseAllCachesImpl(const SizeCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(initialized_);
@@ -1003,15 +1158,15 @@ void CacheStorage::GetSizeThenCloseAllCachesImpl(const SizeCallback& callback) {
   int64_t* accumulator_ptr = accumulator.get();
 
   base::Closure barrier_closure = base::BarrierClosure(
-      ordered_cache_names_.size(),
+      index_->num_entries(),
       base::Bind(&SizeRetrievedFromAllCaches,
                  base::Passed(std::move(accumulator)), callback));
 
-  for (const std::string& cache_name : ordered_cache_names_) {
-    std::unique_ptr<CacheStorageCacheHandle> cache_handle =
-        GetLoadedCache(cache_name);
+  for (const CacheMetadata& cache_metadata : index_->ordered_cache_metadata()) {
+    auto cache_handle = GetLoadedCache(cache_metadata.name);
     CacheStorageCache* cache = cache_handle->value();
-    cache->GetSizeThenClose(base::Bind(&SizeRetrievedFromCache,
+    cache->GetSizeThenClose(base::Bind(&CacheStorage::SizeRetrievedFromCache,
+                                       weak_factory_.GetWeakPtr(),
                                        base::Passed(std::move(cache_handle)),
                                        barrier_closure, accumulator_ptr));
   }
@@ -1021,19 +1176,31 @@ void CacheStorage::SizeImpl(const SizeCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(initialized_);
 
+  if (index_->GetStorageSize() != kSizeUnknown) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, index_->GetStorageSize()));
+    return;
+  }
+
   std::unique_ptr<int64_t> accumulator(new int64_t(0));
   int64_t* accumulator_ptr = accumulator.get();
 
   base::Closure barrier_closure = base::BarrierClosure(
-      ordered_cache_names_.size(),
+      index_->num_entries(),
       base::Bind(&SizeRetrievedFromAllCaches,
                  base::Passed(std::move(accumulator)), callback));
 
-  for (const std::string& cache_name : ordered_cache_names_) {
+  for (const CacheMetadata& cache_metadata : index_->ordered_cache_metadata()) {
+    if (cache_metadata.size != CacheStorage::kSizeUnknown) {
+      *accumulator_ptr += cache_metadata.size;
+      barrier_closure.Run();

[jkarlin, 2016/11/11 18:24:56]

This can cause trouble. If all of the indexes have known size then running the
closure could invoke SizeRetrievedFromAllCaches, which invokes the callback
(which could delete this for all we know). 

[cmumford, 2016/11/22 17:45:03]

Agreed, but SizeRetrievedFromAllCaches uses PostTask instead of Run. That should
be safe right? Or am I mistaken?

[jkarlin, 2016/11/23 16:02:17]

Ah, I was looking at SizeRetrievedFromCache which doesn't PostTask, but you're
right this does. Okay, all good.

+      continue;
+    }
     std::unique_ptr<CacheStorageCacheHandle> cache_handle =
-        GetLoadedCache(cache_name);
+        GetLoadedCache(cache_metadata.name);
     CacheStorageCache* cache = cache_handle->value();
-    cache->Size(base::Bind(&SizeRetrievedFromCache,
+    cache->Size(base::Bind(&CacheStorage::SizeRetrievedFromCache,
+                           weak_factory_.GetWeakPtr(),
                            base::Passed(std::move(cache_handle)),
                            barrier_closure, accumulator_ptr));
   }