Disallow cross-thread Persistent<> read access.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b

[sof, 2017-02-20 14:30:34]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 14:30:47]

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

[sof, 2017-02-20 15:30:45]

Description was changed from

==========
Check same-thread Persistent<> read access.

R=
BUG=
==========

to

==========
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=
BUG=693988
==========

[sof, 2017-02-20 15:44:33]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2017-02-20 15:44:35]

please take a look.

as can be seen, no major sinkholes discovered in the codebase by adding this
constraint. Open to discuss if we want to adopt such a check at this time, of
course.

[commit-bot: I haz the power, 2017-02-20 16:33:42]

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

[commit-bot: I haz the power, 2017-02-20 16:33:43]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-20 23:57:17]

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

[haraken, 2017-02-20 23:57:18]

LGTM to introducing the runtime verification.

However, I think we should try to convert most of the CrossThreadPersistents
introduced by this CL to Persistents by restructuring the code base. For
example, it's not really nice that we're letting workers access Frame,
ExecutionContext etc.

In any case, this CL is a good step forward to identify problematic
CrossThreadPersistents.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/DummyPageHolder.h (right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/DummyPageHolder.h:91:
CrossThreadPersistent<LocalFrame> m_frame;

// TODO: This should be a Persistent. Frame must not be accessed by non-main
threads.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h
(right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.h:96:
CrossThreadPersistent<ExecutionContext> m_executionContext;

Ditto.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.h
(right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.h:81:
CrossThreadPersistent<HTMLMediaElement> m_mediaElement;

Ditto.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h
(right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.h:62:
CrossThreadPersistent<MediaStreamSource> m_source;

Ditto.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebScrollbarImpl.h (right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebScrollbarImpl.h:73:
CrossThreadPersistent<Scrollbar> m_scrollbar;

Ditto.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h:141:
CrossThreadPersistent<WebLocalFrameImpl> m_mainFrame;

Ditto.

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.h (right):

https://codereview.chromium.org/2702243003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebSharedWorkerImpl.h:161:
CrossThreadPersistent<ExecutionContext> m_loadingDocument;

Ditto.

[sof, 2017-02-21 06:24:04]

On 2017/02/20 23:57:18, haraken wrote:
> LGTM to introducing the runtime verification.
> 
> However, I think we should try to convert most of the CrossThreadPersistents
> introduced by this CL to Persistents by restructuring the code base. For
> example, it's not really nice that we're letting workers access Frame,
> ExecutionContext etc.
> 

Agreed. I considered having Persistent::crossThreadGet(), but opted against in
the end. For some of these uses, a non-owning thread reaches in and grabs out a
reference, only to hand it over by posting that reference to the owning thread.
A single crossThreadGet() (which doesn't call checkPointer()) would allow those,
and could be made safe.

At the cost of adding more to the programming model, which is why there are CTPs
only here.

[sof, 2017-02-21 11:35:02]

Description was changed from

==========
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=
BUG=693988
==========

to

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

[sof, 2017-02-21 11:36:37]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-02-21 11:36:38]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2702243003/#ps60001
(title: "rebased upto r451733")

[commit-bot: I haz the power, 2017-02-21 11:36:51]

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-21 14:03:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487676997457430,
"parent_rev": "0c6f1cf6eb88a9e0a2382f78ec4202a162901c8d", "commit_rev":
"2d8378bf293f5a0a0dbec1f5c2436fe7eaa51d3b"}

[commit-bot: I haz the power, 2017-02-21 14:03:58]

Description was changed from

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

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d8378bf293f5a0a0dbec1f5c243...
==========

[commit-bot: I haz the power, 2017-02-21 14:04:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2d8378bf293f5a0a0dbec1f5c243...