ServiceWorker: Schedule DeleteAndStartOver when failing to store resource ids

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 8f0bba109dae3c7d4fdea1490d3f7c09c6e9eebb..b1468442088b086147cee4e82a70e142b0144560 100644
--- a/content/browser/service_worker/service_worker_storage.cc
+++ b/content/browser/service_worker/service_worker_storage.cc
@@ -536,30 +536,35 @@ ServiceWorkerStorage::CreateResponseMetadataWriter(int64 response_id) {
       new ServiceWorkerResponseMetadataWriter(response_id, disk_cache()));
 }
 
-void ServiceWorkerStorage::StoreUncommittedResponseId(int64 id) {
-  DCHECK_NE(kInvalidServiceWorkerResponseId, id);
+void ServiceWorkerStorage::StoreUncommittedResourceId(int64 resource_id) {
+  DCHECK_NE(kInvalidServiceWorkerResponseId, resource_id);

[falken, 2015/10/15 05:03:40]

kInvalidServiceWorkerResourceId?

probably we want a bigger code cleanup that just deletes one

[nhiroki, 2015/10/16 09:15:40]

Yeah, I'll make a follow-up patch to deprecate kInvalidSWResponseId.

   DCHECK_EQ(INITIALIZED, state_);
 
   if (!has_checked_for_stale_resources_)
     DeleteStaleResources();
 
-  database_task_manager_->GetTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(base::IgnoreResult(
-          &ServiceWorkerDatabase::WriteUncommittedResourceIds),
-          base::Unretained(database_.get()),
-          std::set<int64>(&id, &id + 1)));
+  PostTaskAndReplyWithResult(
+      database_task_manager_->GetTaskRunner(), FROM_HERE,
+      base::Bind(&ServiceWorkerDatabase::WriteUncommittedResourceIds,
+                 base::Unretained(database_.get()),
+                 std::set<int64>(&resource_id, &resource_id + 1)),
+      base::Bind(&ServiceWorkerStorage::DidWriteUncommittedResourceIds,
+                 weak_factory_.GetWeakPtr()));
 }
 
-void ServiceWorkerStorage::DoomUncommittedResponse(int64 id) {
-  DCHECK_NE(kInvalidServiceWorkerResponseId, id);
-  database_task_manager_->GetTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(base::IgnoreResult(
-          &ServiceWorkerDatabase::PurgeUncommittedResourceIds),
-          base::Unretained(database_.get()),
-          std::set<int64>(&id, &id + 1)));
-  StartPurgingResources(std::vector<int64>(1, id));
+void ServiceWorkerStorage::DoomUncommittedResource(int64 resource_id) {
+  DCHECK_NE(kInvalidServiceWorkerResponseId, resource_id);
+  DoomUncommittedResources(std::set<int64>(&resource_id, &resource_id + 1));
+}
+
+void ServiceWorkerStorage::DoomUncommittedResources(
+    const std::set<int64>& resource_ids) {
+  PostTaskAndReplyWithResult(
+      database_task_manager_->GetTaskRunner(), FROM_HERE,
+      base::Bind(&ServiceWorkerDatabase::PurgeUncommittedResourceIds,
+                 base::Unretained(database_.get()), resource_ids),
+      base::Bind(&ServiceWorkerStorage::DidPurgeUncommittedResourceIds,
+                 weak_factory_.GetWeakPtr(), resource_ids));
 }
 
 void ServiceWorkerStorage::StoreUserData(
@@ -729,16 +734,10 @@ void ServiceWorkerStorage::NotifyDoneInstallingRegistration(
     ResourceList resources;
     version->script_cache_map()->GetResources(&resources);
 
-    std::set<int64> ids;
+    std::set<int64> resource_ids;
     for (const auto& resource : resources)
-      ids.insert(resource.resource_id);
-
-    database_task_manager_->GetTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(base::IgnoreResult(
-            &ServiceWorkerDatabase::PurgeUncommittedResourceIds),
-            base::Unretained(database_.get()),
-            ids));
+      resource_ids.insert(resource.resource_id);
+    DoomUncommittedResources(resource_ids);
   }
 }
 
@@ -1152,6 +1151,22 @@ void ServiceWorkerStorage::DidDeleteRegistration(
     StartPurgingResources(newly_purgeable_resources);
 }
 
+void ServiceWorkerStorage::DidWriteUncommittedResourceIds(
+    ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK)

[falken, 2015/10/15 05:03:40]

Do we have/want UMA for delete and start over frequency and the failure that
triggered it?

[nhiroki, 2015/10/16 09:15:40]

What does "frequency" means? The ratio of DeleteAndStartOver to all operations?
If so, we might be able to calculate the ratio using results of lower level
database/diskcache operations possibly to trigger DeleteAndStartOver (ie.
ServiceWorker.Database.* and ServiceWorker.Storage.* UMAs) like this:

  frequency = (all failures on database and DiskCache) / (all results on
database and diskcache)

[falken, 2015/10/19 04:09:35]

Yea... I didn't have something specific in mind, just wanted to know if we have
UMA that gives an idea of how often this failure happens.

I guess we have ServiceWorker.Storage.DeleteAndStartOverResult so can monitor
the raw count of that, or maybe the ratio of user count of
DeleteAndStartOverResult to user count of OpenResult? Would it help to  have
something like DeleteAndStartOverReason that's logged whenever it's called, to
help debugging if someday we discover a rise in DeleteAndStartOver calls?

Anyway, maybe this discussion is out of the scope of this particular CL.

[nhiroki, 2015/10/19 09:50:42]

We'd like to monitor two things: (1) how many users encounter DASO, and (2) how
many times DASO happen on the same user. The ratio of UC of DASO to UC of
OpenResult would be helpful for (1). The ratio of the raw count of DASO to UC of
DASO would be helpful for (2).
Recording failure reasons could be helpful to know which storage operation
(database access pattern) tends to fail and contains coding error... I'll
consider adding them.

+    ScheduleDeleteAndStartOver();
+}
+
+void ServiceWorkerStorage::DidPurgeUncommittedResourceIds(
+    const std::set<int64>& resource_ids,
+    ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    ScheduleDeleteAndStartOver();
+    return;
+  }
+  StartPurgingResources(resource_ids);
+}
+
 void ServiceWorkerStorage::DidStoreUserData(
     const StatusCallback& callback,
     ServiceWorkerDatabase::Status status) {
@@ -1388,10 +1403,18 @@ void ServiceWorkerStorage::DeleteOldDiskCache() {
 }
 
 void ServiceWorkerStorage::StartPurgingResources(
-    const std::vector<int64>& ids) {
+    const std::set<int64>& resource_ids) {
+  DCHECK(has_checked_for_stale_resources_);
+  for (int64 resource_id : resource_ids)
+    purgeable_resource_ids_.push_back(resource_id);
+  ContinuePurgingResources();
+}
+
+void ServiceWorkerStorage::StartPurgingResources(
+    const std::vector<int64>& resource_ids) {
   DCHECK(has_checked_for_stale_resources_);
-  for (const auto& id : ids)
-    purgeable_resource_ids_.push_back(id);
+  for (int64 resource_id : resource_ids)
+    purgeable_resource_ids_.push_back(resource_id);
   ContinuePurgingResources();
 }