WorkerLoaderProxy: stop accessing cross-thread execution context

WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927, 673711
Review-Url: https://codereview.chromium.org/2709193002
Cr-Commit-Position: refs/heads/master@{#452458}
Committed: https://chromium.googlesource.com/chromium/src/+/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd

[kinuko, 2017-02-22 10:14:38]

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

[commit-bot: I haz the power, 2017-02-22 10:15:28]

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, 2017-02-22 10:30:58]

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

[commit-bot: I haz the power, 2017-02-22 10:30:59]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kinuko, 2017-02-22 12:53:20]

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

[commit-bot: I haz the power, 2017-02-22 12:53:40]

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, 2017-02-22 13:10:40]

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

[commit-bot: I haz the power, 2017-02-22 13:10:41]

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

[kinuko, 2017-02-22 14:23:38]

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

[kinuko, 2017-02-22 14:23:43]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-22 14:24:03]

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, 2017-02-22 14:34:12]

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

[commit-bot: I haz the power, 2017-02-22 14:34:13]

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

[kinuko, 2017-02-22 15:41:52]

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

[commit-bot: I haz the power, 2017-02-22 15:42:14]

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, 2017-02-22 15:52:49]

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

[commit-bot: I haz the power, 2017-02-22 15:52:50]

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

[kinuko, 2017-02-22 15:56:44]

Patchset #3 (id:60001) has been deleted

[kinuko, 2017-02-22 15:56:58]

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

[commit-bot: I haz the power, 2017-02-22 15:57:24]

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, 2017-02-22 16:09:25]

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

[commit-bot: I haz the power, 2017-02-22 16:09:26]

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

[kinuko, 2017-02-23 00:28:26]

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

[commit-bot: I haz the power, 2017-02-23 00:29:27]

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, 2017-02-23 00:46:36]

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

[commit-bot: I haz the power, 2017-02-23 00:46:37]

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

[kinuko, 2017-02-23 04:43:45]

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

[kinuko, 2017-02-23 04:43:50]

Patchset #4 (id:100001) has been deleted

[kinuko, 2017-02-23 04:43:57]

Patchset #4 (id:110001) has been deleted

[commit-bot: I haz the power, 2017-02-23 04:44:04]

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

[kinuko, 2017-02-23 04:45:47]

Description was changed from

==========
WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927
==========

to

==========
WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927
==========

[kinuko, 2017-02-23 04:45:48]

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

[kinuko, 2017-02-23 04:46:19]

Wdyt?

[nhiroki, 2017-02-23 05:09:20]

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

[nhiroki, 2017-02-23 05:09:21]

+tzik@ fyi

[nhiroki, 2017-02-23 05:27:12]

Nice patch! LGTM :)

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerLoaderProxy.h (right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerLoaderProxy.h:102: // This may
return nullptr.
Can you add a comment about thread restriction?

[nhiroki, 2017-02-23 05:28:30]

Can you add 673711 to BUG= ?

[kinuko, 2017-02-23 05:33:18]

Description was changed from

==========
WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927
==========

to

==========
WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927, 673711
==========

[tzik, 2017-02-23 05:46:14]

lgtm

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h
(right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:163:
PassRefPtr<WorkerLoaderProxy>,
PassRefPtr will be removed soon. Can we use RefPtr + std::move?

[yhirano, 2017-02-23 05:55:58]

lgtm

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:461:
*toDocument(loaderProxy->getLoaderExecutionContext()), std::move(request),
loaderContext

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
(right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:48:
ExecutionContext* getExecutionContext() const {
DCHECK(isParentContextThread())

[commit-bot: I haz the power, 2017-02-23 06:16:13]

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

[commit-bot: I haz the power, 2017-02-23 06:16:14]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-02-23 07:28:53]

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

[commit-bot: I haz the power, 2017-02-23 07:29:04]

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

[kinuko, 2017-02-23 07:35:30]

Thanks!

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:461:
*toDocument(loaderProxy->getLoaderExecutionContext()), std::move(request),
On 2017/02/23 05:55:58, yhirano wrote:
> loaderContext

Done.

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
(right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:48:
ExecutionContext* getExecutionContext() const {
On 2017/02/23 05:55:58, yhirano wrote:
> DCHECK(isParentContextThread())

Done.

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerLoaderProxy.h (right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerLoaderProxy.h:102: // This may
return nullptr.
On 2017/02/23 05:27:12, nhiroki wrote:
> Can you add a comment about thread restriction?

Done.

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h
(right):

https://codereview.chromium.org/2709193002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h:163:
PassRefPtr<WorkerLoaderProxy>,
On 2017/02/23 05:46:14, tzik wrote:
> PassRefPtr will be removed soon. Can we use RefPtr + std::move?

Sure. Yay for the deprecation!

Done.

[kinuko, 2017-02-23 07:36:26]

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

[kinuko, 2017-02-23 07:47:16]

kinuko@chromium.org changed reviewers:
- haraken@chromium.org

[kinuko, 2017-02-23 07:50:58]

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

[kinuko, 2017-02-23 07:54:06]

kinuko@chromium.org changed reviewers:
- haraken@chromium.org

[commit-bot: I haz the power, 2017-02-23 07:57:08]

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

[commit-bot: I haz the power, 2017-02-23 07:57:09]

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

[kinuko, 2017-02-23 08:08:19]

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

[commit-bot: I haz the power, 2017-02-23 08:08:34]

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

[kinuko, 2017-02-23 08:09:05]

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

[kinuko, 2017-02-23 08:09:39]

+haraken for owner review, thanks.

[haraken, 2017-02-23 09:11:54]

LGTM

https://codereview.chromium.org/2709193002/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerLoaderProxy.cpp (right):

https://codereview.chromium.org/2709193002/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerLoaderProxy.cpp:48: MutexLocker
locker(m_lock);

Would you help me understand why we need this lock? I was assuming that
getLoaderExecutionContext is accessed only by the main thread.

[kinuko, 2017-02-23 09:25:18]

Thanks!

https://codereview.chromium.org/2709193002/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerLoaderProxy.cpp (right):

https://codereview.chromium.org/2709193002/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerLoaderProxy.cpp:48: MutexLocker
locker(m_lock);
On 2017/02/23 09:11:54, haraken wrote:
> 
> Would you help me understand why we need this lock? I was assuming that
> getLoaderExecutionContext is accessed only by the main thread.

This was to protect m_loaderProxyProvider access. But looking into the calling
convention detachProvider can be only called on the main thread too, so looks
like we can remove this (and the one in postTaskToWorkerGlobalScope).  Done.

[kinuko, 2017-02-23 09:25:25]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-02-23 09:25:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, yhirano@chromium.org, tzik@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2709193002/#ps210001
(title: ".")

[commit-bot: I haz the power, 2017-02-23 09:25:34]

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, 2017-02-23 11:14:11]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1487841925252980,
"parent_rev": "fa2337ad665fad794a067127e6e4bca8d30764c6", "commit_rev":
"2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd"}

[commit-bot: I haz the power, 2017-02-23 11:14:46]

Description was changed from

==========
WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927, 673711
==========

to

==========
WorkerLoaderProxy: stop accessing cross-thread execution context

* Removes ExecutionContextTask
* Adds getLoadingExecutionContext() accessor to WorkerLoaderProxy{,Provider}
* Stops accessing execution context in a cross-thread way

We could possibly further clean up WorkerLoaderProxy, but I'll save it
for later changes.

BUG=625927, 673711

Review-Url: https://codereview.chromium.org/2709193002
Cr-Commit-Position: refs/heads/master@{#452458}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a4d9d1f74a04c1122bc7ca32027...
==========

[commit-bot: I haz the power, 2017-02-23 11:14:47]

Committed patchset #8 (id:210001) as
https://chromium.googlesource.com/chromium/src/+/2a4d9d1f74a04c1122bc7ca32027...