Closed Bug 1317384 Opened 7 years ago Closed 7 years ago

%TypedArray%.prototype.set should use ToInteger instead of ToInt32

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

For example `new Int8Array(10).set([], -Infinity)` should throw a RangeError.


ES2017 spec:
https://tc39.github.io/ecma262/#sec-%typedarray%.prototype.set-overloaded-offset


The following test262 tests are currently failing because of this issue:
built-ins/TypedArray/prototype/set/array-arg-negative-integer-offset-throws.js
built-ins/TypedArray/prototype/set/typedarray-arg-negative-integer-offset-throws.js
built-ins/TypedArray/prototype/set/typedarray-arg-src-range-greather-than-target-throws-rangeerror.js
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attached patch bug1317384.patchSplinter Review
This updates %TypedArray%.prototype.set() to be compliant with the latest ECMAScript draft (with the exception of bug 1314148). There are three notable changes:
1) The offset parameter for %TypedArray%.prototype.set() is now converted with ToInteger instead of ToInt32.
2) For the non-TypedArray case (ES2017, 22.2.3.23.1), the argument gets converted to an object with ToObject. That means |new Int32Array(0).set("")| is now allowed.
3) Additional checks for detached ArrayBuffers are now present. This change shouldn't lead to web-compatibility problems, because JavaScriptCore and Chakra also already check for detached buffers.

I've added three new test files to cover each change. 

And I've also added new assertions to the various ElementSpecific methods to ensure they're never called with detached ArrayBuffers, so it's easier to reason about their behaviour. (And one comment was declassified, because the sec-issue was fixed three years ago, so we longer need to care about not 0-daying ourselves.)
Attachment #8834372 - Flags: review?(lhansen)
I forgot to mention that the patch applies on top of bug 1225031.
Comment on attachment 8834372 [details] [diff] [review]
bug1317384.patch

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

I bow down in awe for some of those tests.
Attachment #8834372 - Flags: review?(lhansen) → review+
(In reply to Lars T Hansen [:lth] from comment #3)
> I bow down in awe for some of those tests.

It may make sense to contribute them to test262, but I'd have to check the existing coverage for %TypedArray%.prototype.set() first.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a930acd18f8
Update TypedArray.prototype.set to be compliant with latest ECMA2017. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a930acd18f8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1341692
You need to log in before you can comment on or make changes to this bug.