Prevent SharedArrayBuffer views from being used in bindings

Prevent SharedArrayBuffer views from being used in bindings

This CL adds a check to any API that takes an ArrayBufferView. This check
ensures that isShared() returns false, so the function cannot be used with an
ArrayBufferView that is backed by a SharedArrayBuffer. Currently, all APIs
match three cases:

1. Taking an ArrayBufferView (or derived type or typedef) directly:

    ImageData(UInt8ClampedArray, ...)

2. Taking a union type where one type is an ArrayBufferView:

    FontFace(DOMString, (DOMString or ArrayBuffer or ArrayBufferView), ...);

3. Taking a sequence of union type where one type is an ArrayBuffeView:

    Blob(sequence<(ArrayBuffer or ArrayBufferView or Blob or DOMString)>, ...);

If a new pattern is introduced, it will raise an exception in the bindings
generator. This will prevent us from accidentally allowing SharedArrayBuffer
views for these new functions.

BUG=569681

[binji, 2017-01-18 22:14:49]

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

[commit-bot: I haz the power, 2017-01-18 22:15:11]

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-01-18 22:25:14]

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

[commit-bot: I haz the power, 2017-01-18 22:25:15]

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

[binji, 2017-01-18 22:49:14]

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

[commit-bot: I haz the power, 2017-01-18 22:49:53]

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-01-19 01:03:22]

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

[commit-bot: I haz the power, 2017-01-19 01:03:23]

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

[binji, 2017-01-19 03:23:42]

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

[binji, 2017-01-19 03:23:42]

Still adding some more tests, but wanted to see if this made sense, or if there
was a better/different way to handle this.

[esprehn, 2017-01-19 03:54:03]

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

[esprehn, 2017-01-19 03:54:04]

Can we introduce two subclasses instead? I'm weary of where we're ending up with
the isShared() checks in all the methods. I'd prefer we handle this with the
type system.

[binji, 2017-01-19 18:44:28]

On 2017/01/19 03:54:04, esprehn wrote:
> Can we introduce two subclasses instead? I'm weary of where we're ending up
with
> the isShared() checks in all the methods. I'd prefer we handle this with the
> type system.

Sorry, I'm not sure if I understand how to handle this with the type system. The
issue is that an ArrayBufferView can be constructed with an ArrayBuffer or
SharedArrayBuffer as backing store, but you can't tell from the type which it
is. The original SharedArrayBuffer proposal actually had a split hierarchy (e.g.
SharedUint8Array, etc.) but we moved away from that long ago.

[jbroman, 2017-01-19 18:47:30]

Have the standards folk talked about the right way to handle this?
- I could see authors having the same issues we have with being surprised by
views into shared buffers.
- What happens if a method declared in WebIDL as taking ArrayBufferView gets one
backed by a shared buffer should be specced somewhere.

[binji, 2017-01-19 18:53:10]

On 2017/01/19 18:47:30, jbroman wrote:
> Have the standards folk talked about the right way to handle this?
> - I could see authors having the same issues we have with being surprised by
> views into shared buffers.
> - What happens if a method declared in WebIDL as taking ArrayBufferView gets
one
> backed by a shared buffer should be specced somewhere.

There's some discussion here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1231687

and here https://github.com/tc39/ecmascript_sharedmem/issues/38

[binji, 2017-01-19 18:55:38]

Description was changed from

==========
Prevent SharedArrayBuffer views from being used in bindings

This CL adds a check to any API that takes an ArrayBufferView. This check
ensures that isShared() returns false, so the function cannot be used with an
ArrayBufferView that is backed by a SharedArrayBuffer. Currently, all APIs
match three cases:

1. Taking an ArrayBufferView (or derived type or typedef) directly:

    ImageData(UInt8ClampedArray, ...)

2. Taking a union type where one type is an ArrayBufferView:

    FontFace(DOMString, (DOMString or ArrayBuffer or ArrayBufferView), ...);

3. Taking a sequence of union type where one type is an ArrayBuffeView:

    Blob(sequence<(ArrayBuffer or ArrayBufferView or Blob or DOMString)>, ...);

If a new pattern is introduced, it will raise an exception in the bindings
generator. This will prevent us from accidentally allowing SharedArrayBuffer
views for these new functions.

BUG=569681
==========

to

==========
Prevent SharedArrayBuffer views from being used in bindings

This CL adds a check to any API that takes an ArrayBufferView. This check
ensures that isShared() returns false, so the function cannot be used with an
ArrayBufferView that is backed by a SharedArrayBuffer. Currently, all APIs
match three cases:

1. Taking an ArrayBufferView (or derived type or typedef) directly:

    ImageData(UInt8ClampedArray, ...)

2. Taking a union type where one type is an ArrayBufferView:

    FontFace(DOMString, (DOMString or ArrayBuffer or ArrayBufferView), ...);

3. Taking a sequence of union type where one type is an ArrayBuffeView:

    Blob(sequence<(ArrayBuffer or ArrayBufferView or Blob or DOMString)>, ...);

If a new pattern is introduced, it will raise an exception in the bindings
generator. This will prevent us from accidentally allowing SharedArrayBuffer
views for these new functions.

BUG=569681
==========

[binji, 2017-01-19 18:56:08]

On 2017/01/19 18:53:10, binji wrote:
> On 2017/01/19 18:47:30, jbroman wrote:
> > Have the standards folk talked about the right way to handle this?
> > - I could see authors having the same issues we have with being surprised by
> > views into shared buffers.
> > - What happens if a method declared in WebIDL as taking ArrayBufferView gets
> one
> > backed by a shared buffer should be specced somewhere.
> 
> There's some discussion here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1231687
> 
> and here https://github.com/tc39/ecmascript_sharedmem/issues/38

+domenic

On 2017/01/19 18:53:10, binji wrote:
> On 2017/01/19 18:47:30, jbroman wrote:
> > Have the standards folk talked about the right way to handle this?
> > - I could see authors having the same issues we have with being surprised by
> > views into shared buffers.
> > - What happens if a method declared in WebIDL as taking ArrayBufferView gets
> one
> > backed by a shared buffer should be specced somewhere.
> 
> There's some discussion here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1231687
> 
> and here https://github.com/tc39/ecmascript_sharedmem/issues/38

+domenic

And here too http://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#webgl

[domenic, 2017-01-19 20:42:26]

The standards plan is to create a Web IDL mechanism for opting in. Currently the
best path forward seems to be (a) disallow shared array buffers to match the
existing typed array types (b) allowing extended attributes to apply to types,
see
https://lists.w3.org/Archives/Public/public-script-coord/2017JanMar/0002.html;
(c) add a [AllowShared] extended attribute to opt-in to allowing shared backing
buffers.

For now in Chromium disallowing by default is the right thing to do. We can also
figure out some way of allowing opt-in for the specific Web GL cases, ahead of
standardization of the right Web IDL way of doing so, as long as there is
cross-vendor agreement on which APIs exactly get that opt-in.

[jbroman, 2017-01-19 21:47:33]

OK. That's approximately (but not quite?) what this CL does.

Is FlexibleArrayBufferView the same thing as the [AllowShared] extended
attribute?

This CL also doesn't seem to look at dictionaries, but (for example)
MIDIMessageEventInit would seem to suffer from the same issue. There's an
allusion to it in the thread dominic linked (#18), but I don't see it mentioned
in this CL or the attached bug.

[binji, 2017-01-20 00:20:16]

> This CL also doesn't seem to look at dictionaries, but (for example)
> MIDIMessageEventInit would seem to suffer from the same issue. There's an
> allusion to it in the thread dominic linked (#18), but I don't see it
mentioned
> in this CL or the attached bug.

You're right, I just missed this case. I was hoping that has_array_buffer_view
would catch cases like this, but it looks like dictionaries are handled
specially.

[binji, 2017-01-26 01:45:23]

On 2017/01/20 00:20:16, binji wrote:
> > This CL also doesn't seem to look at dictionaries, but (for example)
> > MIDIMessageEventInit would seem to suffer from the same issue. There's an
> > allusion to it in the thread dominic linked (#18), but I don't see it
> mentioned
> > in this CL or the attached bug.
> 
> You're right, I just missed this case. I was hoping that has_array_buffer_view
> would catch cases like this, but it looks like dictionaries are handled
> specially.

I discussed this with esprehn@, and we agreed that it would be better to use the
C++ type system to catch these cases, rather than the binding system.