Worker: Remove locking from WorkerThread::postTask()

Worker: Remove locking from WorkerThread::postTask()

WorkerThread::postTask() can be called from various places and locking in the
function may lead to deadlock. To avoid the problem, this CL removes locking
from WorkerThread::postTask() and makes it possible to safely access to shared
flags without locking.

BUG=620442
Committed: https://crrev.com/18d1be3c48e503e5d080361f8fe6df7cfd2b503e
Cr-Commit-Position: refs/heads/master@{#405715}

[nhiroki, 2016-07-14 00:47:38]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-07-14 00:50:22]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2016-07-14 00:57:02]

Description was changed from

==========
Worker: Remove locking from WorkerThread::postTask()

BUG=620442
==========

to

==========
Worker: Remove locking from WorkerThread::postTask()

WorkerThread::postTask() can be called from various places and locking in the
function may lead to deadlock. To avoid the problem, this CL removed locking
from WorkerThread::postTask() and makes it possible to safely access to shared
flags without locking.

BUG=620442
==========

[nhiroki, 2016-07-14 01:01:22]

Description was changed from

==========
Worker: Remove locking from WorkerThread::postTask()

WorkerThread::postTask() can be called from various places and locking in the
function may lead to deadlock. To avoid the problem, this CL removed locking
from WorkerThread::postTask() and makes it possible to safely access to shared
flags without locking.

BUG=620442
==========

to

==========
Worker: Remove locking from WorkerThread::postTask()

WorkerThread::postTask() can be called from various places and locking in the
function may lead to deadlock. To avoid the problem, this CL removes locking
from WorkerThread::postTask() and makes it possible to safely access to shared
flags without locking.

BUG=620442
==========

[nhiroki, 2016-07-14 01:01:35]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2016-07-14 02:14:13]

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

[nhiroki, 2016-07-14 02:14:13]

Hi, can you review this? Thanks!

[yhirano, 2016-07-14 05:21:54]

Is it possible to remove m_terminate / m_readyShutdown checks from postTask and
put those checks on performShutdownOnWorkerThread?

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

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:567: // the flag is set
only from this main thread.
s/this main thread/the main thread/

[yhirano, 2016-07-14 05:23:40]

On 2016/07/14 05:21:54, yhirano wrote:
> Is it possible to remove m_terminate / m_readyShutdown checks from postTask
and
> put those checks on performShutdownOnWorkerThread?
> 
Sorry, I meant performTaskOnWorkerThread (and it already has a m_readyToShutdown
check).

[kinuko, 2016-07-14 05:30:53]

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:248: return;
Actually it seems ok to just remove this check... or doesn't it?

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

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:248:
postTaskOnWorkerThread(location, std::move(task), isInstrumented);
nit: the names might be a bit confusing, On -> From ?

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:594: MutexLocker
lock(m_threadStateMutex);
we could drop this lock too?

[nhiroki, 2016-07-14 08:00:41]

Thank you for reviewing! Updated a patch and replied to comments about removing
flag checks on postTask(). PTAL.

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:248: return;
On 2016/07/14 05:30:52, kinuko wrote:
> Actually it seems ok to just remove this check... or doesn't it?

See my review comment in WorkerThread::postTaskOnMainThread on PS2.

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

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:248:
postTaskOnWorkerThread(location, std::move(task), isInstrumented);
On 2016/07/14 05:30:52, kinuko wrote:
> nit: the names might be a bit confusing, On -> From ?

Done.

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:567: // the flag is set
only from this main thread.
On 2016/07/14 05:21:54, yhirano wrote:
> s/this main thread/the main thread/

Done.

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:571:
workerBackingThread().backingThread().postTask(location,
crossThreadBind(&WorkerThread::performTaskOnWorkerThread,
crossThreadUnretained(this), passed(std::move(task)), isInstrumented));
To remove |m_terminated| check from postTaskOnMainThread(), it would be
necessary to make this task cancellable because without the flag check and
cancellability, the task may run after shutdown is performed in a case(*) and
|crossThreadUnretained(this)| is dangling. This results in a crash.

(*) If the underlying thread is shared among multiple CompositorWorkerThreads,
the task scheduler is not closed by shutdown of a CWThread.

I'm not really sure which is the better way (ie. cancellable task vs. flag
check) because cancellable task machinery is supposed to be redesigned as a part
of per-frame scheduler work and cancelling a cross thread task could be somewhat
complicated. WDYT?

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:587:
workerBackingThread().backingThread().postTask(location,
WTF::bind(&WorkerThread::performTaskOnWorkerThread, unretained(this),
passed(std::move(task)), isInstrumented));
Ditto. Without |m_readyToShutdown| check, this task may run after shutdown and
it results in a crash.

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:594: MutexLocker
lock(m_threadStateMutex);
On 2016/07/14 05:30:52, kinuko wrote:
> we could drop this lock too?

Yes! Removed.

[kinuko, 2016-07-14 09:31:49]

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

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:571:
workerBackingThread().backingThread().postTask(location,
crossThreadBind(&WorkerThread::performTaskOnWorkerThread,
crossThreadUnretained(this), passed(std::move(task)), isInstrumented));
On 2016/07/14 08:00:40, nhiroki (slow) wrote:
> To remove |m_terminated| check from postTaskOnMainThread(), it would be
> necessary to make this task cancellable because without the flag check and
> cancellability, the task may run after shutdown is performed in a case(*) and
> |crossThreadUnretained(this)| is dangling. This results in a crash.
> 
> (*) If the underlying thread is shared among multiple CompositorWorkerThreads,
> the task scheduler is not closed by shutdown of a CWThread.
> 
> I'm not really sure which is the better way (ie. cancellable task vs. flag
> check) because cancellable task machinery is supposed to be redesigned as a
part
> of per-frame scheduler work and cancelling a cross thread task could be
somewhat
> complicated. WDYT?

Oh I see.. thanks. I was overlooking the point that this is calling a method on
unretained this. I feel I kinda want to avoid multiple postTask variants, which
we used to do before.

Is it any cleaner if we have a method like

bool isInShutdown()
{
   // Check if we've started termination or shutdown sequence. Avoid
   // acquiring a lock here to avoid introducing a risk of deadlock.
   // Note that accessing m_terminated on main thread or
   // m_readyToShutdown is safe as the flag is set only on the thread.
   if (isMainThread() && m_terminated)
     return true;
   if (isCurrentThread() && m_readyToShutdown)
     return true;
   return false;
}

and just call that in the postTask method? I'm open to push-back / other
suggestions too.

[nhiroki, 2016-07-15 00:20:02]

Patchset #4 (id:120001) has been deleted

[nhiroki, 2016-07-15 02:15:47]

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

[commit-bot: I haz the power, 2016-07-15 02:16:17]

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

[nhiroki, 2016-07-15 02:17:50]

Thank you! Addressed all review comments. Can you take another look?

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

https://codereview.chromium.org/2142273004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:571:
workerBackingThread().backingThread().postTask(location,
crossThreadBind(&WorkerThread::performTaskOnWorkerThread,
crossThreadUnretained(this), passed(std::move(task)), isInstrumented));
On 2016/07/14 09:31:48, kinuko wrote:
> On 2016/07/14 08:00:40, nhiroki (slow) wrote:
> > To remove |m_terminated| check from postTaskOnMainThread(), it would be
> > necessary to make this task cancellable because without the flag check and
> > cancellability, the task may run after shutdown is performed in a case(*)
and
> > |crossThreadUnretained(this)| is dangling. This results in a crash.
> > 
> > (*) If the underlying thread is shared among multiple
CompositorWorkerThreads,
> > the task scheduler is not closed by shutdown of a CWThread.
> > 
> > I'm not really sure which is the better way (ie. cancellable task vs. flag
> > check) because cancellable task machinery is supposed to be redesigned as a
> part
> > of per-frame scheduler work and cancelling a cross thread task could be
> somewhat
> > complicated. WDYT?
> 
> Oh I see.. thanks. I was overlooking the point that this is calling a method
on
> unretained this. I feel I kinda want to avoid multiple postTask variants,
which
> we used to do before.
> 
> Is it any cleaner if we have a method like
> 
> bool isInShutdown()
> {
>    // Check if we've started termination or shutdown sequence. Avoid
>    // acquiring a lock here to avoid introducing a risk of deadlock.
>    // Note that accessing m_terminated on main thread or
>    // m_readyToShutdown is safe as the flag is set only on the thread.
>    if (isMainThread() && m_terminated)
>      return true;
>    if (isCurrentThread() && m_readyToShutdown)
>      return true;
>    return false;
> }
> 
> and just call that in the postTask method? I'm open to push-back / other
> suggestions too.

Looks much better! Done.

[kinuko, 2016-07-15 03:14:36]

lgtm

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

https://codereview.chromium.org/2142273004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:257: if (m_terminated)
nit: at this point we could also use isInShutdown() here, that gives better
comment in general

https://codereview.chromium.org/2142273004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:576: if
(m_readyToShutdown)
ditto

[commit-bot: I haz the power, 2016-07-15 03:21:11]

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

[commit-bot: I haz the power, 2016-07-15 03:21:11]

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

[nhiroki, 2016-07-15 04:34:36]

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

[commit-bot: I haz the power, 2016-07-15 04:34:56]

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

[nhiroki, 2016-07-15 04:35:43]

Thank you!

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

https://codereview.chromium.org/2142273004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:257: if (m_terminated)
On 2016/07/15 03:14:36, kinuko wrote:
> nit: at this point we could also use isInShutdown() here, that gives better
> comment in general

Replaced.

I thought it'd be better to directly check the flags as much as possible because
isInShutdown() seems to hide what we really want to check. However, as you
commented, isInShutdown() would provide more information about thread safety.

https://codereview.chromium.org/2142273004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:576: if
(m_readyToShutdown)
On 2016/07/15 03:14:36, kinuko wrote:
> ditto

Done.

[kinuko, 2016-07-15 04:54:42]

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

https://codereview.chromium.org/2142273004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:257: if (m_terminated)
On 2016/07/15 04:35:43, nhiroki (slow) wrote:
> On 2016/07/15 03:14:36, kinuko wrote:
> > nit: at this point we could also use isInShutdown() here, that gives better
> > comment in general
> 
> Replaced.
> 
> I thought it'd be better to directly check the flags as much as possible
because
> isInShutdown() seems to hide what we really want to check. However, as you
> commented, isInShutdown() would provide more information about thread safety.

Yep I can imagine the concern, while I have another concern that if we keep
relying on the comment & careful coding not to update & access the flag on wrong
thread it's also error prone.  (We could possibly have dedicated accessor
methods for each flag with necessary assertion too)

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

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

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

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-07-15 06:13:56]

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

https://codereview.chromium.org/2142273004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:257: if (m_terminated)
On 2016/07/15 04:54:42, kinuko wrote:
> On 2016/07/15 04:35:43, nhiroki (slow) wrote:
> > On 2016/07/15 03:14:36, kinuko wrote:
> > > nit: at this point we could also use isInShutdown() here, that gives
better
> > > comment in general
> > 
> > Replaced.
> > 
> > I thought it'd be better to directly check the flags as much as possible
> because
> > isInShutdown() seems to hide what we really want to check. However, as you
> > commented, isInShutdown() would provide more information about thread
safety.
> 
> Yep I can imagine the concern, while I have another concern that if we keep
> relying on the comment & careful coding not to update & access the flag on
wrong
> thread it's also error prone.

Acknowledged.

> (We could possibly have dedicated accessor
> methods for each flag with necessary assertion too)

I'll consider this in a separate CL.

[nhiroki, 2016-07-15 06:15:04]

yhirano-san, can I land this?

[yhirano, 2016-07-15 07:07:56]

On 2016/07/15 06:15:04, nhiroki (slow) wrote:
> yhirano-san, can I land this?

Sorry, can you wait for a while?

[nhiroki, 2016-07-15 07:11:07]

On 2016/07/15 07:07:56, yhirano wrote:
> On 2016/07/15 06:15:04, nhiroki (slow) wrote:
> > yhirano-san, can I land this?
> 
> Sorry, can you wait for a while?

No problem! :)

[yhirano, 2016-07-15 07:20:03]

lgtm

[nhiroki, 2016-07-15 07:24:48]

On 2016/07/15 07:20:03, yhirano wrote:
> lgtm

Thank yoU!

[nhiroki, 2016-07-15 07:24:53]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-07-15 07:24:53]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2142273004/#ps180001
(title: "call isInShutdown() more")

[commit-bot: I haz the power, 2016-07-15 07:25:13]

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-07-15 07:30:13]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-07-15 07:32:23]

Description was changed from

==========
Worker: Remove locking from WorkerThread::postTask()

WorkerThread::postTask() can be called from various places and locking in the
function may lead to deadlock. To avoid the problem, this CL removes locking
from WorkerThread::postTask() and makes it possible to safely access to shared
flags without locking.

BUG=620442
==========

to

==========
Worker: Remove locking from WorkerThread::postTask()

WorkerThread::postTask() can be called from various places and locking in the
function may lead to deadlock. To avoid the problem, this CL removes locking
from WorkerThread::postTask() and makes it possible to safely access to shared
flags without locking.

BUG=620442

Committed: https://crrev.com/18d1be3c48e503e5d080361f8fe6df7cfd2b503e
Cr-Commit-Position: refs/heads/master@{#405715}
==========

[commit-bot: I haz the power, 2016-07-15 07:32:25]

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