Guarantee ServiceWorkerCacheStorage::LazyInit calls its callback

content/browser/service_worker/service_worker_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/service_worker/service_worker_cache_storage.h"
 
 #include <string>
 
 #include "base/file_util.h"
 #include "base/files/memory_mapped_file.h"
@@ skipping 389 matching lines @@
       weak_factory_(this) {
   if (memory_only)
     cache_loader_ = new MemoryLoader(
         cache_task_runner_.get(), request_context, blob_context);
   else
     cache_loader_ = new SimpleCacheLoader(
         origin_path_, cache_task_runner_.get(), request_context, blob_context);
 }
 
 ServiceWorkerCacheStorage::~ServiceWorkerCacheStorage() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // If init is running call any pending callbacks. Init will cease (due to
+  // checking the WeakPtr) once this object is deleted.
+  for (std::vector<base::Closure>::iterator it = init_callbacks_.begin();
+       it != init_callbacks_.end();
+       ++it) {
+    it->Run();

[michaeln, 2014/08/27 22:00:54]

This doesn't look quite right. The set of possible init_callbacks are these
intsance methods: CreateCache(), GetCache(), EnumerateCaches(). Looks like those
methods will append another callback to themselve since the class is still
!initialized_ at this time, and this loop may never terminate.

Maybe i'm missing something?

Invoking client callbacks from the dtor is dicey since the callback may invoke
another public method of this class.

+  }
+
   STLDeleteContainerPairSecondPointers(cache_map_.begin(), cache_map_.end());
 }
 
 void ServiceWorkerCacheStorage::CreateCache(
     const std::string& cache_name,
     const CacheAndErrorCallback& callback) {
   if (!initialized_) {
     LazyInit(base::Bind(&ServiceWorkerCacheStorage::CreateCache,
                         weak_factory_.GetWeakPtr(),
                         cache_name,
@@ skipping 144 matching lines @@
   if (error != ServiceWorkerCache::ErrorTypeOK || !cache) {
     // TODO(jkarlin): This should delete the directory and try again in case
     // the cache is simply corrupt.
     callback.Run(kInvalidCacheID, CACHE_STORAGE_ERROR_STORAGE);
     return;
   }
 
   callback.Run(cache_id, CACHE_STORAGE_ERROR_NO_ERROR);
 }
 
-// Init is run lazily so that it is called on the proper MessageLoop.
 void ServiceWorkerCacheStorage::LazyInit(const base::Closure& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!initialized_);
 
   init_callbacks_.push_back(callback);
 
   // If this isn't the first call to LazyInit then return as the initialization
   // has already started.
   if (init_callbacks_.size() > 1u)
     return;
 
   // 1. Get the list of cache names (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.
 
   scoped_ptr<std::vector<std::string> > indexed_cache_names(
       new std::vector<std::string>());
 
   cache_loader_->LoadIndex(
       indexed_cache_names.Pass(),
       base::Bind(&ServiceWorkerCacheStorage::LazyInitDidLoadIndex,
-                 weak_factory_.GetWeakPtr(),
-                 callback));
+                 callback,
+                 weak_factory_.GetWeakPtr()));
 }
 
+// static
 void ServiceWorkerCacheStorage::LazyInitDidLoadIndex(
     const base::Closure& callback,
+    base::WeakPtr<ServiceWorkerCacheStorage> storage,
     scoped_ptr<std::vector<std::string> > indexed_cache_names) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  if (indexed_cache_names->empty()) {
-    LazyInitDone();
-    return;
-  }
+  if (!storage)
+    return;  // The callbacks were already run by the destructor.
+
+  if (indexed_cache_names->empty())
+    return LazyInitDone(storage);
 
   std::vector<std::string>::const_iterator iter = indexed_cache_names->begin();
   std::vector<std::string>::const_iterator iter_next = iter + 1;
 
-  cache_loader_->LoadCache(
+  storage->cache_loader_->LoadCache(
       *iter,
       base::Bind(&ServiceWorkerCacheStorage::LazyInitIterateAndLoadCacheName,
-                 weak_factory_.GetWeakPtr(),
                  callback,
+                 storage,
                  base::Passed(indexed_cache_names.Pass()),
                  iter_next,
                  *iter));
 }
 
+// static
 void ServiceWorkerCacheStorage::LazyInitIterateAndLoadCacheName(
     const base::Closure& callback,
+    base::WeakPtr<ServiceWorkerCacheStorage> storage,
     scoped_ptr<std::vector<std::string> > indexed_cache_names,
     const std::vector<std::string>::const_iterator& iter,
     const std::string& cache_name,
     scoped_ptr<ServiceWorkerCache> cache) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  if (!storage)
+    return;  // The callbacks were already run by the destructor.
+
   if (cache)
-    AddCacheToMaps(cache_name, cache.Pass());
+    storage->AddCacheToMaps(cache_name, cache.Pass());
 
   if (iter == indexed_cache_names->end()) {
-    LazyInitDone();
+    LazyInitDone(storage);
     return;
   }
 
   std::vector<std::string>::const_iterator iter_next = iter + 1;
-  cache_loader_->LoadCache(
+  storage->cache_loader_->LoadCache(
       *iter,
       base::Bind(&ServiceWorkerCacheStorage::LazyInitIterateAndLoadCacheName,
-                 weak_factory_.GetWeakPtr(),
                  callback,
+                 storage,
                  base::Passed(indexed_cache_names.Pass()),
                  iter_next,
                  *iter));
 }
 
-void ServiceWorkerCacheStorage::LazyInitDone() {
-  initialized_ = true;
-  for (std::vector<base::Closure>::iterator it = init_callbacks_.begin();
-       it != init_callbacks_.end();
+// static
+void ServiceWorkerCacheStorage::LazyInitDone(
+    base::WeakPtr<ServiceWorkerCacheStorage> storage) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!storage)
+    return;  // The callbacks were already run by the destructor.
+
+  storage->initialized_ = true;
+  for (std::vector<base::Closure>::iterator it =
+           storage->init_callbacks_.begin();
+       it != storage->init_callbacks_.end();
        ++it) {
     it->Run();
   }
-  init_callbacks_.clear();
+  storage->init_callbacks_.clear();
 }
 
 ServiceWorkerCacheStorage::CacheContext*
 ServiceWorkerCacheStorage::AddCacheToMaps(
     const std::string& cache_name,
     scoped_ptr<ServiceWorkerCache> cache) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   CacheID id = next_cache_id_++;
   CacheContext* cache_context = new CacheContext(cache_name, id, cache.Pass());
@@ skipping 71 matching lines @@
   NameMap::const_iterator name_iter = name_map_.find(cache_name);
   if (name_iter == name_map_.end())
     return NULL;
 
   CacheMap::const_iterator map_iter = cache_map_.find(name_iter->second);
   DCHECK(map_iter != cache_map_.end());
   return map_iter->second;
 }
 
 }  // namespace content