Make DevTools support for the embedded SharedWorker.

Make DevTools support for the embedded SharedWorker.

BUG=327256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261314

[horo, 2014-03-12 06:48:29]

kinuko@
Could you please review?

[pfeldman, 2014-03-12 08:05:44]

I was hoping that we would not need worker manager operating on the IO thread
for the new type of workers. It seems like the SharedWorkerHost operates on the
IO thread, while devtools infrastructure requires a debugging target represented
by an entity on the UI thread (such as RenderViewHost/RenderFrameHost). Will
there ever be such an entity for the shared workers? I ask, because once it is
there, there would be no need in WorkerDevToolsManager altogether.

[horo, 2014-03-12 13:37:47]

On 2014/03/12 08:05:44, pfeldman wrote:
> I was hoping that we would not need worker manager operating on the IO thread
> for the new type of workers. It seems like the SharedWorkerHost operates on
the
> IO thread, while devtools infrastructure requires a debugging target
represented
> by an entity on the UI thread (such as RenderViewHost/RenderFrameHost). Will
> there ever be such an entity for the shared workers? I ask, because once it is
> there, there would be no need in WorkerDevToolsManager altogether.

Thank you for your advice.

I think SharedWorkerServiceImpl and SharedWorkerHost should be in IO thread
because
they just do some wiring tasks for SharedWorkers and there is no need to move it
to
UI thread except for DevTools.

But I think WorkerDevToolsManager don't need to be in IO thread for the new type
of
workers. It is because if WorkerDevToolsManager knows the worker_process_id and
worker_route_id it can send messages to the SharedWorker in the renderer process
using RenderProcessHost::FromID(worker_process_id).

[horo, 2014-03-13 08:07:26]

pfeldman@

I'm planning to introduce SharedWorkerDehvToolsManager which lives in UI thread.
Please see "Patch Set 3".
This class will be used instead of WorkerDevToolsManager when
"enable-embedded-shared-worker" flag is set.
SharedWorkerDehvToolsManager is more simpler than WorkerDevToolsManager because
it doesn't need thread hopping.

Could you please let me know how do you think about this idea?
Thank you.

[yurys, 2014-03-13 09:06:51]

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:162: return;
Looks like you need to remove the id from uninspected_workers_ here.

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:167:
!agent_host_it->second->IsAttached()) {
If the worker is destroyed and there is a detached agent host associated with
it, shouldn't we remove it?

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:259: if
(url.GetOrigin() != it->second->worker_url.GetOrigin())
Can we reuse a mechanism that implements a spec-compliant web workers equals
check (like WorkerProcessHost::WorkerInstance::Matches) instead of writing this
manually?

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:25: class
SharedWorkerDevtoolsAgentHost;
SharedWorkerDevtoolsAgentHost -> SharedWorkerDevToolsAgentHost

https://codereview.chromium.org/196503005/diff/100001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:67:
IPC_BEGIN_MESSAGE_MAP(WorkerDevToolsMessageHandler, msg)
This messages are DevTools-specific and we should only receive them if there is
a SharedWorkerDevtoolsAgentHost. So it would makes sense to move this logic into
SharedWorkerDevtoolsAgentHost, wdyt?

[horo, 2014-03-14 08:53:19]

Thank you very much for your review.

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:162: return;
On 2014/03/13 09:06:52, yurys wrote:
> Looks like you need to remove the id from uninspected_workers_ here.

The id is removed from uninspected_workers_ in line 158.

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:167:
!agent_host_it->second->IsAttached()) {
On 2014/03/13 09:06:52, yurys wrote:
> If the worker is destroyed and there is a detached agent host associated with
> it, shouldn't we remove it?

Done.

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:259: if
(url.GetOrigin() != it->second->worker_url.GetOrigin())
On 2014/03/13 09:06:52, yurys wrote:
> Can we reuse a mechanism that implements a spec-compliant web workers equals
> check (like WorkerProcessHost::WorkerInstance::Matches) instead of writing
this
> manually?

Done.

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:25: class
SharedWorkerDevtoolsAgentHost;
On 2014/03/13 09:06:52, yurys wrote:
> SharedWorkerDevtoolsAgentHost -> SharedWorkerDevToolsAgentHost

Done.

https://codereview.chromium.org/196503005/diff/100001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:67:
IPC_BEGIN_MESSAGE_MAP(WorkerDevToolsMessageHandler, msg)
On 2014/03/13 09:06:52, yurys wrote:
> This messages are DevTools-specific and we should only receive them if there
is
> a SharedWorkerDevtoolsAgentHost. So it would makes sense to move this logic
into
> SharedWorkerDevtoolsAgentHost, wdyt?

Done.

[horo, 2014-03-20 08:40:50]

yurys@
Ping?

[kinuko, 2014-03-24 12:24:01]

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:9: #include
"base/memory/singleton.h"
nit: we already have this in .h

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:51:
SetWorkerId(worker_id, false);
nit: can you add '/* reattach */' comment after 'false'?

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:68:
OnSaveAgentRumtimeState)
typo: Rumtime -> Runtime

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:76: message);
indent is off

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:77: }
nit: please have an empty line between methods unless they're for overrides from
the same class

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:79:
SaveAgentRuntimeState(state);
nit: as far as this is the only place we call it, I think we can just do state_
= state here

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:91: }
nit: no need of { } for single-line body

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:115: if
(has_worker_id_) {
Could we reach here other without going through ResetWorkerId() (which resets
has_worker_id_ and calls Release)?

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:125: }
nit: can we have one empty line here?

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:155: return new
SharedWorkerDevToolsAgentHost(id);
It looks we could return a new refptr without storing it to the map if
ConnectDevToolsAgentHostToWorker fails, is that ok?

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:21: // This class
lives in UI thread.
nit: in -> on

https://codereview.chromium.org/196503005/diff/240001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/196503005/diff/240001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:105:
base::Bind(WorkerCreatedOnUI,
& is missing?

[horo, 2014-03-25 06:28:17]

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:9: #include
"base/memory/singleton.h"
On 2014/03/24 12:24:02, kinuko wrote:
> nit: we already have this in .h

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:51:
SetWorkerId(worker_id, false);
On 2014/03/24 12:24:02, kinuko wrote:
> nit: can you add '/* reattach */' comment after 'false'?

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:68:
OnSaveAgentRumtimeState)
On 2014/03/24 12:24:02, kinuko wrote:
> typo: Rumtime -> Runtime

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:76: message);
On 2014/03/24 12:24:02, kinuko wrote:
> indent is off

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:77: }
On 2014/03/24 12:24:02, kinuko wrote:
> nit: please have an empty line between methods unless they're for overrides
from
> the same class

Done.
Moved to private.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:79:
SaveAgentRuntimeState(state);
On 2014/03/24 12:24:02, kinuko wrote:
> nit: as far as this is the only place we call it, I think we can just do
state_
> = state here

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:91: }
On 2014/03/24 12:24:02, kinuko wrote:
> nit: no need of { } for single-line body

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:115: if
(has_worker_id_) {
On 2014/03/24 12:24:02, kinuko wrote:
> Could we reach here other without going through ResetWorkerId() (which resets
> has_worker_id_ and calls Release)?

Done.
We should call RemoveInspectedWorkerData and
g_orphan_map.Get().erase(old_worker_id_) when has_old_worker_id_ is true.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:125: }
On 2014/03/24 12:24:02, kinuko wrote:
> nit: can we have one empty line here?

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:155: return new
SharedWorkerDevToolsAgentHost(id);
On 2014/03/24 12:24:02, kinuko wrote:
> It looks we could return a new refptr without storing it to the map if
> ConnectDevToolsAgentHostToWorker fails, is that ok? 

It is OK.
When ConnectDevToolsAgentHostToWorker fails, it means that there is no such a
SharedWorker.
The return value (scoped_refptr<DevToolsAgentHost>) of
DevToolsAgentHost::GetForWorker will have only one reference count, and
SharedWorkerDevToolsAgentHost will be deleted when the refptr will be released.

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/240001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:21: // This class
lives in UI thread.
On 2014/03/24 12:24:02, kinuko wrote:
> nit: in -> on

Done.

https://codereview.chromium.org/196503005/diff/240001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/196503005/diff/240001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:105:
base::Bind(WorkerCreatedOnUI,
On 2014/03/24 12:24:02, kinuko wrote:
> & is missing?

Done.

[kinuko, 2014-03-25 13:08:03]

lgtm

https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:32: void
WorkerCreatedOnUI(int worker_process_id,
(minor nit) I prefer naming these methods 'NotifyWorkerCreatedOnUI' etc since
the current method name sounds like 'a worker is created on UI' while it's not.

[horo, 2014-03-26 01:55:14]

https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.cc (right):

https://codereview.chromium.org/196503005/diff/300001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.cc:32: void
WorkerCreatedOnUI(int worker_process_id,
On 2014/03/25 13:08:04, kinuko wrote:
> (minor nit) I prefer naming these methods 'NotifyWorkerCreatedOnUI' etc since
> the current method name sounds like 'a worker is created on UI' while it's
not.

Done.

[horo, 2014-03-26 06:23:54]

yurys@chromium.org
Could you please review this CL?
I want to land this CL before the next branch cut.

Thank you.

[yurys, 2014-03-26 13:11:32]

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/100001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:259: if
(url.GetOrigin() != it->second->worker_url.GetOrigin())
On 2014/03/14 08:53:20, horo wrote:
> On 2014/03/13 09:06:52, yurys wrote:
> > Can we reuse a mechanism that implements a spec-compliant web workers equals
> > check (like WorkerProcessHost::WorkerInstance::Matches) instead of writing
> this
> > manually?
> 
> Done.
I wonder if we can use SharedWorkerInstance::Matches instead of the newly added
UrlNameMatches which does more relaxed comparison? We probably want to reattach
only the matching workers in the same profile. This would help us unify the code
with service workers.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:76:
->ConnectDevToolsAgentHostToWorker(worker_id)) {
style: wrong alignment

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:77:
NotifyCloseListener();
When this method is called from the constructor there is no close listener yet
and the listener added later will never be notified that the agent host doesn't
have underlying worker. We should probably return NULL from
SharedWorkerDevToolsManager::GetDevToolsAgentHostForWorker if there is no worker
with requested id and have ConnectDevToolsAgentHostToWorker always succeed when
called from here.

Also it may make sense to put the host into inspected_workers and
uninspected_workers maps in SharedWorkerDevToolsManager. In that case you
wouldn't have to first put the agent into uninspected_workers map in
WorkerContextStarted before calling SetWorkerId.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:106: if
(has_old_worker_id_) {
Please add a CHECK that has_worker_id_ is false or that the agent host is not in
g_agent_map.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:179:
AgentHosts::iterator orphan_agent_host_it = g_orphan_map.Get().find(old_id);
Storing reference to existing agent host in a field on WorkerInfo would allow us
to get rid of g_orphan_map and g_agent_map and I believe would simplify the
code, wdyt?

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:184:
SendMessageToWorker(
Now that this is done on the UI thread, how can we make sure that this messages
will be handled in the worker before its script starts running (i.e. before
EmbeddedWorkerMsg_StartWorker message is handled if understand embedded worker
implementation correctly)?

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:203: if
(!agent_host_it->second->IsAttached()) {
agent_host_it->second is used 5 times in this method, consider extracting it
into a local variable.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:273:
it->second->worker_url, it->second->worker_name, url, name)) {
style: wrong alignment

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:74: // that sent
"resume" message to the worker.
typo: was notified about that  and  sent ...

[alecflett, 2014-03-28 17:21:00]

yurys: ping. I'm kind of blocked on this CL as well.. I'm doing the same thing
for service workers and I'm basing a few things on this patch. (trying to share
devtools implementation with service workers as much as possible)

[pfeldman, 2014-03-28 17:24:56]

@alecflett: i can see comment #14 with yurys's comments. or is there anything
else you are missing feedback on?

[horo, 2014-03-29 06:53:38]

alecflett@
I'm sorry to have kept you waiting.

yurys@
I changed this cl.
* Stop using g_orphan_map and g_agent_map.
* Store the pointer of SharedWorkerDevToolsAgentHost in
SharedWorkerDevToolsManager::WorkerInfo.
* Store SharedWorkerInstance in SharedWorkerDevToolsManager::WorkerInfo to be
used in FindTerminatedWorker().
  - To do so we need to move the status variables of SharedWorker from
SharedWorkerInstance to SharedWorkerHost. 
    This will be done in http://crrev.com/214343002.
* Stop using DevToolsAgentMsg_PauseWorkerContextOnStart message.
* Use pause_on_start field to WorkerProcessMsg_CreateWorker_Params which is
(will be) introduced in http://crrev.com/213423008.

This cl includes http://crrev.com/213423008 (in CQ) and 
http://crrev.com/214343002 (under review) now.
Please review the files in content/browser/devtools/.

Thank you.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:76:
->ConnectDevToolsAgentHostToWorker(worker_id)) {
On 2014/03/26 13:11:32, yurys wrote:
> style: wrong alignment

Deleted

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:77:
NotifyCloseListener();
On 2014/03/26 13:11:32, yurys wrote:
> When this method is called from the constructor there is no close listener yet
> and the listener added later will never be notified that the agent host
doesn't
> have underlying worker. We should probably return NULL from
> SharedWorkerDevToolsManager::GetDevToolsAgentHostForWorker if there is no
worker
> with requested id and have ConnectDevToolsAgentHostToWorker always succeed
when
> called from here.
> 
> Also it may make sense to put the host into inspected_workers and
> uninspected_workers maps in SharedWorkerDevToolsManager. In that case you
> wouldn't have to first put the agent into uninspected_workers map in
> WorkerContextStarted before calling SetWorkerId.

Done.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:106: if
(has_old_worker_id_) {
On 2014/03/26 13:11:32, yurys wrote:
> Please add a CHECK that has_worker_id_ is false or that the agent host is not
in
> g_agent_map.

Changed to use worker_attached_.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:179:
AgentHosts::iterator orphan_agent_host_it = g_orphan_map.Get().find(old_id);
On 2014/03/26 13:11:32, yurys wrote:
> Storing reference to existing agent host in a field on WorkerInfo would allow
us
> to get rid of g_orphan_map and g_agent_map and I believe would simplify the
> code, wdyt?

Done.
I removed g_orphan_map and g_agent_map and change to store the pointer of agent
host in a field on WorkerInfo.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:184:
SendMessageToWorker(
On 2014/03/26 13:11:32, yurys wrote:
> Now that this is done on the UI thread, how can we make sure that this
messages
> will be handled in the worker before its script starts running (i.e. before
> EmbeddedWorkerMsg_StartWorker message is handled if understand embedded worker
> implementation correctly)?

I added pause_on_start field to WorkerProcessMsg_CreateWorker_Params in
http://crrev.com/213423008.
And I changed this cl to use this flag.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:203: if
(!agent_host_it->second->IsAttached()) {
On 2014/03/26 13:11:32, yurys wrote:
> agent_host_it->second is used 5 times in this method, consider extracting it
> into a local variable.

I changed this cl and use agent_host local variable.

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:273:
it->second->worker_url, it->second->worker_name, url, name)) {
On 2014/03/26 13:11:32, yurys wrote:
> style: wrong alignment

deleted

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/330001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:74: // that sent
"resume" message to the worker.
On 2014/03/26 13:11:32, yurys wrote:
> typo: was notified about that  and  sent ...

Done.

[yurys, 2014-03-31 13:48:23]

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:121: const
SharedWorkerInstance instance_;
Is it OK to store it by value?

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:235: for
(WorkerInfoMap::iterator paused_it = paused_workers_.begin();
How can the worker be in paused_workers_ map when this method is called? It
seems to me that DetachFromWorker must have been called before to release a
reference on corresponding SharedWorkerDevToolsAgentHost which happens only in
SharedWorkerDevToolsManager::WorkerDestroyed which in turn would have resulted
in moving the WorkerInfo from paused_workers_ to terminated_workers_.

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:73: WorkerInfoMap
paused_workers_;
All these worker WorkerInfo maps blow up the code and obscure the code making it
hard to track whether we always correctly dispose of WorkerInfo. My
understanding is that WorkerInfo should be created when a new worker is created
and destroyed when both following conditions are true:
1) The worker has been destroyed;
2) Corresponding ShareWorkerDevToolsAgentHost has been detroyed or never
existed.

The worker can be in one of the 4 states from the DevTools point of view:
uninspected (no SharedWorkerDevToolsAgentHost) -> inspected (there is
SharedWorkerDevToolsAgentHost for the worker) -> terminated (the worker was
destroyed while being inspected by a DevTools client) -> paused (terminated
worker was restarted and it should be reported to the DevTools client) ->
inspected.

Based on these observations it might be easier to have a single map worker id ->
WorkerInfo and have a flag stored in field on WorkerInfo which would reflect one
of the 4 DevTools states. Along with a pointer to SharedWorkerDevToolsAgentHost
that you have added this should cover all use cases be more readable. Is it
worth doing?

Sorry for such a long review, I understand that this code was already very
involved but as far as I remember that was mostly caused by jumping between the
UI and IO threads. Now that it works on a single thread I'd love to have it less
complicated.

[horo, 2014-04-01 08:34:08]

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:121: const
SharedWorkerInstance instance_;
On 2014/03/31 13:48:23, yurys wrote:
> Is it OK to store it by value?

Yes it is OK.
SharedWorkerInstance has WorkerStoragePartition, but copying it is thread-safe.

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:235: for
(WorkerInfoMap::iterator paused_it = paused_workers_.begin();
On 2014/03/31 13:48:23, yurys wrote:
> How can the worker be in paused_workers_ map when this method is called? It
> seems to me that DetachFromWorker must have been called before to release a
> reference on corresponding SharedWorkerDevToolsAgentHost which happens only in
> SharedWorkerDevToolsManager::WorkerDestroyed which in turn would have resulted
> in moving the WorkerInfo from paused_workers_ to terminated_workers_.

This could be occur when the DevToolsAgent is detached soon after the worker is
re-created and before the worker starts.


Created -> GetDevToolsAgentHost -> Register -> Started -> Destroyed ->
Re-created -> ClientHostClosing -> Destroyed

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:73: WorkerInfoMap
paused_workers_;
On 2014/03/31 13:48:23, yurys wrote:
> All these worker WorkerInfo maps blow up the code and obscure the code making
it
> hard to track whether we always correctly dispose of WorkerInfo. My
> understanding is that WorkerInfo should be created when a new worker is
created
> and destroyed when both following conditions are true:
> 1) The worker has been destroyed;
> 2) Corresponding ShareWorkerDevToolsAgentHost has been detroyed or never
> existed.
> 
> The worker can be in one of the 4 states from the DevTools point of view:
> uninspected (no SharedWorkerDevToolsAgentHost) -> inspected (there is
> SharedWorkerDevToolsAgentHost for the worker) -> terminated (the worker was
> destroyed while being inspected by a DevTools client) -> paused (terminated
> worker was restarted and it should be reported to the DevTools client) ->
> inspected.
> 
> Based on these observations it might be easier to have a single map worker id
->
> WorkerInfo and have a flag stored in field on WorkerInfo which would reflect
one
> of the 4 DevTools states. Along with a pointer to
SharedWorkerDevToolsAgentHost
> that you have added this should cover all use cases be more readable. Is it
> worth doing?
> 
> Sorry for such a long review, I understand that this code was already very
> involved but as far as I remember that was mostly caused by jumping between
the
> UI and IO threads. Now that it works on a single thread I'd love to have it
less
> complicated.

Done.
Yes that sounds good to me.
I introduced WorkerState enum.

[yurys, 2014-04-01 09:01:44]

LGTM!

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/570001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:235: for
(WorkerInfoMap::iterator paused_it = paused_workers_.begin();
On 2014/04/01 08:34:09, horo wrote:
> On 2014/03/31 13:48:23, yurys wrote:
> > How can the worker be in paused_workers_ map when this method is called? It
> > seems to me that DetachFromWorker must have been called before to release a
> > reference on corresponding SharedWorkerDevToolsAgentHost which happens only
in
> > SharedWorkerDevToolsManager::WorkerDestroyed which in turn would have
resulted
> > in moving the WorkerInfo from paused_workers_ to terminated_workers_.
> 
> This could be occur when the DevToolsAgent is detached soon after the worker
is
> re-created and before the worker starts.
> 
> 
> Created -> GetDevToolsAgentHost -> Register -> Started -> Destroyed ->
> Re-created -> ClientHostClosing -> Destroyed

Got it, thanks for explaining.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:164: if
(info->state() == WORKER_UNINSPECTED) {
nit: you might want to use switch here to have the compiler check that all enum
values are handled.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:187:
workers_.set(old_id, worker_info.Pass());
I don't think you need to reinsert the worker_info in this case, old_id should
be equal to id.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2014

[kinuko, 2014-04-01 16:01:05]

Some nits

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:40:
SharedWorkerDevToolsAgentHost(WorkerId worker_id)
nit: explicit

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:75:
host->AddRoute(worker_id_.second, this);
I assume for all these code we should be always safe.. (host != null)

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:150:
DCHECK(it->second->state() == WORKER_TERMINATED);
nit: use DCHECK_EQ() for better logging

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:164: if
(info->state() == WORKER_UNINSPECTED) {
On 2014/04/01 09:01:45, yurys wrote:
> nit: you might want to use switch here to have the compiler check that all
enum
> values are handled.

+1

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:54: };
Ah, this is neat

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:58: WorkerInfo(const
SharedWorkerInstance& instance)
nit: explicit

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.h:31: explicit
SharedWorkerHost(SharedWorkerInstance* instance,
no need of explicit now

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_instance.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_instance.h:19: class CONTENT_EXPORT
SharedWorkerInstance {
Might be better to have a comment to note that this is copyable value-type data
type and all given params are used only for comparison?

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.h:120:
SharedWorkerHost* FindSharedWorkerHost(
nit: maybe this one could be a (local) static method?

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl_unittest.cc
(right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:361: //
SharedWorkerDevToolsManager::WorkerCreated()
nit: please end comment with '.' (here and below)

[horo, 2014-04-02 04:31:57]

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.cc (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:40:
SharedWorkerDevToolsAgentHost(WorkerId worker_id)
On 2014/04/01 16:01:05, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:75:
host->AddRoute(worker_id_.second, this);
On 2014/04/01 16:01:05, kinuko wrote:
> I assume for all these code we should be always safe.. (host != null)

host is NULL in the unit tests.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:150:
DCHECK(it->second->state() == WORKER_TERMINATED);
On 2014/04/01 16:01:05, kinuko wrote:
> nit: use DCHECK_EQ() for better logging

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:164: if
(info->state() == WORKER_UNINSPECTED) {
On 2014/04/01 16:01:05, kinuko wrote:
> On 2014/04/01 09:01:45, yurys wrote:
> > nit: you might want to use switch here to have the compiler check that all
> enum
> > values are handled.
> 
> +1

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.cc:187:
workers_.set(old_id, worker_info.Pass());
On 2014/04/01 09:01:45, yurys wrote:
> I don't think you need to reinsert the worker_info in this case, old_id should
> be equal to id.

While in WORKER_PAUSED state, agent_host's worker_id is the id of the last
worker which has already been terminated.
While in WORKER_TERMINATED state, worker_info's key id in WorkerInfoMap should
be same as the agent_host's worker_id.
So I reinsert the worker_info here.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
File content/browser/devtools/shared_worker_devtools_manager.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/04/01 09:01:45, yurys wrote:
> 2014

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/devtool...
content/browser/devtools/shared_worker_devtools_manager.h:58: WorkerInfo(const
SharedWorkerInstance& instance)
On 2014/04/01 16:01:05, kinuko wrote:
> nit: explicit

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_host.h:31: explicit
SharedWorkerHost(SharedWorkerInstance* instance,
On 2014/04/01 16:01:05, kinuko wrote:
> no need of explicit now

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_instance.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_instance.h:19: class CONTENT_EXPORT
SharedWorkerInstance {
On 2014/04/01 16:01:05, kinuko wrote:
> Might be better to have a comment to note that this is copyable value-type
data
> type and all given params are used only for comparison?

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl.h (right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl.h:120:
SharedWorkerHost* FindSharedWorkerHost(
On 2014/04/01 16:01:05, kinuko wrote:
> nit: maybe this one could be a (local) static method?

Done.

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_service_impl_unittest.cc
(right):

https://codereview.chromium.org/196503005/diff/590001/content/browser/shared_...
content/browser/shared_worker/shared_worker_service_impl_unittest.cc:361: //
SharedWorkerDevToolsManager::WorkerCreated()
On 2014/04/01 16:01:05, kinuko wrote:
> nit: please end comment with '.' (here and below)

Done.

[horo, 2014-04-02 06:18:47]

jochen@
Could you please review content/renderer/render_thread_impl.cc in this cl?

Thank you.

[jochen (gone - plz use gerrit), 2014-04-02 09:27:07]

lgtm

[horo, 2014-04-02 13:36:38]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-02 13:37:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001

[commit-bot: I haz the power, 2014-04-02 18:15:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 18:15:22]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[horo, 2014-04-02 22:12:12]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-02 22:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001

[commit-bot: I haz the power, 2014-04-03 03:22:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 03:22:59]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[horo, 2014-04-03 04:04:13]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-03 04:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001

[commit-bot: I haz the power, 2014-04-03 06:47:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 06:47:16]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[horo, 2014-04-03 07:11:48]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-03 07:12:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/196503005/640001

[commit-bot: I haz the power, 2014-04-03 07:55:23]

Change committed as 261314

[horo, 2014-04-03 08:52:06]

A revert of this CL has been created in
https://codereview.chromium.org/223583002/ by horo@chromium.org.

The reason for reverting is: buildbot failure in Chromium Memory on Linux
ASan+LSan Tests

SharedWorkerDevToolsManagerTest.AttachTest test causes memory leaks..