Closed Bug 1342663 Opened 7 years ago Closed 7 years ago

Check for detached array buffers in TypedArray.prototype.slice

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Add an explicit check for detached array buffers because we don't throw for detached array buffers in typed array's [[Get]] method.
Attached patch bug1342663.patch (obsolete) — Splinter Review
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.
Attachment #8841191 - Flags: review?(arai.unmht)
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);
Attachment #8841191 - Flags: review?(arai.unmht) → review+
(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
(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 :)
Attached patch bug1342663.patchSplinter Review
Updated per review comments, carrying r+ from arai.
Attachment #8841191 - Attachment is obsolete: true
Attachment #8841697 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2358cbc6056
Check for detached source buffer in TypedArray.prototype.slice. r=arai
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2358cbc6056
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.