service worker: Avoid starting up for activation during shutdown

content/browser/service_worker/service_worker_registration.cc

Index: content/browser/service_worker/service_worker_registration.cc
diff --git a/content/browser/service_worker/service_worker_registration.cc b/content/browser/service_worker/service_worker_registration.cc
index b803edf1865954db918665b644c0d8f173607f8d..869d53eb214933cca147229ca996075ead457210 100644
--- a/content/browser/service_worker/service_worker_registration.cc
+++ b/content/browser/service_worker/service_worker_registration.cc
@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "content/browser/service_worker/service_worker_context_core.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_info.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_register_job.h"
@@ -24,6 +25,13 @@ ServiceWorkerVersionInfo GetVersionInfo(ServiceWorkerVersion* version) {
   return version->GetInfo();
 }
 
+bool GetShutdownStatusOnUIThread(
+    scoped_refptr<ServiceWorkerContextWrapper> wrapper,
+    ServiceWorkerStatusCode status) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  return wrapper->process_manager()->IsShutdown();
+}
+
 }  // namespace
 
 ServiceWorkerRegistration::ServiceWorkerRegistration(
@@ -176,8 +184,9 @@ void ServiceWorkerRegistration::ActivateWaitingVersionWhenReady() {
   should_activate_when_ready_ = true;
 
   if (!active_version() || !active_version()->HasControllee() ||
-      waiting_version()->skip_waiting())
-    ActivateWaitingVersion();
+      waiting_version()->skip_waiting()) {
+    ActivateWaitingVersion(false);
+  }
 }
 
 void ServiceWorkerRegistration::ClaimClients() {
@@ -243,12 +252,12 @@ void ServiceWorkerRegistration::OnNoControllees(ServiceWorkerVersion* version) {
   if (is_uninstalling_)
     Clear();
   else if (should_activate_when_ready_)
-    ActivateWaitingVersion();
+    ActivateWaitingVersion(true);
   is_uninstalling_ = false;
   should_activate_when_ready_ = false;
 }
 
-void ServiceWorkerRegistration::ActivateWaitingVersion() {
+void ServiceWorkerRegistration::ActivateWaitingVersion(bool delay) {
   DCHECK(context_);
   DCHECK(waiting_version());
   DCHECK(should_activate_when_ready_);
@@ -283,6 +292,24 @@ void ServiceWorkerRegistration::ActivateWaitingVersion() {
     FOR_EACH_OBSERVER(Listener, listeners_, OnSkippedWaiting(this));
 
   // "10. Queue a task to fire an event named activate..."
+  // The browser could be shutting down. To avoid spurious start worker
+  // failures, wait a bit before continuing.

[nhiroki, 2016/06/01 05:51:47]

Let me add this comment just for future reference...

[Before offline chat w/ falken@]

When delay == true, how about checking shutdown state first and avoiding
activation when in shutdown instead of using a timer?

[After offline chat]

According to falken@, ServiceWorkerContextWrapper::Shutdown() seems to be called
after this point and we may not be able to see the precise shutdown state here.

[falken, 2016/06/01 09:20:05]

Acknowledged.

+  if (delay) {
+    timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(1),
+                 base::Bind(&ServiceWorkerRegistration::ContinueActivation,
+                            this, activating_version));
+  } else {
+    ContinueActivation(activating_version);
+  }
+}
+
+void ServiceWorkerRegistration::ContinueActivation(
+    const scoped_refptr<ServiceWorkerVersion>& activating_version) {
+  if (!context_)
+    return;
+  if (active_version() != activating_version.get())
+    return;
+  DCHECK_EQ(ServiceWorkerVersion::ACTIVATING, activating_version->status());
   activating_version->RunAfterStartWorker(
       ServiceWorkerMetrics::EventType::ACTIVATE,
       base::Bind(&ServiceWorkerRegistration::DispatchActivateEvent, this,
@@ -331,6 +358,11 @@ void ServiceWorkerRegistration::NotifyRegistrationFinished() {
     callback.Run();
 }
 
+void ServiceWorkerRegistration::SetTimerTaskRunnerForTest(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  timer_.SetTaskRunner(task_runner);

[nhiroki, 2016/06/01 05:51:47]

std::move(task_runner) ?

[falken, 2016/06/01 09:20:05]

Done.

+}
+
 void ServiceWorkerRegistration::RegisterRegistrationFinishedCallback(
     const base::Closure& callback) {
   // This should only be called if the registration is in progress.
@@ -361,13 +393,41 @@ void ServiceWorkerRegistration::DispatchActivateEvent(
 void ServiceWorkerRegistration::OnActivateEventFinished(
     const scoped_refptr<ServiceWorkerVersion>& activating_version,
     ServiceWorkerStatusCode status) {
+  ServiceWorkerMetrics::RecordActivateEventStatus(status);
+
+  if (!context_)
+    return;
+
+  if (status == SERVICE_WORKER_OK) {
+    CompleteActivation(activating_version, status, false);

[nhiroki, 2016/06/01 05:51:47]

Always recording OK w/ |is_shutdown = false| would be misleading. How about
checking the shutdown state even when an activation succeeds, or skipping
recording OK when not in shutdown?

[falken, 2016/06/01 09:20:05]

OK I went with checking the state always. I had wanted to make the shutdown
stuff be the slow path only taken when needed (e.g., the 1 sec delay is only
when a tab closes, and this UI hop was only on failure). But you're right it's
more accurate UMA to get the shutdown state in all cases, and the UI hop
shouldn't be too costly.

+    return;
+  }
+
+  // Activate is prone to failing due to shutdown, because it's triggered when
+  // tabs close. Try to detect that case.
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&GetShutdownStatusOnUIThread,
+                 make_scoped_refptr(context_->wrapper()), status),
+      base::Bind(&ServiceWorkerRegistration::CompleteActivation, this,
+                 activating_version, status));
+}
+
+void ServiceWorkerRegistration::CompleteActivation(
+    const scoped_refptr<ServiceWorkerVersion>& activating_version,
+    ServiceWorkerStatusCode status,
+    bool is_shutdown) {
+  ServiceWorkerMetrics::RecordActivateEventStatusWithShutdownStatus(
+      status, is_shutdown);
   if (!context_ || activating_version != active_version() ||
-      activating_version->status() != ServiceWorkerVersion::ACTIVATING)
+      activating_version->status() != ServiceWorkerVersion::ACTIVATING) {
     return;
+  }
 
-  // |status| is just for UMA. Once we've attempted to dispatch the activate
-  // event to an installed worker, it's committed to becoming active.
-  ServiceWorkerMetrics::RecordActivateEventStatus(status);
+  if (is_shutdown) {
+    DCHECK_NE(SERVICE_WORKER_OK, status);
+    return;
+  }
 
   // "Run the Update State algorithm passing registration's active worker and
   // 'activated' as the arguments."