ServiceWorker: Schedule DeleteAndStartOver when failing to store resource ids

content/browser/service_worker/service_worker_storage.cc

 // Copyright 2013 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_storage.h"
 
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/sequenced_task_runner.h"
@@ skipping 518 matching lines @@
   return make_scoped_ptr(
       new ServiceWorkerResponseWriter(response_id, disk_cache()));
 }
 
 scoped_ptr<ServiceWorkerResponseMetadataWriter>
 ServiceWorkerStorage::CreateResponseMetadataWriter(int64 response_id) {
   return make_scoped_ptr(
       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(
     int64 registration_id,
     const GURL& origin,
     const std::string& key,
     const std::string& data,
     const StatusCallback& callback) {
   DCHECK(state_ == INITIALIZED || state_ == DISABLED) << state_;
   if (IsDisabled()) {
@@ skipping 149 matching lines @@
 
 void ServiceWorkerStorage::NotifyDoneInstallingRegistration(
       ServiceWorkerRegistration* registration,
       ServiceWorkerVersion* version,
       ServiceWorkerStatusCode status) {
   installing_registrations_.erase(registration->id());
   if (status != SERVICE_WORKER_OK && version) {
     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);
   }
 }
 
 void ServiceWorkerStorage::NotifyUninstallingRegistration(
     ServiceWorkerRegistration* registration) {
   DCHECK(uninstalling_registrations_.find(registration->id()) ==
          uninstalling_registrations_.end());
   uninstalling_registrations_[registration->id()] = registration;
 }
 
@@ skipping 393 matching lines @@
         -deleted_version.resources_total_size_bytes);
   }
   if (origin_is_deletable)
     registered_origins_.erase(params.origin);
   params.callback.Run(SERVICE_WORKER_OK);
 
   if (!context_->GetLiveVersion(deleted_version.version_id))
     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) {
   // |status| can be NOT_FOUND when the associated registration did not exist in
   // the database. In the case, we don't have to schedule the corruption
   // recovery.
   if (status != ServiceWorkerDatabase::STATUS_OK &&
       status != ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND) {
     ScheduleDeleteAndStartOver();
   }
@@ skipping 216 matching lines @@
 void ServiceWorkerStorage::DeleteOldDiskCache() {
   DCHECK(state_ == INITIALIZED || state_ == DISABLED) << state_;
   if (IsDisabled())
     return;
   database_task_manager_->GetTaskRunner()->PostTask(
       FROM_HERE, base::Bind(&ServiceWorkerStorage::DeleteOldDiskCacheInDB,
                             database_.get(), GetOldDiskCachePath()));
 }
 
 void ServiceWorkerStorage::StartPurgingResources(
-    const std::vector<int64>& ids) {
+    const std::set<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();
 }
 
+void ServiceWorkerStorage::StartPurgingResources(
+    const std::vector<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 ResourceList& resources) {
   DCHECK(has_checked_for_stale_resources_);
   for (const auto& resource : resources)
     purgeable_resource_ids_.push_back(resource.resource_id);
   ContinuePurgingResources();
 }
 
 void ServiceWorkerStorage::ContinuePurgingResources() {
   if (purgeable_resource_ids_.empty() || is_purge_pending_)
@@ skipping 412 matching lines @@
     callback.Run(SERVICE_WORKER_ERROR_FAILED);
     return;
   }
   DVLOG(1) << "Deleted ServiceWorkerDiskCache successfully.";
   ServiceWorkerMetrics::RecordDeleteAndStartOverResult(
       ServiceWorkerMetrics::DELETE_OK);
   callback.Run(SERVICE_WORKER_OK);
 }
 
 }  // namespace content