Call EmbeddedWorkerDevToolsManager::ServiceWorkerCreated, WorkerContextStarted and WorkerDestroyed.

content/browser/service_worker/embedded_worker_registry.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_registry.h"
 
 #include "base/bind_helpers.h"
 #include "base/stl_util.h"
 #include "content/browser/renderer_host/render_widget_helper.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/common/service_worker/embedded_worker_messages.h"
+#include "content/common/service_worker/service_worker_messages.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_process_host.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_sender.h"
 
 namespace content {
 
 EmbeddedWorkerRegistry::EmbeddedWorkerRegistry(
     base::WeakPtr<ServiceWorkerContextCore> context)
     : context_(context),
       next_embedded_worker_id_(0) {}
 
 scoped_ptr<EmbeddedWorkerInstance> EmbeddedWorkerRegistry::CreateWorker() {
-  scoped_ptr<EmbeddedWorkerInstance> worker(
-      new EmbeddedWorkerInstance(this, next_embedded_worker_id_));
+  scoped_ptr<EmbeddedWorkerInstance> worker(new EmbeddedWorkerInstance(
+      this, next_embedded_worker_id_, context_->storage_partition_path()));
   worker_map_[next_embedded_worker_id_++] = worker.get();
   return worker.Pass();
 }
 
 void EmbeddedWorkerRegistry::StartWorker(const std::vector<int>& process_ids,
                                          int embedded_worker_id,
                                          int64 service_worker_version_id,
                                          const GURL& scope,
                                          const GURL& script_url,
                                          const StatusCallback& callback) {
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
   scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
       new EmbeddedWorkerMsg_StartWorker_Params());
   params->embedded_worker_id = embedded_worker_id;
   params->service_worker_version_id = service_worker_version_id;
   params->scope = scope;
   params->script_url = script_url;
   params->worker_devtools_agent_route_id = MSG_ROUTING_NONE;
+  params->pause_on_start = false;
   context_->process_manager()->AllocateWorkerProcess(
       process_ids,
       script_url,
-      base::Bind(&EmbeddedWorkerRegistry::StartWorkerWithProcessId,
+      base::Bind(&EmbeddedWorkerRegistry::WorkerProcessAllocated,
                  this,
-                 embedded_worker_id,
                  base::Passed(&params),
                  callback));
 }
 
 ServiceWorkerStatusCode EmbeddedWorkerRegistry::StopWorker(
     int process_id, int embedded_worker_id) {
   if (context_)
     context_->process_manager()->ReleaseWorkerProcess(process_id);
   return Send(process_id,
               new EmbeddedWorkerMsg_StopWorker(embedded_worker_id));
@@ skipping 138 matching lines @@
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
   if (found == worker_map_.end())
     return NULL;
   return found->second;
 }
 
 EmbeddedWorkerRegistry::~EmbeddedWorkerRegistry() {
   Shutdown();
 }
 
-void EmbeddedWorkerRegistry::StartWorkerWithProcessId(
-    int embedded_worker_id,
+void EmbeddedWorkerRegistry::WorkerProcessAllocated(
     scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params,
     const StatusCallback& callback,
     ServiceWorkerStatusCode status,
     int process_id) {
   WorkerInstanceMap::const_iterator worker =
-      worker_map_.find(embedded_worker_id);
+      worker_map_.find(params->embedded_worker_id);
   if (worker == worker_map_.end()) {
     // The Instance was destroyed before it could finish starting.  Undo what
     // we've done so far.
     if (context_)
       context_->process_manager()->ReleaseWorkerProcess(process_id);
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
-  if (status == SERVICE_WORKER_OK) {
-    // Gets the new routing id for the renderer process.
-    scoped_refptr<RenderWidgetHelper> helper(
-        RenderWidgetHelper::FromProcessHostID(process_id));
-    // |helper| may be NULL in unittest.
-    params->worker_devtools_agent_route_id =
-        helper ? helper->GetNextRoutingID() : MSG_ROUTING_NONE;
-  }
-  worker->second->RecordProcessId(
-      process_id, status, params->worker_devtools_agent_route_id);
-
   if (status != SERVICE_WORKER_OK) {
+    worker->second->WorkerProcessAllocationFailed();
     callback.Run(status);
     return;
   }
+  worker->second->WorkerProcessAllocated(
+      process_id,
+      base::Bind(&EmbeddedWorkerRegistry::StartWorkerWithProcessId,
+                 this,
+                 base::Passed(&params),
+                 callback,
+                 process_id));

[kinuko, 2014/05/08 11:17:31]

The call sequence for starting a worker is becoming a bit convoluted as it now
goes through a long  sequence like:

 Instance -> Registry -> ProcessManager -> Registry -> Instance ->
DevToolsMgrBridge -> Registry

Also the fact that interface between Registry and Instance is becoming more
complex worries me. Looking into the code again now, I feel that we might be
able to struct this code better if:

- The worker process allocation code is moved to Instance (Instance can hold
context_ weakptr too)
- Registry::StartWorkerWithProcessId() can be renamed to just StartWorker() and
be called after the allocation's done

Then it'll look like:

 Instance -> ProcessManager -> Instance -> DevToolsMgrBridge -> Registry

Wdyt?  (I might be missing some details, please just let me know if so)

[horo, 2014/05/08 14:17:46]

I think we need to go through Registry after the thread hopping (IO->UI->IO),
because EmbeddedWorkerInstance could be deleted while the hopping.

So we have to go Registry after ProcessManager.
And to call ProcessManager from Instance, Instance have to know about context.
I think the current code is better.

[kinuko, 2014/05/08 16:34:57]

Ok thanks, but I'm not fully convinced by the current code either... it feels
they're too entangled.

So the problem is not really about Registry but is about releasing a worker
process when Instance is gone, is that right?  Let me see... would something
like following work?


namespace {

// Helper function which invokes Instance's method if it's still there, or
releases the process ID.
template <typename Method, typename Params>
void RunOrReleaseProcess(WeakPtr<Instance>, WeakPtr<Context>, int pid, Method m,
const Params& p) {
   if (!instance) {
       if (context) context->process_mgr()->ReleaseProcess(pid);
       return;
   }
   DispatchToMethod(instance, m, p);
}

void RunProcessAllocated(WeakPtr<Instance>, WeakPtr<Context>, ...) {
  RunOrReleaseProcess(..., &Instance::ProcessAllocated, MakeTuple(...));
}

void RunStartWorker(...) {
  RunOrReleaseProcess(..., &Instance::StartWorker, MakeTuple(...));
}

void RegisterToDevToolsMgr(..., callback) {
  if (!RunningOnUI) {
     PostTask(UI, Bind(&RegisterToDevToolsMgr, callback));
     return;
  }
  ...
  PostTask(IO, callback);
}

}  // namespace

// (This class may not be that necessary now)
class DevToolsMgrBridge {
  DevToolsMgrBridge(...) { instance_->AddListener(this); }
  ~DevToolsMgrBridge(...) {
     instance_->RemoveListener(this);
     PostTask(UI, NotifyWorkerDestroyedOnUI, ...);
  }
  OnScriptLoaded() {
     PostTask(UI, NotifyWorkerContextStartedOnUI, ...);
  }
};

void Instance::StartWorker() {
   context_->process_manager()->AllocateWorkerProcess(
      ..., Bind(&RunProcessAllocated, weakptr, context_)); 
}

void Instance::ProcessAllocated() {
  pid_ = pid;
  RegisterToDevToolsManager(
     ..., Bind(&RunStartWorker, weakptr, context_));
}

void Instance::StartWorker() {
  if (route_id != ROUTING_NONE) {
    DCHECK(!devtools_mgr_bridge_);
    devtools_mgr_bridge_.reset(new DevToolsMgrBridge(this, route_id, pid_,
...));
  }
  registry_->StartWorker(...);
}

void Instance::OnStopped() {
  DCHECK(devtools_mgr_bridge_);
  devtools_mgr_bridge_.reset();
}

[horo, 2014/05/12 10:18:49]

Done.

+}
+
+void EmbeddedWorkerRegistry::StartWorkerWithProcessId(
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params,
+    const StatusCallback& callback,
+    int process_id,
+    int worker_devtools_agent_route_id,
+    bool pause_on_start) {
+  WorkerInstanceMap::const_iterator worker =
+      worker_map_.find(params->embedded_worker_id);
+  DCHECK(worker != worker_map_.end());
+  worker->second->set_worker_devtools_agent_route_id(
+      worker_devtools_agent_route_id);
   // The ServiceWorkerDispatcherHost is supposed to be created when the process
   // is created, and keep an entry in process_sender_map_ for its whole
   // lifetime.
   DCHECK(ContainsKey(process_sender_map_, process_id));
+  params->worker_devtools_agent_route_id = worker_devtools_agent_route_id;
+  params->pause_on_start = pause_on_start;
   callback.Run(Send(process_id, new EmbeddedWorkerMsg_StartWorker(*params)));
 }
 
 ServiceWorkerStatusCode EmbeddedWorkerRegistry::Send(
     int process_id, IPC::Message* message) {
   if (!context_)
     return SERVICE_WORKER_ERROR_ABORT;
   ProcessToSenderMap::iterator found = process_sender_map_.find(process_id);
   if (found == process_sender_map_.end())
     return SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND;
   if (!found->second->Send(message))
     return SERVICE_WORKER_ERROR_IPC_FAILED;
   return SERVICE_WORKER_OK;
 }
 
 void EmbeddedWorkerRegistry::RemoveWorker(int process_id,
                                           int embedded_worker_id) {
   DCHECK(ContainsKey(worker_map_, embedded_worker_id));
   worker_map_.erase(embedded_worker_id);
   worker_process_map_.erase(process_id);
 }
 
 }  // namespace content