ServiceWorker: Mojofication of EWInstance::StopWorker

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 0f7e5303406f20b2d3999c061a538e385a3d7bcc..ff7591fc36ba3e823208befcc2cae2a91c18c9ab 100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -462,6 +462,8 @@ void EmbeddedWorkerInstance::Start(
 
   mojom::EmbeddedWorkerInstanceClientRequest request =
       GetProxy(&instance_client_);
+  instance_client_.set_connection_error_handler(
+      base::Bind(&EmbeddedWorkerInstance::Detach, AsWeakPtr()));
 
   inflight_start_task_.reset(
       new StartTask(this, params->script_url, std::move(request)));
@@ -476,8 +478,19 @@ ServiceWorkerStatusCode EmbeddedWorkerInstance::Stop() {
   // Abort an inflight start task.
   inflight_start_task_.reset();
 
-  ServiceWorkerStatusCode status =
-      registry_->StopWorker(process_id(), embedded_worker_id_);
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_IPC_FAILED;
+  if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
+    if (!instance_client_) {
+      registry_->OnWorkerStopped(process_id(), embedded_worker_id());
+      return SERVICE_WORKER_ERROR_IPC_FAILED;
+    }
+    status = SERVICE_WORKER_OK;
+    instance_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,
@@ -772,7 +785,7 @@ void EmbeddedWorkerInstance::OnDetached() {
 }
 
 void EmbeddedWorkerInstance::Detach() {
-  registry_->RemoveWorker(process_id(), embedded_worker_id_);
+  registry_->DetachWorker(process_id(), embedded_worker_id());

[horo, 2016/09/05 08:05:57]

Could you please explain why you don't erase the EmbeddedWorker from
worker_map_?

[shimazu, 2016/09/06 08:53:59]

If I understand correctly, EmbeddedWorkerInstance is alive though it's stopped
or failing to send a StartWorker message except for Detach. 
The only user of Detach() is SWVersion::OnTimeoutTimer [1], but I think
OnTimeoutTimer doesn't intend to kill the instance, but intend to annotate
EWInstance as "detached".

Plus, I'd like to use Detach() as the error handler of mojo in this patch
because connection failures should be handled as "detach". However, if Detach()
removes the instance by calling worker_map_.erase(), the object failing IPC will
get deleted. I think this doesn't seem expected behavior because an unit test
[2] calls worker->Start() after failing IPC.

I don't have 100% confidence of this observation, so please tell me if my
understanding is incorrect.


[1]
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...

[2]
https://cs.chromium.org/chromium/src/content/browser/service_worker/embedded_...

[horo, 2016/09/13 08:25:32]

I see.

But I think the name of the error handler should be OnConnectionError().

   OnDetached();
 }
 
@@ -852,6 +865,7 @@ void EmbeddedWorkerInstance::ReleaseProcess() {
   // Abort an inflight start task.
   inflight_start_task_.reset();
 
+  instance_client_.reset();
   devtools_proxy_.reset();
   process_handle_.reset();
   status_ = EmbeddedWorkerStatus::STOPPED;