Don't handle DispatchOnInspectorBackend msg for SharedWorkers in DevToolsAgentFilter.

Don't handle DispatchOnInspectorBackend msg for SharedWorkers in DevToolsAgentFilter.

DevToolsAgentMsg_DispatchOnInspectorBackend message for SharedWorkers must be handled in SharedWorkerDevToolsAgent.

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

[horo, 2014-03-13 08:14:55]

kinuko@
Could you please review this cl?

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

https://codereview.chromium.org/198753002/diff/1/content/renderer/devtools/de...
File content/renderer/devtools/devtools_agent_filter.cc (right):

https://codereview.chromium.org/198753002/diff/1/content/renderer/devtools/de...
content/renderer/devtools/devtools_agent_filter.cc:67: message_handled_ = false;
Can you describe how you are going to execute devtools commands that require
pausing worker? In case of page scripts we interrupt v8 on the main thread and
make sure the messages are dispatched. What is the route for shared worker
messages, do they get directly from the IO thread to the shared worker thread?

[horo, 2014-03-14 10:21:04]

On 2014/03/13 09:18:06, yurys wrote:
>
https://codereview.chromium.org/198753002/diff/1/content/renderer/devtools/de...
> File content/renderer/devtools/devtools_agent_filter.cc (right):
> 
>
https://codereview.chromium.org/198753002/diff/1/content/renderer/devtools/de...
> content/renderer/devtools/devtools_agent_filter.cc:67: message_handled_ =
false;
> Can you describe how you are going to execute devtools commands that require
> pausing worker? In case of page scripts we interrupt v8 on the main thread and
> make sure the messages are dispatched. What is the route for shared worker
> messages, do they get directly from the IO thread to the shared worker thread?

DevToolsAgentMsg_DispatchOnInspectorBackend message to SharedWorker will go
through...
———
In UI thread
EmbeddedSharedWorkerStub::OnMessageReceived()
SharedWorkerDevToolsAgent::OnMessageReceived()
SharedWorkerDevToolsAgent::OnDispatchOnInspectorBackend()
void WebSharedWorkerImpl::dispatchDevToolsMessage(const WebString& message)
{
   
workerThread()->runLoop().postDebuggerTask(createCallbackTask(dispatchOnInspectorBackendTask,
String(message)));
    WorkerDebuggerAgent::interruptAndDispatchInspectorCommands(workerThread());
}
-> Worker thread
static void dispatchOnInspectorBackendTask(ExecutionContext* context, const
String& message)
{
   
toWorkerGlobalScope(context)->workerInspectorController()->dispatchMessageFromFrontend(message);
}
———
and will be executed in the worker thread.

Is my understanding incorrect?

[yurys, 2014-03-14 10:56:04]

On 2014/03/14 10:21:04, horo wrote:
> On 2014/03/13 09:18:06, yurys wrote:
> >
>
https://codereview.chromium.org/198753002/diff/1/content/renderer/devtools/de...
> > File content/renderer/devtools/devtools_agent_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/198753002/diff/1/content/renderer/devtools/de...
> > content/renderer/devtools/devtools_agent_filter.cc:67: message_handled_ =
> false;
> > Can you describe how you are going to execute devtools commands that require
> > pausing worker? In case of page scripts we interrupt v8 on the main thread
and
> > make sure the messages are dispatched. What is the route for shared worker
> > messages, do they get directly from the IO thread to the shared worker
thread?
> 
> DevToolsAgentMsg_DispatchOnInspectorBackend message to SharedWorker will go
> through...
> ———
> In UI thread
> EmbeddedSharedWorkerStub::OnMessageReceived()
> SharedWorkerDevToolsAgent::OnMessageReceived()
> SharedWorkerDevToolsAgent::OnDispatchOnInspectorBackend()
> void WebSharedWorkerImpl::dispatchDevToolsMessage(const WebString& message)
> {
>    
>
workerThread()->runLoop().postDebuggerTask(createCallbackTask(dispatchOnInspectorBackendTask,
> String(message)));
>    
WorkerDebuggerAgent::interruptAndDispatchInspectorCommands(workerThread());
> }
> -> Worker thread
> static void dispatchOnInspectorBackendTask(ExecutionContext* context, const
> String& message)
> {
>    
>
toWorkerGlobalScope(context)->workerInspectorController()->dispatchMessageFromFrontend(message);
> }
> ———
> and will be executed in the worker thread.
> 
> Is my understanding incorrect?

If a Page is running a script in a tight loop on the main thread then
EmbeddedSharedWorkerStub::OnMessageReceived will not be executed until the
script finishes. This will make worker debugger unresponsive. I see to options
here:
1) Do something like WebDevToolsAgent::interruptAndDispatch when message
addressed to a worker arrives
2) Have a way to call WebSharedWorkerImpl::dispatchDevToolsMessage on the IO
thread and post message to the worker directly bypassing main thread.

Do we have to pass all shared worker messages through the main thread?

[horo, 2014-03-17 10:55:41]

> If a Page is running a script in a tight loop on the main thread then
> EmbeddedSharedWorkerStub::OnMessageReceived will not be executed until the
> script finishes. This will make worker debugger unresponsive. I see to options
> here:
> 1) Do something like WebDevToolsAgent::interruptAndDispatch when message
> addressed to a worker arrives
> 2) Have a way to call WebSharedWorkerImpl::dispatchDevToolsMessage on the IO
> thread and post message to the worker directly bypassing main thread.

Oh yes, I see.
But how critical is this problem?
I think calling WebSharedWorkerImpl::dispatchDevToolsMessage from IO thread is
possible, but it makes the code complicated.


> Do we have to pass all shared worker messages through the main thread?
Not all.
But CreateWorker and TerminateWorkerContext message must be handled in the main
thread in the current implementation.

[yurys, 2014-03-18 06:00:31]

On 2014/03/17 10:55:41, horo wrote:
> > If a Page is running a script in a tight loop on the main thread then
> > EmbeddedSharedWorkerStub::OnMessageReceived will not be executed until the
> > script finishes. This will make worker debugger unresponsive. I see to
options
> > here:
> > 1) Do something like WebDevToolsAgent::interruptAndDispatch when message
> > addressed to a worker arrives
> > 2) Have a way to call WebSharedWorkerImpl::dispatchDevToolsMessage on the IO
> > thread and post message to the worker directly bypassing main thread.
> 
> Oh yes, I see.
> But how critical is this problem?
> I think calling WebSharedWorkerImpl::dispatchDevToolsMessage from IO thread is
> possible, but it makes the code complicated.
> 
I believe it is pretty uncommon and if it makes the code a way more complicated
then it is probably not worth implementing. As far as I understand you would
encounter the same issue if you passed messages addressed to worker through the
main thread (they would be blocked until current task finishes on the main
thread) but from your comment below it seems that they are routed in a different
manner. Can we simply pass DevTools messages the same way as we do with
postMessage requests addressed to the worker?

> 
> > Do we have to pass all shared worker messages through the main thread?
> Not all.
> But CreateWorker and TerminateWorkerContext message must be handled in the
main
> thread in the current implementation.
Can you point me at the route of a postMessage in case of shared worker?

[horo, 2014-03-18 11:41:28]

On 2014/03/18 06:00:31, yurys wrote:
> On 2014/03/17 10:55:41, horo wrote:
> > > If a Page is running a script in a tight loop on the main thread then
> > > EmbeddedSharedWorkerStub::OnMessageReceived will not be executed until the
> > > script finishes. This will make worker debugger unresponsive. I see to
> options
> > > here:
> > > 1) Do something like WebDevToolsAgent::interruptAndDispatch when message
> > > addressed to a worker arrives
> > > 2) Have a way to call WebSharedWorkerImpl::dispatchDevToolsMessage on the
IO
> > > thread and post message to the worker directly bypassing main thread.
> > 
> > Oh yes, I see.
> > But how critical is this problem?
> > I think calling WebSharedWorkerImpl::dispatchDevToolsMessage from IO thread
is
> > possible, but it makes the code complicated.
> > 
> I believe it is pretty uncommon and if it makes the code a way more
complicated
> then it is probably not worth implementing. As far as I understand you would
> encounter the same issue if you passed messages addressed to worker through
the
> main thread (they would be blocked until current task finishes on the main
> thread) but from your comment below it seems that they are routed in a
different
> manner. Can we simply pass DevTools messages the same way as we do with
> postMessage requests addressed to the worker?
> 
> > 
> > > Do we have to pass all shared worker messages through the main thread?
> > Not all.
> > But CreateWorker and TerminateWorkerContext message must be handled in the
> main
> > thread in the current implementation.
> Can you point me at the route of a postMessage in case of shared worker?

In the current implementation, postMessage of SharedWorker is also handled in
the main thread by WebMessagePortChannelImpl::OnMessageReceived().

I created a cl which enables the handling of MessagePort messages to bypass the
main thread.
https://codereview.chromium.org/203073005

But I'm wondering whether we should bypass the main thread or not.

[yurys, 2014-03-18 11:46:50]

On 2014/03/18 11:41:28, horo wrote:
> On 2014/03/18 06:00:31, yurys wrote:
> > On 2014/03/17 10:55:41, horo wrote:
> > > > If a Page is running a script in a tight loop on the main thread then
> > > > EmbeddedSharedWorkerStub::OnMessageReceived will not be executed until
the
> > > > script finishes. This will make worker debugger unresponsive. I see to
> > options
> > > > here:
> > > > 1) Do something like WebDevToolsAgent::interruptAndDispatch when message
> > > > addressed to a worker arrives
> > > > 2) Have a way to call WebSharedWorkerImpl::dispatchDevToolsMessage on
the
> IO
> > > > thread and post message to the worker directly bypassing main thread.
> > > 
> > > Oh yes, I see.
> > > But how critical is this problem?
> > > I think calling WebSharedWorkerImpl::dispatchDevToolsMessage from IO
thread
> is
> > > possible, but it makes the code complicated.
> > > 
> > I believe it is pretty uncommon and if it makes the code a way more
> complicated
> > then it is probably not worth implementing. As far as I understand you would
> > encounter the same issue if you passed messages addressed to worker through
> the
> > main thread (they would be blocked until current task finishes on the main
> > thread) but from your comment below it seems that they are routed in a
> different
> > manner. Can we simply pass DevTools messages the same way as we do with
> > postMessage requests addressed to the worker?
> > 
> > > 
> > > > Do we have to pass all shared worker messages through the main thread?
> > > Not all.
> > > But CreateWorker and TerminateWorkerContext message must be handled in the
> > main
> > > thread in the current implementation.
> > Can you point me at the route of a postMessage in case of shared worker?
> 
> In the current implementation, postMessage of SharedWorker is also handled in
> the main thread by WebMessagePortChannelImpl::OnMessageReceived().
> 
> I created a cl which enables the handling of MessagePort messages to bypass
the
> main thread.
> https://codereview.chromium.org/203073005
> 
> But I'm wondering whether we should bypass the main thread or not.

OK, let's start with handling devtools messages on the main thread and
reconsider this later when you decide on postMessage handling. If postMessages
are dispatched on the main thread it won't make sense to have devtools messages
routed differently.

[yurys, 2014-03-18 11:46:59]

LGTM

[horo, 2014-03-18 11:51:09]

On 2014/03/18 11:46:59, yurys wrote:
> LGTM

Thank you.

[horo, 2014-03-18 11:53:29]

jochen@
Could you please review the following files?
  content/renderer/render_thread_impl.h
  content/renderer/render_thread_impl.cc

[jochen (gone - plz use gerrit), 2014-03-19 11:02:58]

lgtm

[horo, 2014-03-20 02:27:35]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-03-20 02:27:54]

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

[commit-bot: I haz the power, 2014-03-20 02:29:40]

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

[commit-bot: I haz the power, 2014-03-20 02:29:41]

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

[horo, 2014-03-20 02:57:29]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-03-20 02:57:43]

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

[commit-bot: I haz the power, 2014-03-20 03:00:07]

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

[commit-bot: I haz the power, 2014-03-20 03:00:08]

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

[horo, 2014-03-20 03:30:25]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-03-20 03:30:48]

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

[commit-bot: I haz the power, 2014-03-20 06:58:09]

Change committed as 258237