Bug 164450 – test262: DataView / TypedArray methods should throw RangeErrors for negative numbers (ToIndex)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

Summary:
DataView / TypedArray methods should throw RangeErrors for negative numbers (ToIndex)

Steps to Reproduce:
1. jsc> new ArrayBuffer(-Infinity)
2. jsc> new DataView(new ArrayBuffer(128), -Infinity)
...

The spec defines a ToIndex function that can throw a RangeError for negative numbers (after ToInteger has already truncated -0.999 to -0).

> 7.1.17 ToIndex ( value )
> https://tc39.github.io/ecma262/#sec-toindex
> 
> The abstract operation ToIndex returns value argument converted to a numeric value
> if it is a valid integer index value. This abstract operation functions as follows:
> 
>     1. If value is undefined, then
>         Let index be 0.
>     2. Else,
>         Let integerIndex be ? ToInteger(value).
>         If integerIndex < 0, throw a RangeError exception.
>         Let index be ! ToLength(integerIndex).
>         If SameValueZero(integerIndex, index) is false, throw a RangeError exception.
>     3. Return index.

ToIndex is only used in DataView accessors and DataView / TypedArray construction.

[Joseph Pecoraro]

Created attachment 293995 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 293995 [details]
[PATCH] Proposed Fix

Attachment 293995 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2466668

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/array-unit-tests.html
webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
fast/canvas/webgl/webgl-array-invalid-ranges.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 294002 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 293995 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=293995&action=review

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:72
> +    double d = toNumber(exec);
> +
> +    if (d <= -1) {
> +        throwException(exec, scope, createRangeError(exec, makeString(errorName, " cannot be negative")));
> +        return 0;
> +    }
> +    if (d > std::numeric_limits<unsigned>::max()) {
> +        throwException(exec, scope, createRangeError(exec, makeString(errorName, " too large")));
> +        return 0;
> +    }
> +
> +    if (isInt32())
> +        return asInt32();

Since we are going to do the isInt32 check earlier, we should do that first; have a fast path for isInt32 before the path for other numbers. That does mean we will need an extra "cannot be negative" case, but the extra speed is possibly worth it. Might need a helper function to avoid repeating the throwException code, or not.

[Build Bot]

Comment on attachment 293995 [details]
[PATCH] Proposed Fix

Attachment 293995 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2468903

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/array-unit-tests.html
webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
fast/canvas/webgl/webgl-array-invalid-ranges.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 294078 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293995 [details]
[PATCH] Proposed Fix

Attachment 293995 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2468929

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/array-unit-tests.html
webgl/1.0.2/conformance/typedarrays/array-unit-tests.html
fast/canvas/webgl/webgl-array-invalid-ranges.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 294079 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Created attachment 294399 [details]
[PATCH] For Landing

[Joseph Pecoraro]

Comment on attachment 293995 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=293995&action=review

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:72
>> +        return asInt32();
> 
> Since we are going to do the isInt32 check earlier, we should do that first; have a fast path for isInt32 before the path for other numbers. That does mean we will need an extra "cannot be negative" case, but the extra speed is possibly worth it. Might need a helper function to avoid repeating the throwException code, or not.

I actually expect to make toIndex return a double eventually and move the >unsigned::max() check out to callers in order to satisfy other test262 tests that expect too large values to be handled later (at allocation of array time, instead of number conversion time). So leaving this as is for now.

[WebKit Commit Bot]

Comment on attachment 294399 [details]
[PATCH] For Landing

Clearing flags on attachment: 294399

Committed r208564: <http://trac.webkit.org/changeset/208564>