ServiceWorker: Fix data race in shutdown sequence

ServiceWorker: Fix data race in shutdown sequence

To avoid Worker thread's termination code touching released objects
owned by WebEmbeddedWorkerImpl, join the worker thread in the destructor
of WebEmbeddedWorkerImpl.

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

[Kunihiko Sakamoto, 2014-10-30 02:30:00]

ksakamoto@chromium.org changed reviewers:
+ haraken@chromium.org, horo@chromium.org

[Kunihiko Sakamoto, 2014-10-30 02:30:00]

Second attempt to fix TSAN warning (first one:
https://codereview.chromium.org/675423002/).

[haraken, 2014-10-30 02:40:25]

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:177:
m_workerThread->terminationEvent()->wait();

Can we create a helper method like WorkerThread::terminateAndWait() and use it
here?

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:178: }

Don't you need to call m_workerInspectorProxy->workerThreadTerminated() ?

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:229: m_workerThread->stop();

Not related to your CL, I wonder why you don't need to wait for the worker
thread termination here.

[Kunihiko Sakamoto, 2014-10-30 04:50:16]

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:177:
m_workerThread->terminationEvent()->wait();
On 2014/10/30 02:40:24, haraken wrote:
> 
> Can we create a helper method like WorkerThread::terminateAndWait() and use it
> here?

Done.

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:178: }
On 2014/10/30 02:40:24, haraken wrote:
> 
> Don't you need to call m_workerInspectorProxy->workerThreadTerminated() ?

No - WebEmbeddedWorkerImpl::terminateWorkerContext() should be called
beforehand.
(For the same reason, m_workerThread->stop() above isn't really needed. I've put
it just in case.)

https://codereview.chromium.org/684223004/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:229: m_workerThread->stop();
On 2014/10/30 02:40:24, haraken wrote:
> 
> Not related to your CL, I wonder why you don't need to wait for the worker
> thread termination here.

Worker thread can do its pending and cleanup tasks while this
WebEmbeddedWorkerImpl is alive.
Waiting its termination here might make things too sequential.

[haraken, 2014-10-30 05:12:13]

LGTM, but I want to have horo-san to confirm.

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

https://codereview.chromium.org/684223004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:406: void WorkerThread::stop()

Nit: We might want to rename "stop" to "terminate" in a follow-up CL.

[horo, 2014-10-30 08:14:47]

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

https://codereview.chromium.org/684223004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:420: stop();
I think we don't need to call stop() from the destructor of
WebEmbeddedWorkerImpl.

WebEmbeddedWorkerImpl is deleted after
ServiceWorkerGlobalScopeProxy::workerThreadTerminated() is called.
So when deleting WebEmbeddedWorkerImpl, m_workerThread->terminated() must be
true.

Adding "ASSERT(m_workerThread->terminated())" in the destructor of
WebEmbeddedWorkerImpl is better.

[Kunihiko Sakamoto, 2014-10-30 08:31:29]

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

https://codereview.chromium.org/684223004/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:420: stop();
On 2014/10/30 08:14:47, horo wrote:
> I think we don't need to call stop() from the destructor of
> WebEmbeddedWorkerImpl.
> 
> WebEmbeddedWorkerImpl is deleted after
> ServiceWorkerGlobalScopeProxy::workerThreadTerminated() is called.
> So when deleting WebEmbeddedWorkerImpl, m_workerThread->terminated() must be
> true.
> 
> Adding "ASSERT(m_workerThread->terminated())" in the destructor of
> WebEmbeddedWorkerImpl is better.

Done.

[horo, 2014-10-30 08:37:29]

lgtm

[Kunihiko Sakamoto, 2014-10-30 09:55:29]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-10-30 09:56:19]

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

[commit-bot: I haz the power, 2014-10-30 13:08:31]

Committed patchset #3 (id:40001) as 184647