Closed
Bug 1317382
Opened 8 years ago
Closed 7 years ago
DataView constructor uses ToIndex in ES2017
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: anba, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
5.79 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
For example `new DataView(new ArrayBuffer(10), -Infinity)` is now expected to throw a RangeError. ES2017 spec: https://tc39.github.io/ecma262/#sec-dataview-buffer-byteoffset-bytelength https://tc39.github.io/ecma262/#sec-toindex The following test262 tests expect the new ES2017 semantics: built-ins/DataView/byteoffset-is-negative-throws.js built-ins/DataView/excessive-byteoffset-throws.js built-ins/DataView/excessive-bytelength-throws.js built-ins/DataView/negative-byteoffset-throws.js built-ins/DataView/negative-bytelength-throws.js
Comment 1•7 years ago
|
||
I seem to have incidentally fixed this in other work, in my tree, so I'll take this.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8836469 -
Flags: review?(jwalden+bmo)
Comment 3•7 years ago
|
||
Comment on attachment 8836469 [details] [diff] [review] Use ToIndex in DataView constructor Review of attachment 8836469 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/DataViewObject.cpp @@ +173,5 @@ > + return false; > + } > + > + MOZ_ASSERT(offset <= INT32_MAX); > + *byteOffsetPtr = uint32_t(offset); Please put the |*byteOffsetPtr| and |*byteLengthPtr| assignments at the end of the function, and assign into locals in earlier code. Makes clearer that we've verified *both* are consistent at one instant, IMO, than assigning earlier. @@ +178,5 @@ > + > + // Step 8. > + if (!args.hasDefined(2)) { > + // Step 8.a. > + *byteLengthPtr = bufferByteLength - uint32_t(offset); Put a |uint64_t viewByteLength;| above the if-else and set it here. @@ +186,5 @@ > + if (!ToIndex(cx, args.get(2), &viewByteLength)) > + return false; > + > + // Step 9.b. > + if (offset + viewByteLength > bufferByteLength) { Before this, add MOZ_ASSERT(offset + viewByteLength >= offset, "can't overflow: both numbers are less than DOUBLE_INTEGRAL_PRECISION_LIMIT");
Attachment #8836469 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Assignee: jwalden+bmo → evilpies
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a394ec7d209 DataView constructor uses ToIndex in ES2017. r=Waldo
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a394ec7d209
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 6•7 years ago
|
||
Is this a potential candidate for 53, or should it ride the trains?
Comment 7•7 years ago
|
||
Changes to web-visible semantics that aren't necessitated by a security fix usually aren't backported, so I'm not sure why we'd consider this.
Flags: needinfo?(evilpies)
Updated•7 years ago
|
Comment 8•7 years ago
|
||
https://developer.mozilla.org/en-US/Firefox/Releases/54#JavaScript https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView#Exceptions
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•