Guarantee ServiceWorkerCacheStorage::LazyInit calls its callback

content/browser/service_worker/service_worker_cache_storage.cc

Index: content/browser/service_worker/service_worker_cache_storage.cc
diff --git a/content/browser/service_worker/service_worker_cache_storage.cc b/content/browser/service_worker/service_worker_cache_storage.cc
index 74d476c4292e39058e3875abce0f6b4e2980a21b..734174eb73444c094a7668d57ff5a1e167727000 100644
--- a/content/browser/service_worker/service_worker_cache_storage.cc
+++ b/content/browser/service_worker/service_worker_cache_storage.cc
@@ -407,6 +407,16 @@ ServiceWorkerCacheStorage::ServiceWorkerCacheStorage(
 }
 
 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());
 }
 
@@ -571,7 +581,6 @@ void ServiceWorkerCacheStorage::DidCreateBackend(
   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_);
@@ -594,68 +603,83 @@ void ServiceWorkerCacheStorage::LazyInit(const base::Closure& callback) {
   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*