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

content/browser/service_worker/embedded_worker_instance.cc

Index: content/browser/service_worker/embedded_worker_instance.cc
diff --git a/content/browser/service_worker/embedded_worker_instance.cc b/content/browser/service_worker/embedded_worker_instance.cc
index 17246a1f1bc6c450541f893130461e1b9bffd0ef..f6bda71d1b75e70a1ac405435494477b71644486 100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -502,20 +502,44 @@ ServiceWorkerStatusCode EmbeddedWorkerInstance::Stop() {
 
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_IPC_FAILED;
   if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
+    if (status_ == EmbeddedWorkerStatus::STARTING) {
+      // Clear internal states and return immediately when the connection hasn't
+      // been established yet.

[falken, 2016/12/20 09:12:07]

More than "connection established", I think this is ensuring we don't send Stop
if Start hasn't been sent yet.

I wonder if this would be more readable as a helper function.

if (status_ == EmbeddedWorkerStatus::STARTING &&
!HasSentStartWorker(starting_phase())) {
  OnDetached();
  return ERROR;
}
status = SERVICE_WORKER_OK;
client_->StopWorker();

[shimazu, 2017/01/05 06:02:41]

Done.

+      switch (starting_phase()) {
+        case NOT_STARTING:
+          NOTREACHED();
+        case ALLOCATING_PROCESS:
+        case REGISTERING_TO_DEVTOOLS:
+          OnDetached();
+          return SERVICE_WORKER_ERROR_FAILED;

[falken, 2016/12/20 09:12:06]

It doesn't seem right to return error here and have the callback to
ServiceWorkerVersion::StopWorker be invoked with an error. Nothing bad happened:
we released the process and the worker is stopped (it never started). However I
see if we return OK here then StopWorker won't invoke its callback. Really in
the Mojo-world it seems Stop() isn't returning a success/failure status but a
boolean about whether the Stop will happen asynchronously or not.

Actually I think the code would be more stable if this path still runs through
the motions of observer.OnStopping() followed by observer.OnStopped() just like
the normal case. It should be abstracted away from the caller whether the Stop
really was sent or not.

WDYT? Should it be a TODO for a later patch?

I think the code would look something like:

status_ = STOPPING;
for (auto& observer : listener_list_)
  observer.OnStopping();
if (!was_stop_sent)
  RunSoon(&OnStopped);  // OnStopped will call ReleaseProcess()
return OK;

"Detach" is kind of a messy hack BTW. It was originally added to try to
workaround a bug where workers got stuck in STOPPING status. It turned out the
bug was a deadlocked renderer
<https://bugs.chromium.org/p/chromium/issues/detail?id=509904>.

[shimazu, 2017/01/05 06:02:41]

That sounds good, but I'd like to create another patch for that after removing
the legacy IPC path.

+        case SENT_START_WORKER:
+        case SCRIPT_DOWNLOADING:
+        case SCRIPT_READ_STARTED:
+        case SCRIPT_READ_FINISHED:
+        case SCRIPT_LOADED:
+        case SCRIPT_EVALUATED:
+        case THREAD_STARTED:
+          break;
+        case STARTING_PHASE_MAX_VALUE:
+          NOTREACHED();
+          OnDetached();

[falken, 2016/12/20 09:12:06]

nit: remove OnDetached(). we shouldn't handle the impossible condition per style
guide

[shimazu, 2017/01/05 06:02:41]

Done.

+          return SERVICE_WORKER_ERROR_ABORT;
+      }
+    }
     status = SERVICE_WORKER_OK;
     client_->StopWorker(base::Bind(&EmbeddedWorkerRegistry::OnWorkerStopped,
                                    base::Unretained(registry_.get()),
                                    process_id(), embedded_worker_id()));
   } 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();
-    return status;
+    UMA_HISTOGRAM_ENUMERATION("ServiceWorker.SendStopWorker.Status", status,

[falken, 2016/12/20 09:12:06]

We should update histograms.xml that it's only used in the disable-mojo case.

[shimazu, 2017/01/05 06:02:41]

Done.

+                              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;