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 484 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()) {
+    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;
   for (auto& observer : listener_list_)
     observer.OnStopping();
   return status;
 }
 
 void EmbeddedWorkerInstance::StopIfIdle() {
   if (devtools_attached_) {
@@ skipping 413 matching lines @@
     case SCRIPT_READ_FINISHED:
       return "Script read finished";
     case STARTING_PHASE_MAX_VALUE:
       NOTREACHED();
   }
   NOTREACHED() << phase;
   return std::string();
 }
 
 }  // namespace content