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

content/browser/service_worker/service_worker_version.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_version.h"
 
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
@@ skipping 296 matching lines @@
     const GURL& script_url,
     int64 version_id,
     base::WeakPtr<ServiceWorkerContextCore> context)
     : version_id_(version_id),
       registration_id_(kInvalidServiceWorkerVersionId),
       script_url_(script_url),
       status_(NEW),
       context_(context),
       script_cache_map_(this, context),
       ping_state_(NOT_PINGING),
-      is_doomed_(false),
-      skip_waiting_(false),
       weak_factory_(this) {
   DCHECK(context_);
   DCHECK(registration);
   if (registration) {
     registration_id_ = registration->id();
     scope_ = registration->pattern();
   }
   context_->AddLiveVersion(this);
   embedded_worker_ = context_->embedded_worker_registry()->CreateWorker();
   embedded_worker_->AddListener(this);
@@ skipping 443 matching lines @@
   if (is_doomed_)
     return;
   is_doomed_ = true;
   if (!HasControllee())
     DoomInternal();
 }
 
 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(
     const net::HttpResponseInfo& http_info) {
   main_script_http_info_.reset(new net::HttpResponseInfo(http_info));
 }
 
 const net::HttpResponseInfo*
 ServiceWorkerVersion::GetMainScriptHttpResponseInfo() {
   return main_script_http_info_.get();
@@ skipping 71 matching lines @@
   streaming_url_request_jobs_.clear();
 
   FOR_EACH_OBSERVER(Listener, listeners_, OnRunningStateChanged(this));
 
   // There should be no more communication from/to a stopped worker. Deleting
   // the listener prevents any pending completion callbacks from causing
   // messages to be sent to the stopped worker.
   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(
     const base::string16& error_message,
     int line_number,
     int column_number,
     const GURL& source_url) {
   FOR_EACH_OBSERVER(
       Listener,
       listeners_,
@@ skipping 514 matching lines @@
 
 void ServiceWorkerVersion::OnPongFromWorker() {
   ClearTick(&ping_time_);
 }
 
 void ServiceWorkerVersion::DidEnsureLiveRegistrationForStartWorker(
     bool pause_after_download,
     const StatusCallback& callback,
     ServiceWorkerStatusCode status,
     const scoped_refptr<ServiceWorkerRegistration>& protect) {
   if (status != SERVICE_WORKER_OK || is_doomed()) {

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

I feel like non-OK status here is unexpected and worth recording in the
StartWorker histogram, WDYT?

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

Sounds reasonable. Added the function call (is_doomed() case is just ignored in
the record function).

     RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_START_WORKER_FAILED));
     return;
   }
 
   switch (running_status()) {
     case RUNNING:
       RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
       return;
     case STOPPING:
     case STOPPED:
     case STARTING:
-      if (!timeout_timer_.IsRunning())
-        StartTimeoutTimer();
-      // Start callbacks keep the live registration.
+      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));
-      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()));
-      }
+      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) {
     embedded_worker_->SendMessage(ServiceWorkerMsg_ClaimClientsError(
         request_id, blink::WebServiceWorkerError::ErrorTypeState,
         base::ASCIIToUTF16(kClaimClientsStateErrorMesage)));
     return;
   }
   if (status == SERVICE_WORKER_ERROR_ABORT) {
     embedded_worker_->SendMessage(ServiceWorkerMsg_ClaimClientsError(
@@ skipping 12 matching lines @@
   if (running_status() != RUNNING)
     return;
 
   embedded_worker_->SendMessage(
       ServiceWorkerMsg_DidGetClients(request_id, clients));
 }
 
 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_);
   ping_state_ = NOT_PINGING;
 
   timeout_timer_.Start(FROM_HERE,
                        base::TimeDelta::FromSeconds(kTimeoutTimerDelaySeconds),
                        this, &ServiceWorkerVersion::OnTimeoutTimer);
 }
 
 void ServiceWorkerVersion::StopTimeoutTimer() {
   timeout_timer_.Stop();
 }
 
 void ServiceWorkerVersion::OnTimeoutTimer() {
   DCHECK(running_status() == STARTING || running_status() == RUNNING ||
          running_status() == STOPPING)
       << running_status();
 
   // 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)
       embedded_worker_->Stop();
     return;
   }
 
   // This check occurs after the start_time_ timeout check, since in that case
   // the start callbacks should fail with ERROR_TIMEOUT. In the other timeout
   // checks, there's nothing more to do as the worker is already stopping.
@@ skipping 73 matching lines @@
   base::TimeTicks start_time = start_time_;
   ClearTick(&start_time_);
 
   // Failing to start a doomed worker isn't interesting and very common when
   // update dooms because the script is byte-to-byte identical.
   if (is_doomed_)
     return;
 
   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));
   }
 
   if (status != SERVICE_WORKER_ERROR_TIMEOUT)
     return;
   EmbeddedWorkerInstance::StartingPhase phase =
       EmbeddedWorkerInstance::NOT_STARTING;
   EmbeddedWorkerInstance::Status running_status = embedded_worker_->status();
   // Build an artifical JavaScript exception to show in the ServiceWorker
@@ skipping 33 matching lines @@
     int request_id) {
   callbacks->Remove(request_id);
   if (is_doomed_) {
     // The stop should be already scheduled, but try to stop immediately, in
     // order to release worker resources soon.
     StopWorkerIfIdle();
   }
 }
 
 }  // namespace content