Pass per-frame task runners to Workers (when possible) [2nd try]

Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1, https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed, but I'd like it to be fixed separately.

BUG=627034
Committed: https://crrev.com/a1e7e84803441a117280bbf0d20e8d7479453691
Cr-Commit-Position: refs/heads/master@{#412347}

[kinuko, 2016-08-11 04:46:22]

Patchset #2 (id:20001) has been deleted

[kinuko, 2016-08-11 04:46:28]

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

[commit-bot: I haz the power, 2016-08-11 04:46:47]

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

[kinuko, 2016-08-11 04:46:52]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

Diff from previous patch (PS1, https://codereview.chromium.org/2163983004):

- If tasks that trigger manual deletion for non-oilpan object are posted
  to a per-frame task runner after the task goes away we could leak the
  object
- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- If tasks that trigger manual deletion for non-oilpan object are posted
  to a per-frame task runner after the task goes away we could leak the
  object
- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver

BUG=627034
==========

[commit-bot: I haz the power, 2016-08-11 04:48:25]

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

[commit-bot: I haz the power, 2016-08-11 04:48:25]

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

[kinuko, 2016-08-11 05:20:07]

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

[commit-bot: I haz the power, 2016-08-11 05:20:19]

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

[kinuko, 2016-08-11 06:23:01]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- If tasks that trigger manual deletion for non-oilpan object are posted
  to a per-frame task runner after the task goes away we could leak the
  object
- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers can post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak)
- Support TaskType in a very limited way

BUG=627034
==========

[kinuko, 2016-08-11 06:29:17]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers can post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak)
- Support TaskType in a very limited way

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers can post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak)
- Support TaskType in a very limited way

The 'manual deletion' part in the worker code should be probably fixed /
removed,
but I'd like it to be fixed separately.

BUG=627034
==========

[kinuko, 2016-08-11 06:29:44]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers can post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak)
- Support TaskType in a very limited way

The 'manual deletion' part in the worker code should be probably fixed /
removed,
but I'd like it to be fixed separately.

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak)
- Support TaskType in a very limited way

The 'manual deletion' part in the worker code should be probably fixed /
removed,
but I'd like it to be fixed separately.

BUG=627034
==========

[kinuko, 2016-08-11 06:30:07]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak)
- Support TaskType in a very limited way

The 'manual deletion' part in the worker code should be probably fixed /
removed,
but I'd like it to be fixed separately.

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed,
but I'd like it to be fixed separately.

BUG=627034
==========

[kinuko, 2016-08-11 06:31:44]

kinuko@chromium.org changed reviewers:
+ haraken@chromium.org, hiroshige@chromium.org, nhiroki@chromium.org,
skyostil@chromium.org

[kinuko, 2016-08-11 06:32:21]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed,
but I'd like it to be fixed separately.

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed, but I'd like it to be fixed separately.

BUG=627034
==========

[kinuko, 2016-08-11 06:34:19]

kinuko@chromium.org changed reviewers:
- haraken@chromium.org, hiroshige@chromium.org, nhiroki@chromium.org,
skyostil@chromium.org

[commit-bot: I haz the power, 2016-08-11 06:44:53]

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

[commit-bot: I haz the power, 2016-08-11 06:44:54]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-08-11 14:11:48]

kinuko@chromium.org changed reviewers:
+ haraken@chromium.org, hiroshige@chromium.org, nhiroki@chromium.org,
skyostil@chromium.org

[kinuko, 2016-08-11 14:13:42]

PTAL. Diff between PS1 and PS2 is the changes from original patch that got
reverted. (Please also see the issue description)  I don't feel this one's in
its cleanest way but want your feedback.

[haraken, 2016-08-11 14:57:43]

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:57:
getParentFrameTaskRunners()->get(TaskType::Unthrottled)->postTask(BLINK_FROM_HERE,
crossThreadBind(&InProcessWorkerMessagingProxy::postMessageToWorkerObject,
crossThreadUnretained(m_messagingProxy), message, passed(std::move(channels))));

Do you think these tasks really need to be posted to the unthrottled task
runner? What happens if we post the tasks to the internal task runner (which may
be throttled)?

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp (right):

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:13: class
ParentFrameTaskRunners::Observer final : public GarbageCollected<Observer>,
public ContextLifecycleObserver {

I'd use LocalFrameLifecycleObserver instead, since ParentFrameTaskRunners is
associated with a frame.

Just help me understand: Why can't you simply make ParentFrameTaskRunners an
observer of frame/document?

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:39:
WebTaskRunner* ParentFrameTaskRunners::get(TaskType type)

Nit: I'd rename this to ParentFrameTaskRunnerHelper or WorkerTaskRunnerHelper
(for consistency with TaskRunnerHelper)

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:42: //
(Probably some unthrottled tasks should be moved to Internal type)

Yeah, our plan is to limit the unthrottled task runner to only very special
tasks. Maybe would it make more sense to expose the internal task runner instead
of the unthrottled task runner?

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:56:
ParentFrameTaskRunners::ParentFrameTaskRunners(Document* document)

Would it make more sense to change the parameter from Document to LocalFrame?

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:68: void
ParentFrameTaskRunners::contextDestroyed()

Wouldn't contextDestroyed() (called by the main thread) cause a threading race
with get() (called by the worker thread)?

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:71:
m_unthrottledTaskRunner = TaskRunnerHelper::getUnthrottledTaskRunner(document);

Note that these APIs are deprecated. Use TaskRunnerHelper::get() instead.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h (right):

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h:50:
Persistent<Observer> m_observer;

It would be slightly better to put ParentFrameTaskRunners on Oilpan's heap to
avoid the Persistent handle and a raw back pointer from the Observer object.

[kinuko, 2016-08-11 21:21:09]

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

[commit-bot: I haz the power, 2016-08-11 21:22:30]

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

[kinuko, 2016-08-11 23:21:37]

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:57:
getParentFrameTaskRunners()->get(TaskType::Unthrottled)->postTask(BLINK_FROM_HERE,
crossThreadBind(&InProcessWorkerMessagingProxy::postMessageToWorkerObject,
crossThreadUnretained(m_messagingProxy), message, passed(std::move(channels))));
On 2016/08/11 14:57:42, haraken wrote:
> 
> Do you think these tasks really need to be posted to the unthrottled task
> runner? What happens if we post the tasks to the internal task runner (which
may
> be throttled)?

I wanted to put off such explorations into follow-up patches.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp (right):

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:13: class
ParentFrameTaskRunners::Observer final : public GarbageCollected<Observer>,
public ContextLifecycleObserver {
On 2016/08/11 14:57:43, haraken wrote:
> 
> I'd use LocalFrameLifecycleObserver instead, since ParentFrameTaskRunners is
> associated with a frame.
> 
> Just help me understand: Why can't you simply make ParentFrameTaskRunners an
> observer of frame/document?

Wanted to make it an observer only in in-process cases.  Done for simplicity.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:39:
WebTaskRunner* ParentFrameTaskRunners::get(TaskType type)
On 2016/08/11 14:57:43, haraken wrote:
> 
> Nit: I'd rename this to ParentFrameTaskRunnerHelper or WorkerTaskRunnerHelper
> (for consistency with TaskRunnerHelper)

I slightly prefer the current name as this class actually stores pointers to
task runners while TaskRunnerHelper only provides static methods.  (Could be
convinced if you feel strongly)

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:42: //
(Probably some unthrottled tasks should be moved to Internal type)
On 2016/08/11 14:57:42, haraken wrote:
> 
> Yeah, our plan is to limit the unthrottled task runner to only very special
> tasks. Maybe would it make more sense to expose the internal task runner
instead
> of the unthrottled task runner?

I agree, but wanted to avoid mixing up additional behavioral changes in one
patch.  Done for try.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:56:
ParentFrameTaskRunners::ParentFrameTaskRunners(Document* document)
On 2016/08/11 14:57:43, haraken wrote:
> 
> Would it make more sense to change the parameter from Document to LocalFrame?

For out-of-process workers taking LocalFrame feels a bit weird, but we just pass
nullptr for those workers for now anyways... so might be fine.  Done.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:68: void
ParentFrameTaskRunners::contextDestroyed()
On 2016/08/11 14:57:43, haraken wrote:
> 
> Wouldn't contextDestroyed() (called by the main thread) cause a threading race
> with get() (called by the worker thread)?

Done.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.cpp:71:
m_unthrottledTaskRunner = TaskRunnerHelper::getUnthrottledTaskRunner(document);
On 2016/08/11 14:57:43, haraken wrote:
> 
> Note that these APIs are deprecated. Use TaskRunnerHelper::get() instead.

Yep, I cut corners before I get initial feedback.

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h (right):

https://codereview.chromium.org/2236833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h:50:
Persistent<Observer> m_observer;
On 2016/08/11 14:57:43, haraken wrote:
> 
> It would be slightly better to put ParentFrameTaskRunners on Oilpan's heap to
> avoid the Persistent handle and a raw back pointer from the Observer object.

Done.

[commit-bot: I haz the power, 2016-08-12 00:11:29]

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

[commit-bot: I haz the power, 2016-08-12 00:11:30]

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

[haraken, 2016-08-12 00:55:46]

LGTM

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

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:69:
getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE,
crossThreadBind(&InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject,
crossThreadUnretained(m_messagingProxy), hasPendingActivity));

I scanned the spec again and begin to think that we should always use
TaskType::PostedMessage for all tasks posted between the worker object and the
main thread. That way we can avoid a risk of breaking the task ordering around
the cross-thread communications. What do you think?

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h (right):

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h:44: struct
TaskTypeTraits : WTF::GenericHashTraits<TaskType> {

Shall we move this to TaskRunnerHelper.h?

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h:55:
TaskRunnerHash m_taskRunners;

TaskRunnerHash => TaskRunnerHashMap

[kinuko, 2016-08-15 22:56:19]

Thanks.  Let me try landing this unless I get further objections (I'll start to
be less available)

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

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:69:
getParentFrameTaskRunners()->get(TaskType::Internal)->postTask(BLINK_FROM_HERE,
crossThreadBind(&InProcessWorkerMessagingProxy::confirmMessageFromWorkerObject,
crossThreadUnretained(m_messagingProxy), hasPendingActivity));
On 2016/08/12 00:55:45, haraken wrote:
> 
> I scanned the spec again and begin to think that we should always use
> TaskType::PostedMessage for all tasks posted between the worker object and the
> main thread. That way we can avoid a risk of breaking the task ordering around
> the cross-thread communications. What do you think?

I'm less sure if that's the most reasonable mapping.  For worker code that could
be probably ok, but if we start to need to assign some particular task types not
to break task ordering that could mean something might be wrong in the current
assumption? (E.g. each task might not be able to be always cleanly mapped to
single Task Type or single dependency attribution, rather each task might belong
to zero to N such types)

For this particular patch let me try to land this as is for now.  If it starts
to break something I suppose that's probably a good sign to reconsider.

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h (right):

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h:44: struct
TaskTypeTraits : WTF::GenericHashTraits<TaskType> {
On 2016/08/12 00:55:45, haraken wrote:
> 
> Shall we move this to TaskRunnerHelper.h?

Done.

https://codereview.chromium.org/2236833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ParentFrameTaskRunners.h:55:
TaskRunnerHash m_taskRunners;
On 2016/08/12 00:55:45, haraken wrote:
> 
> TaskRunnerHash => TaskRunnerHashMap

Done.

[kinuko, 2016-08-15 22:56:37]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-08-15 22:56:38]

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/2236833002/#ps80001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-08-15 22:57:01]

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

[nhiroki, 2016-08-16 00:49:49]

lgtm

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

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

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

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

[nhiroki, 2016-08-16 02:21:52]

Failures on linux_chromium_asan_rel_ng look real.

On 2016/08/15 22:56:19, kinuko wrote:
> Thanks.  Let me try landing this unless I get further objections (I'll start
to
> be less available)

Do you think you can have time to fix failures? If it's difficult, I'll be happy
to take over this :)

[kinuko, 2016-08-16 15:08:28]

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

[commit-bot: I haz the power, 2016-08-16 15:08:43]

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-08-16 17:24:06]

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

[commit-bot: I haz the power, 2016-08-16 17:24:07]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-08-16 21:09:44]

On 2016/08/16 02:21:52, nhiroki wrote:
> Failures on linux_chromium_asan_rel_ng look real.
> 
> On 2016/08/15 22:56:19, kinuko wrote:
> > Thanks.  Let me try landing this unless I get further objections (I'll start
> to
> > be less available)
> 
> Do you think you can have time to fix failures? If it's difficult, I'll be
happy
> to take over this :)

Thanks, I think it's fixed in PS5.  The previous code could have assigned the
same value for 'isdeleted' and 'empty' slot in TaskTypeTraits (was a silly
bug..).

[kinuko, 2016-08-16 21:43:32]

Checking CQ again, but please feel free to clear if anyone's uncomfortable &|
take it over if it makes things smoother.  Thanks

[kinuko, 2016-08-16 21:43:42]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-08-16 21:43:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2236833002/#ps100001
(title: "fixing hash traits")

[commit-bot: I haz the power, 2016-08-16 21:44:12]

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-08-16 21:48:38]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed, but I'd like it to be fixed separately.

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed, but I'd like it to be fixed separately.

BUG=627034
==========

[commit-bot: I haz the power, 2016-08-16 21:48:40]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-16 21:52:46]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed, but I'd like it to be fixed separately.

BUG=627034
==========

to

==========
Pass per-frame task runners to Workers (when possible) [2nd try]

- For in-process workers (i.e. dedicated/compositor workers): always use
  the associated document's task runners
- For out-of-process workers (i.e. service/shared workers): keep using the
  default task runner of the main thread, but via TaskRunnerHelper so
  we could still start using different task runners for different tasks
  (e.g. loading vs timer) if we want.

[New] Diff from previous patch (PS1,
https://codereview.chromium.org/2163983004):

- No longer clone and hold per-frame task runners, instead reset them
  when the parent context goes away using ContextLifecycleObserver
  (because Workers could post tasks that trigger manual deletion for non-
  oilpan objects and if such tasks get dropped we could leak [*])
- Support TaskType in a very limited way

[*] The 'manual deletion' part in the worker code should be probably fixed /
removed, but I'd like it to be fixed separately.

BUG=627034

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

[commit-bot: I haz the power, 2016-08-16 21:52:47]

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