Worker: Initiate worker thread shutdown always on the main thread

Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and just prepare shutdown instead.

APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()

BUG=487050, 575532
Committed: https://crrev.com/2d6728fa52504193b1c511a4f675894e2d56e2e1
Cr-Commit-Position: refs/heads/master@{#395600}

[nhiroki, 2016-05-16 03:51:01]

Description was changed from

==========
Worker: Always initiate worker thread shutdown on the main thread

BUG=575532
==========

to

==========
Worker: Always initiate worker thread shutdown on the main thread

In the current implementation, worker shutdown is initiated twice on
WorkerMicrotaskRunner via workerGlobalScopeClosed() (see the following call
stacks) on the main thread and terminateFromWorkerThread() on the worker thread.
This is valid because actually the shutdown sequence does not run twice thanks
to the thread-safe |m_shutdown| flag.

However, to make it easier to implement the thread shutodwn observation
mechanism (see the issue), it would be better always to initiate the shutdown
from the main thread. To achieve that, this CL removes the shutdown call from
the worker thread and tweak the microtask runner implementation to initiate
the shutdown only one time.

Thread termination sequence via workerGlobalScoped() on each worker mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

[nhiroki, 2016-05-16 03:58:22]

Description was changed from

==========
Worker: Always initiate worker thread shutdown on the main thread

In the current implementation, worker shutdown is initiated twice on
WorkerMicrotaskRunner via workerGlobalScopeClosed() (see the following call
stacks) on the main thread and terminateFromWorkerThread() on the worker thread.
This is valid because actually the shutdown sequence does not run twice thanks
to the thread-safe |m_shutdown| flag.

However, to make it easier to implement the thread shutodwn observation
mechanism (see the issue), it would be better always to initiate the shutdown
from the main thread. To achieve that, this CL removes the shutdown call from
the worker thread and tweak the microtask runner implementation to initiate
the shutdown only one time.

Thread termination sequence via workerGlobalScoped() on each worker mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown is initiated twice on
WorkerMicrotaskRunner via WorkerReportingProxy::workerGlobalScopeClosed() (see
below) on the main thread and WorkerThread::terminateFromWorkerThread() on the
worker thread. This is valid because actually the shutdown sequence does not
run twice thanks to the thread-safe WorkerThread::m_shutdown flag.

However, to achieve the thread shutodwn observation mechanism (see the issue),
it would be better to initiate the shutdown always on the main thread. For
that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

[nhiroki, 2016-05-16 03:58:43]

Description was changed from

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown is initiated twice on
WorkerMicrotaskRunner via WorkerReportingProxy::workerGlobalScopeClosed() (see
below) on the main thread and WorkerThread::terminateFromWorkerThread() on the
worker thread. This is valid because actually the shutdown sequence does not
run twice thanks to the thread-safe WorkerThread::m_shutdown flag.

However, to achieve the thread shutodwn observation mechanism (see the issue),
it would be better to initiate the shutdown always on the main thread. For
that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown is initiated twice on
WorkerMicrotaskRunner via WorkerReportingProxy::workerGlobalScopeClosed() (see
below) on the main thread and WorkerThread::terminateFromWorkerThread() on the
worker thread. This is valid because actually the shutdown sequence does not
run twice thanks to the thread-safe WorkerThread::m_shutdown flag.

However, to achieve the thread shutodwn observation mechanism (see the issue),
it would be better to initiate the shutdown always on the main thread. For
that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

[nhiroki, 2016-05-16 04:06:20]

Description was changed from

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown is initiated twice on
WorkerMicrotaskRunner via WorkerReportingProxy::workerGlobalScopeClosed() (see
below) on the main thread and WorkerThread::terminateFromWorkerThread() on the
worker thread. This is valid because actually the shutdown sequence does not
run twice thanks to the thread-safe WorkerThread::m_shutdown flag.

However, to achieve the thread shutodwn observation mechanism (see the issue),
it would be better to initiate the shutdown always on the main thread. For
that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask():

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see the call stacks below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutodwn observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread.

For that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

[nhiroki, 2016-05-16 04:08:48]

Description was changed from

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask():

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see the call stacks below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutodwn observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread.

For that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This code path will be removed by [1]).
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();

[1] https://codereview.chromium.org/1970003004/

BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask():

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutodwn observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread.

For that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

[nhiroki, 2016-05-16 04:13:06]

Description was changed from

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask():

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutodwn observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread.

For that, this CL removes the shutdown call from the worker thread and tweak the
microtask runner implementation.

Thread termination sequence via workerGlobalScopeClosedd() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when WorkerGlobalScope::close() is
called:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and tweak the microtask runner implementation.


Thread termination sequence via workerGlobalScopeClosed() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

[nhiroki, 2016-05-16 04:51:21]

Description was changed from

==========
Worker: Initiate worker thread shutdown alyways on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when WorkerGlobalScope::close() is
called:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and tweak the microtask runner implementation.


Thread termination sequence via workerGlobalScopeClosed() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when WorkerGlobalScope::close() is
called:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and tweak the microtask runner implementation.


Thread termination sequence via workerGlobalScopeClosed() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

[nhiroki, 2016-05-16 04:58:48]

nhiroki@chromium.org changed reviewers:
+ kinuko@chromium.org, yhirano@chromium.org

[nhiroki, 2016-05-16 04:59:28]

PTAL, thanks!

[nhiroki, 2016-05-16 05:00:46]

Description was changed from

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when WorkerGlobalScope::close() is
called:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and tweak the microtask runner implementation.


Thread termination sequence via workerGlobalScopeClosed() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when WorkerGlobalScope::close() is
called:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and tweaks the microtask runner implementation.


Thread termination sequence via workerGlobalScopeClosed() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

[nhiroki, 2016-05-16 21:45:56]

On 2016/05/16 04:59:28, nhiroki wrote:
> PTAL, thanks!

Hmm..., apparently this CL is not complete for initiating a shutdown task on the
main thread. Let me recall this and make a review request again.

[nhiroki, 2016-05-16 21:46:08]

nhiroki@chromium.org changed reviewers:
- kinuko@chromium.org, yhirano@chromium.org

[nhiroki, 2016-05-20 02:01:40]

Description was changed from

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when WorkerGlobalScope::close() is
called:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and tweaks the microtask runner implementation.


Thread termination sequence via workerGlobalScopeClosed() on each worker
mechanism:

// For DedicatedWorker and CompositorWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// For SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// For ServiceWorker (NOTE: This call path will be removed by
// https://codereview.chromium.org/1970003004/)
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  WebEmbeddedWorkerImpl::terminateWorkerContext()
    m_workerThread->terminate();


BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and does just shutdown preparation instead.


APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()


BUG=575532
==========

[nhiroki, 2016-05-20 02:06:10]

nhiroki@chromium.org changed reviewers:
+ kinuko@chromium.org, yhirano@chromium.org

[nhiroki, 2016-05-20 02:06:10]

The patchset 2 is now ready to review. PTAL, thanks!

[nhiroki, 2016-05-20 04:55:04]

Description was changed from

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and does just shutdown preparation instead.


APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()


BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and just prepare shutdown instead.


APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()


BUG=575532
==========

[kinuko, 2016-05-20 15:19:30]

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:78: // Dispose
WorkerGlobalScope to avoid processing the next task.
Wouldn't this make DCHECK fail in WorkerThread::performTask?

Should we instead just flip a flag to indicate that the worker's closed (and
check that in performTask) so that WorkerGlobalScope is always disposed in the
termination sequence from main thread?

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:349: if (m_shutdown)
Do we still need this flag?

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:420: if (m_shutdown)
This could be check m_readyToShutdown instead?

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:80: void
prepareShutdownTask();
nit: prepareForShutdownTask? or maybe just prepareForShutdown?

[nhiroki, 2016-05-23 01:32:27]

Thank you for reviewing! Comment replies only (not updated yet).

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:78: // Dispose
WorkerGlobalScope to avoid processing the next task.
On 2016/05/20 15:19:29, kinuko wrote:
> Wouldn't this make DCHECK fail in WorkerThread::performTask?

We never hit the check because prepareShutdownTask does not nullify
m_workerGlobalScope.

> Should we instead just flip a flag to indicate that the worker's closed (and
> check that in performTask) so that WorkerGlobalScope is always disposed in the
> termination sequence from main thread?

Hmm... I may misunderstand the task/event execution flow on the worker thread.
Let me confirm it...

In my understanding, we need to dispose WorkerOrWorkletScriptController,
ActiveDOMObjects, EventListeners and WorkerEventQueue via
WorkerGlobalScope::dispose() here so that they don't run already queued
tasks/events after close(). If we don't call dispose() here, queued tasks/events
can be executed before the shutdown task is posted from the main thread.

Isn't this correct? Are all tasks/events executed via
WorkerThread::performTask()?

[nhiroki, 2016-05-23 06:21:58]

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:349: if (m_shutdown)
On 2016/05/20 15:19:29, kinuko wrote:
> Do we still need this flag?

Yes, it's not necessary. Removed.

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:420: if (m_shutdown)
On 2016/05/20 15:19:29, kinuko wrote:
> This could be check m_readyToShutdown instead?

That should work. Replaced.

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:80: void
prepareShutdownTask();
On 2016/05/20 15:19:29, kinuko wrote:
> nit: prepareForShutdownTask? or maybe just prepareForShutdown?

Done.

[kinuko, 2016-05-23 09:35:26]

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:78: // Dispose
WorkerGlobalScope to avoid processing the next task.
On 2016/05/23 01:32:27, nhiroki wrote:
> On 2016/05/20 15:19:29, kinuko wrote:
> > Wouldn't this make DCHECK fail in WorkerThread::performTask?
> 
> We never hit the check because prepareShutdownTask does not nullify
> m_workerGlobalScope.
> 
> > Should we instead just flip a flag to indicate that the worker's closed (and
> > check that in performTask) so that WorkerGlobalScope is always disposed in
the
> > termination sequence from main thread?
> 
> Hmm... I may misunderstand the task/event execution flow on the worker thread.
> Let me confirm it...
> 
> In my understanding, we need to dispose WorkerOrWorkletScriptController,
> ActiveDOMObjects, EventListeners and WorkerEventQueue via
> WorkerGlobalScope::dispose() here so that they don't run already queued
> tasks/events after close(). If we don't call dispose() here, queued
tasks/events
> can be executed before the shutdown task is posted from the main thread.
> 
> Isn't this correct? Are all tasks/events executed via
> WorkerThread::performTask()?

Hm, I think we need to call WebThreadSupportingGC::shutdown() (that's called via
WorkerBackingThread::detach and calls WebScheduler::shutdown for owning thread)
to make really sure no tasks that have been already posted run.  The tasks that
are queued to event queue seem to be also posted via createWorkerThread (via
WorkerGlobalScope::postTask -> WorkerThread::postTask) so they should go through
WorkerThread::performTask().  Reg: the last question that was my assumption but
I haven't literally made sure if every task goes through that path and I could
be wrong.

[yhirano, 2016-05-23 10:05:19]

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

https://codereview.chromium.org/1978163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:245:
DCHECK(isCurrentThread());
It might be good to assert m_terminated && m_readyToShutdown while you need a
#if DCHECK_IS_ON block for a mutex.

https://codereview.chromium.org/1978163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:379:
workerBackingThread().backingThread().postTask(BLINK_FROM_HERE,
threadSafeBind(&WorkerThread::shutdown, AllowCrossThreadAccess(this)));
Can you post two tasks (prepareForShutdown and performShutdownTask) here and
remove shutdown method? It's a bit confusing to me that shutdown can be called
even when prepareForShutdown is called.

[nhiroki, 2016-05-24 06:50:43]

Thank you for reviewing! Updated. PTAL :)

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:78: // Dispose
WorkerGlobalScope to avoid processing the next task.
On 2016/05/23 09:35:25, kinuko wrote:
> On 2016/05/23 01:32:27, nhiroki wrote:
> > On 2016/05/20 15:19:29, kinuko wrote:
> > > Wouldn't this make DCHECK fail in WorkerThread::performTask?
> > 
> > We never hit the check because prepareShutdownTask does not nullify
> > m_workerGlobalScope.
> > 
> > > Should we instead just flip a flag to indicate that the worker's closed
(and
> > > check that in performTask) so that WorkerGlobalScope is always disposed in
> the
> > > termination sequence from main thread?
> > 
> > Hmm... I may misunderstand the task/event execution flow on the worker
thread.
> > Let me confirm it...
> > 
> > In my understanding, we need to dispose WorkerOrWorkletScriptController,
> > ActiveDOMObjects, EventListeners and WorkerEventQueue via
> > WorkerGlobalScope::dispose() here so that they don't run already queued
> > tasks/events after close(). If we don't call dispose() here, queued
> tasks/events
> > can be executed before the shutdown task is posted from the main thread.
> > 
> > Isn't this correct? Are all tasks/events executed via
> > WorkerThread::performTask()?
> 
> Hm, I think we need to call WebThreadSupportingGC::shutdown() (that's called
via
> WorkerBackingThread::detach and calls WebScheduler::shutdown for owning
thread)
> to make really sure no tasks that have been already posted run.  The tasks
that
> are queued to event queue seem to be also posted via createWorkerThread (via
> WorkerGlobalScope::postTask -> WorkerThread::postTask) so they should go
through
> WorkerThread::performTask().

Thank you for clarifying this. I got a better understanding of the shutdown
sequence and the event queue mechanism. 

> Reg: the last question that was my assumption but
> I haven't literally made sure if every task goes through that path and I could
> be wrong.

AFAICS, DOMTimer could dispatch an event via a different path than
WorkerThread::performTask() as follows:

DOMTimer::DOMTimer()  // Called on the worker thread
  TimerBase::start()
    TimerBase::setNextFireTime()
      timerTaskRunner()->postDelayedTask()  // post a task to the worker thread

TimerBase::runInternal()  // Called on the worker thread
  DOMTimer::fired()
    ScheduledAction::execute()
      WorkerOrWorkletScriptController::evaluate()

WorkerGlobalScope::stopActiveDOMObjects() (called in
WorkerGlobalScope::dispose()) can prevent an event dispatch via this path, so
it'd be safer to dispose WorkerGlobalScope here. Wdyt?

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

https://codereview.chromium.org/1978163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:245:
DCHECK(isCurrentThread());
On 2016/05/23 10:05:19, yhirano wrote:
> It might be good to assert m_terminated && m_readyToShutdown while you need a
> #if DCHECK_IS_ON block for a mutex.

Done.

https://codereview.chromium.org/1978163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:379:
workerBackingThread().backingThread().postTask(BLINK_FROM_HERE,
threadSafeBind(&WorkerThread::shutdown, AllowCrossThreadAccess(this)));
On 2016/05/23 10:05:19, yhirano wrote:
> Can you post two tasks (prepareForShutdown and performShutdownTask) here and
> remove shutdown method? It's a bit confusing to me that shutdown can be called
> even when prepareForShutdown is called.

Done.

[yhirano, 2016-05-24 07:30:23]

lgtm

[kinuko, 2016-05-24 09:07:49]

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

https://codereview.chromium.org/1978163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:78: // Dispose
WorkerGlobalScope to avoid processing the next task.
On 2016/05/24 06:50:43, nhiroki wrote:
> On 2016/05/23 09:35:25, kinuko wrote:
> > On 2016/05/23 01:32:27, nhiroki wrote:
> > > On 2016/05/20 15:19:29, kinuko wrote:
> > > > Wouldn't this make DCHECK fail in WorkerThread::performTask?
> > > 
> > > We never hit the check because prepareShutdownTask does not nullify
> > > m_workerGlobalScope.
> > > 
> > > > Should we instead just flip a flag to indicate that the worker's closed
> (and
> > > > check that in performTask) so that WorkerGlobalScope is always disposed
in
> > the
> > > > termination sequence from main thread?
> > > 
> > > Hmm... I may misunderstand the task/event execution flow on the worker
> thread.
> > > Let me confirm it...
> > > 
> > > In my understanding, we need to dispose WorkerOrWorkletScriptController,
> > > ActiveDOMObjects, EventListeners and WorkerEventQueue via
> > > WorkerGlobalScope::dispose() here so that they don't run already queued
> > > tasks/events after close(). If we don't call dispose() here, queued
> > tasks/events
> > > can be executed before the shutdown task is posted from the main thread.
> > > 
> > > Isn't this correct? Are all tasks/events executed via
> > > WorkerThread::performTask()?
> > 
> > Hm, I think we need to call WebThreadSupportingGC::shutdown() (that's called
> via
> > WorkerBackingThread::detach and calls WebScheduler::shutdown for owning
> thread)
> > to make really sure no tasks that have been already posted run.  The tasks
> that
> > are queued to event queue seem to be also posted via createWorkerThread (via
> > WorkerGlobalScope::postTask -> WorkerThread::postTask) so they should go
> through
> > WorkerThread::performTask().
> 
> Thank you for clarifying this. I got a better understanding of the shutdown
> sequence and the event queue mechanism. 
> 
> > Reg: the last question that was my assumption but
> > I haven't literally made sure if every task goes through that path and I
could
> > be wrong.
> 
> AFAICS, DOMTimer could dispatch an event via a different path than
> WorkerThread::performTask() as follows:
> 
> DOMTimer::DOMTimer()  // Called on the worker thread
>   TimerBase::start()
>     TimerBase::setNextFireTime()
>       timerTaskRunner()->postDelayedTask()  // post a task to the worker
thread
> 
> TimerBase::runInternal()  // Called on the worker thread
>   DOMTimer::fired()
>     ScheduledAction::execute()
>       WorkerOrWorkletScriptController::evaluate()
> 
> WorkerGlobalScope::stopActiveDOMObjects() (called in
> WorkerGlobalScope::dispose()) can prevent an event dispatch via this path, so
> it'd be safer to dispose WorkerGlobalScope here. Wdyt?

Hmm, thanks for taking a look, looks like we still need to call it. This timer
sequence is a bit unfortunate given that we're deprecating timer functions...

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:77: // Dispose
WorkerGlobalScope to avoid processing the next task.
Could we make this comment a bit clearer, e.g. "to stop associated
ActiveDOMObjects and close the event queue" or sth like that

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:119:
task->performTask(globalScope);
Going back to this method... how do we prevent tasks that are already posted
from running here?

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:234: void
WorkerThread::performShutdownTask()
nit: maybe we could rename this to performShutdown() (method renaming /
reordering could be done in a follow-up cleanup CL too)

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:301: if
(workerBackingThread().workerScriptCount() == 1) {
Actually if we've passed m_readyToShutdown we won't need to go through this
block either?

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.h:79: void
prepareForShutdown();
nit: could we make WorkerMicroTaskRunner a private inner class of WorkerThread
and make this method private as well?  I don't feel this needs to be a public
method.

[nhiroki, 2016-05-24 09:50:40]

Description was changed from

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and just prepare shutdown instead.


APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()


BUG=575532
==========

to

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and just prepare shutdown instead.


APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()


BUG=487050, 575532
==========

[nhiroki, 2016-05-24 12:29:35]

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

[commit-bot: I haz the power, 2016-05-24 12:29:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978163002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978163002/120001

[nhiroki, 2016-05-24 13:36:06]

Thank you! Updated.

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:77: // Dispose
WorkerGlobalScope to avoid processing the next task.
On 2016/05/24 09:07:48, kinuko wrote:
> Could we make this comment a bit clearer, e.g. "to stop associated
> ActiveDOMObjects and close the event queue" or sth like that

Done.

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:119:
task->performTask(globalScope);
On 2016/05/24 09:07:48, kinuko wrote:
> Going back to this method... how do we prevent tasks that are already posted
> from running here?

We could just return if |m_readyToShutdown| is set. Done.

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:234: void
WorkerThread::performShutdownTask()
On 2016/05/24 09:07:48, kinuko wrote:
> nit: maybe we could rename this to performShutdown() (method renaming /
> reordering could be done in a follow-up cleanup CL too)

I'll make a separate CL.

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:301: if
(workerBackingThread().workerScriptCount() == 1) {
On 2016/05/24 09:07:49, kinuko wrote:
> Actually if we've passed m_readyToShutdown we won't need to go through this
> block either?

Right. We can assume that the execution context is not running in the case.
Moved this block into the block conditioned with !m_readyToShutdown and added
comments.

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.h:79: void
prepareForShutdown();
On 2016/05/24 09:07:49, kinuko wrote:
> nit: could we make WorkerMicroTaskRunner a private inner class of WorkerThread
> and make this method private as well?  I don't feel this needs to be a public
> method.

Agreed. I'll make a separate CL for this.

[commit-bot: I haz the power, 2016-05-24 13:52:19]

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

[commit-bot: I haz the power, 2016-05-24 13:52:20]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-05-24 14:49:10]

lgtm

(It'd be nice if we could have tests that could test some shutdown
corner-cases.. but we could see if this works first and iterate on this too)

[nhiroki, 2016-05-24 15:15:08]

On 2016/05/24 14:49:10, kinuko wrote:
> lgtm
> 
> (It'd be nice if we could have tests that could test some shutdown
> corner-cases.. but we could see if this works first and iterate on this too)

Thank you! I plan to add some tests w/ graceful shutdown changes.

[nhiroki, 2016-05-24 15:17:13]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-05-24 15:17:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1978163002/#ps120001
(title: "address kinuko's comments")

[commit-bot: I haz the power, 2016-05-24 15:17:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978163002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978163002/120001

[commit-bot: I haz the power, 2016-05-24 15:21:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-24 15:23:24]

Description was changed from

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and just prepare shutdown instead.


APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()


BUG=487050, 575532
==========

to

==========
Worker: Initiate worker thread shutdown always on the main thread

In the current implementation, worker shutdown can be initiated twice on
WorkerMicrotaskRunner::didProcessTask() when close() is called on
{Dedicated,Shared}WorkerGlobalScope:

  1. WorkerReportingProxy::workerGlobalScopeClosed() eventually calls
     WorkerThread::terminate() on the main thread (see call pathes below).
  2. WorkerThread::terminateFromWorkerThread() is directly called on the worker
     thread.

These are valid because the shutdown sequence actually does not run twice thanks
to the thread-safe WorkerThread::m_shutdown flag. However, to achieve the thread
shutdown observation mechanism (see the issue), it would be better to initiate
the shutdown always on the main thread. For that, this CL removes the shutdown
call from the worker thread and just prepare shutdown instead.

APPENDIX: Thread termination sequence via workerGlobalScopeClosed() on each
worker mechanism:

// DedicatedWorker
InProcessWorkerObjectProxy::workerGlobalScopeClosed()
  InProcessWorkerMessagingProxy::terminateWorkerGlobalScope()
    m_workerThread->terminate();

// SharedWorker
WebSharedWorkerImpl::workerGlobalScopeClosed()
  WebSharedWorkerImpl::workerGlobalScopeClosedOnMainThread()
    WebSharedWorkerImpl::terminateWorkerThread()
      m_workerThread->terminate();

// ServiceWorker does not support close()
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed()
  NOTREACHED()

BUG=487050, 575532

Committed: https://crrev.com/2d6728fa52504193b1c511a4f675894e2d56e2e1
Cr-Commit-Position: refs/heads/master@{#395600}
==========

[commit-bot: I haz the power, 2016-05-24 15:23:25]

Patchset 7 (id:??) landed as
https://crrev.com/2d6728fa52504193b1c511a4f675894e2d56e2e1
Cr-Commit-Position: refs/heads/master@{#395600}

[nhiroki, 2016-05-25 01:58:34]

JFYI

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:234: void
WorkerThread::performShutdownTask()
On 2016/05/24 13:36:06, nhiroki wrote:
> On 2016/05/24 09:07:48, kinuko wrote:
> > nit: maybe we could rename this to performShutdown() (method renaming /
> > reordering could be done in a follow-up cleanup CL too)
> 
> I'll make a separate CL.

Uploaded a CL: https://codereview.chromium.org/2015453002/

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/1978163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.h:79: void
prepareForShutdown();
On 2016/05/24 13:36:06, nhiroki wrote:
> On 2016/05/24 09:07:49, kinuko wrote:
> > nit: could we make WorkerMicroTaskRunner a private inner class of
WorkerThread
> > and make this method private as well?  I don't feel this needs to be a
public
> > method.
> 
> Agreed. I'll make a separate CL for this.

Uploaded a CL: https://codereview.chromium.org/2006273003/