Pass per-frame task runners to Workers (when possible)

Pass per-frame task runners to Workers (when possible)

- 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.

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

[kinuko, 2016-07-21 07:47:11]

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

[commit-bot: I haz the power, 2016-07-21 07:47:23]

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-07-21 08:42:16]

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

[commit-bot: I haz the power, 2016-07-21 08:42:17]

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

[kinuko, 2016-07-26 14:10:24]

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

[commit-bot: I haz the power, 2016-07-26 14:10:39]

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-07-26 14:20:45]

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

[commit-bot: I haz the power, 2016-07-26 14:20:46]

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

[kinuko, 2016-07-26 14:29:14]

Patchset #2 (id:20001) has been deleted

[kinuko, 2016-07-26 15:19:34]

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

[commit-bot: I haz the power, 2016-07-26 15:19:47]

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-07-26 16:36:30]

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

[commit-bot: I haz the power, 2016-07-26 16:36:31]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-07-26 21:47:39]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible)

- 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.

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

to

==========
Pass per-frame task runners to Workers (when possible)

- 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.

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

[kinuko, 2016-07-26 21:47:39]

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

[kinuko, 2016-07-26 21:48:29]

Something like this would probably work? PTL

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

https://codereview.chromium.org/2163983004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:102:
WebTaskRunner* InProcessWorkerMessagingProxy::getParentFrameTaskRunner()
Currently this returns unthrottled task runner just because it was mentioned in
the original thread, but can be changed to whatever we want

[nhiroki, 2016-07-26 22:44:01]

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

[nhiroki, 2016-07-26 22:44:01]

drive-by comment

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

https://codereview.chromium.org/2163983004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:108: //
yet.
Just a question: Why don't you initialize |m_parentFrameTaskRunners| on the
ctor?

[kinuko, 2016-07-27 02:11:49]

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

https://codereview.chromium.org/2163983004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:108: //
yet.
On 2016/07/26 22:44:01, nhiroki wrote:
> Just a question: Why don't you initialize |m_parentFrameTaskRunners| on the
> ctor?

Hmm, probably that's simpler. (Hadn't expected we'd need this before we create
worker thread until I hit some test failures)

Done.

[kinuko, 2016-07-27 02:11:57]

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

[commit-bot: I haz the power, 2016-07-27 02:12:16]

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-07-27 02:14:33]

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

[commit-bot: I haz the power, 2016-07-27 02:14:34]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[nhiroki, 2016-07-27 03:55:59]

lgtm from worker's POV

[kinuko, 2016-07-27 08:14:55]

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

[commit-bot: I haz the power, 2016-07-27 08:15:21]

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

[haraken, 2016-07-27 09:30:12]

LGTM

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

https://codereview.chromium.org/2163983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:104:
WebTaskRunner* InProcessWorkerMessagingProxy::getParentFrameTaskRunner()

I'd avoid adding this helper method though. Each call site should explicitly
specify what task runner it should use.

https://codereview.chromium.org/2163983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:237:
getParentFrameTaskRunner()->postTask(BLINK_FROM_HERE,
WTF::bind(&InProcessWorkerMessagingProxy::workerObjectDestroyedInternal,
unretained(this)));

Don't we need to make this a cross-thread task?

[commit-bot: I haz the power, 2016-07-27 10:21:21]

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

[commit-bot: I haz the power, 2016-07-27 10:21:22]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-07-27 17:11:31]

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

[commit-bot: I haz the power, 2016-07-27 17:11:51]

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-07-27 18:35:38]

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

[commit-bot: I haz the power, 2016-07-27 18:35:39]

Dry run: 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_...)

[kinuko, 2016-07-27 23:21:02]

Thanks,

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

https://codereview.chromium.org/2163983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:104:
WebTaskRunner* InProcessWorkerMessagingProxy::getParentFrameTaskRunner()
On 2016/07/27 09:30:12, haraken wrote:
> 
> I'd avoid adding this helper method though. Each call site should explicitly
> specify what task runner it should use.

Done.

https://codereview.chromium.org/2163983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/InProcessWorkerMessagingProxy.cpp:237:
getParentFrameTaskRunner()->postTask(BLINK_FROM_HERE,
WTF::bind(&InProcessWorkerMessagingProxy::workerObjectDestroyedInternal,
unretained(this)));
On 2016/07/27 09:30:12, haraken wrote:
> 
> Don't we need to make this a cross-thread task?

This looks to be always same-thread post task.

[haraken, 2016-07-28 08:22:14]

Still LGTM

[Sami, 2016-07-28 13:56:14]

lgtm.

I was trying to think of a better alternative to unthrottledTaskRunner() here
but couldn't come up with one. One option would be to add a workerTaskRunner()
but I'm not sure if that would be useful or not yet.

[haraken, 2016-07-28 14:02:37]

On 2016/07/28 13:56:14, Sami wrote:
> lgtm.
> 
> I was trying to think of a better alternative to unthrottledTaskRunner() here
> but couldn't come up with one. One option would be to add a workerTaskRunner()
> but I'm not sure if that would be useful or not yet.

I think unthrottledTaskRunner() should be renamed to something like
internalTaskRunner() and the usage should be highly limited. Alternately, we
should introduce defaultTaskRunner() or some specific task runner like
workerTaskRunner() (if we don't want to introduce defaultTaskRunner()). That's
what we've been discussing in platform-architecture-dev@. I think we can land
this CL as is and then update the task runners once we reach consensus about it.

[Sami, 2016-07-28 14:47:10]

On 2016/07/28 14:02:37, haraken wrote:
> On 2016/07/28 13:56:14, Sami wrote:
> > lgtm.
> > 
> > I was trying to think of a better alternative to unthrottledTaskRunner()
here
> > but couldn't come up with one. One option would be to add a
workerTaskRunner()
> > but I'm not sure if that would be useful or not yet.
> 
> I think unthrottledTaskRunner() should be renamed to something like
> internalTaskRunner() and the usage should be highly limited. Alternately, we
> should introduce defaultTaskRunner() or some specific task runner like
> workerTaskRunner() (if we don't want to introduce defaultTaskRunner()). That's
> what we've been discussing in platform-architecture-dev@. I think we can land
> this CL as is and then update the task runners once we reach consensus about
it.

Agreed. FWIW I'm opposed anything with "default" for the same reasons as Ojan --
it sounds too attractive to use :)

[kinuko, 2016-07-28 15:43:54]

Yep, I assume we can change the unloved unthrottledTaskRunner one to something
better later... (I can explicitly leave some TODO comment if it's preferable).

I'm going to land this in a day or so if hiroshige or no one else objects.

[kinuko, 2016-07-29 09:31:53]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-07-29 09:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2163983004/#ps100001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-07-29 09:32:02]

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-29 12:44:19]

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

[commit-bot: I haz the power, 2016-07-29 12:44:20]

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_...)

[kinuko, 2016-07-29 15:03:02]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2016-07-29 15:03:16]

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-29 16:28:21]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible)

- 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.

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

to

==========
Pass per-frame task runners to Workers (when possible)

- 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.

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

[commit-bot: I haz the power, 2016-07-29 16:28:22]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-29 16:30:10]

Description was changed from

==========
Pass per-frame task runners to Workers (when possible)

- 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.

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

to

==========
Pass per-frame task runners to Workers (when possible)

- 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.

BUG=627034

Committed: https://crrev.com/66b07a326160fed57046fbb454f424a302d4cc6b
Cr-Commit-Position: refs/heads/master@{#408652}
==========

[commit-bot: I haz the power, 2016-07-29 16:30:12]

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

[tapted, 2016-08-01 02:43:26]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2196133002/ by tapted@chromium.org.

The reason for reverting is: Suspected for multiple failures in `WebKit Linux
Leak` since
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...

e.g. 

http/tests/serviceworker/foreign-fetch-workers.html
fast/workers/worker-document-leak.html
+ 7 more

errors like

10:21:04.195 21200 worker/1 http/tests/serviceworker/foreign-fetch-workers.html
leaked
10:21:04.196 21133 [14636/39978]
http/tests/serviceworker/foreign-fetch-workers.html failed unexpectedly (leak
detected:
({"numberOfLiveActiveDOMObjects":[2,4],"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,28],"numberOfLiveResources":[0,2]}))
10:21:04.195 21200 worker/1 http/tests/serviceworker/foreign-fetch-workers.html
failed:
10:21:04.195 21200 worker/1  leak detected:
({"numberOfLiveActiveDOMObjects":[2,4],"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,28],"numberOfLiveResources":[0,2]})
.

[tzik, 2016-08-01 05:24:56]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2199633002/ by tzik@chromium.org.

The reason for reverting is: This CL seems to cause a test failure on "WebKit
Linux Leak" bot.
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu...

An example of the failure is:
http/tests/serviceworker/foreign-fetch-workers.html
({"numberOfLiveActiveDOMObjects":[2,3],"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,15],"numberOfLiveResources":[0,1]}).

[tzik, 2016-08-01 05:26:27]

On 2016/08/01 02:43:26, tapted wrote:
> A revert of this CL (patchset #5 id:100001) has been created in
> https://codereview.chromium.org/2196133002/ by mailto:tapted@chromium.org.
> 
> The reason for reverting is: Suspected for multiple failures in `WebKit Linux
> Leak` since
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
> 
> e.g. 
> 
> http/tests/serviceworker/foreign-fetch-workers.html
> fast/workers/worker-document-leak.html
> + 7 more
> 
> errors like
> 
> 10:21:04.195 21200 worker/1
http/tests/serviceworker/foreign-fetch-workers.html
> leaked
> 10:21:04.196 21133 [14636/39978]
> http/tests/serviceworker/foreign-fetch-workers.html failed unexpectedly (leak
> detected:
>
({"numberOfLiveActiveDOMObjects":[2,4],"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,28],"numberOfLiveResources":[0,2]}))
> 10:21:04.195 21200 worker/1
http/tests/serviceworker/foreign-fetch-workers.html
> failed:
> 10:21:04.195 21200 worker/1  leak detected:
>
({"numberOfLiveActiveDOMObjects":[2,4],"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,28],"numberOfLiveResources":[0,2]})
> .

Let me make another try.