Worker: Factor out a part of WorkerThread::terminateInternal() into its own function for cleanup

Worker: Factor out a part of WorkerThread::terminateInternal() into its own function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=640843
Committed: https://crrev.com/ece89237c1fe0cf950bfb405d9c214b7b0fd7aff
Cr-Commit-Position: refs/heads/master@{#418103}

[nhiroki, 2016-09-09 04:51:52]

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

[commit-bot: I haz the power, 2016-09-09 04:52:00]

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 04:52:01]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-09-09 04:54:56]

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

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

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 04:58:34]

Description was changed from

==========
Worker: Factor out parts of WorkerThread::terminateInternal() into their own
functions for cleanup

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

to

==========
Worker: Factor out parts of WorkerThread::terminateInternal() into their own
functions for cleanup

BUG=n/a
==========

[nhiroki, 2016-09-09 04:58:44]

Patchset #1 (id:20001) has been deleted

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

Description was changed from

==========
Worker: Factor out parts of WorkerThread::terminateInternal() into their own
functions for cleanup

BUG=n/a
==========

to

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
functions for cleanup

BUG=n/a
==========

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

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

[nhiroki, 2016-09-09 05:41:03]

Patchset #1 (id:10003) has been deleted

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

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 05:42:13]

Description was changed from

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
functions for cleanup

BUG=n/a
==========

to

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

BUG=n/a
==========

[nhiroki, 2016-09-09 05:51:03]

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

[nhiroki, 2016-09-09 05:51:04]

Hi, can you review this?

My motivation for this change is to improve readability/testability of the
current complex critical section, and to make it more robust to future changes.

Thanks!

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

https://codereview.chromium.org/2325923002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:204: bool
shouldScheduleToTerminateExecution(const MutexLocker&);
What do you think about this pattern to ensure a caller holds a lock?

[haraken, 2016-09-09 06:41:01]

LGTM

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

https://codereview.chromium.org/2325923002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:204: bool
shouldScheduleToTerminateExecution(const MutexLocker&);
On 2016/09/09 05:51:04, nhiroki wrote:
> What do you think about this pattern to ensure a caller holds a lock?

I'm fine with this but can we add DCHECK(locked()) to those methods?

[nhiroki, 2016-09-09 07:14:10]

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

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

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 07:14:41]

Thank you!

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

https://codereview.chromium.org/2325923002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:204: bool
shouldScheduleToTerminateExecution(const MutexLocker&);
On 2016/09/09 06:41:01, haraken wrote:
> On 2016/09/09 05:51:04, nhiroki wrote:
> > What do you think about this pattern to ensure a caller holds a lock?
> 
> I'm fine with this but can we add DCHECK(locked()) to those methods?

Good idea. Added checks.

[commit-bot: I haz the power, 2016-09-09 07:24:47]

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

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

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[haraken, 2016-09-09 07:40:32]

LGTM

[nhiroki, 2016-09-09 08:28:37]

Hmmm..., chromeos builds are failing because Mutex.locked() is not defined.
Apparently, on the bots, DCHECK() is defined but ENABLE(ASSERT)[1] is not
defined. I'll investigate this more...

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/ThreadingP...

[nhiroki, 2016-09-09 10:08:08]

On 2016/09/09 08:28:37, nhiroki wrote:
> Hmmm..., chromeos builds are failing because Mutex.locked() is not defined.
> Apparently, on the bots, DCHECK() is defined but ENABLE(ASSERT)[1] is not
> defined. I'll investigate this more...
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/ThreadingP...

OK, I understand the situation. I wrongly assumed that DCHECK(condition) is
completely no-op on release build, but actually it generates a code like this:

#define DCHECK(condition)                                                \
  LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
      << "Check failed: " #condition ". "

So, |condition| must be valid even on release build. I'll fix the condition...

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

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:36:59]

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 11:03:40]

On 2016/09/09 10:08:08, nhiroki wrote:
> On 2016/09/09 08:28:37, nhiroki wrote:
> > Hmmm..., chromeos builds are failing because Mutex.locked() is not defined.
> > Apparently, on the bots, DCHECK() is defined but ENABLE(ASSERT)[1] is not
> > defined. I'll investigate this more...
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/ThreadingP...
> 
> OK, I understand the situation. I wrongly assumed that DCHECK(condition) is
> completely no-op on release build, but actually it generates a code like this:
> 
> #define DCHECK(condition)                                                \
>   LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
>       << "Check failed: " #condition ". "
> 
> So, |condition| must be valid even on release build. I'll fix the condition...

Fixed. Can you take another look?

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

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

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

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-09-12 07:48:02]

Description was changed from

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

BUG=n/a
==========

to

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=n/a
==========

[yhirano, 2016-09-12 09:14:48]

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

https://codereview.chromium.org/2325923002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:695: void
WorkerThread::setExitCode(const MutexLocker& lock, ExitCode exitCode)
Sorry, can you explain why |lock| is needed?

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

https://codereview.chromium.org/2325923002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:201: // terminate the
worker execution so that a shutdown task can be handle by
s/be handle/be handled/

https://codereview.chromium.org/2325923002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:202: // thread event loop.
This must be called with |m_threadStateMutex|
[optional] perhaps "the" thread event loop is better?

[nhiroki, 2016-09-12 09:25:25]

Thank you!

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

https://codereview.chromium.org/2325923002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:695: void
WorkerThread::setExitCode(const MutexLocker& lock, ExitCode exitCode)
On 2016/09/12 09:14:48, yhirano (slow) wrote:
> Sorry, can you explain why |lock| is needed?

This is a kind of an annotation to clarify that this function requires a caller
to acquire the lock.

(See
https://codereview.chromium.org/2325923002/diff/50001/third_party/WebKit/Sour...)

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

https://codereview.chromium.org/2325923002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:201: // terminate the
worker execution so that a shutdown task can be handle by
On 2016/09/12 09:14:48, yhirano (slow) wrote:
> s/be handle/be handled/

Done.

https://codereview.chromium.org/2325923002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:202: // thread event loop.
This must be called with |m_threadStateMutex|
On 2016/09/12 09:14:48, yhirano (slow) wrote:
> [optional] perhaps "the" thread event loop is better?

Done.

[yhirano, 2016-09-12 10:15:05]

lgtm

[nhiroki, 2016-09-12 11:58:31]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-09-12 11:58:32]

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

[nhiroki, 2016-09-12 11:58:36]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2016-09-12 12:04:56]

Description was changed from

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=n/a
==========

to

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=640843
==========

[nhiroki, 2016-09-12 12:05:01]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-09-12 12:05:10]

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-12 16:59:49]

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

[commit-bot: I haz the power, 2016-09-12 16:59:50]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nhiroki, 2016-09-12 22:11:32]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-09-12 22:11:57]

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-12 23:31:24]

Description was changed from

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=640843
==========

to

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=640843
==========

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

Committed patchset #4 (id:110001)

[commit-bot: I haz the power, 2016-09-12 23:33:16]

Description was changed from

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=640843
==========

to

==========
Worker: Factor out a part of WorkerThread::terminateInternal() into its own
function for cleanup

Motivation of this change is to improve readability/testability of the current
complex critical section, and to make it more robust to future changes.

BUG=640843

Committed: https://crrev.com/ece89237c1fe0cf950bfb405d9c214b7b0fd7aff
Cr-Commit-Position: refs/heads/master@{#418103}
==========

[commit-bot: I haz the power, 2016-09-12 23:33:18]

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