1317382 - DataView constructor uses ToIndex in ES2017

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

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

[Jeff Walden [:Waldo]]

I seem to have incidentally fixed this in other work, in my tree, so I'll take this.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: Use ToIndex in DataView constructor

Comment 3

[Jeff Walden [:Waldo]]

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");

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a394ec7d209
DataView constructor uses ToIndex in ES2017. r=Waldo

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a394ec7d209

Comment 6

[Julien Cristau [:jcristau]]

Is this a potential candidate for 53, or should it ride the trains?

Comment 7

[Jeff Walden [:Waldo]]

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.

Comment 8

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/54#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView#Exceptions