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)
Core
JavaScript: Standard Library
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)
7.85 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Add an explicit check for detached array buffers because we don't throw for detached array buffers in typed array's [[Get]] method.
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
(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•7 years ago
|
||
(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 :)
Assignee | ||
Comment 5•7 years ago
|
||
Updated per review comments, carrying r+ from arai.
Attachment #8841191 -
Attachment is obsolete: true
Attachment #8841697 -
Flags: review+
Assignee | ||
Comment 6•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d15077517d955321c70654b36ac1679bad990012
Keywords: checkin-needed
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
Comment 8•7 years ago
|
||
bugherder |
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.
Description
•