Call EmbeddedWorkerDevToolsManager::ServiceWorkerCreated, WorkerContextStarted and WorkerDestroyed.

content/browser/service_worker/embedded_worker_registry.cc

Index: content/browser/service_worker/embedded_worker_registry.cc
diff --git a/content/browser/service_worker/embedded_worker_registry.cc b/content/browser/service_worker/embedded_worker_registry.cc
index a859268c396b222b3eec789382d84b4ba1c48f5f..35661cbad5d75d35fb98055f2c68f1f61f8a2f3b 100644
--- a/content/browser/service_worker/embedded_worker_registry.cc
+++ b/content/browser/service_worker/embedded_worker_registry.cc
@@ -11,7 +11,9 @@
 #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"
 
@@ -23,8 +25,8 @@ EmbeddedWorkerRegistry::EmbeddedWorkerRegistry(
       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();
 }
@@ -46,12 +48,12 @@ void EmbeddedWorkerRegistry::StartWorker(const std::vector<int>& process_ids,
   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));
 }
@@ -210,14 +212,13 @@ 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.
@@ -226,25 +227,37 @@ void EmbeddedWorkerRegistry::StartWorkerWithProcessId(
     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)));
 }