Service Worker: Delay stale resource cleanup

content/browser/service_worker/service_worker_storage.cc

Index: content/browser/service_worker/service_worker_storage.cc
diff --git a/content/browser/service_worker/service_worker_storage.cc b/content/browser/service_worker/service_worker_storage.cc
index 05e65323ca88818b8e574aa08c3cc9ff314d22c1..1e09a127b0dd356a5abb3126eb44d5d4f319dc74 100644
--- a/content/browser/service_worker/service_worker_storage.cc
+++ b/content/browser/service_worker/service_worker_storage.cc
@@ -129,7 +129,7 @@ void ServiceWorkerStorage::FindRegistrationForDocument(
     }
     return;
   }
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
 
   // See if there are any stored registrations for the origin.
   if (!ContainsKey(registered_origins_, document_url.GetOrigin())) {
@@ -167,7 +167,7 @@ void ServiceWorkerStorage::FindRegistrationForPattern(
     }
     return;
   }
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
 
   // See if there are any stored registrations for the origin.
   if (!ContainsKey(registered_origins_, scope.GetOrigin())) {
@@ -206,7 +206,7 @@ void ServiceWorkerStorage::FindRegistrationForId(
     }
     return;
   }
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
 
   // See if there are any stored registrations for the origin.
   if (!ContainsKey(registered_origins_, origin)) {
@@ -249,7 +249,7 @@ void ServiceWorkerStorage::GetAllRegistrations(
     }
     return;
   }
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
 
   RegistrationList* registrations = new RegistrationList;
   PostTaskAndReplyWithResult(
@@ -271,8 +271,8 @@ void ServiceWorkerStorage::StoreRegistration(
   DCHECK(registration);
   DCHECK(version);
 
-  DCHECK(state_ == INITIALIZED || state_ == DISABLED);
-  if (state_ != INITIALIZED || !context_) {
+  DCHECK(IsReady() || IsDisabled()) << state_;
+  if (IsDisabled() || !context_) {
     RunSoon(FROM_HERE, base::Bind(callback, SERVICE_WORKER_ERROR_FAILED));
     return;
   }
@@ -289,6 +289,9 @@ void ServiceWorkerStorage::StoreRegistration(
   ResourceList resources;
   version->script_cache_map()->GetResources(&resources);
 
+  if (state_ == HAS_STALE_RESOURCES)
+    DeleteStaleResources();
+
   database_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&WriteRegistrationInDB,
@@ -305,8 +308,8 @@ void ServiceWorkerStorage::UpdateToActiveState(
     const StatusCallback& callback) {
   DCHECK(registration);
 
-  DCHECK(state_ == INITIALIZED || state_ == DISABLED);
-  if (state_ != INITIALIZED || !context_) {
+  DCHECK(IsReady() || IsDisabled()) << state_;
+  if (IsDisabled() || !context_) {
     RunSoon(FROM_HERE, base::Bind(callback, SERVICE_WORKER_ERROR_FAILED));
     return;
   }
@@ -327,12 +330,15 @@ void ServiceWorkerStorage::DeleteRegistration(
     int64 registration_id,
     const GURL& origin,
     const StatusCallback& callback) {
-  DCHECK(state_ == INITIALIZED || state_ == DISABLED);
-  if (state_ != INITIALIZED || !context_) {
+  DCHECK(IsReady() || IsDisabled());
+  if (IsDisabled() || !context_) {
     RunSoon(FROM_HERE, base::Bind(callback, SERVICE_WORKER_ERROR_FAILED));
     return;
   }
 
+  if (state_ == HAS_STALE_RESOURCES)
+    DeleteStaleResources();
+
   database_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DeleteRegistrationFromDB,
@@ -348,20 +354,19 @@ void ServiceWorkerStorage::DeleteRegistration(
   // thereafter.
 }
 
-scoped_ptr<ServiceWorkerResponseReader>
-ServiceWorkerStorage::CreateResponseReader(int64 response_id) {
-  return make_scoped_ptr(
-      new ServiceWorkerResponseReader(response_id, disk_cache()));
-}
-
 scoped_ptr<ServiceWorkerResponseWriter>
 ServiceWorkerStorage::CreateResponseWriter(int64 response_id) {
   return make_scoped_ptr(
       new ServiceWorkerResponseWriter(response_id, disk_cache()));
 }
 
-void ServiceWorkerStorage::StoreUncommittedReponseId(int64 id) {
+void ServiceWorkerStorage::StoreUncommittedResponseId(int64 id) {
   DCHECK_NE(kInvalidServiceWorkerResponseId, id);
+  DCHECK(IsReady()) << state_;
+
+  if (state_ == HAS_STALE_RESOURCES)
+    DeleteStaleResources();
+
   database_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(base::IgnoreResult(
@@ -397,21 +402,21 @@ void ServiceWorkerStorage::DeleteAndStartOver(const StatusCallback& callback) {
 int64 ServiceWorkerStorage::NewRegistrationId() {
   if (state_ == DISABLED)
     return kInvalidServiceWorkerRegistrationId;
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
   return next_registration_id_++;
 }
 
 int64 ServiceWorkerStorage::NewVersionId() {
   if (state_ == DISABLED)
     return kInvalidServiceWorkerVersionId;
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
   return next_version_id_++;
 }
 
 int64 ServiceWorkerStorage::NewResourceId() {
   if (state_ == DISABLED)
     return kInvalidServiceWorkerResourceId;
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(IsReady()) << state_;
   return next_resource_id_++;
 }
 
@@ -486,11 +491,17 @@ base::FilePath ServiceWorkerStorage::GetDiskCachePath() {
   return path_.Append(kServiceWorkerDirectory).Append(kDiskCacheName);
 }
 
+bool ServiceWorkerStorage::IsReady() {
+  return state_ == HAS_STALE_RESOURCES || state_ == INITIALIZED;
+}
+
 bool ServiceWorkerStorage::LazyInitialize(const base::Closure& callback) {
   if (!context_)
     return false;
 
   switch (state_) {
+    case HAS_STALE_RESOURCES:
+      return true;
     case INITIALIZED:
       return true;
     case DISABLED:
@@ -525,10 +536,7 @@ void ServiceWorkerStorage::DidReadInitialData(
     next_version_id_ = data->next_version_id;
     next_resource_id_ = data->next_resource_id;
     registered_origins_.swap(data->origins);
-    state_ = INITIALIZED;
-    StartPurgingResources(
-        std::vector<int64>(data->purgeable_resource_ids.begin(),
-                           data->purgeable_resource_ids.end()));
+    state_ = HAS_STALE_RESOURCES;

[michaeln, 2014/07/09 02:36:11]

we don't really know if we have stale resources yet, we know we have to check,
is that right?

[falken, 2014/07/09 04:31:14]

correct, not the best name

   } else {
     // TODO(nhiroki): Stringify |status| using StatusToString() defined in
     // service_worker_database.cc.
@@ -863,6 +871,7 @@ void ServiceWorkerStorage::SchedulePurgeResources(
 
 void ServiceWorkerStorage::StartPurgingResources(
     const std::vector<int64>& ids) {
+  DCHECK_EQ(INITIALIZED, state_);
   for (size_t i = 0; i < ids.size(); ++i)
     purgeable_resource_ids_.push_back(ids[i]);
   ContinuePurgingResources();
@@ -912,6 +921,64 @@ void ServiceWorkerStorage::OnResourcePurged(int64 id, int rv) {
   ContinuePurgingResources();
 }
 
+void ServiceWorkerStorage::DeleteStaleResources() {
+  DCHECK_EQ(HAS_STALE_RESOURCES, state_);
+  state_ = INITIALIZED;
+  database_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&ServiceWorkerStorage::CollectStaleResourcesFromDB,
+                 database_.get(),
+                 base::MessageLoopProxy::current(),
+                 base::Bind(&ServiceWorkerStorage::DidCollectStaleResources,
+                            weak_factory_.GetWeakPtr())));
+}
+
+void ServiceWorkerStorage::DidCollectStaleResources(
+    const std::vector<int64>& stale_resource_ids,
+    ServiceWorkerDatabase::Status status) {
+  DCHECK_EQ(ServiceWorkerDatabase::STATUS_OK, status);
+  if (status != ServiceWorkerDatabase::STATUS_OK)
+    return;
+  StartPurgingResources(stale_resource_ids);
+}
+
+void ServiceWorkerStorage::CollectStaleResourcesFromDB(
+    ServiceWorkerDatabase* database,
+    scoped_refptr<base::SequencedTaskRunner> original_task_runner,
+    const GetResourcesCallback& callback) {
+  std::set<int64> ids;
+  ServiceWorkerDatabase::Status status =
+      database->GetUncommittedResourceIds(&ids);
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    original_task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(
+            callback, std::vector<int64>(ids.begin(), ids.end()), status));
+    return;
+  }
+
+  status = database->PurgeUncommittedResourceIds(ids);
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    original_task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(
+            callback, std::vector<int64>(ids.begin(), ids.end()), status));
+    return;
+  }
+
+  ids.clear();
+  status = database->GetPurgeableResourceIds(&ids);
+  original_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(callback, std::vector<int64>(ids.begin(), ids.end()), status));
+}
+
+scoped_ptr<ServiceWorkerResponseReader>
+ServiceWorkerStorage::CreateResponseReader(int64 response_id) {

[michaeln, 2014/07/09 02:36:11]

i think this accidentally got move far away from its sibling
CreateResponseWriter method?

[falken, 2014/07/09 04:31:14]

oops yes, done

+  return make_scoped_ptr(
+      new ServiceWorkerResponseReader(response_id, disk_cache()));
+}
+
 void ServiceWorkerStorage::ReadInitialDataFromDB(
     ServiceWorkerDatabase* database,
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
@@ -931,14 +998,6 @@ void ServiceWorkerStorage::ReadInitialDataFromDB(
   }
 
   status = database->GetOriginsWithRegistrations(&data->origins);
-  if (status != ServiceWorkerDatabase::STATUS_OK) {
-    original_task_runner->PostTask(
-        FROM_HERE, base::Bind(callback, base::Owned(data.release()), status));
-    return;
-  }
-
-  // TODO: Also purge uncommitted resources.
-  status = database->GetPurgeableResourceIds(&data->purgeable_resource_ids);
   original_task_runner->PostTask(
       FROM_HERE, base::Bind(callback, base::Owned(data.release()), status));
 }