Worker: Protect a running debugger task from forcible worker termination

Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
Committed: https://crrev.com/48b51b66bbd7193a56102c49dc2b6ebea67274dc
Cr-Commit-Position: refs/heads/master@{#398837}

[nhiroki, 2016-06-06 07:51:00]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-06-06 07:51:10]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2016-06-06 09:57:52]

Description was changed from

==========
Worker: Avoid a debugger task from running after termination is scheduled

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) runDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before runDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   runDebuggerTaskOnWorkerThread() sets it, schedules a force termination task,
   and releases the lock.
3) runDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and runs
   the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post a task after terminations starts.

BUG=616775
==========

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

Description was changed from

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) runDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before runDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   runDebuggerTaskOnWorkerThread() sets it, schedules a force termination task,
   and releases the lock.
3) runDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and runs
   the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post a task after terminations starts.

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) runDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before runDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   runDebuggerTaskOnWorkerThread() sets it, schedules a force termination task,
   and releases the lock.
3) runDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and runs
   the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post a task after terminations starts.

BUG=616775
==========

[nhiroki, 2016-06-06 10:12:35]

Description was changed from

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) runDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before runDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   runDebuggerTaskOnWorkerThread() sets it, schedules a force termination task,
   and releases the lock.
3) runDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and runs
   the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post a task after terminations starts.

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) runDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before runDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   runDebuggerTaskOnWorkerThread() sets it, schedules a force termination task,
   and releases the lock.
3) runDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and runs
   the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

[nhiroki, 2016-06-06 10:13:56]

Description was changed from

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) runDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before runDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   runDebuggerTaskOnWorkerThread() sets it, schedules a force termination task,
   and releases the lock.
3) runDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and runs
   the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

[nhiroki, 2016-06-06 10:14:02]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2016-06-06 10:14:10]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2016-06-06 10:14:18]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2016-06-06 10:14:32]

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

[nhiroki, 2016-06-06 10:14:32]

PTAL, thanks!

[nhiroki, 2016-06-07 01:46:16]

FYI: I found another corner case to unexpectedly terminate the running debugger
task. I made a separate CL based on this CL:
https://codereview.chromium.org/2046483002/

[kinuko, 2016-06-07 06:03:54]

Description was changed from

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

[kinuko, 2016-06-07 06:03:54]

kinuko@chromium.org changed reviewers:
+ dgozman@chromium.org

[kinuko, 2016-06-07 06:04:08]

+dgozman

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

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:235: if (m_terminated ||
m_readyToShutdown)
Do you know if this method's called only on the main thread?  If that's the case
checking the latter (m_readyToShutdown) here is probably not that useful.

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:342:
forciblyTerminateExecution();
We could forcibly kill inspector task here too?

[dgozman, 2016-06-07 10:25:32]

lgtm from inspector side. Thank you for fixing this!

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

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:235: if (m_terminated ||
m_readyToShutdown)
On 2016/06/07 06:04:08, kinuko wrote:
> Do you know if this method's called only on the main thread?  If that's the
case
> checking the latter (m_readyToShutdown) here is probably not that useful.

This is main-thread only method.

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:342:
forciblyTerminateExecution();
On 2016/06/07 06:04:08, kinuko wrote:
> We could forcibly kill inspector task here too?

I would advise against that. We'll just get more crashes not really gaining
anything. If user is debugging, they can definitely wait for debugger task to
finish.

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:495:
m_inspectorTaskRunner->kill();
I think this is redundant right now (it's getting called earlier in
terminateInternal), but it won't hurt anybody and may save in future
refactorings, so I'd leave it.

[nhiroki, 2016-06-08 04:19:55]

Thank you for reviewing! Slightly updated.

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

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:235: if (m_terminated ||
m_readyToShutdown)
On 2016/06/07 06:04:08, kinuko wrote:
> Do you know if this method's called only on the main thread?  If that's the
case
> checking the latter (m_readyToShutdown) here is probably not that useful.

I think the latter could still be useful even if it's called only on the main
thread because w/ the check we can skip following appending, locking and posting
operations when prepareForShutdownOnWorkerThread() is called but
terminateInternal() is not yet.

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:342:
forciblyTerminateExecution();
On 2016/06/07 10:25:31, dgozman_slow wrote:
> On 2016/06/07 06:04:08, kinuko wrote:
> > We could forcibly kill inspector task here too?
> 
> I would advise against that. We'll just get more crashes not really gaining
> anything. If user is debugging, they can definitely wait for debugger task to
> finish.

Probably kinuko@ does not suggest killing a running debugger task here but just
point out another corner case to unexpectedly kill it. I'll address the case in
a subsequent CL:
https://codereview.chromium.org/2046483002/

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:495:
m_inspectorTaskRunner->kill();
On 2016/06/07 10:25:31, dgozman_slow wrote:
> I think this is redundant right now (it's getting called earlier in
> terminateInternal), but it won't hurt anybody and may save in future
> refactorings, so I'd leave it.

Acknowledged.

[kinuko, 2016-06-08 12:43:56]

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

https://codereview.chromium.org/2041753002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:342:
forciblyTerminateExecution();
On 2016/06/08 04:19:55, nhiroki wrote:
> On 2016/06/07 10:25:31, dgozman_slow wrote:
> > On 2016/06/07 06:04:08, kinuko wrote:
> > > We could forcibly kill inspector task here too?
> > 
> > I would advise against that. We'll just get more crashes not really gaining
> > anything. If user is debugging, they can definitely wait for debugger task
to
> > finish.
> 
> Probably kinuko@ does not suggest killing a running debugger task here but
just
> point out another corner case to unexpectedly kill it. I'll address the case
in
> a subsequent CL:
> https://codereview.chromium.org/2046483002/

Yup, wanted to say that we shouldn't (while the current code has the possibility
of sudden termination)

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

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:236: if (m_terminated ||
m_readyToShutdown)
I'm not still very sure checking m_readyToShutdown here is so useful.

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:242: if (isolate())
should we check m_readyToShutdown here rather than isolate() and just return if
it's true?

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:585: (*task)();
Hm... might this task still run even after prepare shutdown is done?

[nhiroki, 2016-06-08 16:00:46]

Thank you! Updated.

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

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:236: if (m_terminated ||
m_readyToShutdown)
On 2016/06/08 12:43:55, kinuko wrote:
> I'm not still very sure checking m_readyToShutdown here is so useful.

Hmm..., it could be more reasonable to check |m_killed| in
InspectorTaskRunner::appendTask() instead of checking |m_readyToShutdown| here?
(see the patchset 3)

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:242: if (isolate())
On 2016/06/08 12:43:55, kinuko wrote:
> should we check m_readyToShutdown here rather than isolate() and just return
if
> it's true?

Done.

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:585: (*task)();
On 2016/06/08 12:43:55, kinuko wrote:
> Hm... might this task still run even after prepare shutdown is done?

takeNextTask() returns nullptr after InspectorTaskRunner::kill() is called in
prepareForShutdownTaskOnWorkerThread(), so the case never happens.

[nhiroki, 2016-06-08 16:02:14]

dgozman@, could you take another look? I changed InspectorTaskRunner.cpp after
your review. Thanks.

[dgozman, 2016-06-08 20:53:21]

> > Probably kinuko@ does not suggest killing a running debugger task here but
> just
> > point out another corner case to unexpectedly kill it. I'll address the case
> in
> > a subsequent CL:
> > https://codereview.chromium.org/2046483002/
> 
> Yup, wanted to say that we shouldn't (while the current code has the
possibility
> of sudden termination)

Sorry, misunderstood you. My bad.

Still looks good, modulo minor comment.

[dgozman, 2016-06-08 20:53:41]

Forgot to public the comment :-)

https://codereview.chromium.org/2041753002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorTaskRunner.cpp (right):

https://codereview.chromium.org/2041753002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorTaskRunner.cpp:37:
m_condition.signal();
Let's not signal if we didn't append a task?

[nhiroki, 2016-06-09 00:01:18]

Thank you for reviewing!

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

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:242: if (isolate())
On 2016/06/08 16:00:46, nhiroki wrote:
> On 2016/06/08 12:43:55, kinuko wrote:
> > should we check m_readyToShutdown here rather than isolate() and just return
> if
> > it's true?
> 
> Done.

I noticed this change crashes some tests because appendDebuggerTask() can be
called before initializeOnWorkerThread() and isolate() returns nullptr on
interruptAndRunAllTasksDontWait().

Should we check both isolate() and m_readyToShutdown here like the patchset4 to
avoid the crash and postTask(), or check only isolate() like the original?

https://codereview.chromium.org/2041753002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorTaskRunner.cpp (right):

https://codereview.chromium.org/2041753002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorTaskRunner.cpp:37:
m_condition.signal();
On 2016/06/08 20:53:41, dgozman_slow wrote:
> Let's not signal if we didn't append a task?

Done.

[kinuko, 2016-06-09 00:33:04]

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

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:236: if (m_terminated ||
m_readyToShutdown)
I see. I wasn't sure we want to call isolate methods to interrupt after we
prepared for shutdown (and felt inconsistency checking the flag early but not
here).  If it looks fine the original code is fine too

[kinuko, 2016-06-09 00:38:47]

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

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:236: if (m_terminated ||
m_readyToShutdown)
On 2016/06/09 00:33:04, kinuko wrote:
> I see. I wasn't sure we want to call isolate methods to interrupt after we
> prepared for shutdown (and felt inconsistency checking the flag early but not
> here).  If it looks fine the original code is fine too

s/it looks fine/it works fine/

[nhiroki, 2016-06-09 08:32:05]

Thank you for your comments!

kinuko@, can you review the patchset 4?

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

https://codereview.chromium.org/2041753002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:236: if (m_terminated ||
m_readyToShutdown)
On 2016/06/09 00:38:47, kinuko wrote:
> On 2016/06/09 00:33:04, kinuko wrote:
> > I see. I wasn't sure we want to call isolate methods to interrupt after we
> > prepared for shutdown (and felt inconsistency checking the flag early but
not
> > here).  If it looks fine the original code is fine too
> 
> s/it looks fine/it works fine/

OK, let me proceed w/ the patchset 4 that does not check |m_readyToShutdown| in
the 1st critical section (here) but in the 2nd critical section in order to
avoid pointless Isolate interruption after ready to shutdown.

[kinuko, 2016-06-09 08:57:42]

lgtm.

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

https://codereview.chromium.org/2041753002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:244: if (isolate())
nit: or this can be if (isolate() && !m_readyToShutdown)

[nhiroki, 2016-06-09 09:33:36]

Thank you!

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

https://codereview.chromium.org/2041753002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:244: if (isolate())
On 2016/06/09 08:57:42, kinuko wrote:
> nit: or this can be if (isolate() && !m_readyToShutdown)

Done.

[nhiroki, 2016-06-09 09:33:41]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-06-09 09:33:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2041753002/#ps180001
(title: "tweak if-condition")

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

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

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

Description was changed from

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

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

Committed patchset #5 (id:180001)

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

Description was changed from

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775
==========

to

==========
Worker: Protect a running debugger task from forcible worker termination

A running debugger task must be protected from forcible worker termination, but
in the current implementation, there is a corner case where the debugger task
can suddenly be killed as follows:

1) performDebuggerTaskOnWorkerThread() is called almost at the same time as
   terminateInternal() on the main thread. terminateInternal() acquires
   |m_threadStateMutex| before performDebuggerTaskOnWorkerThread().
2) terminateInternal() checks a flag |m_runningDebuggerTask| before
   performDebuggerTaskOnWorkerThread() sets it, schedules a force termination
   task, and releases the lock.
3) performDebuggerTaskOnWorkerThread() acquires the lock, sets the flag, and
   runs the debugger task.
4) The scheduled task is fired while the debugger task is running and attempts
   to terminate it. This may cause crashes.

To avoid the case, this CL makes a scheduled task check the flag before
terminating the worker execution. In addition, this ensures that postTask() and
appendDebuggerTask() don't post new tasks after terminateInternal() or
prepareForShutdownOnWorkerThread() is called.

BUG=616775

Committed: https://crrev.com/48b51b66bbd7193a56102c49dc2b6ebea67274dc
Cr-Commit-Position: refs/heads/master@{#398837}
==========

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

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