New issue
Advanced search Search tips

Issue 673711 link

Starred by 0 users

Issue metadata

Status: Closed
Owner:
Closed: Nov 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Make thread boundaries clearer in worker components

Project Member Reported by nhiroki@chromium.org, Dec 13 2016

Issue description

Some worker classes can be accessed from both the main thread and worker thread. This would be error-prone. For example, WebEmbeddedWorkerImpl::postTaskToLoader() traversals GC'ed objects on the main thread from the worker thread as follows:

// Called on the worker thread.
void WebEmbeddedWorkerImpl::postTaskToLoader(
    const WebTraceLocation& location,
    std::unique_ptr<ExecutionContextTask> task) {
  // m_mainFrame, frame() and document() are on the main thread.
  m_mainFrame->frame()->document()->postTask(location, std::move(task));
}

This may lead to chaos when GC occurs simultaneously. To improve the situation, we might want to separate such classes into 2 parts: one for the main thread, and another for the worker thread. Then, we could limit communication channels among them: ParentFrameTaskRunners for worker->main communication and *something* for main->worker communication. ThreadedMessagingProxyBase and ThreadedObjectProxyBase would be a good example of this way.

This is an incomplete list of classes that can be accessed from both the main thread and worker thread:

- WorkerThread
- WebEmbeddedWorkerImpl
- WebSharedWorkerImpl
 
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18959bb68b0d32d7ff551b82aaaa9817154146f4

commit 18959bb68b0d32d7ff551b82aaaa9817154146f4
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Feb 21 14:42:07 2017

Worker: Fix wrong comment in InProcessWorkerObjectProxy

This wrong comment looks trivial but very confusing because
ThreadedMessagingProxyBase lives on the main thread, while
ThreadedObjectProxyBase lives on a worker thread.

BUG= 673711 

Review-Url: https://codereview.chromium.org/2710563003
Cr-Commit-Position: refs/heads/master@{#451758}

[modify] https://crrev.com/18959bb68b0d32d7ff551b82aaaa9817154146f4/third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd

commit 2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd
Author: kinuko <kinuko@chromium.org>
Date: Thu Feb 23 11:14:07 2017

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}

[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/workers/WorkerLoaderProxy.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/workers/WorkerLoaderProxy.h
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/core/workers/WorkerThreadTestHelper.h
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.h
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/2a4d9d1f74a04c1122bc7ca32027da87cb14d3bd/third_party/WebKit/Source/web/WebSharedWorkerImpl.h

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0decaefae68c308e45aad3b3fbda9c5b9a6793a8

commit 0decaefae68c308e45aad3b3fbda9c5b9a6793a8
Author: nhiroki <nhiroki@chromium.org>
Date: Tue Feb 28 04:47:38 2017

SharedWorker: Factor out WorkerReportingProxy impl from WebSharedWorkerImpl

WebSharedWorkerImpl is a heavily threaded class. Some methods are called on the
main thread, while others are called on a worker thread. This often confuses my
head...

This CL factors out a part of WebSharedWorkerImpl that implement
WorkerReportingProxy and are called on a worker thread into
WebSharedWorkerReportingProxyImpl. Even with this cleanup, some methods called
on a worker thread still remain in WebSharedWorkerImpl, but this would somewhat
mitigate the situation and make threading boundary clearer.

BUG= 673711 

Review-Url: https://codereview.chromium.org/2721543002
Cr-Commit-Position: refs/heads/master@{#453499}

[modify] https://crrev.com/0decaefae68c308e45aad3b3fbda9c5b9a6793a8/third_party/WebKit/Source/web/BUILD.gn
[modify] https://crrev.com/0decaefae68c308e45aad3b3fbda9c5b9a6793a8/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/0decaefae68c308e45aad3b3fbda9c5b9a6793a8/third_party/WebKit/Source/web/WebSharedWorkerImpl.h
[add] https://crrev.com/0decaefae68c308e45aad3b3fbda9c5b9a6793a8/third_party/WebKit/Source/web/WebSharedWorkerReportingProxyImpl.cpp
[add] https://crrev.com/0decaefae68c308e45aad3b3fbda9c5b9a6793a8/third_party/WebKit/Source/web/WebSharedWorkerReportingProxyImpl.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5

commit 1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Jun 12 03:07:47 2017

Worker: Tighten the visibility of functions on ThreadedMessagingProxyBase

This is a refactoring CL and does not change behavior.

This CL...
 - moves CreateWorkerThread() from 'protected' to 'private'.
 - moves getters from 'public' to 'protected'.
 - renames GetThreadableLoadingContext() to CreateThreadableLoadingContext()
   because this returns a newly created context.
 - adds class-level comments and thread checks.

Bug:  673711 ,  719775 
Change-Id: I855e43eba5f51858cf0afb68a074682de8ef0523
Reviewed-on: https://chromium-review.googlesource.com/527714
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478541}
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/core/workers/DedicatedWorkerMessagingProxy.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/core/workers/DedicatedWorkerMessagingProxy.h
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/core/workers/DedicatedWorkerTest.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/core/workers/ThreadedWorkletTest.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletMessagingProxy.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/modules/compositorworker/AnimationWorkletMessagingProxy.h
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerMessagingProxy.h
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/modules/webaudio/AudioWorkletMessagingProxy.cpp
[modify] https://crrev.com/1ee2c8a31e8dd7be86f1acde8aac1f7411bf06b5/third_party/WebKit/Source/modules/webaudio/AudioWorkletMessagingProxy.h

Status: Closed (was: Assigned)
I have no plans on this for now. I'll file a separate issue if we need more work.

Sign in to add a comment