Detach buffer in decodeAudioData

Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

Chrome Feature: https://www.chromestatus.com/feature/5539919174828032

BUG=704679, 696179
TEST=decodeAudioData/decode-audio-data-neuter.html

Review-Url: https://codereview.chromium.org/2746913003
Cr-Commit-Position: refs/heads/master@{#459266}
Committed: https://chromium.googlesource.com/chromium/src/+/3010bfc167e7af9d4ecd48a54189c721c2b93dbf

[Raymond Toy, 2017-03-13 20:40:06]

rtoy@chromium.org changed reviewers:
+ haraken@chromium.org, hongchan@chromium.org

[Raymond Toy, 2017-03-13 20:40:07]

PTAL.

haraken: this is a new area for me.  Is this even close?

The webaudio spec is here: 
https://webaudio.github.io/web-audio-api/#widl-BaseAudioContext-decodeAudioDa...

[haraken, 2017-03-14 03:17:48]

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

[haraken, 2017-03-14 03:17:49]

+binji

Also, can we add some tests?

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

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:288: // If
audioData is detached, we need to reject the promise with

Is isShared() equivalent to a detached buffer in the spec?

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:291:
DOMException::create(V8TypeError, "Cannot decode detached buffer");

Nit: resolver->reject(V8ThrowException::createTypeError(...))

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:300:
audioData->transfer(buf);

Why was this detachment & transfer not needed before this CL?

[Raymond Toy, 2017-03-14 15:01:03]

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

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:288: // If
audioData is detached, we need to reject the promise with
On 2017/03/14 03:17:49, haraken wrote:
> 
> Is isShared() equivalent to a detached buffer in the spec?

I don't know. I can't seem to find the correspondence between the ECMAscript
IsDetachedBuffer and DetachArrayBuffer to anything in blink.

But transfer() has a DCHECK(!isShared()), so that seems, at a minimum, something
we need to ensure.

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:300:
audioData->transfer(buf);
On 2017/03/14 03:17:49, haraken wrote:
> 
> Why was this detachment & transfer not needed before this CL?

The original version of the spec didn't have this.  Detach and transfer was
added a while ago and I just never got around to implementing it until now. 
(There are other places we need to do this, which I'l do in a followup.)

[Raymond Toy, 2017-03-14 15:02:22]

On 2017/03/14 03:17:49, haraken wrote:
> +binji
> 
> Also, can we add some tests?

Yes, I'll add some tests before landing this.  Once I figure out how to see
things are working.  But the repro case in the bug no longer happens with this
CL.

> 
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):
> 
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:288: // If
> audioData is detached, we need to reject the promise with
> 
> Is isShared() equivalent to a detached buffer in the spec?
> 
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:291:
> DOMException::create(V8TypeError, "Cannot decode detached buffer");
> 
> Nit: resolver->reject(V8ThrowException::createTypeError(...))
> 
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:300:
> audioData->transfer(buf);
> 
> Why was this detachment & transfer not needed before this CL?

[binji, 2017-03-14 17:50:55]

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

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:288: // If
audioData is detached, we need to reject the promise with
On 2017/03/14 15:01:03, Raymond Toy wrote:
> On 2017/03/14 03:17:49, haraken wrote:
> > 
> > Is isShared() equivalent to a detached buffer in the spec?
> 
> I don't know. I can't seem to find the correspondence between the ECMAscript
> IsDetachedBuffer and DetachArrayBuffer to anything in blink.
> 
> But transfer() has a DCHECK(!isShared()), so that seems, at a minimum,
something
> we need to ensure.

Right, a SharedArrayBuffer cannot be detached, and it cannot be transferred. Not
exactly sure what it should do here, but throwing an error seems reasonable.

[haraken, 2017-03-14 18:16:55]

On 2017/03/14 17:50:55, binji wrote:
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):
> 
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:288: // If
> audioData is detached, we need to reject the promise with
> On 2017/03/14 15:01:03, Raymond Toy wrote:
> > On 2017/03/14 03:17:49, haraken wrote:
> > > 
> > > Is isShared() equivalent to a detached buffer in the spec?
> > 
> > I don't know. I can't seem to find the correspondence between the ECMAscript
> > IsDetachedBuffer and DetachArrayBuffer to anything in blink.
> > 
> > But transfer() has a DCHECK(!isShared()), so that seems, at a minimum,
> something
> > we need to ensure.
> 
> Right, a SharedArrayBuffer cannot be detached, and it cannot be transferred.
Not
> exactly sure what it should do here, but throwing an error seems reasonable.

My question is about the matching between what the spec is saying and what we're
doing here.

The spec is saying that we should throw a type error if  IsDetachedBuffer
returns true. However, we're checking isShared() here. Is it correct?

[hongchan, 2017-03-14 18:22:50]

On 2017/03/14 18:16:55, haraken wrote:
> On 2017/03/14 17:50:55, binji wrote:
> >
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:288: // If
> > audioData is detached, we need to reject the promise with
> > On 2017/03/14 15:01:03, Raymond Toy wrote:
> > > On 2017/03/14 03:17:49, haraken wrote:
> > > > 
> > > > Is isShared() equivalent to a detached buffer in the spec?
> > > 
> > > I don't know. I can't seem to find the correspondence between the
ECMAscript
> > > IsDetachedBuffer and DetachArrayBuffer to anything in blink.
> > > 
> > > But transfer() has a DCHECK(!isShared()), so that seems, at a minimum,
> > something
> > > we need to ensure.
> > 
> > Right, a SharedArrayBuffer cannot be detached, and it cannot be transferred.
> Not
> > exactly sure what it should do here, but throwing an error seems reasonable.
> 
> My question is about the matching between what the spec is saying and what
we're
> doing here.
> 
> The spec is saying that we should throw a type error if  IsDetachedBuffer
> returns true. However, we're checking isShared() here. Is it correct?

Yes, Ray and I discussed that issue offline. It seems like Blink does not the
equivalent of "IsDetachedBuffer".

[binji, 2017-03-14 18:28:11]

> > My question is about the matching between what the spec is saying and what
> we're
> > doing here.
> > 
> > The spec is saying that we should throw a type error if  IsDetachedBuffer
> > returns true. However, we're checking isShared() here. Is it correct?
> 
> Yes, Ray and I discussed that issue offline. It seems like Blink does not the
> equivalent of "IsDetachedBuffer".

Well, there is WTF::ArrayBuffer::isNeutered, which is another name for the same
thing, I believe.

[Raymond Toy, 2017-03-14 18:52:45]

On 2017/03/14 18:28:11, binji wrote:
> > > My question is about the matching between what the spec is saying and what
> > we're
> > > doing here.
> > > 
> > > The spec is saying that we should throw a type error if  IsDetachedBuffer
> > > returns true. However, we're checking isShared() here. Is it correct?
> > 
> > Yes, Ray and I discussed that issue offline. It seems like Blink does not
the
> > equivalent of "IsDetachedBuffer".
> 
> Well, there is WTF::ArrayBuffer::isNeutered, which is another name for the
same
> thing, I believe.

Is it?  The code paths for isShared and isNeutered seem to end up in different
places.  It looks like isShared eventually gets to m_isShared == Shared and
isNeutered is m_isNeutered.

(I didn't investigate how m_isNeutered is set vs hos m_isShared is set.  It
seems bad if we have two names for the same thing.)

[binji, 2017-03-14 19:02:15]

On 2017/03/14 18:52:45, Raymond Toy wrote:
> On 2017/03/14 18:28:11, binji wrote:
> > > > My question is about the matching between what the spec is saying and
what
> > > we're
> > > > doing here.
> > > > 
> > > > The spec is saying that we should throw a type error if 
IsDetachedBuffer
> > > > returns true. However, we're checking isShared() here. Is it correct?
> > > 
> > > Yes, Ray and I discussed that issue offline. It seems like Blink does not
> the
> > > equivalent of "IsDetachedBuffer".
> > 
> > Well, there is WTF::ArrayBuffer::isNeutered, which is another name for the
> same
> > thing, I believe.
> 
> Is it?  The code paths for isShared and isNeutered seem to end up in different
> places.  It looks like isShared eventually gets to m_isShared == Shared and
> isNeutered is m_isNeutered.
> 
> (I didn't investigate how m_isNeutered is set vs hos m_isShared is set.  It
> seems bad if we have two names for the same thing.)

Sorry, I think I made things less clear. :-)

isShared is true if the ArrayBuffer is actually a SharedArrayBuffer (see
https://tc39.github.io/ecma262/#sec-sharedarraybuffer-objects)

isNeutered is true when the ArrayBuffer is neutered (which is called detached in
the es spec -- my understanding is that "neutered" is the old terminology back
before ArrayBuffers were standardized in ecmascript).

[Raymond Toy, 2017-03-14 19:11:03]

On 2017/03/14 19:02:15, binji wrote:
> On 2017/03/14 18:52:45, Raymond Toy wrote:
> > On 2017/03/14 18:28:11, binji wrote:
> > > > > My question is about the matching between what the spec is saying and
> what
> > > > we're
> > > > > doing here.
> > > > > 
> > > > > The spec is saying that we should throw a type error if 
> IsDetachedBuffer
> > > > > returns true. However, we're checking isShared() here. Is it correct?
> > > > 
> > > > Yes, Ray and I discussed that issue offline. It seems like Blink does
not
> > the
> > > > equivalent of "IsDetachedBuffer".
> > > 
> > > Well, there is WTF::ArrayBuffer::isNeutered, which is another name for the
> > same
> > > thing, I believe.
> > 
> > Is it?  The code paths for isShared and isNeutered seem to end up in
different
> > places.  It looks like isShared eventually gets to m_isShared == Shared and
> > isNeutered is m_isNeutered.
> > 
> > (I didn't investigate how m_isNeutered is set vs hos m_isShared is set.  It
> > seems bad if we have two names for the same thing.)
> 
> Sorry, I think I made things less clear. :-)
> 
> isShared is true if the ArrayBuffer is actually a SharedArrayBuffer (see
> https://tc39.github.io/ecma262/#sec-sharedarraybuffer-objects)
> 
> isNeutered is true when the ArrayBuffer is neutered (which is called detached
in
> the es spec -- my understanding is that "neutered" is the old terminology back
> before ArrayBuffers were standardized in ecmascript).

Ah, ok.

So isNeutered is what we want.  But what about the DCHECK(!isShared()) in
transfer()?

Should the test be isNeutered() || isShared() cause us to throw an error?  If
this fails, we can safely transfer the buffer for decoding.

[Raymond Toy, 2017-03-14 19:32:59]

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

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:291:
DOMException::create(V8TypeError, "Cannot decode detached buffer");
On 2017/03/14 03:17:49, haraken wrote:
> 
> Nit: resolver->reject(V8ThrowException::createTypeError(...))

I still need an DOMException object for the errorCallback.  Do you want me to
create that separately before line 294 below?

[haraken, 2017-03-14 19:57:51]

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

https://codereview.chromium.org/2746913003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:291:
DOMException::create(V8TypeError, "Cannot decode detached buffer");
On 2017/03/14 19:32:59, Raymond Toy wrote:
> On 2017/03/14 03:17:49, haraken wrote:
> > 
> > Nit: resolver->reject(V8ThrowException::createTypeError(...))
> 
> I still need an DOMException object for the errorCallback.  Do you want me to
> create that separately before line 294 below?

Ah, then DOMException is fine :)

[binji, 2017-03-14 20:08:05]

On 2017/03/14 19:11:03, Raymond Toy wrote:
> On 2017/03/14 19:02:15, binji wrote:
> > On 2017/03/14 18:52:45, Raymond Toy wrote:
> > > On 2017/03/14 18:28:11, binji wrote:
> > > > > > My question is about the matching between what the spec is saying
and
> > what
> > > > > we're
> > > > > > doing here.
> > > > > > 
> > > > > > The spec is saying that we should throw a type error if 
> > IsDetachedBuffer
> > > > > > returns true. However, we're checking isShared() here. Is it
correct?
> > > > > 
> > > > > Yes, Ray and I discussed that issue offline. It seems like Blink does
> not
> > > the
> > > > > equivalent of "IsDetachedBuffer".
> > > > 
> > > > Well, there is WTF::ArrayBuffer::isNeutered, which is another name for
the
> > > same
> > > > thing, I believe.
> > > 
> > > Is it?  The code paths for isShared and isNeutered seem to end up in
> different
> > > places.  It looks like isShared eventually gets to m_isShared == Shared
and
> > > isNeutered is m_isNeutered.
> > > 
> > > (I didn't investigate how m_isNeutered is set vs hos m_isShared is set. 
It
> > > seems bad if we have two names for the same thing.)
> > 
> > Sorry, I think I made things less clear. :-)
> > 
> > isShared is true if the ArrayBuffer is actually a SharedArrayBuffer (see
> > https://tc39.github.io/ecma262/#sec-sharedarraybuffer-objects)
> > 
> > isNeutered is true when the ArrayBuffer is neutered (which is called
detached
> in
> > the es spec -- my understanding is that "neutered" is the old terminology
back
> > before ArrayBuffers were standardized in ecmascript).
> 
> Ah, ok.
> 
> So isNeutered is what we want.  But what about the DCHECK(!isShared()) in
> transfer()?
> 
> Should the test be isNeutered() || isShared() cause us to throw an error?  If
> this fails, we can safely transfer the buffer for decoding.

I think that makes sense, yeah.

[Raymond Toy, 2017-03-14 23:05:20]

Description was changed from

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with a TypeError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696176
TEST=
==========

to

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with a TypeError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696179
TEST=
==========

[Raymond Toy, 2017-03-15 15:03:50]

https://codereview.chromium.org/2746913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2746913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:291:
V8TypeError, "Cannot decode detached or shared ArrayBuffer");
Can't actually create an exception of type V8TypeError. (It's causing a crash in
create.)

Maybe DataCloneError would be more appropriate?

[Raymond Toy, 2017-03-17 17:54:37]

Description was changed from

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with a TypeError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696179
TEST=
==========

to

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with a TypeError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

[Raymond Toy, 2017-03-17 17:55:50]

binji:  How do I make an ArrayBuffer that is shared? I'd like to add a test for
that.

[binji, 2017-03-17 18:02:03]

On 2017/03/17 17:55:50, Raymond Toy wrote:
> binji:  How do I make an ArrayBuffer that is shared? I'd like to add a test
for
> that.

You can create one in JavaScript like this:

var sab = new SharedArrayBuffer(16);
var ta = new Uint32Array(sab);

But it's currently behind a flag: chrome://flags/#shared-array-buffer. You can
use the "sharedarraybuffer" virtual testsuite to enable it.

In Blink you can create one with the createShared API:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...

[Raymond Toy, 2017-03-17 18:07:21]

On 2017/03/17 18:02:03, binji wrote:
> On 2017/03/17 17:55:50, Raymond Toy wrote:
> > binji:  How do I make an ArrayBuffer that is shared? I'd like to add a test
> for
> > that.
> 
> You can create one in JavaScript like this:
> 
> var sab = new SharedArrayBuffer(16);
> var ta = new Uint32Array(sab);
> 
> But it's currently behind a flag: chrome://flags/#shared-array-buffer. You can
> use the "sharedarraybuffer" virtual testsuite to enable it.
> 
> In Blink you can create one with the createShared API:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...

Thanks!  I'll update the tests in decode-audio-data-neuter.html with this.

[Raymond Toy, 2017-03-17 18:27:55]

On 2017/03/17 18:07:21, Raymond Toy wrote:
> On 2017/03/17 18:02:03, binji wrote:
> > On 2017/03/17 17:55:50, Raymond Toy wrote:
> > > binji:  How do I make an ArrayBuffer that is shared? I'd like to add a
test
> > for
> > > that.
> > 
> > You can create one in JavaScript like this:
> > 
> > var sab = new SharedArrayBuffer(16);
> > var ta = new Uint32Array(sab);
> > 
> > But it's currently behind a flag: chrome://flags/#shared-array-buffer. You
can
> > use the "sharedarraybuffer" virtual testsuite to enable it.
> > 
> > In Blink you can create one with the createShared API:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...
> 
> Thanks!  I'll update the tests in decode-audio-data-neuter.html with this.

Hmm. I tried something like this (after enabling the shared array feature via
command-line args):

c = new AudioContext()
s = new SharedArrayBuffer(16);
b = new ArrayBuffer(s)
c.decodeAudioData(b)

Should this signal an error that b is shared? (Because I was expecting
isShared() to return true).

[binji, 2017-03-17 18:39:34]

On 2017/03/17 18:27:55, Raymond Toy wrote:
> On 2017/03/17 18:07:21, Raymond Toy wrote:
> > On 2017/03/17 18:02:03, binji wrote:
> > > On 2017/03/17 17:55:50, Raymond Toy wrote:
> > > > binji:  How do I make an ArrayBuffer that is shared? I'd like to add a
> test
> > > for
> > > > that.
> > > 
> > > You can create one in JavaScript like this:
> > > 
> > > var sab = new SharedArrayBuffer(16);
> > > var ta = new Uint32Array(sab);
> > > 
> > > But it's currently behind a flag: chrome://flags/#shared-array-buffer. You
> can
> > > use the "sharedarraybuffer" virtual testsuite to enable it.
> > > 
> > > In Blink you can create one with the createShared API:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...
> > 
> > Thanks!  I'll update the tests in decode-audio-data-neuter.html with this.
> 
> Hmm. I tried something like this (after enabling the shared array feature via
> command-line args):
> 
> c = new AudioContext()
> s = new SharedArrayBuffer(16);
> b = new ArrayBuffer(s)
> c.decodeAudioData(b)
> 
> Should this signal an error that b is shared? (Because I was expecting
> isShared() to return true).

No, because you can't construct an ArrayBuffer from a SharedArrayBuffer. You
could try passing s directly to decodeAudioData.

[binji, 2017-03-17 18:40:54]

On 2017/03/17 18:39:34, binji wrote:
> On 2017/03/17 18:27:55, Raymond Toy wrote:
> > On 2017/03/17 18:07:21, Raymond Toy wrote:
> > > On 2017/03/17 18:02:03, binji wrote:
> > > > On 2017/03/17 17:55:50, Raymond Toy wrote:
> > > > > binji:  How do I make an ArrayBuffer that is shared? I'd like to add a
> > test
> > > > for
> > > > > that.
> > > > 
> > > > You can create one in JavaScript like this:
> > > > 
> > > > var sab = new SharedArrayBuffer(16);
> > > > var ta = new Uint32Array(sab);
> > > > 
> > > > But it's currently behind a flag: chrome://flags/#shared-array-buffer.
You
> > can
> > > > use the "sharedarraybuffer" virtual testsuite to enable it.
> > > > 
> > > > In Blink you can create one with the createShared API:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...
> > > 
> > > Thanks!  I'll update the tests in decode-audio-data-neuter.html with this.
> > 
> > Hmm. I tried something like this (after enabling the shared array feature
via
> > command-line args):
> > 
> > c = new AudioContext()
> > s = new SharedArrayBuffer(16);
> > b = new ArrayBuffer(s)
> > c.decodeAudioData(b)
> > 
> > Should this signal an error that b is shared? (Because I was expecting
> > isShared() to return true).
> 
> No, because you can't construct an ArrayBuffer from a SharedArrayBuffer. You
> could try passing s directly to decodeAudioData.

I should add that in Blink we use wtf::ArrayBuffer for both DOMArrayBuffer and
DOMSharedArrayBuffer, but those types are otherwise unrelated.

[Raymond Toy, 2017-03-17 21:39:29]

On 2017/03/17 18:40:54, binji wrote:
> On 2017/03/17 18:39:34, binji wrote:
> > On 2017/03/17 18:27:55, Raymond Toy wrote:
> > > On 2017/03/17 18:07:21, Raymond Toy wrote:
> > > > On 2017/03/17 18:02:03, binji wrote:
> > > > > On 2017/03/17 17:55:50, Raymond Toy wrote:
> > > > > > binji:  How do I make an ArrayBuffer that is shared? I'd like to add
a
> > > test
> > > > > for
> > > > > > that.
> > > > > 
> > > > > You can create one in JavaScript like this:
> > > > > 
> > > > > var sab = new SharedArrayBuffer(16);
> > > > > var ta = new Uint32Array(sab);
> > > > > 
> > > > > But it's currently behind a flag: chrome://flags/#shared-array-buffer.
> You
> > > can
> > > > > use the "sharedarraybuffer" virtual testsuite to enable it.
> > > > > 
> > > > > In Blink you can create one with the createShared API:
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...
> > > > 
> > > > Thanks!  I'll update the tests in decode-audio-data-neuter.html with
this.
> > > 
> > > Hmm. I tried something like this (after enabling the shared array feature
> via
> > > command-line args):
> > > 
> > > c = new AudioContext()
> > > s = new SharedArrayBuffer(16);
> > > b = new ArrayBuffer(s)
> > > c.decodeAudioData(b)
> > > 
> > > Should this signal an error that b is shared? (Because I was expecting
> > > isShared() to return true).
> > 
> > No, because you can't construct an ArrayBuffer from a SharedArrayBuffer. You
> > could try passing s directly to decodeAudioData.
> 
> I should add that in Blink we use wtf::ArrayBuffer for both DOMArrayBuffer and
> DOMSharedArrayBuffer, but those types are otherwise unrelated.

We're not using wtf::ArrayBuffer.  The parameter is a DOMArrayBuffer.  Is it
possible to create a DOMArrayBuffer such that isShared() would return true?  I'd
prefer to be able to do that in Javascript.  This API is only used from JS.

[binji, 2017-03-17 21:44:10]

On 2017/03/17 21:39:29, Raymond Toy wrote:
> On 2017/03/17 18:40:54, binji wrote:
> > On 2017/03/17 18:39:34, binji wrote:
> > > On 2017/03/17 18:27:55, Raymond Toy wrote:
> > > > On 2017/03/17 18:07:21, Raymond Toy wrote:
> > > > > On 2017/03/17 18:02:03, binji wrote:
> > > > > > On 2017/03/17 17:55:50, Raymond Toy wrote:
> > > > > > > binji:  How do I make an ArrayBuffer that is shared? I'd like to
add
> a
> > > > test
> > > > > > for
> > > > > > > that.
> > > > > > 
> > > > > > You can create one in JavaScript like this:
> > > > > > 
> > > > > > var sab = new SharedArrayBuffer(16);
> > > > > > var ta = new Uint32Array(sab);
> > > > > > 
> > > > > > But it's currently behind a flag:
chrome://flags/#shared-array-buffer.
> > You
> > > > can
> > > > > > use the "sharedarraybuffer" virtual testsuite to enable it.
> > > > > > 
> > > > > > In Blink you can create one with the createShared API:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...
> > > > > 
> > > > > Thanks!  I'll update the tests in decode-audio-data-neuter.html with
> this.
> > > > 
> > > > Hmm. I tried something like this (after enabling the shared array
feature
> > via
> > > > command-line args):
> > > > 
> > > > c = new AudioContext()
> > > > s = new SharedArrayBuffer(16);
> > > > b = new ArrayBuffer(s)
> > > > c.decodeAudioData(b)
> > > > 
> > > > Should this signal an error that b is shared? (Because I was expecting
> > > > isShared() to return true).
> > > 
> > > No, because you can't construct an ArrayBuffer from a SharedArrayBuffer.
You
> > > could try passing s directly to decodeAudioData.
> > 
> > I should add that in Blink we use wtf::ArrayBuffer for both DOMArrayBuffer
and
> > DOMSharedArrayBuffer, but those types are otherwise unrelated.
> 
> We're not using wtf::ArrayBuffer.  The parameter is a DOMArrayBuffer.  Is it
> possible to create a DOMArrayBuffer such that isShared() would return true? 
I'd
> prefer to be able to do that in Javascript.  This API is only used from JS.

No, it should not be possible to have a DOMArrayBuffer where isShared() is true.

[Raymond Toy, 2017-03-17 21:51:28]

On 2017/03/17 21:44:10, binji wrote:
> On 2017/03/17 21:39:29, Raymond Toy wrote:
> > On 2017/03/17 18:40:54, binji wrote:
> > > On 2017/03/17 18:39:34, binji wrote:
> > > > On 2017/03/17 18:27:55, Raymond Toy wrote:
> > > > > On 2017/03/17 18:07:21, Raymond Toy wrote:
> > > > > > On 2017/03/17 18:02:03, binji wrote:
> > > > > > > On 2017/03/17 17:55:50, Raymond Toy wrote:
> > > > > > > > binji:  How do I make an ArrayBuffer that is shared? I'd like to
> add
> > a
> > > > > test
> > > > > > > for
> > > > > > > > that.
> > > > > > > 
> > > > > > > You can create one in JavaScript like this:
> > > > > > > 
> > > > > > > var sab = new SharedArrayBuffer(16);
> > > > > > > var ta = new Uint32Array(sab);
> > > > > > > 
> > > > > > > But it's currently behind a flag:
> chrome://flags/#shared-array-buffer.
> > > You
> > > > > can
> > > > > > > use the "sharedarraybuffer" virtual testsuite to enable it.
> > > > > > > 
> > > > > > > In Blink you can create one with the createShared API:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/typed_arra...
> > > > > > 
> > > > > > Thanks!  I'll update the tests in decode-audio-data-neuter.html with
> > this.
> > > > > 
> > > > > Hmm. I tried something like this (after enabling the shared array
> feature
> > > via
> > > > > command-line args):
> > > > > 
> > > > > c = new AudioContext()
> > > > > s = new SharedArrayBuffer(16);
> > > > > b = new ArrayBuffer(s)
> > > > > c.decodeAudioData(b)
> > > > > 
> > > > > Should this signal an error that b is shared? (Because I was expecting
> > > > > isShared() to return true).
> > > > 
> > > > No, because you can't construct an ArrayBuffer from a SharedArrayBuffer.
> You
> > > > could try passing s directly to decodeAudioData.
> > > 
> > > I should add that in Blink we use wtf::ArrayBuffer for both DOMArrayBuffer
> and
> > > DOMSharedArrayBuffer, but those types are otherwise unrelated.
> > 
> > We're not using wtf::ArrayBuffer.  The parameter is a DOMArrayBuffer.  Is it
> > possible to create a DOMArrayBuffer such that isShared() would return true? 
> I'd
> > prefer to be able to do that in Javascript.  This API is only used from JS.
> 
> No, it should not be possible to have a DOMArrayBuffer where isShared() is
true.

Thanks.  Then I can remove the isShared() test from the code.

[Raymond Toy, 2017-03-20 15:46:37]

Description was changed from

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with a TypeError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

to

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

[Raymond Toy, 2017-03-20 18:00:01]

PTAL; I think I've addressed everything.

[Raymond Toy, 2017-03-23 16:34:49]

Ping

[hongchan, 2017-03-23 16:40:26]

lgtm

https://codereview.chromium.org/2746913003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2746913003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:289: // promise
with error..
super nit: two periods?

https://codereview.chromium.org/2746913003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:299:
WTF::ArrayBufferContents buf;
s/buf/bufferContent/

[Raymond Toy, 2017-03-23 16:50:34]

Before landing, I'm going to add a chrome feature to let people know that the
buffer is detached and not usable.  Paul Adenot (Mozilla) says that they had a
few reports about this when first implemented in Firefox, but they don't see any
more reports about this).

I'll update the description too.

[Raymond Toy, 2017-03-23 16:55:26]

Description was changed from

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

BUG=696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

to

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

Chrome Feature: https://www.chromestatus.com/feature/5539919174828032

BUG=696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

[Raymond Toy, 2017-03-23 17:49:35]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2017-03-23 17:49:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2746913003/#ps140001
(title: "Address review comments")

[commit-bot: I haz the power, 2017-03-23 17:50:05]

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-03-23 19:43:34]

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

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

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_...)

[Raymond Toy, 2017-03-23 20:45:17]

Description was changed from

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

Chrome Feature: https://www.chromestatus.com/feature/5539919174828032

BUG=696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

to

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

Chrome Feature: https://www.chromestatus.com/feature/5539919174828032

BUG=704679, 696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

[Raymond Toy, 2017-03-23 20:48:42]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:49:29]

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-03-23 23:02:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490302122580900,
"parent_rev": "e5d18d9dbefde2956d137ef766faec573724ca46", "commit_rev":
"3010bfc167e7af9d4ecd48a54189c721c2b93dbf"}

[commit-bot: I haz the power, 2017-03-23 23:04:17]

Description was changed from

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

Chrome Feature: https://www.chromestatus.com/feature/5539919174828032

BUG=704679, 696179
TEST=decodeAudioData/decode-audio-data-neuter.html
==========

to

==========
Detach buffer in decodeAudioData

Handle audio buffer according to WebAudio spec.  If the audio buffer
is detached, reject the promise with DataCloneError.  Otherwise, detach
the buffer and decode the buffer.

Chrome Feature: https://www.chromestatus.com/feature/5539919174828032

BUG=704679, 696179
TEST=decodeAudioData/decode-audio-data-neuter.html

Review-Url: https://codereview.chromium.org/2746913003
Cr-Commit-Position: refs/heads/master@{#459266}
Committed:
https://chromium.googlesource.com/chromium/src/+/3010bfc167e7af9d4ecd48a54189...
==========

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

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3010bfc167e7af9d4ecd48a54189...

[haraken, 2017-03-23 23:30:12]

LGTM