Bug 164456 – test262: Array.prototype methods on non-Array objects do not properly run ToLength on length property

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Joseph Pecoraro]

Summary:
Array.prototype methods on non-Array objects do not properly run ToLength on length property

In tests like:
test262/test/built-ins/Array/prototype/unshift/S15.4.4.13_A3_T2.js

> var obj = {};
> obj.unshift = Array.prototype.unshift;
> obj[0] = "";
> obj.length = -4294967295;
> 
> var unshift = obj.unshift("x", "y", "z");
> assert(unshift === 3) // was 4
> assert(obj.length === 3) // was 4

Spec:

> Array.prototype.unshift
> https://tc39.github.io/ecma262/#sec-array.prototype.unshift
> 
> 1. Let O be ? ToObject(this value).
> 2. Let len be ? ToLength(? Get(O, "length")).
> ...

> ToLength
> https://tc39.github.io/ecma262/#sec-tolength
> 
> 1. Let len be ? ToInteger(argument).
> 2. If len ≤ +0, return +0.
> 3. If len is +∞, return 2^53-1.
> 4. Return min(len, 2^53-1).

In cases like these we are converting the "length" property directly to unsigned without running the logic of ToLength which could convert some negative values (like the one in the test above) to 0. Instead we were getting a value of 1.

[Joseph Pecoraro]

Created attachment 294003 [details]
[PATCH] Proposed Fix

[Build Bot]

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

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

New failing tests:
ietestcenter/Javascript/15.4.4.15-3-8.html
ietestcenter/Javascript/15.4.4.15-3-12.html
ietestcenter/Javascript/15.4.4.14-3-7.html
ietestcenter/Javascript/15.4.4.14-3-12.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.8_Array_prototype_reverse/S15.4.4.8_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.12_Array_prototype_splice/S15.4.4.12_A3_T3.html
ietestcenter/Javascript/15.4.4.14-3-14.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A2_T2.html
ietestcenter/Javascript/15.4.4.14-3-25.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A3_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A2_T2.html
ietestcenter/Javascript/15.4.4.14-3-8.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A4_T3.html
ietestcenter/Javascript/15.4.4.15-3-14.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A3_T3.html
ietestcenter/Javascript/15.4.4.15-3-7.html
ietestcenter/Javascript/15.4.4.15-3-25.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A2_T2.html

[Build Bot]

Created attachment 294006 [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 294003 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/runtime/JSArrayInlines.h:80
> -    return lengthValue.toUInt32(exec);
> +    return JSC::toInt32(lengthValue.toLength(exec));

It makes me sad to lose the inlined fast path for values stored as integers. If it was valuable before, I see no reason why it would not still be valuable even if we have to add one more branch to check for negative numbers. So perhaps we should have a version of this that still contains an inline special case for isInt32().

As for the non-Int32 case, it is not obvious to me that it is good to have toInt32 be inlined; perhaps we should add a toLengthInt32 function rather than including MathCommon.h in the inlines header.

Side note: When studying if this change was correct, I noticed that the std::isinf if statement can be deleted from the JSValue::toLength function; the code already handles infinity correctly in the call to std::min on the next line. The code is just a too-literal copy of what the specification says; the explicit check for infinity is not needed. I also noticed that function writes out minSafeInteger() rather than using the constexpr function, and I think using the function would be better.

Looks like all the old “sputnik” tests we got from the V8 team expect different behavior, so we have to update the test results to expect failure or possibly even change the tests before landing this.

I’m going to say review+, but maybe this inlining stuff above needs some thought.

[Build Bot]

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

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

New failing tests:
ietestcenter/Javascript/15.4.4.15-3-8.html
ietestcenter/Javascript/15.4.4.15-3-12.html
ietestcenter/Javascript/15.4.4.14-3-7.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.8_Array_prototype_reverse/S15.4.4.8_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.12_Array_prototype_splice/S15.4.4.12_A3_T3.html
ietestcenter/Javascript/15.4.4.14-3-14.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A3_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A2_T2.html
ietestcenter/Javascript/15.4.4.14-3-8.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A4_T3.html
ietestcenter/Javascript/15.4.4.15-3-14.html
ietestcenter/Javascript/15.4.4.14-3-12.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A3_T3.html
ietestcenter/Javascript/15.4.4.14-3-25.html
ietestcenter/Javascript/15.4.4.15-3-7.html
ietestcenter/Javascript/15.4.4.15-3-25.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A2_T2.html

[Build Bot]

Created attachment 294075 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

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

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

New failing tests:
ietestcenter/Javascript/15.4.4.15-3-8.html
ietestcenter/Javascript/15.4.4.15-3-12.html
ietestcenter/Javascript/15.4.4.14-3-7.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.8_Array_prototype_reverse/S15.4.4.8_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.12_Array_prototype_splice/S15.4.4.12_A3_T3.html
ietestcenter/Javascript/15.4.4.14-3-14.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A2_T2.html
ietestcenter/Javascript/15.4.4.14-3-25.html
ietestcenter/Javascript/15.4.4.15-3-7.html
ietestcenter/Javascript/15.4.4.14-3-12.html
ietestcenter/Javascript/15.4.4.15-3-14.html
ietestcenter/Javascript/15.4.4.14-3-8.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A4_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A3_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A2_T2.html
ietestcenter/Javascript/15.4.4.15-3-25.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A2_T2.html

[Build Bot]

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

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

[Build Bot]

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

Attachment 294003 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2468953

New failing tests:
ietestcenter/Javascript/15.4.4.15-3-8.html
ietestcenter/Javascript/15.4.4.15-3-12.html
ietestcenter/Javascript/15.4.4.14-3-7.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.8_Array_prototype_reverse/S15.4.4.8_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.12_Array_prototype_splice/S15.4.4.12_A3_T3.html
ietestcenter/Javascript/15.4.4.14-3-14.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.9_Array_prototype_shift/S15.4.4.9_A2_T2.html
ietestcenter/Javascript/15.4.4.14-3-25.html
ietestcenter/Javascript/15.4.4.15-3-7.html
ietestcenter/Javascript/15.4.4.14-3-12.html
ietestcenter/Javascript/15.4.4.15-3-14.html
ietestcenter/Javascript/15.4.4.14-3-8.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A4_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A3_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A2_T2.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.6_Array_prototype_pop/S15.4.4.6_A3_T3.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.7_Array_prototype_push/S15.4.4.7_A2_T2.html
ietestcenter/Javascript/15.4.4.15-3-25.html
sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.13_Array_prototype_unshift/S15.4.4.13_A2_T2.html

[Build Bot]

Created attachment 294081 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Alexey Shvayka]

r260990 aligned ToLength with the spec, including "length" values of non-Array objects near/exceeding 2 ** 53 - 1.

*** This bug has been marked as a duplicate of bug 211205 ***