[Worker] Call prepareForShutdown ASAP when terminated forcibly

[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
before executing all the queued tasks.

BUG=641215
Committed: https://crrev.com/7a2a29de535c01c40ef8ee11621064572676e61d
Cr-Commit-Position: refs/heads/master@{#416598}

[yhirano, 2016-09-02 02:07:57]

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

[commit-bot: I haz the power, 2016-09-02 02:08:12]

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-09-02 02:16:41]

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

[commit-bot: I haz the power, 2016-09-02 02:16:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[yhirano, 2016-09-02 03:17:17]

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

[commit-bot: I haz the power, 2016-09-02 03:17:26]

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-09-02 03:24:47]

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

[commit-bot: I haz the power, 2016-09-02 03:24:48]

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

[yhirano, 2016-09-02 03:35:02]

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

[commit-bot: I haz the power, 2016-09-02 03:35:07]

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

[yhirano, 2016-09-02 04:24:34]

Description was changed from

==========
[wip] call prepareShutdown in willProcessTask

BUG=
==========

to

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding 
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
as soon as possible.

BUG=641215
==========

[yhirano, 2016-09-02 04:25:57]

Description was changed from

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding 
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
as soon as possible.

BUG=641215
==========

to

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding 
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
before executing all the queued tasks.

BUG=641215
==========

[commit-bot: I haz the power, 2016-09-02 04:47:58]

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

[commit-bot: I haz the power, 2016-09-02 04:47:59]

Dry run: 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_...)

[yhirano, 2016-09-02 07:48:48]

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

[commit-bot: I haz the power, 2016-09-02 07:49:09]

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

[yhirano, 2016-09-02 09:10:08]

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

[commit-bot: I haz the power, 2016-09-02 11:12:35]

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

[commit-bot: I haz the power, 2016-09-02 11:12:38]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-09-05 03:26:45]

lgtm

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:258:
postTaskToWorkerGlobalScope(BLINK_FROM_HERE,
createCrossThreadTask(&WorkerThreadableLoaderTestHelper::clearLoader,
crossThreadUnretained(this)));
How about passing WaitableEvent object to clearLoader() and making it call
WaitableEvent::signal()? It's used only from here.

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

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:141: if
(scriptController && scriptController->isExecutionTerminating()) {
else if?

[yhirano, 2016-09-06 04:12:31]

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

[commit-bot: I haz the power, 2016-09-06 04:12:43]

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

[yhirano, 2016-09-06 04:29:51]

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:258:
postTaskToWorkerGlobalScope(BLINK_FROM_HERE,
createCrossThreadTask(&WorkerThreadableLoaderTestHelper::clearLoader,
crossThreadUnretained(this)));
On 2016/09/05 03:26:45, nhiroki wrote:
> How about passing WaitableEvent object to clearLoader() and making it call
> WaitableEvent::signal()? It's used only from here.

That will change clearLoader's signature in for ThreadableLoaderTestHelper and
it will affect DocumentThreadableLoaderTestHelper as well. Do you think it's
better than the current code?

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

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:141: if
(scriptController && scriptController->isExecutionTerminating()) {
On 2016/09/05 03:26:45, nhiroki wrote:
> else if?

Done.

[nhiroki, 2016-09-06 05:06:40]

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2299883006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:258:
postTaskToWorkerGlobalScope(BLINK_FROM_HERE,
createCrossThreadTask(&WorkerThreadableLoaderTestHelper::clearLoader,
crossThreadUnretained(this)));
On 2016/09/06 04:29:51, yhirano wrote:
> On 2016/09/05 03:26:45, nhiroki wrote:
> > How about passing WaitableEvent object to clearLoader() and making it call
> > WaitableEvent::signal()? It's used only from here.
> 
> That will change clearLoader's signature in for ThreadableLoaderTestHelper and
> it will affect DocumentThreadableLoaderTestHelper as well. Do you think it's
> better than the current code?

Ah, I missed the interface is shared with DocumentThreadableLoaderTestHelper.
Then, I'm ok with keeping the code as is :)

https://codereview.chromium.org/2299883006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2299883006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:334: static void
signal(WaitableEvent* event)
We could remove this as follows?

postTaskToWorkerGlobalScope(BLINK_FROM_HERE,
createCrossThreadTask(&WaitableEvent::signal, crossThreadUnretained(&event)));

[yhirano, 2016-09-06 05:28:37]

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

[commit-bot: I haz the power, 2016-09-06 05:28:51]

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

[yhirano, 2016-09-06 05:38:03]

https://codereview.chromium.org/2299883006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp (right):

https://codereview.chromium.org/2299883006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp:334: static void
signal(WaitableEvent* event)
On 2016/09/06 05:06:40, nhiroki wrote:
> We could remove this as follows?
> 
> postTaskToWorkerGlobalScope(BLINK_FROM_HERE,
> createCrossThreadTask(&WaitableEvent::signal, crossThreadUnretained(&event)));

It's a good idea, thank you!

[nhiroki, 2016-09-06 05:40:16]

lgtm2

[yhirano, 2016-09-06 05:41:05]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2016-09-06 05:41:10]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-09-06 05:41:10]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2299883006/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2016-09-06 05:41:29]

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-09-06 06:56:51]

Description was changed from

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding 
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
before executing all the queued tasks.

BUG=641215
==========

to

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding 
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
before executing all the queued tasks.

BUG=641215
==========

[commit-bot: I haz the power, 2016-09-06 06:56:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-06 06:58:11]

Description was changed from

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding 
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
before executing all the queued tasks.

BUG=641215
==========

to

==========
[Worker] Call prepareForShutdown ASAP when terminated forcibly

When a worker is terminated forcibly (i.e., TerminateExecution
is called), it is possible that many tasks are preceding
performShutdownOnWorkerThread on the worker task queue. As the v8 context
is in an invalid state, we need to ask worker objects to stop working
before executing all the queued tasks.

BUG=641215

Committed: https://crrev.com/7a2a29de535c01c40ef8ee11621064572676e61d
Cr-Commit-Position: refs/heads/master@{#416598}
==========

[commit-bot: I haz the power, 2016-09-06 06:58:12]

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