Destruct base::Thread before WorkerThread::terminateAndWait returns

Destruct base::Thread before WorkerThread::terminateAndWait returns

This CL destructs base::Thread and its underlying system thread before
WorkerThread::terminateAndWait returns. This is important to make sure that
the main thread calls WTF::shutdown() after ThreadSpecifics of all threads
are destructed. See 345240 for more details.

BUG=345240
Committed: https://crrev.com/30d88091658428a57d4567d72f41a8a251031f68
Cr-Commit-Position: refs/heads/master@{#412735}

[haraken, 2016-08-17 06:52:23]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 06:52:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-08-17 06:53:05]

haraken@chromium.org changed reviewers:
+ glider@chromium.org, nhiroki@chromium.org

[haraken, 2016-08-17 06:53:05]

PTAL

[commit-bot: I haz the power, 2016-08-17 07:03:36]

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

[commit-bot: I haz the power, 2016-08-17 07:03:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-17 07:04:57]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 07:05:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-17 07:15:53]

yhirano@chromium.org changed reviewers:
+ yhirano@chromium.org

[yhirano, 2016-08-17 07:15:54]

https://codereview.chromium.org/2251903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2251903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:220:
clearWorkerBackingThread();
Don't you need to call this function in terminateAndWaitForAllWorkers as well?

[nhiroki, 2016-08-17 07:24:20]

Thank you for working on this!

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/SharedWorkerThread.h (right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/SharedWorkerThread.h:48: void
clearWorkerBackingThread() override {}
Why don't we nullify the thread here?

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:237:
thread->m_shutdownEvent->wait();
We may want to clear the worker backing thread here, too.

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/compositorworker/AbstractAnimationWorkletThread.h
(right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/compositorworker/AbstractAnimationWorkletThread.h:24:
void clearWorkerBackingThread() override {}
Just to confirm: We don't have to clear the backing thread here because it's
cleared in CompositorWorkerThread::clearSharedBackingThread(), which is called
from ModulesInitializer::shutdown() before WTF::shutdown().

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.h
(right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.h:48: void
clearWorkerBackingThread() override {}
Why don't we nullify the thread here?

[haraken, 2016-08-17 08:02:34]

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/SharedWorkerThread.h (right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/SharedWorkerThread.h:48: void
clearWorkerBackingThread() override {}
On 2016/08/17 07:24:19, nhiroki wrote:
> Why don't we nullify the thread here?

Done.

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:237:
thread->m_shutdownEvent->wait();
On 2016/08/17 07:24:19, nhiroki wrote:
> We may want to clear the worker backing thread here, too.

Done.

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/compositorworker/AbstractAnimationWorkletThread.h
(right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/compositorworker/AbstractAnimationWorkletThread.h:24:
void clearWorkerBackingThread() override {}
On 2016/08/17 07:24:19, nhiroki wrote:
> Just to confirm: We don't have to clear the backing thread here because it's
> cleared in CompositorWorkerThread::clearSharedBackingThread(), which is called
> from ModulesInitializer::shutdown() before WTF::shutdown().

You're right. Added a comment.

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.h
(right):

https://codereview.chromium.org/2251903002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerThread.h:48: void
clearWorkerBackingThread() override {}
On 2016/08/17 07:24:19, nhiroki wrote:
> Why don't we nullify the thread here?

Done.

https://codereview.chromium.org/2251903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2251903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:220:
clearWorkerBackingThread();
On 2016/08/17 07:15:54, yhirano wrote:
> Don't you need to call this function in terminateAndWaitForAllWorkers as well?

Done.

[nhiroki, 2016-08-17 08:15:02]

lgtm

[haraken, 2016-08-17 08:16:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-17 08:16:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-17 08:37:25]

lgtm

[Alexander Potapenko, 2016-08-17 10:02:53]

On 2016/08/17 08:37:25, yhirano wrote:
> lgtm

Please remove the suppression from build/sanitizers/tsan_suppressions.cc and run
the CL on the linux_tsan trybot.

[commit-bot: I haz the power, 2016-08-17 10:42:54]

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

[commit-bot: I haz the power, 2016-08-17 10:42:55]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-17 11:10:46]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[haraken, 2016-08-17 11:10:56]

On 2016/08/17 10:02:53, Alexander Potapenko wrote:
> On 2016/08/17 08:37:25, yhirano wrote:
> > lgtm
> 
> Please remove the suppression from build/sanitizers/tsan_suppressions.cc and
run
> the CL on the linux_tsan trybot.

Done. Trying the tsan bot.

[commit-bot: I haz the power, 2016-08-17 11:11:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 13:12:28]

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

[commit-bot: I haz the power, 2016-08-17 13:12:29]

Dry run: This issue passed the CQ dry run.

[Alexander Potapenko, 2016-08-17 13:36:17]

On 2016/08/17 11:10:56, haraken wrote:
> On 2016/08/17 10:02:53, Alexander Potapenko wrote:
> > On 2016/08/17 08:37:25, yhirano wrote:
> > > lgtm
> > 
> > Please remove the suppression from build/sanitizers/tsan_suppressions.cc and
> run
> > the CL on the linux_tsan trybot.
> 
> Done. Trying the tsan bot.

Can you please check whether the race in
ScreenOrientationBrowserTest.ScreenOrientationChange is related (see
https://chromium-swarm.appspot.com/user/task/30b126bf3b5cac10)?

[haraken, 2016-08-18 02:22:18]

On 2016/08/17 13:36:17, Alexander Potapenko wrote:
> On 2016/08/17 11:10:56, haraken wrote:
> > On 2016/08/17 10:02:53, Alexander Potapenko wrote:
> > > On 2016/08/17 08:37:25, yhirano wrote:
> > > > lgtm
> > > 
> > > Please remove the suppression from build/sanitizers/tsan_suppressions.cc
and
> > run
> > > the CL on the linux_tsan trybot.
> > 
> > Done. Trying the tsan bot.
> 
> Can you please check whether the race in
> ScreenOrientationBrowserTest.ScreenOrientationChange is related (see
> https://chromium-swarm.appspot.com/user/task/30b126bf3b5cac10)

How can I check if this CL fixes it? Can I just land the CL and see if the swarm
error is gone?

Either way I'd like to land this CL and address the issue separately.

[haraken, 2016-08-18 02:22:24]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-08-18 02:22:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2251903002/#ps80001
(title: "temp")

[commit-bot: I haz the power, 2016-08-18 02:22:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 04:02:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-18 04:06:03]

Description was changed from

==========
Destruct base::Thread before WorkerThread::terminateAndWait returns

This CL destructs base::Thread and its underlying system thread before
WorkerThread::terminateAndWait returns. This is important to make sure that
the main thread calls WTF::shutdown() after ThreadSpecifics of all threads
are destructed. See 345240 for more details.

BUG=345240
==========

to

==========
Destruct base::Thread before WorkerThread::terminateAndWait returns

This CL destructs base::Thread and its underlying system thread before
WorkerThread::terminateAndWait returns. This is important to make sure that
the main thread calls WTF::shutdown() after ThreadSpecifics of all threads
are destructed. See 345240 for more details.

BUG=345240

Committed: https://crrev.com/30d88091658428a57d4567d72f41a8a251031f68
Cr-Commit-Position: refs/heads/master@{#412735}
==========

[commit-bot: I haz the power, 2016-08-18 04:06:04]

Patchset 5 (id:??) landed as
https://crrev.com/30d88091658428a57d4567d72f41a8a251031f68
Cr-Commit-Position: refs/heads/master@{#412735}

[Alexander Potapenko, 2016-08-18 07:07:32]

On 2016/08/18 04:06:04, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/30d88091658428a57d4567d72f41a8a251031f68
> Cr-Commit-Position: refs/heads/master@{#412735}

Sorry, I just meant check that the stacks in the trybot log don't ring the bell.

[haraken, 2016-08-18 08:08:20]

On 2016/08/18 07:07:32, Alexander Potapenko wrote:
> On 2016/08/18 04:06:04, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/30d88091658428a57d4567d72f41a8a251031f68
> > Cr-Commit-Position: refs/heads/master@{#412735}
> 
> Sorry, I just meant check that the stacks in the trybot log don't ring the
bell.

Ah, I don't think that's related to this CL.