ServiceWorker: Don't start the timeout timer when DevTools is detached

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 113ed1c908112f70e2645945d9e22c821fbb226c..c15edd5bcd03dbf8806c481fb98bf6caa3728682 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -314,8 +314,6 @@ ServiceWorkerVersion::ServiceWorkerVersion(
       context_(context),
       script_cache_map_(this, context),
       ping_state_(NOT_PINGING),
-      is_doomed_(false),
-      skip_waiting_(false),
       weak_factory_(this) {
   DCHECK(context_);
   DCHECK(registration);
@@ -779,12 +777,16 @@ void ServiceWorkerVersion::Doom() {
 void ServiceWorkerVersion::SetDevToolsAttached(bool attached) {
   embedded_worker()->set_devtools_attached(attached);
   if (attached) {
-    // Set to null time so we don't record the startup time metric.
+    // Don't record the startup time metric once DevTools is attached.
     ClearTick(&start_time_);
+    skip_recording_startup_time_ = true;
     return;
   }
-  if (!timeout_timer_.IsRunning())
-    StartTimeoutTimer();
+  if (running_status() == STARTING || running_status() == STOPPING) {

[falken, 2015/03/23 00:14:25]

This is kind of a non-obvious condition. Would it make sense to do if
(!start_callbacks_.empty()) and maybe DCHECK for running_status?

[nhiroki, 2015/03/23 00:43:17]

Done.

[nhiroki, 2015/03/23 00:43:17]

Done.

+    // Reactivate the timer for start timeout.
+    DCHECK(timeout_timer_.IsRunning()) << running_status();
+    RestartTick(&start_time_);
+  }
 }
 
 void ServiceWorkerVersion::SetMainScriptHttpResponseInfo(
@@ -876,17 +878,8 @@ void ServiceWorkerVersion::OnStopped(
   cache_listener_.reset();
 
   // Restart worker if we have any start callbacks and the worker isn't doomed.
-  if (should_restart) {
-    if (embedded_worker_->devtools_attached())
-      ClearTick(&start_time_);
-    else
-      RestartTick(&start_time_);

[nhiroki, 2015/03/16 01:58:02]

FYI: We may have to call StartTimeoutTimer() here. The patchset3 will get to
call it via StartWorkerInternal().

-    cache_listener_.reset(new ServiceWorkerCacheListener(this, context_));
-    embedded_worker_->Start(
-        version_id_, scope_, script_url_, false /* pause_after_download */,
-        base::Bind(&ServiceWorkerVersion::OnStartSentAndScriptEvaluated,
-                   weak_factory_.GetWeakPtr()));
-  }
+  if (should_restart)
+    StartWorkerInternal(false /* pause_after_download */);

[falken, 2015/03/23 00:14:25]

Thanks for this helper function.

 }
 
 void ServiceWorkerVersion::OnReportException(
@@ -1433,23 +1426,32 @@ void ServiceWorkerVersion::DidEnsureLiveRegistrationForStartWorker(
     case STOPPING:
     case STOPPED:
     case STARTING:
-      if (!timeout_timer_.IsRunning())
-        StartTimeoutTimer();
-      // Start callbacks keep the live registration.
-      start_callbacks_.push_back(
-          base::Bind(&RunStartWorkerCallback, callback, protect));
-      if (running_status() == STOPPED) {
-        DCHECK(!cache_listener_.get());
-        cache_listener_.reset(new ServiceWorkerCacheListener(this, context_));
-        embedded_worker_->Start(
-            version_id_, scope_, script_url_, pause_after_download,
-            base::Bind(&ServiceWorkerVersion::OnStartSentAndScriptEvaluated,
+      if (start_callbacks_.empty()) {
+        start_callbacks_.push_back(
+            base::Bind(&ServiceWorkerVersion::RecordStartWorkerResult,
                        weak_factory_.GetWeakPtr()));
       }
+      // Keep the live registration while staring the worker.

[falken, 2015/03/23 00:14:25]

nit: starting

[nhiroki, 2015/03/23 00:43:17]

Done.

+      start_callbacks_.push_back(
+          base::Bind(&RunStartWorkerCallback, callback, protect));
+      StartWorkerInternal(pause_after_download);
       return;
   }
 }
 
+void ServiceWorkerVersion::StartWorkerInternal(bool pause_after_download) {
+  if (!timeout_timer_.IsRunning())
+    StartTimeoutTimer();
+  if (running_status() == STOPPED) {
+    DCHECK(!cache_listener_.get());
+    cache_listener_.reset(new ServiceWorkerCacheListener(this, context_));
+    embedded_worker_->Start(
+        version_id_, scope_, script_url_, pause_after_download,
+        base::Bind(&ServiceWorkerVersion::OnStartSentAndScriptEvaluated,
+                   weak_factory_.GetWeakPtr()));
+  }
+}
+
 void ServiceWorkerVersion::DidClaimClients(
     int request_id, ServiceWorkerStatusCode status) {
   if (status == SERVICE_WORKER_ERROR_STATE) {
@@ -1482,13 +1484,14 @@ void ServiceWorkerVersion::DidGetClients(
 void ServiceWorkerVersion::StartTimeoutTimer() {
   DCHECK(!timeout_timer_.IsRunning());
 
-  if (embedded_worker_->devtools_attached())
+  if (embedded_worker_->devtools_attached()) {
+    // Don't record the startup time metric once DevTools is attached.
     ClearTick(&start_time_);
-  else
+    skip_recording_startup_time_ = true;
+  } else {
     RestartTick(&start_time_);
-  start_callbacks_.push_back(
-      base::Bind(&ServiceWorkerVersion::RecordStartWorkerResult,
-                 weak_factory_.GetWeakPtr()));
+    skip_recording_startup_time_ = false;
+  }
 
   ClearTick(&idle_time_);
   ClearTick(&ping_time_);
@@ -1511,7 +1514,8 @@ void ServiceWorkerVersion::OnTimeoutTimer() {
   // Starting a worker hasn't finished within a certain period.
   if (GetTickDuration(start_time_) >
       base::TimeDelta::FromMinutes(kStartWorkerTimeoutMinutes)) {
-    DCHECK_NE(RUNNING, running_status());
+    DCHECK(running_status() == STARTING || running_status() == STOPPING)
+        << running_status();
     scoped_refptr<ServiceWorkerVersion> protect(this);
     RunCallbacks(this, &start_callbacks_, SERVICE_WORKER_ERROR_TIMEOUT);
     if (running_status() == STARTING)
@@ -1605,7 +1609,8 @@ void ServiceWorkerVersion::RecordStartWorkerResult(
 
   UMA_HISTOGRAM_ENUMERATION("ServiceWorker.StartWorker.Status", status,
                             SERVICE_WORKER_ERROR_MAX_VALUE);
-  if (status == SERVICE_WORKER_OK && !start_time.is_null()) {
+  if (status == SERVICE_WORKER_OK && !start_time.is_null() &&
+      !skip_recording_startup_time_) {
     UMA_HISTOGRAM_MEDIUM_TIMES("ServiceWorker.StartWorker.Time",
                                GetTickDuration(start_time));
   }