Fix a race condition during worker thread initialization

Fix a race condition during worker thread initialization

This patch fixes a race condition which can result in a worker thread
accessing freed memory during its initialization. One possible sequence
of events is:

1. The main thread creates the worker thread and posts a task to run
   WorkerThread::initialize().

2. Immediately after this, the main thread calls
   WorkerThread::terminateAndWait() to shut down the worker thread.

3. WorkerThread::terminateAndWait() notices that the worker hasn't
   initialized yet and signals m_terminationEvent.

4. The caller of terminateAndWait() assumes that the WorkerThread has
   been terminated and deallocates either the WorkerThread itself or
   some object that it depends on.

5. Finally, WorkerThread::initialize starts to run on the worker thread,
   accessing memory freed in step #4.

The fix is to always signal m_terminationEvent from the worker thread to
guarantee that we don't think the thread terminated before it actually
did.

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

[Sami, 2015-06-12 17:58:09]

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

[Sami, 2015-06-12 18:02:34]

Hi, does this look plausible?-) I can sort of reproduce the crash if I add this
at the end of WorkerThread::start() to make the termination and the
initialization race each other:

  Platform::current()->currentThread()->postTask(FROM_HERE, new
Task(bind(&WorkerThread::terminateAndWait, this)));

I feel the initialization/shutdown sequence is still a little more complicated
that it needs to be, but I'm not sure I want to try and rework it just now.

[kinuko, 2015-06-13 22:55:28]

Yes, this fix looks plausible.

I think I once suggested something like this in
https://code.google.com/p/chromium/issues/detail?id=492592#c6 but at the time we
concluded that WorkerThread itself is kept around until the task finishes, but
it looks there's still a corner case?  Anyways this lgtm.

On 2015/06/12 18:02:34, Sami wrote:
>   Platform::current()->currentThread()->postTask(FROM_HERE, new
> Task(bind(&WorkerThread::terminateAndWait, this)));

Is it possible to have this as a test?

> I feel the initialization/shutdown sequence is still a little more complicated
> that it needs to be, but I'm not sure I want to try and rework it just now.

Totally agreed. We recently started to re-examine these sequences (context:
crbug.com/487050) I hope we can make some improvements in this area too.

[Sami, 2015-06-15 20:18:25]

On 2015/06/13 22:55:28, kinuko wrote:
> Yes, this fix looks plausible.
> 
> I think I once suggested something like this in
> https://code.google.com/p/chromium/issues/detail?id=492592#c6 but at the time
we
> concluded that WorkerThread itself is kept around until the task finishes, but
> it looks there's still a corner case?  Anyways this lgtm.

Great, thanks for checking. I'm sorry for overlooking your original suggestion
-- I think I was trying too hard to minimize the changes in that earlier patch.

> On 2015/06/12 18:02:34, Sami wrote:
> >   Platform::current()->currentThread()->postTask(FROM_HERE, new
> > Task(bind(&WorkerThread::terminateAndWait, this)));
> 
> Is it possible to have this as a test?

Great idea. Just doing the above doesn't really trigger the bug reliably without
some strategically placed sleep()s, but I think it's still good insurance to
have the test. PTAL.

> > I feel the initialization/shutdown sequence is still a little more
complicated
> > that it needs to be, but I'm not sure I want to try and rework it just now.
> 
> Totally agreed. We recently started to re-examine these sequences (context:
> crbug.com/487050) I hope we can make some improvements in this area too.

Great to hear!

[kinuko, 2015-06-16 01:35:19]

lgtm, thanks!

https://codereview.chromium.org/1184833002/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/1184833002/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:233:
EXPECT_CALL(*m_mockWorkerReportingProxy,
workerGlobalScopeStarted(_)).Times(AnyNumber());
AnyNumber() -> AtMostOne(1) ?

[Sami, 2015-06-16 16:03:45]

https://codereview.chromium.org/1184833002/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/1184833002/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:233:
EXPECT_CALL(*m_mockWorkerReportingProxy,
workerGlobalScopeStarted(_)).Times(AnyNumber());
On 2015/06/16 01:35:19, kinuko wrote:
> AnyNumber() -> AtMostOne(1) ?

Ah, neat, I didn't know that!

[Sami, 2015-06-16 16:03:50]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-06-16 16:03:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1184833002/#ps60001
(title: "AnyNumber => AtMost")

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

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

[commit-bot: I haz the power, 2015-06-16 18:09:33]

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

[commit-bot: I haz the power, 2015-06-16 18:09:34]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66824)

[Sami, 2015-06-16 20:15:29]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-06-16 20:16:19]

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

[commit-bot: I haz the power, 2015-06-16 21:13:05]

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