1342663 - Check for detached array buffers in TypedArray.prototype.slice

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Add an explicit check for detached array buffers because we don't throw for detached array buffers in typed array's [[Get]] method.

Comment 1

[André Bargull [:anba]]

Attachment: bug1342663.patch

This adds an explicit detached array buffer check for step 14.b.ii, in the spec this check is implicitly performed through the detached check in typed array's [[Get]] method.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8841191 [details] [diff] [review]
bug1342663.patch

Review of attachment 8841191 [details] [diff] [review]:
-----------------------------------------------------------------

Great testcase :)

::: js/src/builtin/TypedArray.js
@@ +1022,5 @@
> +        // Steps 14.b.ii, 15.b.
> +        if (buffer === null)
> +            buffer = GetAttachedArrayBuffer(O);
> +
> +        if (IsDetachedBuffer(buffer))

GetAttachedArrayBuffer checks IsDetachedBuffer internally, so it's redundant to check it here for `buffer === null` case above.

how about this?

  if (buffer === null)
    GetAttachedArrayBuffer(O);
  else if (IsDetachedBuffer(buffer))
    ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);

Comment 3

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> ::: js/src/builtin/TypedArray.js
> @@ +1022,5 @@
> > +        // Steps 14.b.ii, 15.b.
> > +        if (buffer === null)
> > +            buffer = GetAttachedArrayBuffer(O);
> > +
> > +        if (IsDetachedBuffer(buffer))
> 
> GetAttachedArrayBuffer checks IsDetachedBuffer internally, so it's redundant
> to check it here for `buffer === null` case above.
> 
> how about this?
> 
>   if (buffer === null)
>     GetAttachedArrayBuffer(O);
>   else if (IsDetachedBuffer(buffer))
>     ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);

Oops, you're totally right. Thanks for spotting that! I had planned to copy the pattern from [1], so the second call to GetAttachedArrayBuffer should instead be a call to ViewedArrayBufferIfReified. Are you okay with that alternative? (I should probably also copy the comments from [1] to slice(), so it's clearer why it's necessary to recheck the buffer slot.)

[1] http://searchfox.org/mozilla-central/rev/93f1641e394cfbdfbeed44e81f7dab0f2aff7b6f/js/src/builtin/TypedArray.js#892-898

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to André Bargull from comment #3)
> (In reply to Tooru Fujisawa [:arai] from comment #2)
> > ::: js/src/builtin/TypedArray.js
> > @@ +1022,5 @@
> > > +        // Steps 14.b.ii, 15.b.
> > > +        if (buffer === null)
> > > +            buffer = GetAttachedArrayBuffer(O);
> > > +
> > > +        if (IsDetachedBuffer(buffer))
> > 
> > GetAttachedArrayBuffer checks IsDetachedBuffer internally, so it's redundant
> > to check it here for `buffer === null` case above.
> > 
> > how about this?
> > 
> >   if (buffer === null)
> >     GetAttachedArrayBuffer(O);
> >   else if (IsDetachedBuffer(buffer))
> >     ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);
> 
> Oops, you're totally right. Thanks for spotting that! I had planned to copy
> the pattern from [1], so the second call to GetAttachedArrayBuffer should
> instead be a call to ViewedArrayBufferIfReified. Are you okay with that
> alternative?

Yes :)

Comment 5

[André Bargull [:anba]]

Attachment: bug1342663.patch

Updated per review comments, carrying r+ from arai.

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d15077517d955321c70654b36ac1679bad990012

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2358cbc6056
Check for detached source buffer in TypedArray.prototype.slice. r=arai

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2358cbc6056