MessagePort::messageAvailable() should not call non-thread-safe function |
|||||
Issue description
MessagePort::messageAvailable() warns that non-thread-safe functions should not be called in this, but it actually calls ContextLifecycleObserver::getExecutionContext() that is a non-thread-safe function.
// Invoked to notify us that there are messages available for this port.
// This code may be called from another thread, and so should not call any
// non-threadsafe APIs (i.e. should not call into the entangled channel or
// access mutable variables).
void MessagePort::messageAvailable() {
DCHECK(getExecutionContext());
// TODO(tzik): Use ParentThreadTaskRunners instead of ExecutionContext here to
// avoid touching foreign thread GCed object.
getExecutionContext()->postTask(
TaskType::PostedMessage, BLINK_FROM_HERE,
createCrossThreadTask(&MessagePort::dispatchMessages,
wrapCrossThreadWeakPersistent(this)));
}
,
Mar 9 2017
,
Apr 12 2017
Once https://codereview.chromium.org/2806623004/ is landed, TaskRunnerHelper is available for WorkerOrWorkletGlobalScope and probably we can fix this issue as follows: 1) Capture a task runner for the current execution context using TaskRunnerHelper in the ctor of MessagePort. 2) Post a task to the task runner in messageAvailable().
,
Apr 14 2017
I have a CL: https://codereview.chromium.org/2818073002/ This is now blocked by https://codereview.chromium.org/2806623004/ as commented above.
,
Apr 14 2017
,
Apr 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/829907ac0d3d2ed39d8752d22a6d0e45df1e8325 commit 829907ac0d3d2ed39d8752d22a6d0e45df1e8325 Author: nhiroki <nhiroki@chromium.org> Date: Thu Apr 20 03:21:04 2017 Messaging: Avoid calling non-thread-safe functions in MessagePort::messageAvailable() As implementation comments in MessagePort::messageAvailable(), it can be called from another thread and should not call non-thread-functions. However, the current implementation wrongly calls ContextLifecycleObserver's GetExecutionContext() that is not a thread-safe function to post a task to the context thread. To avoid that, this CL replaces ExecutionContext::PostTask() with WebTaskRunner::PostTask(). The task runner is captured in the ctor of MessagePort called on the context thread. BUG= 694925 Review-Url: https://codereview.chromium.org/2818073002 Cr-Commit-Position: refs/heads/master@{#465886} [modify] https://crrev.com/829907ac0d3d2ed39d8752d22a6d0e45df1e8325/third_party/WebKit/Source/core/dom/MessagePort.cpp [modify] https://crrev.com/829907ac0d3d2ed39d8752d22a6d0e45df1e8325/third_party/WebKit/Source/core/dom/MessagePort.h [modify] https://crrev.com/829907ac0d3d2ed39d8752d22a6d0e45df1e8325/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
,
Apr 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7cbe41ace5629ba6b9ec5502803f02fb2f09b0fd commit 7cbe41ace5629ba6b9ec5502803f02fb2f09b0fd Author: nhiroki <nhiroki@chromium.org> Date: Thu Apr 20 03:40:38 2017 Revert of Messaging: Avoid calling non-thread-safe functions in MessagePort::messageAvailable() (patchset #2 id:20001 of https://codereview.chromium.org/2818073002/ ) Reason for revert: [1] that this CL depends on needs to be reverted: https://codereview.chromium.org/2831843002/ Original issue's description: > Messaging: Avoid calling non-thread-safe functions in MessagePort::messageAvailable() > > As implementation comments in MessagePort::messageAvailable(), it can be called > from another thread and should not call non-thread-functions. However, the > current implementation wrongly calls ContextLifecycleObserver's > GetExecutionContext() that is not a thread-safe function to post a task to the > context thread. > > To avoid that, this CL replaces ExecutionContext::PostTask() with > WebTaskRunner::PostTask(). The task runner is captured in the ctor of > MessagePort called on the context thread. > > BUG= 694925 > > Review-Url: https://codereview.chromium.org/2818073002 > Cr-Commit-Position: refs/heads/master@{#465886} > Committed: https://chromium.googlesource.com/chromium/src/+/829907ac0d3d2ed39d8752d22a6d0e45df1e8325 TBR=tzik@chromium.org,yuryu@chromium.org,kinuko@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 694925 Review-Url: https://codereview.chromium.org/2833673002 Cr-Commit-Position: refs/heads/master@{#465890} [modify] https://crrev.com/7cbe41ace5629ba6b9ec5502803f02fb2f09b0fd/third_party/WebKit/Source/core/dom/MessagePort.cpp [modify] https://crrev.com/7cbe41ace5629ba6b9ec5502803f02fb2f09b0fd/third_party/WebKit/Source/core/dom/MessagePort.h [modify] https://crrev.com/7cbe41ace5629ba6b9ec5502803f02fb2f09b0fd/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90cbb1bd3931fb8ee58c729bbbcdb32f4915dd1f commit 90cbb1bd3931fb8ee58c729bbbcdb32f4915dd1f Author: nhiroki <nhiroki@chromium.org> Date: Fri Apr 21 01:59:19 2017 Reland of Messaging: Avoid calling non-thread-safe functions in MessagePort::messageAvailable() (patchset #1 id:1 of https://codereview.chromium.org/2833673002/ ) Reason for revert: The CL that this CL depends on was relanded: https://codereview.chromium.org/2832763002/ Original issue's description: > Revert of Messaging: Avoid calling non-thread-safe functions in MessagePort::messageAvailable() (patchset #2 id:20001 of https://codereview.chromium.org/2818073002/ ) > > Reason for revert: > [1] that this CL depends on needs to be reverted: > https://codereview.chromium.org/2831843002/ > > Original issue's description: > > Messaging: Avoid calling non-thread-safe functions in MessagePort::messageAvailable() > > > > As implementation comments in MessagePort::messageAvailable(), it can be called > > from another thread and should not call non-thread-functions. However, the > > current implementation wrongly calls ContextLifecycleObserver's > > GetExecutionContext() that is not a thread-safe function to post a task to the > > context thread. > > > > To avoid that, this CL replaces ExecutionContext::PostTask() with > > WebTaskRunner::PostTask(). The task runner is captured in the ctor of > > MessagePort called on the context thread. > > > > BUG= 694925 > > > > Review-Url: https://codereview.chromium.org/2818073002 > > Cr-Commit-Position: refs/heads/master@{#465886} > > Committed: https://chromium.googlesource.com/chromium/src/+/829907ac0d3d2ed39d8752d22a6d0e45df1e8325 > > TBR=tzik@chromium.org,yuryu@chromium.org,kinuko@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 694925 > > Review-Url: https://codereview.chromium.org/2833673002 > Cr-Commit-Position: refs/heads/master@{#465890} > Committed: https://chromium.googlesource.com/chromium/src/+/7cbe41ace5629ba6b9ec5502803f02fb2f09b0fd TBR=tzik@chromium.org,yuryu@chromium.org,kinuko@chromium.org BUG= 694925 Review-Url: https://codereview.chromium.org/2836473002 Cr-Commit-Position: refs/heads/master@{#466223} [modify] https://crrev.com/90cbb1bd3931fb8ee58c729bbbcdb32f4915dd1f/third_party/WebKit/Source/core/dom/MessagePort.cpp [modify] https://crrev.com/90cbb1bd3931fb8ee58c729bbbcdb32f4915dd1f/third_party/WebKit/Source/core/dom/MessagePort.h [modify] https://crrev.com/90cbb1bd3931fb8ee58c729bbbcdb32f4915dd1f/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
,
Apr 21 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by nhiroki@chromium.org
, Feb 22 2017