New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 693988 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Disallow cross-thread Persistent<> read access

Reported by sigbjo...@opera.com, Feb 19 2017

Issue description

The Blink GC infrastructure has per-thread heaps - a thread's heap allocations and references will be relative to it. With the allocated objects using Member<> if the reference is to another GCed heap object, or Persistent<> if it is a reference from outside the heap, (strongly) pointing into it and keeping the object alive.

For the (few) times that cross-heap references need to be kept, the CrossThreadPersistent<> abstraction is used. Using Persistent<> for the same is not appropriate.

To detect such Persistent<> use, asserts are in place to verify that a Persistent<> isn't assigned a heap object from another thread's heap. But there are currently no checks in place upon a Persistent<> read -- that it happens only on the owning thread.

Disallowing cross-thread Persistent<> read access has some merit, now and later, hence we should consider adding extra checks to prevent those.

Such read access could indicate that code hasn't fully considered cross-thread use, and the non-barrier'ed cross-heap access is unsafe in the presences of GCs being triggered on the heap owning the object.
 
Project Member

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

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

commit e27847a80deaea0674c951a7be26dda066a05cb8
Author: sigbjornf <sigbjornf@opera.com>
Date: Tue Feb 21 11:17:03 2017

Simplify MediaStreamAudioSourceNode ownerships.

The processing that happens on the audio thread for this object
doesn't access the MediaStream. Hence the reference to it and the
track can be kept directly on the node object, thereby avoiding the
use of a pair of Persistent<>s.

R=haraken
BUG= 693988 

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

[modify] https://crrev.com/e27847a80deaea0674c951a7be26dda066a05cb8/third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp
[modify] https://crrev.com/e27847a80deaea0674c951a7be26dda066a05cb8/third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.h

Project Member

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

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

commit 2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b
Author: sigbjornf <sigbjornf@opera.com>
Date: Tue Feb 21 14:03:11 2017

Disallow cross-thread Persistent<> read access.

A Persistent<> reference is belongs to the thread that created it,
read and write access must only be performed by that thread.

Debug verification have been in place for some time to verify that Persistent<>
updates only happen on its creation thread, and that the updated heap pointer
resides on that thread's heap. Extend the debug checks to also apply to read
access, checking that no other thread accesses the Persistent<>.

This requires converting a handful of Persistent<>s to CrossThreadPersistent<>s.

R=haraken
BUG= 693988 

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

[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/core/testing/DummyPageHolder.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/core/workers/WorkerThread.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/ConvolverNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.cpp
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/OscillatorNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/modules/webaudio/PannerNode.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/platform/exported/WebScrollbarImpl.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/platform/graphics/CompositorMutatorClient.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/platform/heap/Persistent.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b/third_party/WebKit/Source/web/WebSharedWorkerImpl.h

Comment 3 by sigbjo...@opera.com, Feb 21 2017

Owner: sigbjo...@opera.com
Status: Fixed (was: Untriaged)

Sign in to add a comment