[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs

[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2707243006
Cr-Commit-Position: refs/heads/master@{#463513}
Committed: https://chromium.googlesource.com/chromium/src/+/39aa85f571c22c63815e7b07069c96ec37b9024b

[binji, 2017-02-24 00:32:53]

Description was changed from

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs
==========

to

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[binji, 2017-02-24 00:38:56]

binji@chromium.org changed reviewers:
+ esprehn@chromium.org

[binji, 2017-02-24 00:38:56]

PTAL, this CL is not complete (the support for unions is missing), but I'd
appreciate if you could take a quick look and tell me if it's going in the right
direction.

Also: I'm not sure about how best to store these values in an object. Does
Member<MaybeShared<...>> make sense, or something else...?

[binji, 2017-03-06 19:57:01]

Friendly ping.

[Raymond Toy, 2017-03-06 20:06:32]

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

[Raymond Toy, 2017-03-06 20:06:33]

DBC:  The WebAudio spec doesn't really say anything about this.  Does the spec
need to be updated to disallow this?

https://codereview.chromium.org/2707243006/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/2707243006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:477: "The provided
curve is backed by a SharedArrayBuffer.");
DBC:  Can we be consistent with the error messages in webaudio about
SharedArrayBuffer?

[binji, 2017-03-13 21:51:50]

On 2017/03/06 20:06:33, Raymond Toy wrote:
> DBC:  The WebAudio spec doesn't really say anything about this.  Does the spec
> need to be updated to disallow this?


There are a number of discussions about this on different issue trackers and
mailing lists, and I'm having trouble finding all the threads :-)

Here's one that discusses using extended attributes to annotate where a
SharedArrayBuffer would be allowed: https://github.com/heycam/webidl/pull/286

But my understanding is that basically we want to whitelist SharedArrayBuffer
use in most APIs, including where an ArrayBufferView or TypedArray is used but
backed by a SharedArrayBuffer. The current whitelist only includes WebGL:
https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#webgl

I suppose this means that you won't need to modify the WebAudio spec unless
there is a desire to allow SharedArrayBuffers.

[binji, 2017-03-14 21:07:08]

Description was changed from

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[binji, 2017-03-14 21:27:34]

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

[binji, 2017-03-14 21:27:35]

+haraken

PTAL, I haven't spent too much time trying to make this code look nice, but the
basic idea is there and seems to be catching all cases where a SharedArrayBuffer
may be passed to a Web API.

I'm curious about a few things:

* Is there a better place/name for MaybeShared?

* What is the best option for handling SharedArrayBuffers when the API doesn't
pass an exceptionState?

* Any ideas on how to have more consistency of error message? Helper functions
on MaybeShared and ExceptionState both seem a bit wrong to me.

* I did a sanity check and it seems that this found the APIs that use
ArrayBuffers that I grepped for manually:
https://github.com/tc39/ecmascript_sharedmem/issues/38#issuecomment-163048837,
any others I might be missing?

* Any clever way to test, or best to just add additional tests to each API
directly?

Thanks :-)

https://codereview.chromium.org/2707243006/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/2707243006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:477: "The provided
curve is backed by a SharedArrayBuffer.");
On 2017/03/06 20:06:33, Raymond Toy wrote:
> DBC:  Can we be consistent with the error messages in webaudio about
> SharedArrayBuffer?

Yes, I was trying to be consistent with other error messages in the same file,
but I'd be fine with using something consistent for all SAB error messages too.

[haraken, 2017-03-15 09:03:00]

On 2017/03/14 21:27:35, binji wrote:
> +haraken
> 
> PTAL, I haven't spent too much time trying to make this code look nice, but
the
> basic idea is there and seems to be catching all cases where a
SharedArrayBuffer
> may be passed to a Web API.
> 
> I'm curious about a few things:
> 
> * Is there a better place/name for MaybeShared?
> 
> * What is the best option for handling SharedArrayBuffers when the API doesn't
> pass an exceptionState?
> 
> * Any ideas on how to have more consistency of error message? Helper functions
> on MaybeShared and ExceptionState both seem a bit wrong to me.
> 
> * I did a sanity check and it seems that this found the APIs that use
> ArrayBuffers that I grepped for manually:
> https://github.com/tc39/ecmascript_sharedmem/issues/38#issuecomment-163048837,
> any others I might be missing?
> 
> * Any clever way to test, or best to just add additional tests to each API
> directly?
> 
> Thanks :-)

Sorry, I'm getting a bit lost. Would you help me remember what the motivation
for this change is?

[binji, 2017-03-15 17:24:30]

On 2017/03/15 09:03:00, haraken wrote:
> On 2017/03/14 21:27:35, binji wrote:
> > +haraken
> > 
> > PTAL, I haven't spent too much time trying to make this code look nice, but
> the
> > basic idea is there and seems to be catching all cases where a
> SharedArrayBuffer
> > may be passed to a Web API.
> > 
> > I'm curious about a few things:
> > 
> > * Is there a better place/name for MaybeShared?
> > 
> > * What is the best option for handling SharedArrayBuffers when the API
doesn't
> > pass an exceptionState?
> > 
> > * Any ideas on how to have more consistency of error message? Helper
functions
> > on MaybeShared and ExceptionState both seem a bit wrong to me.
> > 
> > * I did a sanity check and it seems that this found the APIs that use
> > ArrayBuffers that I grepped for manually:
> >
https://github.com/tc39/ecmascript_sharedmem/issues/38#issuecomment-163048837,
> > any others I might be missing?
> > 
> > * Any clever way to test, or best to just add additional tests to each API
> > directly?
> > 
> > Thanks :-)
> 
> Sorry, I'm getting a bit lost. Would you help me remember what the motivation
> for this change is?

No problem. The context is that a TypedArray can be constructed with a
SharedArrayBuffer, but we don't want to allow these to be passed to most APIs.
Originally I tried to handle this in the binding layer, but esprehn@ suggested
that it would be better to have it handled by the C++ type system, so I created
MaybeShared<T>, which is returned from the binding layer so all user code must
explicitly determine whether it allows a view with a SharedArrayBuffer.

A future change will allow certain APIs to use SharedArrayBuffer views --
currently that only includes WebGL APIs.

[haraken, 2017-03-15 21:17:12]

Elliott: I'm not sure if this is going to be a nice way to implement the
isShared check. I'd prefer implementing the isShared check in the IDL compiler
(i.e., https://codereview.chromium.org/1526183004/) for the following reasons.

- https://codereview.chromium.org/1526183004/ is less intrusive than this CL. It
seems tidier to implement the isShared check in the auto-generated bindings
instead of asking all C++ implementations to handle it.

- On the spec side, there's some discussion to introduce an IDL extended
attribute to specify if the API accepts a shared buffer or not
(https://github.com/heycam/webidl/pull/286). Then it seems nicer to implement
the functionality in the IDL compiler.

Any thoughts?

[esprehn, 2017-03-23 21:55:09]

On 2017/03/15 at 21:17:12, haraken wrote:
> Elliott: I'm not sure if this is going to be a nice way to implement the
isShared check. I'd prefer implementing the isShared check in the IDL compiler
(i.e., https://codereview.chromium.org/1526183004/) for the following reasons.
> 
> - https://codereview.chromium.org/1526183004/ is less intrusive than this CL.
It seems tidier to implement the isShared check in the auto-generated bindings
instead of asking all C++ implementations to handle it.
> 
> - On the spec side, there's some discussion to introduce an IDL extended
attribute to specify if the API accepts a shared buffer or not
(https://github.com/heycam/webidl/pull/286). Then it seems nicer to implement
the functionality in the IDL compiler.
> 
> Any thoughts?

I didn't want this in the bindings. That means from C++ you can call a method
with an invalid input by accident and you end up with magic internal behavior
the spec says is not really allowed.

ex. If a spec requires that a string input is never null, the C++ code should
enforce that, not the bindings.

If the spec says that only some method should accept SharedArrayBuffers then we
should enforce that here too.

[haraken, 2017-03-24 02:33:11]

On 2017/03/23 21:55:09, esprehn wrote:
> On 2017/03/15 at 21:17:12, haraken wrote:
> > Elliott: I'm not sure if this is going to be a nice way to implement the
> isShared check. I'd prefer implementing the isShared check in the IDL compiler
> (i.e., https://codereview.chromium.org/1526183004/) for the following reasons.
> > 
> > - https://codereview.chromium.org/1526183004/ is less intrusive than this
CL.
> It seems tidier to implement the isShared check in the auto-generated bindings
> instead of asking all C++ implementations to handle it.
> > 
> > - On the spec side, there's some discussion to introduce an IDL extended
> attribute to specify if the API accepts a shared buffer or not
> (https://github.com/heycam/webidl/pull/286). Then it seems nicer to implement
> the functionality in the IDL compiler.
> > 
> > Any thoughts?
> 
> I didn't want this in the bindings. That means from C++ you can call a method
> with an invalid input by accident and you end up with magic internal behavior
> the spec says is not really allowed.
> 
> ex. If a spec requires that a string input is never null, the C++ code should
> enforce that, not the bindings.
> 
> If the spec says that only some method should accept SharedArrayBuffers then
we
> should enforce that here too.

Where is the behavior about SharedArrayBuffers defined in the spec? Does each
DOM attribute/method describe if it should accept SharedArrayBuffers or not?

[binji, 2017-03-27 18:06:45]

> > If the spec says that only some method should accept SharedArrayBuffers then
> we
> > should enforce that here too.
> 
> Where is the behavior about SharedArrayBuffers defined in the spec? Does each
DOM attribute/method describe if it should accept SharedArrayBuffers or not?

As I mentioned above, I don't know if this is spec'd anywhere other than
https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html. I'm guessing it
would be nicest to not have to update all the affected specs just to say that
SharedArrayBuffer-backed views are not allowed, but perhaps it's best for
clarity?

[binji, 2017-03-27 21:21:35]

@esprehn: thanks for your feedback, that helps me understand the motivation.

Do you mind taking a look at the implementation here? If you're too busy, can
you suggest someone who can take a look? Thanks. :-)

[haraken, 2017-03-28 02:18:01]

On 2017/03/27 18:06:45, binji wrote:
> > > If the spec says that only some method should accept SharedArrayBuffers
then
> > we
> > > should enforce that here too.
> > 
> > Where is the behavior about SharedArrayBuffers defined in the spec? Does
each
> DOM attribute/method describe if it should accept SharedArrayBuffers or not?
> 
> As I mentioned above, I don't know if this is spec'd anywhere other than
> https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html. I'm guessing it
> would be nicest to not have to update all the affected specs just to say that
> SharedArrayBuffer-backed views are not allowed, but perhaps it's best for
> clarity?

I asked the question because I thought that:

- if it's speced in the WebIDL spec, it makes sense to implement the check in
the IDL compiler.
- if it's speced in the spec of each DOM attribute/method, it makes sense to
implement the check in each C++ implementation.

According to your answer, it seems like that none of the two is the case.

Regarding Elliott's concern, do you know how many sites are calling the C++
methods without going through the binding layer? If it's many, it would make
sense to implement the check in each C++ implementation (i.e., this CL).
Otherwise, I'd prefer implementing the check in the binding layer.

[domenic, 2017-03-28 07:43:23]

On 2017/03/28 at 02:18:01, haraken wrote:
> On 2017/03/27 18:06:45, binji wrote:
> > > > If the spec says that only some method should accept SharedArrayBuffers
then
> > > we
> > > > should enforce that here too.
> > > 
> > > Where is the behavior about SharedArrayBuffers defined in the spec? Does
each
> > DOM attribute/method describe if it should accept SharedArrayBuffers or not?
> > 
> > As I mentioned above, I don't know if this is spec'd anywhere other than
> > https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html. I'm guessing it
> > would be nicest to not have to update all the affected specs just to say
that
> > SharedArrayBuffer-backed views are not allowed, but perhaps it's best for
> > clarity?
> 
> I asked the question because I thought that:
> 
> - if it's speced in the WebIDL spec, it makes sense to implement the check in
the IDL compiler.
> - if it's speced in the spec of each DOM attribute/method, it makes sense to
implement the check in each C++ implementation.
> 
> According to your answer, it seems like that none of the two is the case.
> 
> Regarding Elliott's concern, do you know how many sites are calling the C++
methods without going through the binding layer? If it's many, it would make
sense to implement the check in each C++ implementation (i.e., this CL).
Otherwise, I'd prefer implementing the check in the binding layer.

Hi all, I am sorry I didn't see this before.

The plan is to spec this in Web IDL. https://github.com/heycam/webidl/pull/286
lays the groundwork and then a follow-up CL will add an [AllowShared] extended
attribute which applies to the various BufferSource types. So then all specs
will be updated so that their IDL contains [AllowShared].

[haraken, 2017-03-28 07:44:47]

On 2017/03/28 07:43:23, domenic wrote:
> On 2017/03/28 at 02:18:01, haraken wrote:
> > On 2017/03/27 18:06:45, binji wrote:
> > > > > If the spec says that only some method should accept
SharedArrayBuffers
> then
> > > > we
> > > > > should enforce that here too.
> > > > 
> > > > Where is the behavior about SharedArrayBuffers defined in the spec? Does
> each
> > > DOM attribute/method describe if it should accept SharedArrayBuffers or
not?
> > > 
> > > As I mentioned above, I don't know if this is spec'd anywhere other than
> > > https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html. I'm guessing
it
> > > would be nicest to not have to update all the affected specs just to say
> that
> > > SharedArrayBuffer-backed views are not allowed, but perhaps it's best for
> > > clarity?
> > 
> > I asked the question because I thought that:
> > 
> > - if it's speced in the WebIDL spec, it makes sense to implement the check
in
> the IDL compiler.
> > - if it's speced in the spec of each DOM attribute/method, it makes sense to
> implement the check in each C++ implementation.
> > 
> > According to your answer, it seems like that none of the two is the case.
> > 
> > Regarding Elliott's concern, do you know how many sites are calling the C++
> methods without going through the binding layer? If it's many, it would make
> sense to implement the check in each C++ implementation (i.e., this CL).
> Otherwise, I'd prefer implementing the check in the binding layer.
> 
> Hi all, I am sorry I didn't see this before.
> 
> The plan is to spec this in Web IDL. https://github.com/heycam/webidl/pull/286
> lays the groundwork and then a follow-up CL will add an [AllowShared] extended
> attribute which applies to the various BufferSource types. So then all specs
> will be updated so that their IDL contains [AllowShared].

Thanks domenic! Given that it will be speced in WebIDL, I'd prefer implementing
the check in the IDL compiler.

[esprehn, 2017-03-28 21:42:06]

On 2017/03/28 at 07:44:47, haraken wrote:
> On 2017/03/28 07:43:23, domenic wrote:
> > On 2017/03/28 at 02:18:01, haraken wrote:
> > ...
> > 
> > The plan is to spec this in Web IDL.
https://github.com/heycam/webidl/pull/286
> > lays the groundwork and then a follow-up CL will add an [AllowShared]
extended
> > attribute which applies to the various BufferSource types. So then all specs
> > will be updated so that their IDL contains [AllowShared].
> 
> Thanks domenic! Given that it will be speced in WebIDL, I'd prefer
implementing the check in the IDL compiler.

The spec should not dictate where we implement things. If we make the code in
the C++ not follow the spec we end up with technical debt where the C++ callers
are depending on something that should be impossible. My idea here was the idl
compiler produces either MaybeSharedArray<> or NotSharedArray<> based on the
annotations in the spec, and the C++ must follow it. Any function which has
[AllowSharedArrayBuffer] in the idl where the C++ didn't expect it would fail to
compile.

[haraken, 2017-03-29 06:52:39]

On 2017/03/28 21:42:06, esprehn wrote:
> On 2017/03/28 at 07:44:47, haraken wrote:
> > On 2017/03/28 07:43:23, domenic wrote:
> > > On 2017/03/28 at 02:18:01, haraken wrote:
> > > ...
> > > 
> > > The plan is to spec this in Web IDL.
> https://github.com/heycam/webidl/pull/286
> > > lays the groundwork and then a follow-up CL will add an [AllowShared]
> extended
> > > attribute which applies to the various BufferSource types. So then all
specs
> > > will be updated so that their IDL contains [AllowShared].
> > 
> > Thanks domenic! Given that it will be speced in WebIDL, I'd prefer
> implementing the check in the IDL compiler.
> 
> The spec should not dictate where we implement things. If we make the code in
> the C++ not follow the spec we end up with technical debt where the C++
callers
> are depending on something that should be impossible. My idea here was the idl
> compiler produces either MaybeSharedArray<> or NotSharedArray<> based on the
> annotations in the spec, and the C++ must follow it. Any function which has
> [AllowSharedArrayBuffer] in the idl where the C++ didn't expect it would fail
to
> compile.

In practice, how many call sites are calling the C++ methods directly (i.e.,
without going through the binding layer)?

I'm a bit worried about introducing MaybeSharedArray<> because 1) this CL is
introducing a lot of churns to the code base and 2) V8's Maybe handles were
introduced with a similar motivation but has messed up the code base a lot :/

[esprehn, 2017-03-30 22:11:26]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org

[dglazkov, 2017-03-30 23:04:51]

On 2017/03/29 at 06:52:39, haraken wrote:
> On 2017/03/28 21:42:06, esprehn wrote:
> > On 2017/03/28 at 07:44:47, haraken wrote:
> > > On 2017/03/28 07:43:23, domenic wrote:
> > > > On 2017/03/28 at 02:18:01, haraken wrote:
> > > > ...
> > > > 
> > > > The plan is to spec this in Web IDL.
> > https://github.com/heycam/webidl/pull/286
> > > > lays the groundwork and then a follow-up CL will add an [AllowShared]
> > extended
> > > > attribute which applies to the various BufferSource types. So then all
specs
> > > > will be updated so that their IDL contains [AllowShared].
> > > 
> > > Thanks domenic! Given that it will be speced in WebIDL, I'd prefer
> > implementing the check in the IDL compiler.
> > 
> > The spec should not dictate where we implement things. If we make the code
in
> > the C++ not follow the spec we end up with technical debt where the C++
callers
> > are depending on something that should be impossible. My idea here was the
idl
> > compiler produces either MaybeSharedArray<> or NotSharedArray<> based on the
> > annotations in the spec, and the C++ must follow it. Any function which has
> > [AllowSharedArrayBuffer] in the idl where the C++ didn't expect it would
fail to
> > compile.
> 
> In practice, how many call sites are calling the C++ methods directly (i.e.,
without going through the binding layer)?
> 
> I'm a bit worried about introducing MaybeSharedArray<> because 1) this CL is
introducing a lot of churns to the code base and 2) V8's Maybe handles were
introduced with a similar motivation but has messed up the code base a lot :/

I share your worry about cleanliness of the code, Kentaro. At the same time, I
like the notion of DOM APIs being as semantically close to what they do and
bindings just being the pass-through code, IDL attribute constraints being
enforced as a compile check. Conceptually, I find this comforting because I need
a clarity and structure around the layering of concerns.

What do you think?

[haraken, 2017-03-31 05:29:20]

On 2017/03/30 23:04:51, dglazkov wrote:
> On 2017/03/29 at 06:52:39, haraken wrote:
> > On 2017/03/28 21:42:06, esprehn wrote:
> > > On 2017/03/28 at 07:44:47, haraken wrote:
> > > > On 2017/03/28 07:43:23, domenic wrote:
> > > > > On 2017/03/28 at 02:18:01, haraken wrote:
> > > > > ...
> > > > > 
> > > > > The plan is to spec this in Web IDL.
> > > https://github.com/heycam/webidl/pull/286
> > > > > lays the groundwork and then a follow-up CL will add an [AllowShared]
> > > extended
> > > > > attribute which applies to the various BufferSource types. So then all
> specs
> > > > > will be updated so that their IDL contains [AllowShared].
> > > > 
> > > > Thanks domenic! Given that it will be speced in WebIDL, I'd prefer
> > > implementing the check in the IDL compiler.
> > > 
> > > The spec should not dictate where we implement things. If we make the code
> in
> > > the C++ not follow the spec we end up with technical debt where the C++
> callers
> > > are depending on something that should be impossible. My idea here was the
> idl
> > > compiler produces either MaybeSharedArray<> or NotSharedArray<> based on
the
> > > annotations in the spec, and the C++ must follow it. Any function which
has
> > > [AllowSharedArrayBuffer] in the idl where the C++ didn't expect it would
> fail to
> > > compile.
> > 
> > In practice, how many call sites are calling the C++ methods directly (i.e.,
> without going through the binding layer)?
> > 
> > I'm a bit worried about introducing MaybeSharedArray<> because 1) this CL is
> introducing a lot of churns to the code base and 2) V8's Maybe handles were
> introduced with a similar motivation but has messed up the code base a lot :/
> 
> I share your worry about cleanliness of the code, Kentaro. At the same time, I
> like the notion of DOM APIs being as semantically close to what they do and
> bindings just being the pass-through code, IDL attribute constraints being
> enforced as a compile check. Conceptually, I find this comforting because I
need
> a clarity and structure around the layering of concerns.
> 
> What do you think?

Assuming that [AllowSharedArrayBuffer] is speced in the Web IDL, I think it
would make more sense to implement the check & error handling in the IDL
compiler.

For example, we implement [Clamp], [EnforceRrange], [TreatNullAs] etc in the IDL
compiler. We're assuming that the necessary check and the value conversion are
done in the auto-generated code and not doing any check in C++ APIs. I'd prefer
following that model for [AllowSharedArrayBuffer].

If there are many cases where the C++ APIs are called directly without going
through the auto-generated code, I'll be convinced that we should implement
[AllowSharedArrayBuffer] in a different way.

Conceptually my mental model is that things defined in the Web IDL spec should
be implemented in the auto-generated code, not in C++ APIs.

[binji, 2017-03-31 17:46:15]

On 2017/03/31 05:29:20, haraken wrote:
> On 2017/03/30 23:04:51, dglazkov wrote:
> > On 2017/03/29 at 06:52:39, haraken wrote:
> > > On 2017/03/28 21:42:06, esprehn wrote:
> > > > On 2017/03/28 at 07:44:47, haraken wrote:
> > > > > On 2017/03/28 07:43:23, domenic wrote:
> > > > > > On 2017/03/28 at 02:18:01, haraken wrote:
> > > > > > ...
> > > > > > 
> > > > > > The plan is to spec this in Web IDL.
> > > > https://github.com/heycam/webidl/pull/286
> > > > > > lays the groundwork and then a follow-up CL will add an
[AllowShared]
> > > > extended
> > > > > > attribute which applies to the various BufferSource types. So then
all
> > specs
> > > > > > will be updated so that their IDL contains [AllowShared].
> > > > > 
> > > > > Thanks domenic! Given that it will be speced in WebIDL, I'd prefer
> > > > implementing the check in the IDL compiler.
> > > > 
> > > > The spec should not dictate where we implement things. If we make the
code
> > in
> > > > the C++ not follow the spec we end up with technical debt where the C++
> > callers
> > > > are depending on something that should be impossible. My idea here was
the
> > idl
> > > > compiler produces either MaybeSharedArray<> or NotSharedArray<> based on
> the
> > > > annotations in the spec, and the C++ must follow it. Any function which
> has
> > > > [AllowSharedArrayBuffer] in the idl where the C++ didn't expect it would
> > fail to
> > > > compile.
> > > 
> > > In practice, how many call sites are calling the C++ methods directly
(i.e.,
> > without going through the binding layer)?
> > > 
> > > I'm a bit worried about introducing MaybeSharedArray<> because 1) this CL
is
> > introducing a lot of churns to the code base and 2) V8's Maybe handles were
> > introduced with a similar motivation but has messed up the code base a lot
:/
> > 
> > I share your worry about cleanliness of the code, Kentaro. At the same time,
I
> > like the notion of DOM APIs being as semantically close to what they do and
> > bindings just being the pass-through code, IDL attribute constraints being
> > enforced as a compile check. Conceptually, I find this comforting because I
> need
> > a clarity and structure around the layering of concerns.
> > 
> > What do you think?
> 
> Assuming that [AllowSharedArrayBuffer] is speced in the Web IDL, I think it
> would make more sense to implement the check & error handling in the IDL
> compiler.
> 
> For example, we implement [Clamp], [EnforceRrange], [TreatNullAs] etc in the
IDL
> compiler. We're assuming that the necessary check and the value conversion are
> done in the auto-generated code and not doing any check in C++ APIs. I'd
prefer
> following that model for [AllowSharedArrayBuffer].
> 
> If there are many cases where the C++ APIs are called directly without going
> through the auto-generated code, I'll be convinced that we should implement
> [AllowSharedArrayBuffer] in a different way.
> 
> Conceptually my mental model is that things defined in the Web IDL spec should
> be implemented in the auto-generated code, not in C++ APIs.

Just thought I'd add some info about the implementations: The obvious way (to
me) of checking for ArrayBufferView was to traverse the interface methods to
find when one was used, but it wasn't obvious to me how to do that. So instead,
I had to explicitly list all the cases where I'd expected to find an
ArrayBufferView and add checks for those cases. But when I first implemented
these checks in the binding layer, I missed a few cases (ArrayBufferViews in
dictionaries, for example); I even wrote what I thought was a reasonable
assertion to catch these, but it failed. When I switched to the C++
implementation, the binding implementation was much simpler (always return
MaybeShared<ArrayBufferView> from binding layer) and it caught the dictionary
case too. Seeing as passing a SharedArrayBuffer view to an API that doesn't
expect it could be a security issue, it seems like being more robust here would
be good.

I should also say that I agree that the code is a little bit nastier using
MaybeShared<>, but perhaps you have ideas about how we could clean it up? It
seems to me that encapsulating the exception handling code somehow would help, I
just don't know enough about Blink to say how that should be done.

[dglazkov, 2017-03-31 17:59:07]

On 2017/03/31 at 05:29:20, haraken wrote:
> On 2017/03/30 23:04:51, dglazkov wrote:
> > On 2017/03/29 at 06:52:39, haraken wrote:
> > > On 2017/03/28 21:42:06, esprehn wrote:
> > > > On 2017/03/28 at 07:44:47, haraken wrote:
> > > > > On 2017/03/28 07:43:23, domenic wrote:
> > > > > > On 2017/03/28 at 02:18:01, haraken wrote:
> > > > > > ...
> > > > > > 
> > > > > > The plan is to spec this in Web IDL.
> > > > https://github.com/heycam/webidl/pull/286
> > > > > > lays the groundwork and then a follow-up CL will add an
[AllowShared]
> > > > extended
> > > > > > attribute which applies to the various BufferSource types. So then
all
> > specs
> > > > > > will be updated so that their IDL contains [AllowShared].
> > > > > 
> > > > > Thanks domenic! Given that it will be speced in WebIDL, I'd prefer
> > > > implementing the check in the IDL compiler.
> > > > 
> > > > The spec should not dictate where we implement things. If we make the
code
> > in
> > > > the C++ not follow the spec we end up with technical debt where the C++
> > callers
> > > > are depending on something that should be impossible. My idea here was
the
> > idl
> > > > compiler produces either MaybeSharedArray<> or NotSharedArray<> based on
the
> > > > annotations in the spec, and the C++ must follow it. Any function which
has
> > > > [AllowSharedArrayBuffer] in the idl where the C++ didn't expect it would
> > fail to
> > > > compile.
> > > 
> > > In practice, how many call sites are calling the C++ methods directly
(i.e.,
> > without going through the binding layer)?
> > > 
> > > I'm a bit worried about introducing MaybeSharedArray<> because 1) this CL
is
> > introducing a lot of churns to the code base and 2) V8's Maybe handles were
> > introduced with a similar motivation but has messed up the code base a lot
:/
> > 
> > I share your worry about cleanliness of the code, Kentaro. At the same time,
I
> > like the notion of DOM APIs being as semantically close to what they do and
> > bindings just being the pass-through code, IDL attribute constraints being
> > enforced as a compile check. Conceptually, I find this comforting because I
need
> > a clarity and structure around the layering of concerns.
> > 
> > What do you think?
> 
> Assuming that [AllowSharedArrayBuffer] is speced in the Web IDL, I think it
would make more sense to implement the check & error handling in the IDL
compiler.
> 
> For example, we implement [Clamp], [EnforceRrange], [TreatNullAs] etc in the
IDL compiler. We're assuming that the necessary check and the value conversion
are done in the auto-generated code and not doing any check in C++ APIs. I'd
prefer following that model for [AllowSharedArrayBuffer].
> 
> If there are many cases where the C++ APIs are called directly without going
through the auto-generated code, I'll be convinced that we should implement
[AllowSharedArrayBuffer] in a different way.
> 
> Conceptually my mental model is that things defined in the Web IDL spec should
be implemented in the auto-generated code, not in C++ APIs.

I see that we have a couple of compelling and different view points here. In the
interests of unblocking the patch, I propose that we split off resolving that
difference into a separate thread (filed bug
https://bugs.chromium.org/p/chromium/issues/detail?id=707287). For this patch
specifically, I defer to Kentaro to make the call.

[binji, 2017-04-01 01:21:29]

It seems I misunderstood esprehn's suggestion. We discussed a bit on
https://bugs.chromium.org/p/chromium/issues/detail?id=707287; perhaps something
of a hybrid approach that does the checks in the bindings layer, but has
safeguards in the C++ layer is best. I've started working on that here, but
haven't yet put the checks in the binding layer. The changes to the C++ code is
much more straightforward now. What do you think?

[haraken, 2017-04-03 00:07:37]

On 2017/04/01 01:21:29, binji wrote:
> It seems I misunderstood esprehn's suggestion. We discussed a bit on
> https://bugs.chromium.org/p/chromium/issues/detail?id=707287; perhaps
something
> of a hybrid approach that does the checks in the bindings layer, but has
> safeguards in the C++ layer is best. I've started working on that here, but
> haven't yet put the checks in the binding layer. The changes to the C++ code
is
> much more straightforward now. What do you think?

Sorry, I was misunderstanding Elliott's suggestion as well (something like this
CL). Elliott's suggestion on
https://bugs.chromium.org/p/chromium/issues/detail?id=707287#c8 sounds nice to
me :)

Regarding where the check should be implemented, I'm happy as long as the check
& the exception handling defined in the Web IDL spec are implemented in the
binding layer (not in C++). Elliott's suggestion meets this.

[binji, 2017-04-06 02:53:13]

I've implemented the checks in the bindings layer, but I'm not sure if this is
the best way. PTAL

[haraken, 2017-04-06 08:43:54]

Thanks for being very persistent on this. Mostly LG!

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:116: this_cpp_type =
'%s::TypedArrayType*' % idl_type.cpp_type

How is this type used? I cannot find this type in the test results in this CL.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:391: ta_includes = set()

ta => typed_array

Blink prefers a fully qualified name.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:393:
ta_includes.update(['core/dom/NotShared.h'])

This is not needed since INCLUDES_FOR_TYPE['ArrayBufferView'] includes
NotShared.h, right?

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFace.h (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFace.h:144: static FontFace*
createError(ExecutionContext*,

Remove this?

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NotShared.h (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:13: class CORE_EXPORT NotShared {

Add a class-level comment.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:33: #if 0

Remove this.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:46: #if 0

Remove this.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:60: operator Member<T>() const {
return Member<T>(m_typedArray.get()); }

I'd prefer not adding these implicit conversions. If we remove them, do we need
to add a lot of manual conversions?

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioBuffer.cpp (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioBuffer.cpp:206: void
AudioBuffer::copyFromChannel(const NotShared<DOMFloat32Array>& destination,

Would it be possible to make NotShared<> be passed by value?

c.f., we're passing RefPtr<> by value.

[esprehn, 2017-04-07 00:03:37]

Description was changed from

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[esprehn, 2017-04-07 00:03:37]

esprehn@chromium.org changed reviewers:
- esprehn@chromium.org

[binji, 2017-04-09 00:40:03]

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:116: this_cpp_type =
'%s::TypedArrayType*' % idl_type.cpp_type
On 2017/04/06 08:43:54, haraken wrote:
> 
> How is this type used? I cannot find this type in the test results in this CL.

This happens when a interface method returns an ArrayBufferView, and also might
raise an exception. I added an example of this to the test results.

But I also changed the way this works; now the C++ methods return NotShared<...>
as well, so this conversion isn't required.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:391: ta_includes = set()
On 2017/04/06 08:43:54, haraken wrote:
> 
> ta => typed_array
> 
> Blink prefers a fully qualified name.

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:393:
ta_includes.update(['core/dom/NotShared.h'])
On 2017/04/06 08:43:54, haraken wrote:
> 
> This is not needed since INCLUDES_FOR_TYPE['ArrayBufferView'] includes
> NotShared.h, right?

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/FontFace.h (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/FontFace.h:144: static FontFace*
createError(ExecutionContext*,
On 2017/04/06 08:43:54, haraken wrote:
> 
> Remove this?

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NotShared.h (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:13: class CORE_EXPORT NotShared {
On 2017/04/06 08:43:54, haraken wrote:
> 
> Add a class-level comment.

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:33: #if 0
On 2017/04/06 08:43:54, haraken wrote:
> 
> Remove this.

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:46: #if 0
On 2017/04/06 08:43:54, haraken wrote:
> 
> Remove this.

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NotShared.h:60: operator Member<T>() const {
return Member<T>(m_typedArray.get()); }
On 2017/04/06 08:43:54, haraken wrote:
> 
> I'd prefer not adding these implicit conversions. If we remove them, do we
need
> to add a lot of manual conversions?

Done.

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioBuffer.cpp (right):

https://codereview.chromium.org/2707243006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioBuffer.cpp:206: void
AudioBuffer::copyFromChannel(const NotShared<DOMFloat32Array>& destination,
On 2017/04/06 08:43:54, haraken wrote:
> 
> Would it be possible to make NotShared<> be passed by value?
> 
> c.f., we're passing RefPtr<> by value.
> 

Done.

[binji, 2017-04-09 00:40:14]

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

[commit-bot: I haz the power, 2017-04-09 00:40:23]

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-04-09 00:55:31]

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

[commit-bot: I haz the power, 2017-04-09 00:55:32]

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

[binji, 2017-04-09 03:15:39]

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

[commit-bot: I haz the power, 2017-04-09 03:15:52]

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-04-09 03:50:53]

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

[commit-bot: I haz the power, 2017-04-09 03:50:55]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[haraken, 2017-04-09 14:11:45]

LGTM. Thanks for being persistent!

https://codereview.chromium.org/2707243006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/NotShared.h (right):

https://codereview.chromium.org/2707243006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/NotShared.h:65: inline T* getPtr(const
::blink::NotShared<T>& p) {

Is this needed?

[binji, 2017-04-09 23:42:28]

https://codereview.chromium.org/2707243006/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/NotShared.h (right):

https://codereview.chromium.org/2707243006/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/NotShared.h:65: inline T* getPtr(const
::blink::NotShared<T>& p) {
On 2017/04/09 at 14:11:45, haraken wrote:
> Is this needed?

It was used by a WaveShaperNode attribute getter, but I modified v8_attributes
to prevent using WTF::getPtr in that case.

[binji, 2017-04-09 23:42:39]

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

[commit-bot: I haz the power, 2017-04-09 23:42:45]

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-04-09 23:46:19]

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

[commit-bot: I haz the power, 2017-04-09 23:46:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[binji, 2017-04-10 01:33:43]

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

[commit-bot: I haz the power, 2017-04-10 01:33:53]

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

[binji, 2017-04-10 02:32:06]

Thanks, haraken! I guess I was filled with determination. :-)

BTW, I had to switch the pointer in in NotShared<...> from a Member to an
UntracedMember. It seems that an offline audio thread was using an
ArrayBufferView and would assert because the ThreadState was not valid. It seems
like this should be safe since NotShared is stack allocated.

[binji, 2017-04-10 02:32:28]

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

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

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

[haraken, 2017-04-10 02:33:24]

On 2017/04/10 02:32:06, binji wrote:
> Thanks, haraken! I guess I was filled with determination. :-)
> 
> BTW, I had to switch the pointer in in NotShared<...> from a Member to an
> UntracedMember. It seems that an offline audio thread was using an
> ArrayBufferView and would assert because the ThreadState was not valid. It
seems
> like this should be safe since NotShared is stack allocated.

Ah, if it's stack-allocated, you can just use a raw pointer. That's totally fine
(and we're doing in many places) since Oilpan's GC finds the pointer by stack
scanning.

[commit-bot: I haz the power, 2017-04-10 03:25:43]

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

[commit-bot: I haz the power, 2017-04-10 03:25:45]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[binji, 2017-04-10 18:56:27]

On 2017/04/10 at 02:33:24, haraken wrote:
> 
> Ah, if it's stack-allocated, you can just use a raw pointer. That's totally
fine (and we're doing in many places) since Oilpan's GC finds the pointer by
stack scanning.

I tried that, but the blink-gc clang plugin complained about it. Maybe there's a
way around that?

[binji, 2017-04-10 18:56:35]

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

[commit-bot: I haz the power, 2017-04-10 18:57:09]

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

[binji, 2017-04-10 20:19:23]

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

[commit-bot: I haz the power, 2017-04-10 20:20:01]

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-04-10 21:23:43]

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

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

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[haraken, 2017-04-11 00:20:21]

On 2017/04/10 18:56:27, binji wrote:
> On 2017/04/10 at 02:33:24, haraken wrote:
> > 
> > Ah, if it's stack-allocated, you can just use a raw pointer. That's totally
> fine (and we're doing in many places) since Oilpan's GC finds the pointer by
> stack scanning.
> 
> I tried that, but the blink-gc clang plugin complained about it. Maybe there's
a
> way around that?

You're right & sorry. I remember that the rule is that we should Member<> for
stack-allocated objects.

> It seems that an offline audio thread was using an
> ArrayBufferView and would assert because the ThreadState was not valid. It
seems
> like this should be safe since NotShared is stack allocated.

This is not really nice. It's not obvious that the raw pointer on the audio
thread is safe (although I think they are safe in practice) because the stack of
the audio thread is not scanned by Oilpan (because the audio thread is not
attached to Oilpan).

Would it be possible to rewrite the audio thread code to not use NotShared? In
general, it's not a good idea to implicitly use Oilpan pointers from a thread
not attached to Oilpan, and that's why you're hitting the assert.

[binji, 2017-04-11 00:33:51]

On 2017/04/11 at 00:20:21, haraken wrote:
> On 2017/04/10 18:56:27, binji wrote:
> > On 2017/04/10 at 02:33:24, haraken wrote:
> > > 
> > > Ah, if it's stack-allocated, you can just use a raw pointer. That's
totally
> > fine (and we're doing in many places) since Oilpan's GC finds the pointer by
> > stack scanning.
> > 
> > I tried that, but the blink-gc clang plugin complained about it. Maybe
there's a
> > way around that?
> 
> You're right & sorry. I remember that the rule is that we should Member<> for
stack-allocated objects.

Yes, but UntracedMember should be OK as well, right? I assume so because of
examples like this:
https://cs.chromium.org/chromium/src/tools/clang/blink_gc_plugin/tests/legacy...

> 
> > It seems that an offline audio thread was using an
> > ArrayBufferView and would assert because the ThreadState was not valid. It
seems
> > like this should be safe since NotShared is stack allocated.
> 
> This is not really nice. It's not obvious that the raw pointer on the audio
thread is safe (although I think they are safe in practice) because the stack of
the audio thread is not scanned by Oilpan (because the audio thread is not
attached to Oilpan).
> 
> Would it be possible to rewrite the audio thread code to not use NotShared? In
general, it's not a good idea to implicitly use Oilpan pointers from a thread
not attached to Oilpan, and that's why you're hitting the assert.

Ah, I see. The audio thread is using a CrossThreadPersistent<AudioBuffer>. This
way it should be rooted so it should be safe, right?

[haraken, 2017-04-11 01:00:56]

On 2017/04/11 00:33:51, binji wrote:
> On 2017/04/11 at 00:20:21, haraken wrote:
> > On 2017/04/10 18:56:27, binji wrote:
> > > On 2017/04/10 at 02:33:24, haraken wrote:
> > > > 
> > > > Ah, if it's stack-allocated, you can just use a raw pointer. That's
> totally
> > > fine (and we're doing in many places) since Oilpan's GC finds the pointer
by
> > > stack scanning.
> > > 
> > > I tried that, but the blink-gc clang plugin complained about it. Maybe
> there's a
> > > way around that?
> > 
> > You're right & sorry. I remember that the rule is that we should Member<>
for
> stack-allocated objects.
> 
> Yes, but UntracedMember should be OK as well, right? I assume so because of
> examples like this:
>
https://cs.chromium.org/chromium/src/tools/clang/blink_gc_plugin/tests/legacy...

You're right, but UntracedMember should be limited to cases where we're pretty
sure that using UntracedMember is safe for some reason. (Yes, we're in this case
now.)

> 
> > 
> > > It seems that an offline audio thread was using an
> > > ArrayBufferView and would assert because the ThreadState was not valid. It
> seems
> > > like this should be safe since NotShared is stack allocated.
> > 
> > This is not really nice. It's not obvious that the raw pointer on the audio
> thread is safe (although I think they are safe in practice) because the stack
of
> the audio thread is not scanned by Oilpan (because the audio thread is not
> attached to Oilpan).
> > 
> > Would it be possible to rewrite the audio thread code to not use NotShared?
In
> general, it's not a good idea to implicitly use Oilpan pointers from a thread
> not attached to Oilpan, and that's why you're hitting the assert.
> 
> Ah, I see. The audio thread is using a CrossThreadPersistent<AudioBuffer>.
This
> way it should be rooted so it should be safe, right?

[haraken, 2017-04-11 01:01:03]

LGTM

https://codereview.chromium.org/2707243006/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/NotShared.h (right):

https://codereview.chromium.org/2707243006/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/NotShared.h:59: // this object is only stack
allocated.

Update this comment. This is safe not because the object is stack allocated but
because the call site guarantees that the pointed-to array buffer is kept alive
in another way (i.e., CrossThreadPersistent).

If the number of the call sites on the audio thread is limited, I'd prefer not
using NotShared on the audio thread and instead use the typed array directly.
Then we can use Member here.

Either way, I don't think we should block this giant CL on it. Feel free to land
this :)

[binji, 2017-04-11 01:17:34]

https://codereview.chromium.org/2707243006/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/NotShared.h (right):

https://codereview.chromium.org/2707243006/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/NotShared.h:59: // this object is only stack
allocated.
On 2017/04/11 at 01:01:03, haraken wrote:
> Update this comment. This is safe not because the object is stack allocated
but because the call site guarantees that the pointed-to array buffer is kept
alive in another way (i.e., CrossThreadPersistent).

Done, and added TODO w/ bug as well.

> 
> If the number of the call sites on the audio thread is limited, I'd prefer not
using NotShared on the audio thread and instead use the typed array directly.
Then we can use Member here.

Yeah, it's just this one case AFAIK. I'll see if I can improve it in the next
CL. :-)

> 
> Either way, I don't think we should block this giant CL on it. Feel free to
land this :)

[binji, 2017-04-11 01:18:31]

The CQ bit was checked by binji@chromium.org

[binji, 2017-04-11 01:18:32]

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/2707243006/#ps320001
(title: "update comment, add TODO")

[commit-bot: I haz the power, 2017-04-11 01:19:17]

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-04-11 03:18:33]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1491873511685120,
"parent_rev": "52347d12f92216a49f113b958d62b1d9a48abfb0", "commit_rev":
"39aa85f571c22c63815e7b07069c96ec37b9024b"}

[commit-bot: I haz the power, 2017-04-11 03:19:33]

Description was changed from

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[SharedArrayBuffer] Prevent SharedArrayBuffer being used in Web APIs

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2707243006
Cr-Commit-Position: refs/heads/master@{#463513}
Committed:
https://chromium.googlesource.com/chromium/src/+/39aa85f571c22c63815e7b07069c...
==========

[commit-bot: I haz the power, 2017-04-11 03:19:35]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/39aa85f571c22c63815e7b07069c...

[haraken, 2017-04-11 06:10:33]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2812833003/ by haraken@chromium.org.

The reason for reverting is: This CL broke a couple of tests on Mac bots.

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1...

Let me revert the CL to make the waterfall happy at the moment.
.

[haraken, 2017-04-11 06:21:23]

I forgot to mention this earlier, but it would be nice to add more explanation
to the CL description.