workers: Change how debugger-tasks are posted to the thread.

workers: Change how debugger-tasks are posted to the thread.

debugger-tasks are only posted by WorkerInspectorProxy, and these tasks
are run somewhat differently from other tasks: a separate task runs
WorkerThread::runDebuggerTask, which ends up running the debugger tasks.
This CL keeps this basic mechanism, but moves the work related to this
from WorkerThread into WorkerInspectorProxy (since it is the only user of
debugger tasks).

BUG=486573
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195655

[sadrul, 2015-05-15 23:20:40]

sadrul@chromium.org changed reviewers:
+ kinuko@chromium.org, yurys@chromium.org

[yurys, 2015-05-15 23:41:41]

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.cpp:82:
workerThread->workerGlobalScope()->workerInspectorController()->dispatchMessageFromFrontend(message);
Why is it safe to access workerGlobalScope here? It seems to me that the task
might get executed on the platform thread when the WorkerGlobalScope is already
disposed.

[sadrul, 2015-05-16 00:50:53]

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.cpp:82:
workerThread->workerGlobalScope()->workerInspectorController()->dispatchMessageFromFrontend(message);
On 2015/05/15 23:41:41, yurys_slow wrote:
> Why is it safe to access workerGlobalScope here? It seems to me that the task
> might get executed on the platform thread when the WorkerGlobalScope is
already
> disposed.

The WorkerGlobalScope gets destroyed at the same time as the thread
(WorkerThread::shutdown()). So if the task is executing, the global-scope is
also still alive.

[yurys, 2015-05-16 00:55:24]

Thanks for explaining. LGTM.

[kinuko, 2015-05-18 15:19:49]

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.cpp:108:
m_workerThread->backingThread().postTask(location, new
Task(threadSafeBind(&runDebuggerTaskForWorker,
AllowCrossThreadAccess(m_workerThread))));
Could this be just threadSafeBind(&WorkerThread::runDebuggerTask,
AllowCrossThreadAccess(m_workerThread))

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:537: void
WorkerThread::appendDebuggerTask(PassOwnPtr<WebThread::Task> task)
Assuming that we're also going to remove willEnterNestedLoop etc from this
class, it feels that these two methods (append/run debugger task) and
m_debuggerMessageQueue could be possibly also moved out of this class, so we
could centralize these debugger-specific post tasking logic into one place--
could it be right?

[sadrul, 2015-05-19 03:50:47]

sadrul@chromium.org changed reviewers:
+ pfeldman@chromium.org

[sadrul, 2015-05-19 03:50:48]

+pfeldman@

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.cpp:108:
m_workerThread->backingThread().postTask(location, new
Task(threadSafeBind(&runDebuggerTaskForWorker,
AllowCrossThreadAccess(m_workerThread))));
On 2015/05/18 15:19:49, kinuko wrote:
> Could this be just threadSafeBind(&WorkerThread::runDebuggerTask,
> AllowCrossThreadAccess(m_workerThread))

backingThread() returns a WebThreadSupportingGC, which doesn't have a postTask()
version that takes a Function.

I am tempted to expose a WorkerThread::backingPlatformThread() (or just
WorkerThread::platformThread()?) that returns the WebThread, instead of
WebThreadSupportingGC. With that change, we won't need the Task to be explicitly
created here. WDYT?

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:537: void
WorkerThread::appendDebuggerTask(PassOwnPtr<WebThread::Task> task)
On 2015/05/18 15:19:49, kinuko wrote:
> Assuming that we're also going to remove willEnterNestedLoop etc from this
> class, it feels that these two methods (append/run debugger task) and
> m_debuggerMessageQueue could be possibly also moved out of this class, so we
> could centralize these debugger-specific post tasking logic into one place--
> could it be right?

That's an interesting idea. pfeldman@/yurus@: what do you think? Is this
something that could go with https://codereview.chromium.org/1133903005/?

[sadrul, 2015-05-19 16:23:06]

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.cpp:108:
m_workerThread->backingThread().postTask(location, new
Task(threadSafeBind(&runDebuggerTaskForWorker,
AllowCrossThreadAccess(m_workerThread))));
On 2015/05/19 03:50:47, sadrul wrote:
> On 2015/05/18 15:19:49, kinuko wrote:
> > Could this be just threadSafeBind(&WorkerThread::runDebuggerTask,
> > AllowCrossThreadAccess(m_workerThread))
> 
> backingThread() returns a WebThreadSupportingGC, which doesn't have a
postTask()
> version that takes a Function.
> 
> I am tempted to expose a WorkerThread::backingPlatformThread() (or just
> WorkerThread::platformThread()?) that returns the WebThread, instead of
> WebThreadSupportingGC. With that change, we won't need the Task to be
explicitly
> created here. WDYT?

I have made this change (moved WorkerThread::backingThread() to private,
introduced a public WorkerThread::platformThread(), which uses the result from
backingThread(), and removed WorkerThread::platformThreadId() in favour of the
new function). PTAL

[kinuko, 2015-05-20 08:17:46]

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerInspectorProxy.cpp (right):

https://codereview.chromium.org/1140503003/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerInspectorProxy.cpp:108:
m_workerThread->backingThread().postTask(location, new
Task(threadSafeBind(&runDebuggerTaskForWorker,
AllowCrossThreadAccess(m_workerThread))));
On 2015/05/19 16:23:06, sadrul wrote:
> On 2015/05/19 03:50:47, sadrul wrote:
> > On 2015/05/18 15:19:49, kinuko wrote:
> > > Could this be just threadSafeBind(&WorkerThread::runDebuggerTask,
> > > AllowCrossThreadAccess(m_workerThread))
> > 
> > backingThread() returns a WebThreadSupportingGC, which doesn't have a
> postTask()
> > version that takes a Function.
> > 
> > I am tempted to expose a WorkerThread::backingPlatformThread() (or just
> > WorkerThread::platformThread()?) that returns the WebThread, instead of
> > WebThreadSupportingGC. With that change, we won't need the Task to be
> explicitly
> > created here. WDYT?
> 
> I have made this change (moved WorkerThread::backingThread() to private,
> introduced a public WorkerThread::platformThread(), which uses the result from
> backingThread(), and removed WorkerThread::platformThreadId() in favour of the
> new function). PTAL

Sorry, I should have made it clearer.  I meant to ask if we could remove the
indirection method, i.e. runDebuggerTaskForWorker, like following:

new Task(threadSafeBind(&WorkerThread::runDebuggerTask,
AllowCrossThreadAccess(m_workerThread), WorkerThread::DontWaitForMessage))

(...but then now I realized that runDebuggerTask returns non-void value which
makes compiling the above code fail)

[kinuko, 2015-05-20 08:18:46]

Reg: PS2 I don't have strong opinion on the change, if it looks good to yury it
lgtm/2.

[sadrul, 2015-05-20 20:06:38]

Patchset #4 (id:60001) has been deleted

[sadrul, 2015-05-20 21:35:42]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-05-20 21:35:43]

The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1140503003/#ps80001
(title: "back-to-patchset2")

[commit-bot: I haz the power, 2015-05-20 21:36:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140503003/80001

[commit-bot: I haz the power, 2015-05-21 00:10:20]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195655