Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877
Committed: https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c
Cr-Commit-Position: refs/heads/master@{#418571}

[nhiroki, 2016-09-09 10:52:11]

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

[commit-bot: I haz the power, 2016-09-09 10:52:23]

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

[nhiroki, 2016-09-09 10:52:26]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-09-09 11:01:49]

Description was changed from

==========
[WIP] Worker: Remove m_executionScheduledToTerminate from
WorkerOrWorkletScriptController

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

to

==========
Worker: Check forcible termination request by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checkes it by Isolate::IsExecutionTerminating() that is a more
natural way.

BUG=638877, 641215
==========

[nhiroki, 2016-09-09 11:02:12]

Description was changed from

==========
Worker: Check forcible termination request by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checkes it by Isolate::IsExecutionTerminating() that is a more
natural way.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination request by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checkes it by Isolate::IsExecutionTerminating() that is a more
natural way.

BUG=638877, 641215
==========

[nhiroki, 2016-09-09 11:02:57]

Description was changed from

==========
Worker: Check forcible termination request by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checkes it by Isolate::IsExecutionTerminating() that is a more
natural way.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination request by Isolate::IsExecutionTerminating

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating. After this CL,
WorkerThread checkes it by Isolate::IsExecutionTerminating that should be a more
natural way.

BUG=638877, 641215
==========

[nhiroki, 2016-09-09 11:06:08]

Description was changed from

==========
Worker: Check forcible termination request by Isolate::IsExecutionTerminating

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating. After this CL,
WorkerThread checkes it by Isolate::IsExecutionTerminating that should be a more
natural way.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by Isolate::IsExecutionTerminating() instead.

BUG=638877, 641215
==========

[nhiroki, 2016-09-09 11:06:39]

Description was changed from

==========
Worker: Check forcible termination by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by Isolate::IsExecutionTerminating() instead.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by Isolate::IsExecutionTerminating() instead. This should
be clearer than the previous way.

BUG=638877, 641215
==========

[nhiroki, 2016-09-09 11:09:01]

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

[nhiroki, 2016-09-09 11:09:02]

PTAL.

This depends on CLs to remove isTerminating() calls from fetch/streams codebase.
I'll land this after we confirm those CLs don't cause crashes.

[commit-bot: I haz the power, 2016-09-09 12:31:19]

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

[commit-bot: I haz the power, 2016-09-09 12:31:19]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-09-12 09:21:08]

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

[yhirano, 2016-09-12 09:21:09]

Sorry, I don't know if Isolate::IsExecutionTerminating() can replace
WorkerOrWorkletScriptController::isExecutionTerminating(). Specifically, I don't
know if Isolate::IsExecutionTerminating() will stay true even after the
execution is terminated.

Adam, can you take a look?

Thanks,

[adamk, 2016-09-12 16:09:25]

adamk@chromium.org changed reviewers:
+ jochen@chromium.org

[adamk, 2016-09-12 16:09:25]

I don't know this off the top of my head, but I'll bet Jochen does.

[haraken, 2016-09-13 00:21:28]

As far as I understand, Isolate::IsExecutionTerminating() keeps returning true
after isolate->TerminateExecution is called.

[yhirano, 2016-09-14 06:44:04]

On 2016/09/13 00:21:28, haraken wrote:
> As far as I understand, Isolate::IsExecutionTerminating() keeps returning true
> after isolate->TerminateExecution is called.

We expected Isolate::IsExecutionTerminating() to return true on the worker
thread after Isolate::TerminateExecution() is called on the main thread. But
apparently it doesn't return true in such a case according to [1].

haraken@, jochen@, Is this the expected behavior? Or am I missing something?
If this is the expected behavior, we should keep isExecutionTerminating in
blink.

1: https://codereview.chromium.org/2342443003/

[haraken, 2016-09-14 06:48:45]

On 2016/09/14 06:44:04, yhirano (slow) wrote:
> On 2016/09/13 00:21:28, haraken wrote:
> > As far as I understand, Isolate::IsExecutionTerminating() keeps returning
true
> > after isolate->TerminateExecution is called.
> 
> We expected Isolate::IsExecutionTerminating() to return true on the worker
> thread after Isolate::TerminateExecution() is called on the main thread. But
> apparently it doesn't return true in such a case according to [1].
> 
> haraken@, jochen@, Is this the expected behavior? Or am I missing something?
> If this is the expected behavior, we should keep isExecutionTerminating in
> blink.
> 
> 1: https://codereview.chromium.org/2342443003/

I'd say it's a bug of V8.

However, I don't think Isolate::IsExecutionTerminating() is a mandatory V8 API
because Blink should already know the isolate state (because Blink has called
isolate->TerminateExecution()). So I'm fine with just keeping the
isExecutionTerminating in Blink.

[nhiroki, 2016-09-14 07:20:45]

On 2016/09/14 06:48:45, haraken wrote:
> On 2016/09/14 06:44:04, yhirano (slow) wrote:
> > On 2016/09/13 00:21:28, haraken wrote:
> > > As far as I understand, Isolate::IsExecutionTerminating() keeps returning
> true
> > > after isolate->TerminateExecution is called.
> > 
> > We expected Isolate::IsExecutionTerminating() to return true on the worker
> > thread after Isolate::TerminateExecution() is called on the main thread. But
> > apparently it doesn't return true in such a case according to [1].
> > 
> > haraken@, jochen@, Is this the expected behavior? Or am I missing something?
> > If this is the expected behavior, we should keep isExecutionTerminating in
> > blink.
> > 
> > 1: https://codereview.chromium.org/2342443003/
> 
> I'd say it's a bug of V8.
> 
> However, I don't think Isolate::IsExecutionTerminating() is a mandatory V8 API
> because Blink should already know the isolate state (because Blink has called
> isolate->TerminateExecution()). So I'm fine with just keeping the
> isExecutionTerminating in Blink.

yhirano and haraken, thank you for your investigation and comments.

I should have mentioned... my original motivation of this change is to remove
|m_globalScope| access from the main thread in
WorkerThread::forciblyTerminatedExecution(). If
Isolate::IsExecutionTerminating() isn't available in the case for now, I can
take another way, for example, WorkerThread has a flag like
"m_forciblyTerminated" instead of the script controller.

[nhiroki, 2016-09-14 07:29:26]

On 2016/09/14 07:20:45, nhiroki wrote:
> On 2016/09/14 06:48:45, haraken wrote:
> > On 2016/09/14 06:44:04, yhirano (slow) wrote:
> > > On 2016/09/13 00:21:28, haraken wrote:
> > > > As far as I understand, Isolate::IsExecutionTerminating() keeps
returning
> > true
> > > > after isolate->TerminateExecution is called.
> > > 
> > > We expected Isolate::IsExecutionTerminating() to return true on the worker
> > > thread after Isolate::TerminateExecution() is called on the main thread.
But
> > > apparently it doesn't return true in such a case according to [1].
> > > 
> > > haraken@, jochen@, Is this the expected behavior? Or am I missing
something?
> > > If this is the expected behavior, we should keep isExecutionTerminating in
> > > blink.
> > > 
> > > 1: https://codereview.chromium.org/2342443003/
> > 
> > I'd say it's a bug of V8.
> > 
> > However, I don't think Isolate::IsExecutionTerminating() is a mandatory V8
API
> > because Blink should already know the isolate state (because Blink has
called
> > isolate->TerminateExecution()). So I'm fine with just keeping the
> > isExecutionTerminating in Blink.
> 
> yhirano and haraken, thank you for your investigation and comments.
> 
> I should have mentioned... my original motivation of this change is to remove
> |m_globalScope| access from the main thread in
> WorkerThread::forciblyTerminatedExecution(). If
> Isolate::IsExecutionTerminating() isn't available in the case for now, I can
> take another way, for example, WorkerThread has a flag like
> "m_forciblyTerminated" instead of the script controller.

I'll re-make a patch in this way and upload it to this review.

[nhiroki, 2016-09-14 07:29:45]

Description was changed from

==========
Worker: Check forcible termination by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by Isolate::IsExecutionTerminating() instead. This should
be clearer than the previous way.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by Isolate::IsExecutionTerminating() instead. This should
be clearer than the previous way.

BUG=638877, 641215
==========

[yhirano, 2016-09-14 07:30:33]

On 2016/09/14 07:20:45, nhiroki wrote:
> On 2016/09/14 06:48:45, haraken wrote:
> > On 2016/09/14 06:44:04, yhirano (slow) wrote:
> > > On 2016/09/13 00:21:28, haraken wrote:
> > > > As far as I understand, Isolate::IsExecutionTerminating() keeps
returning
> > true
> > > > after isolate->TerminateExecution is called.
> > > 
> > > We expected Isolate::IsExecutionTerminating() to return true on the worker
> > > thread after Isolate::TerminateExecution() is called on the main thread.
But
> > > apparently it doesn't return true in such a case according to [1].
> > > 
> > > haraken@, jochen@, Is this the expected behavior? Or am I missing
something?
> > > If this is the expected behavior, we should keep isExecutionTerminating in
> > > blink.
> > > 
> > > 1: https://codereview.chromium.org/2342443003/
> > 
> > I'd say it's a bug of V8.
> > 
> > However, I don't think Isolate::IsExecutionTerminating() is a mandatory V8
API
> > because Blink should already know the isolate state (because Blink has
called
> > isolate->TerminateExecution()). So I'm fine with just keeping the
> > isExecutionTerminating in Blink.
> 
> yhirano and haraken, thank you for your investigation and comments.
> 
> I should have mentioned... my original motivation of this change is to remove
> |m_globalScope| access from the main thread in
> WorkerThread::forciblyTerminatedExecution(). If
> Isolate::IsExecutionTerminating() isn't available in the case for now, I can
> take another way, for example, WorkerThread has a flag like
> "m_forciblyTerminated" instead of the script controller.

Can we use |m_exitCode == ExitCode::SyncForciblyTerminated|?

[jochen (gone - plz use gerrit), 2016-09-14 09:00:46]

can you file a bug for the execution termination stuff?

[nhiroki, 2016-09-14 10:43:11]

On 2016/09/14 09:00:46, jochen wrote:
> can you file a bug for the execution termination stuff?

Filed: https://bugs.chromium.org/p/v8/issues/detail?id=5389

[nhiroki, 2016-09-14 10:43:48]

On 2016/09/14 07:30:33, yhirano (slow) wrote:
> On 2016/09/14 07:20:45, nhiroki wrote:
> > On 2016/09/14 06:48:45, haraken wrote:
> > > On 2016/09/14 06:44:04, yhirano (slow) wrote:
> > > > On 2016/09/13 00:21:28, haraken wrote:
> > > > > As far as I understand, Isolate::IsExecutionTerminating() keeps
> returning
> > > true
> > > > > after isolate->TerminateExecution is called.
> > > > 
> > > > We expected Isolate::IsExecutionTerminating() to return true on the
worker
> > > > thread after Isolate::TerminateExecution() is called on the main thread.
> But
> > > > apparently it doesn't return true in such a case according to [1].
> > > > 
> > > > haraken@, jochen@, Is this the expected behavior? Or am I missing
> something?
> > > > If this is the expected behavior, we should keep isExecutionTerminating
in
> > > > blink.
> > > > 
> > > > 1: https://codereview.chromium.org/2342443003/
> > > 
> > > I'd say it's a bug of V8.
> > > 
> > > However, I don't think Isolate::IsExecutionTerminating() is a mandatory V8
> API
> > > because Blink should already know the isolate state (because Blink has
> called
> > > isolate->TerminateExecution()). So I'm fine with just keeping the
> > > isExecutionTerminating in Blink.
> > 
> > yhirano and haraken, thank you for your investigation and comments.
> > 
> > I should have mentioned... my original motivation of this change is to
remove
> > |m_globalScope| access from the main thread in
> > WorkerThread::forciblyTerminatedExecution(). If
> > Isolate::IsExecutionTerminating() isn't available in the case for now, I can
> > take another way, for example, WorkerThread has a flag like
> > "m_forciblyTerminated" instead of the script controller.
> 
> Can we use |m_exitCode == ExitCode::SyncForciblyTerminated|?

This seems to be feasible. I'll try it.

[nhiroki, 2016-09-14 11:30:57]

Patchset #2 (id:40001) has been deleted

[nhiroki, 2016-09-14 11:31:54]

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

[commit-bot: I haz the power, 2016-09-14 11:32:07]

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-14 11:34:16]

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

[commit-bot: I haz the power, 2016-09-14 11:34:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nhiroki, 2016-09-14 11:45:47]

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

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

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

[nhiroki, 2016-09-14 11:49:07]

yhirano@, can you review this again?

I'll update the CL subject and description later. Thanks!

[yhirano, 2016-09-14 11:54:38]

lgtm

https://codereview.chromium.org/2324693003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
(right):

https://codereview.chromium.org/2324693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h:40:
#include "wtf/ThreadingPrimitives.h"
Not needed?

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

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

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

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-09-14 13:40:48]

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

[commit-bot: I haz the power, 2016-09-14 13:41:09]

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

[nhiroki, 2016-09-14 13:44:30]

Description was changed from

==========
Worker: Check forcible termination by Isolate::IsExecutionTerminating()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by Isolate::IsExecutionTerminating() instead. This should
be clearer than the previous way.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877, 641215
==========

[nhiroki, 2016-09-14 13:44:53]

Description was changed from

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877, 641215
==========

[nhiroki, 2016-09-14 13:46:41]

Description was changed from

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877, 641215
==========

to

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877
==========

[nhiroki, 2016-09-14 13:47:18]

Thank you!

https://codereview.chromium.org/2324693003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
(right):

https://codereview.chromium.org/2324693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h:40:
#include "wtf/ThreadingPrimitives.h"
On 2016/09/14 11:54:38, yhirano (slow) wrote:
> Not needed?

Removed.

[nhiroki, 2016-09-14 13:47:25]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2016-09-14 13:47:26]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-09-14 13:47:26]

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/2324693003/#ps100001
(title: "remove unnecessary header inclusion")

[commit-bot: I haz the power, 2016-09-14 13:47:43]

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-14 15:42:08]

Description was changed from

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877
==========

to

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877
==========

[commit-bot: I haz the power, 2016-09-14 15:42:09]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-09-14 15:44:57]

Description was changed from

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877
==========

to

==========
Worker: Check forcible termination by WorkerThread::isForciblyTerminated()

Before this CL, WorkerThread checks whether forcible termination happened by
WorkerOrWorkletScriptController::isExecutionTerminating(). After this CL,
WorkerThread checks it by WorkerThread::isForciblyTerminated() instead. This
should be clearer than the previous way and enables to remove an access to
WorkerThread::m_globalScope from the main thread.

BUG=638877

Committed: https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c
Cr-Commit-Position: refs/heads/master@{#418571}
==========

[commit-bot: I haz the power, 2016-09-14 15:44:58]

Patchset 4 (id:??) landed as
https://crrev.com/01ff9de2e66ce93e2dc7bc9f0d7ee2681f32367c
Cr-Commit-Position: refs/heads/master@{#418571}