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

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8834372 [details] [diff] [review]
bug1317384.patch

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)
(Assignee)

Comment 2

2 years ago
I forgot to mention that the patch applies on top of bug 1225031.

Comment 3

2 years ago
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+
(Assignee)

Comment 5

2 years ago
(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.

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a930acd18f8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1341692
status-firefox52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.