WorkerThread should not start execution if WorkerThread::stop is already called

WorkerThread should not start execution if WorkerThread::stop is already called

If WorkerThread::stop is already called, WorkerThread::initialize should return immediately without starting the thread execution.

BUG=403179
TEST=none (it's timing-dependent)

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

[haraken, 2014-08-13 02:03:13]

PTAL

[horo, 2014-08-13 03:32:29]

I think we have to call workerGlobalScopeDestroyed() to delete
WorkerMessagingProxy (for dedicated workers), WebSharedWorkerImpl (for shared
workers) and WebEmbeddedWorker (for service workers).

Before r179309, workerGlobalScopeDestroyed() was always called when we call
WorkerThread::start() and WorkerThread::stop().

https://codereview.chromium.org/462423003/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/462423003/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:244:
m_thread->addTaskObserver(m_microtaskRunner.get());
Why do you remove these lines?

[haraken, 2014-08-13 04:13:42]

> I think we have to call workerGlobalScopeDestroyed() to delete
> WorkerMessagingProxy (for dedicated workers), WebSharedWorkerImpl (for shared
> workers) and WebEmbeddedWorker (for service workers).
> 
> Before r179309, workerGlobalScopeDestroyed() was always called when we call
> WorkerThread::start() and WorkerThread::stop().

Nice catch; Fixed.

https://codereview.chromium.org/462423003/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/462423003/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:244:
m_thread->addTaskObserver(m_microtaskRunner.get());
On 2014/08/13 03:32:29, horo wrote:
> Why do you remove these lines?

These lines are moved to after the 'if(m_terminated)' check. If the thread is
already stopped, we don't want to create a microtask runner.

[haraken, 2014-08-13 04:48:55]

https://codereview.chromium.org/462423003/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/462423003/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:251:
m_workerReportingProxy.workerGlobalScopeDestroyed();

I think that the name "workerGlobalScopeDestroyed" is a bit confusing. It's
weird that workerGlobalScopeStarted is called but workerGlobalScopeDestroyed is
not called. I'd suggest to rename it to workerThreadTerminated. (I'll do it in a
follow-up CL if you like it.)

[nasko, 2014-08-13 05:23:25]

lgtm

[horo, 2014-08-13 05:24:36]

lgtm

[haraken, 2014-08-13 05:24:40]

horo-san: Would you take another look?

[haraken, 2014-08-13 05:24:52]

oh, thanks!

[haraken, 2014-08-13 05:24:54]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-13 05:25:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/462423003/80001

[commit-bot: I haz the power, 2014-08-13 05:30:19]

Change committed as 180139