Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
261753008:
Call EmbeddedWorkerDevToolsManager::ServiceWorkerCreated, WorkerContextStarted and WorkerDestroyed. (Closed)
|
Created:
6 years, 7 months ago by horo Modified:
6 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, vsevik, tzik, alecflett+watch_chromium.org, serviceworker-reviews, jam, yurys, paulirish+reviews_chromium.org, darin-cc_chromium.org, nhiroki, devtools-reviews_chromium.org, horo+watch_chromium.org, kinuko+watch, aandrey+blink_chromium.org, pfeldman, Jeffrey Yasskin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCall EmbeddedWorkerDevToolsManager::ServiceWorkerCreated, WorkerContextStarted and WorkerDestroyed.
BUG=358657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271144
Patch Set 1 : #
Total comments: 2
Patch Set 2 : incorporated kinuko's comment #Patch Set 3 : delete ServiceWorkerStorage::path() #
Total comments: 6
Patch Set 4 : DevToolsManagerBridge #Patch Set 5 : fix typo #Patch Set 6 : fix compile error #
Total comments: 17
Patch Set 7 : incorporated kinuko's comment #Patch Set 8 : call ServiceWorkerProcessManager from EmbeddedWorkerInstance #Patch Set 9 : fix compile error in service_worker_browsertest.cc #Patch Set 10 : Call WorkerDestroyed in ServiceWorkerCreated when it is not called correctly before. #
Total comments: 6
Patch Set 11 : incorporated kinuko's comment #
Total comments: 8
Patch Set 12 : rebase #Patch Set 13 : incorporated kinuko's comment #
Total comments: 4
Patch Set 14 : Use ServiceWorkerContextCore to identify ServiceWorkers in EmbeddedWorkerDevToolsManager #Patch Set 15 : pass scope in StartWorker() #
Total comments: 4
Patch Set 16 : Introduce ServiceWorkerIdentifier #Patch Set 17 : add explicit #Messages
Total messages: 42 (0 generated)
kinuko@ Could you please review this?
https://codereview.chromium.org/261753008/diff/60001/content/browser/service_... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/60001/content/browser/service_... content/browser/service_worker/embedded_worker_registry.cc:53: context_->storage()->path(), Getting path from storage() to allocate a process feels too mysterious, if what we want here is an identifier of the storage partition we're in could we store it in context_ (as a name, like, storage_partition_path) and use it instead? Also as I commented on the other CL I don't think we will have same scope in different storage partitions, and if it's true scope() will be enough..?
https://codereview.chromium.org/261753008/diff/60001/content/browser/service_... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/60001/content/browser/service_... content/browser/service_worker/embedded_worker_registry.cc:53: context_->storage()->path(), On 2014/05/02 08:55:47, kinuko wrote: > Getting path from storage() to allocate a process feels too mysterious, if what > we want here is an identifier of the storage partition we're in could we store > it in context_ (as a name, like, storage_partition_path) and use it instead? > > Also as I commented on the other CL I don't think we will have same scope in > different storage partitions, and if it's true scope() will be enough..? Done.
What this patch does look good in general, but I wonder if DevTools related code could be added in a less intrusive way? https://codereview.chromium.org/261753008/diff/100001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/261753008/diff/100001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:36: } This part... and GetRoutingIDAndNotifyWorkerCreated, is it possible to factor out these DevTools related code into one place (like we hide process creation details in ProcessManager class)? They're not doing much (just calling notification methods etc), but still I feel these instruments / notifications could be abstracted a bit more. (Say, could we utilize EmbeddedWorkerInstance::Listener interface for this?) https://codereview.chromium.org/261753008/diff/100001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/100001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:53: context_->storage_partition_path(), Btw this could be given to ctor of ServiceWorkerProcessManager https://codereview.chromium.org/261753008/diff/100001/content/browser/service... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/261753008/diff/100001/content/browser/service... content/browser/service_worker/service_worker_process_manager.cc:24: // |rph| may NULL in unit tests. nit: may -> may be
https://codereview.chromium.org/261753008/diff/100001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/261753008/diff/100001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:36: } On 2014/05/02 11:12:45, kinuko wrote: > This part... and GetRoutingIDAndNotifyWorkerCreated, is it possible to factor > out these DevTools related code into one place (like we hide process creation > details in ProcessManager class)? They're not doing much (just calling > notification methods etc), but still I feel these instruments / notifications > could be abstracted a bit more. (Say, could we utilize > EmbeddedWorkerInstance::Listener interface for this?) Introduced DevToolsManagerBridge and moved DevToolsManager related codes to this class. https://codereview.chromium.org/261753008/diff/100001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/100001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:53: context_->storage_partition_path(), On 2014/05/02 11:12:45, kinuko wrote: > Btw this could be given to ctor of ServiceWorkerProcessManager Done. https://codereview.chromium.org/261753008/diff/100001/content/browser/service... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/261753008/diff/100001/content/browser/service... content/browser/service_worker/service_worker_process_manager.cc:24: // |rph| may NULL in unit tests. On 2014/05/02 11:12:45, kinuko wrote: > nit: may -> may be Done.
Sorry for my slow review.. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:17: namespace { nit: can we have an empty line (to match the end of block) https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:132: RemoveListener(dev_tools_manager_bridge_.get()); nit: doesn't feel it's necessary https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:142: dev_tools_manager_bridge_->set_scope(scope); Looks like the scope can be given to ctor of EmbeddedWorkerInstance (as it doesn't change during the lifetime) and it's probably better to do so for clarity? https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:199: storage_partition_path)) { nit: you can get this via registry ptr here rather than via ctor param (as EmbeddedWorkerInstance itself doesn't need it) https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:109: class DevToolsManagerBridge : public Listener, nit: Could be a private inner class (or could be in a separate file) https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:110: public base::RefCounted<DevToolsManagerBridge> { Does this need to be ref-counted? https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:241: process_id)); 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)
https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:17: namespace { On 2014/05/08 11:17:31, kinuko wrote: > nit: can we have an empty line (to match the end of block) Done. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:132: RemoveListener(dev_tools_manager_bridge_.get()); On 2014/05/08 11:17:31, kinuko wrote: > nit: doesn't feel it's necessary Done. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:142: dev_tools_manager_bridge_->set_scope(scope); On 2014/05/08 11:17:31, kinuko wrote: > Looks like the scope can be given to ctor of EmbeddedWorkerInstance (as it > doesn't change during the lifetime) and it's probably better to do so for > clarity? Done. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:199: storage_partition_path)) { On 2014/05/08 11:17:31, kinuko wrote: > nit: you can get this via registry ptr here rather than via ctor param (as > EmbeddedWorkerInstance itself doesn't need it) Done. Added EmbeddedWorkerRegistry::storage_partition_path(). https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:109: class DevToolsManagerBridge : public Listener, On 2014/05/08 11:17:31, kinuko wrote: > nit: Could be a private inner class (or could be in a separate file) Done. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:110: public base::RefCounted<DevToolsManagerBridge> { On 2014/05/08 11:17:31, kinuko wrote: > Does this need to be ref-counted? EmbeddedWorkerInstance could be deleted while the thread hopping. IO-thread DevToolsManagerBridge::RegisterToDevToolsManager() UI-thread RegisterToDevToolsManagerOnUI() IO-thread DevToolsManagerBridge::RegisteredCallback() So I think it must be ref-counted. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:241: process_id)); On 2014/05/08 11:17:31, kinuko wrote: > 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) 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.
https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:110: public base::RefCounted<DevToolsManagerBridge> { On 2014/05/08 14:17:46, horo wrote: > On 2014/05/08 11:17:31, kinuko wrote: > > Does this need to be ref-counted? > > EmbeddedWorkerInstance could be deleted while the thread hopping. > IO-thread DevToolsManagerBridge::RegisterToDevToolsManager() > UI-thread RegisterToDevToolsManagerOnUI() > IO-thread DevToolsManagerBridge::RegisteredCallback() > So I think it must be ref-counted. This doesn't necessarily mean we need ref-counting. Also now that most of devtools related code is in this file having this class may not be that important. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:241: process_id)); On 2014/05/08 14:17:46, horo wrote: > 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. 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(); }
On 2014/05/08 16:34:56, kinuko wrote: > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > File content/browser/service_worker/embedded_worker_instance.h (right): > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > content/browser/service_worker/embedded_worker_instance.h:110: public > base::RefCounted<DevToolsManagerBridge> { > On 2014/05/08 14:17:46, horo wrote: > > On 2014/05/08 11:17:31, kinuko wrote: > > > Does this need to be ref-counted? > > > > EmbeddedWorkerInstance could be deleted while the thread hopping. > > IO-thread DevToolsManagerBridge::RegisterToDevToolsManager() > > UI-thread RegisterToDevToolsManagerOnUI() > > IO-thread DevToolsManagerBridge::RegisteredCallback() > > So I think it must be ref-counted. > > This doesn't necessarily mean we need ref-counting. Also now that most of > devtools related code is in this file having this class may not be that > important. > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > File content/browser/service_worker/embedded_worker_registry.cc (right): > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > content/browser/service_worker/embedded_worker_registry.cc:241: process_id)); > On 2014/05/08 14:17:46, horo wrote: > > 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. > > 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() { oops, this needs to be named something else (we already have 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(); > }
On 2014/05/08 16:36:43, kinuko wrote: > On 2014/05/08 16:34:56, kinuko wrote: > > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > > File content/browser/service_worker/embedded_worker_instance.h (right): > > > > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > > content/browser/service_worker/embedded_worker_instance.h:110: public > > base::RefCounted<DevToolsManagerBridge> { > > On 2014/05/08 14:17:46, horo wrote: > > > On 2014/05/08 11:17:31, kinuko wrote: > > > > Does this need to be ref-counted? > > > > > > EmbeddedWorkerInstance could be deleted while the thread hopping. > > > IO-thread DevToolsManagerBridge::RegisterToDevToolsManager() > > > UI-thread RegisterToDevToolsManagerOnUI() > > > IO-thread DevToolsManagerBridge::RegisteredCallback() > > > So I think it must be ref-counted. > > > > This doesn't necessarily mean we need ref-counting. Also now that most of > > devtools related code is in this file having this class may not be that > > important. > > > > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > > File content/browser/service_worker/embedded_worker_registry.cc (right): > > > > > https://codereview.chromium.org/261753008/diff/200001/content/browser/service... > > content/browser/service_worker/embedded_worker_registry.cc:241: process_id)); > > On 2014/05/08 14:17:46, horo wrote: > > > 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. > > > > 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); > > } Probably we don't need this helper either, we can do this for ProcessAllocated callback and after that the dtor of Instance can take care of it if pid_ is not -1. > > 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() { > > oops, this needs to be named something else (we already have 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(); > > }
kinuko@ Please review patch set 8. I changed to call ServiceWorkerProcessManager from EmbeddedWorkerInstance. https://codereview.chromium.org/261753008/diff/200001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/200001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:241: process_id)); On 2014/05/08 16:34:57, kinuko wrote: > On 2014/05/08 14:17:46, horo wrote: > > 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. > > 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(); > } Done.
Thanks, looking good to me, haven't reviewed details yet but sending some comments earlier https://codereview.chromium.org/261753008/diff/320001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/261753008/diff/320001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:225: // TODO(horo): Remove this if statement when it becomes unnecessary. Um... looks like this is another undesirable effects of the fact that we don't properly stop workers https://codereview.chromium.org/261753008/diff/320001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/261753008/diff/320001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:38: : NON_EXPORTED_BASE(public base::SupportsWeakPtr<EmbeddedWorkerInstance>) { Can we not inherit from SupportsWeakPtr but use WeakPtrFactory instead? https://codereview.chromium.org/261753008/diff/320001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/320001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:36: context_->process_manager()->ReleaseWorkerProcess(process_id); Should we probably move this to EmbeddedWorkerInstance too? Also... looking this again perhaps this should be done in OnStopped rather than Stop() (and possibly in its dtor too)? (Per http://crbug.com/368832)
https://codereview.chromium.org/261753008/diff/320001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/261753008/diff/320001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:225: // TODO(horo): Remove this if statement when it becomes unnecessary. On 2014/05/13 05:45:19, kinuko wrote: > Um... looks like this is another undesirable effects of the fact that we don't > properly stop workers I think so too. This is a work around code. In current implementation of ServiceWorker, stopping instance code is not correctly implemented. https://codereview.chromium.org/261753008/diff/320001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/261753008/diff/320001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:38: : NON_EXPORTED_BASE(public base::SupportsWeakPtr<EmbeddedWorkerInstance>) { On 2014/05/13 05:45:19, kinuko wrote: > Can we not inherit from SupportsWeakPtr but use WeakPtrFactory instead? Done. https://codereview.chromium.org/261753008/diff/320001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (right): https://codereview.chromium.org/261753008/diff/320001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:36: context_->process_manager()->ReleaseWorkerProcess(process_id); On 2014/05/13 05:45:19, kinuko wrote: > Should we probably move this to EmbeddedWorkerInstance too? > > Also... looking this again perhaps this should be done in OnStopped rather than > Stop() (and possibly in its dtor too)? (Per http://crbug.com/368832) Done. Moved to EmbeddedWorkerInstance::Stop().
some more comments... https://codereview.chromium.org/261753008/diff/340001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/261753008/diff/340001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:228: // TODO(horo): Remove this if statement when it becomes unnecessary. nit: You can refer to the issue for stopping worker in the comment https://codereview.chromium.org/261753008/diff/340001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:344: DCHECK_EQ(WORKER_TERMINATED, it->second->state()); nit: extra space got in? https://codereview.chromium.org/261753008/diff/340001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/261753008/diff/340001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:127: context_->process_manager()->ReleaseWorkerProcess(process_id_); I wonder this should probably be in OnStopped and in dtor (as we do for NotifyWorkerDestroyed)?? https://codereview.chromium.org/261753008/diff/340001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:218: void EmbeddedWorkerInstance::RunSendStartWorker( I think we can likely remove this intermediate static method as the callback given to EmbeddedWorkerInstance::Start() is bound to the ServiceWorkerVersion's weak ptr (i.e. it won't fire anyway, or if it matters we can fire them at once in SWV's dtor).
https://codereview.chromium.org/261753008/diff/340001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/261753008/diff/340001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:228: // TODO(horo): Remove this if statement when it becomes unnecessary. On 2014/05/13 16:25:14, kinuko wrote: > nit: You can refer to the issue for stopping worker in the comment As we talked offline, I changed EmbeddedWorkerDevToolsManager to identify ServiceWorkers with storage_partition_path and service_worker_version_id, instead of service_worker_scope. So we can inspect different version of ServiceWorkers for same scope. https://codereview.chromium.org/261753008/diff/340001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:344: DCHECK_EQ(WORKER_TERMINATED, it->second->state()); On 2014/05/13 16:25:14, kinuko wrote: > nit: extra space got in? Done. https://codereview.chromium.org/261753008/diff/340001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/261753008/diff/340001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:127: context_->process_manager()->ReleaseWorkerProcess(process_id_); On 2014/05/13 16:25:14, kinuko wrote: > I wonder this should probably be in OnStopped and in dtor (as we do for > NotifyWorkerDestroyed)?? Done. https://codereview.chromium.org/261753008/diff/340001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:218: void EmbeddedWorkerInstance::RunSendStartWorker( On 2014/05/13 16:25:14, kinuko wrote: > I think we can likely remove this intermediate static method as the callback > given to EmbeddedWorkerInstance::Start() is bound to the ServiceWorkerVersion's > weak ptr (i.e. it won't fire anyway, or if it matters we can fire them at once > in SWV's dtor). Done.
Thanks, I might be still missing some corner cases but this lgtm
yurys@ Could you please review content/browser/devtools/embedded_worker_devtools_manager.*?
nasko Could you please review content/common/service_worker/embedded_worker_messages.h?
IPC LGTM
The CQ bit was checked by horo@chromium.org
The CQ bit was unchecked by horo@chromium.org
yurys@ ping?
https://codereview.chromium.org/261753008/diff/420001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/261753008/diff/420001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.h:44: // partition and the version id of ServiceWorker. Consider having a class like identifying by path is a good idea, one possible problem spot is with incognito profiles, i think storage_partition_path_ is empty for those
https://codereview.chromium.org/261753008/diff/420001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (left): https://codereview.chromium.org/261753008/diff/420001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:62: context_->process_manager()->ReleaseWorkerProcess(process_id); I suspect we're going to want a "Release" call both here and in Instance::OnStopped, like I have in https://github.com/jyasskin/chromium/compare/7915d09e...eb1569fc9#diff-0. If this Embedded Worker is the last worker in the process, we want to let the RPH kill the process just after StopWorker(), rather than needing to wait for the shutdown round trip. On the other hand, if it's not the last worker, we need the OnStopped handling per http://crbug.com/368832.
https://codereview.chromium.org/261753008/diff/420001/content/browser/service... File content/browser/service_worker/embedded_worker_registry.cc (left): https://codereview.chromium.org/261753008/diff/420001/content/browser/service... content/browser/service_worker/embedded_worker_registry.cc:62: context_->process_manager()->ReleaseWorkerProcess(process_id); On 2014/05/15 20:34:43, Jeffrey Yasskin wrote: > I suspect we're going to want a "Release" call both here and in > Instance::OnStopped, like I have in > https://github.com/jyasskin/chromium/compare/7915d09e...eb1569fc9#diff-0. If > this Embedded Worker is the last worker in the process, we want to let the RPH > kill the process just after StopWorker(), rather than needing to wait for the > shutdown round trip. On the other hand, if it's not the last worker, we need the > OnStopped handling per http://crbug.com/368832. Releasing the worker is also going to release the process (immediately), and I thought it'd be what we should do in the final shutdown phase.
https://codereview.chromium.org/261753008/diff/420001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/261753008/diff/420001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.h:44: // partition and the version id of ServiceWorker. Consider having a class like On 2014/05/15 19:15:15, michaeln wrote: > identifying by path is a good idea, one possible problem spot is with incognito > profiles, i think storage_partition_path_ is empty for those Thank you for pointing out. I didn't know that. I changed to use the pointer of ServiceWorkerContextCore.
yurys@ Could you please review content/browser/devtools/embedded_worker_devtools_manager.*?
https://codereview.chromium.org/261753008/diff/520001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/261753008/diff/520001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.h:45: // ServiceWorkerContextCore and the version id of ServiceWorker. Consider It may be a good time to fix this TODO. https://codereview.chromium.org/261753008/diff/520001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.h:89: const ServiceWorkerContextCore* const service_worker_context_; Could you educate me on what the lifetime of ServiceWorkerContextCore is? I'm wondering why we can be sure that instance referenced here is not dead. Also ServiceWorkerContextCore is supposed to be used on the IO thread-only [1] while this class is used on the UI thread. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
https://codereview.chromium.org/261753008/diff/520001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/261753008/diff/520001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.h:45: // ServiceWorkerContextCore and the version id of ServiceWorker. Consider On 2014/05/16 13:27:05, yurys wrote: > It may be a good time to fix this TODO. Done. https://codereview.chromium.org/261753008/diff/520001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.h:89: const ServiceWorkerContextCore* const service_worker_context_; On 2014/05/16 13:27:05, yurys wrote: > Could you educate me on what the lifetime of ServiceWorkerContextCore is? I'm > wondering why we can be sure that instance referenced here is not dead. Also > ServiceWorkerContextCore is supposed to be used on the IO thread-only [1] while > this class is used on the UI thread. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... ServiceWorkerContextCore is deleted after Profile (content::BrowserContext) is deleted. In EmbeddedWorkerDevToolsManager we use this pointer just for identifying ServiceWorker. So it is safe to use it on the UI thread.
content/browser/devtools/ - LGTM
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/261753008/520002
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/261753008/520002
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by horo@chromium.org
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/261753008/520002
Message was sent while issue was closed.
Change committed as 271144 |
