ServiceWorker: Stop don't send a message before connection established

content/browser/service_worker/embedded_worker_instance.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/embedded_worker_instance.h"
 
 #include <utility>
 
 #include "base/bind_helpers.h"
 #include "base/macros.h"
@@ skipping 482 matching lines @@
 }
 
 ServiceWorkerStatusCode EmbeddedWorkerInstance::Stop() {
   DCHECK(status_ == EmbeddedWorkerStatus::STARTING ||
          status_ == EmbeddedWorkerStatus::RUNNING)
       << static_cast<int>(status_);
 
   // Abort an inflight start task.
   inflight_start_task_.reset();
 
-  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_IPC_FAILED;
   if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
-    status = SERVICE_WORKER_OK;
     client_->StopWorker(base::Bind(&EmbeddedWorkerRegistry::OnWorkerStopped,
                                    base::Unretained(registry_.get()),
                                    process_id(), embedded_worker_id()));
+    UMA_HISTOGRAM_ENUMERATION("ServiceWorker.SendStopWorker.Status",
+                              SERVICE_WORKER_OK,

[nhiroki, 2016/12/15 09:53:15]

Does StopWorker() always succeed? If so, do we still need to record the result?

[shimazu, 2016/12/19 08:25:12]

Oops, I forgot to change that. Fixed it.

+                              SERVICE_WORKER_ERROR_MAX_VALUE);
+    if (status_ == EmbeddedWorkerStatus::STARTING) {
+      // Handles like detach when connection hasn't been established yet.
+      switch (starting_phase()) {
+        case NOT_STARTING:

[nhiroki, 2016/12/15 09:53:15]

Is this case really possible?

[shimazu, 2016/12/19 08:25:12]

No actually. Stop should be called when status_ == STARTING || RUNNING, and that
means Stop isn't called before Start.
Added NOTREACHED().

+        case ALLOCATING_PROCESS:
+          OnDetached();
+          return SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND;

[nhiroki, 2016/12/15 09:53:15]

Why do we handle this case as an error? I wonder if this can easily happen when
a web developer consecutively clicks a start/stop button on the devtools.

[shimazu, 2016/12/19 08:25:12]

This is because it's the same with legacy IPC.

+        case REGISTERING_TO_DEVTOOLS:
+          OnDetached();
+          return SERVICE_WORKER_ERROR_IPC_FAILED;

[nhiroki, 2016/12/15 09:53:15]

ditto.

[shimazu, 2016/12/19 08:25:12]

Acknowledged.

+        default:

[nhiroki, 2016/12/15 09:53:15]

I'd prefer to avoid 'default' because this could silently break when we add a
new phase during starting a worker.

[shimazu, 2016/12/19 08:25:12]

Done.

+          break;
+      }
+    }
+    status_ = EmbeddedWorkerStatus::STOPPING;
+    for (auto& observer : listener_list_)
+      observer.OnStopping();
+    return SERVICE_WORKER_OK;
   } else {
-    status = registry_->StopWorker(process_id(), embedded_worker_id_);
-  }
-  UMA_HISTOGRAM_ENUMERATION("ServiceWorker.SendStopWorker.Status", status,
-                            SERVICE_WORKER_ERROR_MAX_VALUE);
-  // StopWorker could fail if we were starting up and don't have a process yet,
-  // or we can no longer communicate with the process. So just detach.
-  if (status != SERVICE_WORKER_OK) {
-    OnDetached();
+    ServiceWorkerStatusCode status =
+        registry_->StopWorker(process_id(), embedded_worker_id_);
+    UMA_HISTOGRAM_ENUMERATION("ServiceWorker.SendStopWorker.Status", status,
+                              SERVICE_WORKER_ERROR_MAX_VALUE);
+    // StopWorker could fail if we were starting up and don't have a process
+    // yet, or we can no longer communicate with the process. So just detach.
+    if (status != SERVICE_WORKER_OK) {
+      OnDetached();
+      return status;
+    }
+    status_ = EmbeddedWorkerStatus::STOPPING;
+    for (auto& observer : listener_list_)
+      observer.OnStopping();
     return status;
   }
-
-  status_ = EmbeddedWorkerStatus::STOPPING;
-  for (auto& observer : listener_list_)
-    observer.OnStopping();
-  return status;
 }
 
 void EmbeddedWorkerInstance::StopIfIdle() {
   if (devtools_attached_) {
     if (devtools_proxy_) {
       // Check ShouldNotifyWorkerStopIgnored not to show the same message
       // multiple times in DevTools.
       if (devtools_proxy_->ShouldNotifyWorkerStopIgnored()) {
         AddMessageToConsole(CONSOLE_MESSAGE_LEVEL_DEBUG,
                             kServiceWorkerTerminationCanceledMesage);
@@ skipping 407 matching lines @@
     case SCRIPT_READ_FINISHED:
       return "Script read finished";
     case STARTING_PHASE_MAX_VALUE:
       NOTREACHED();
   }
   NOTREACHED() << phase;
   return std::string();
 }
 
 }  // namespace content