Array.prototype methods must use ToLength

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jordan Harband]

Many of the Array.prototype methods have bugs in that they use `array.length >>> 0` to handle array-like objects. This violates https://people.mozilla.org/~jorendorff/es6-draft.html#sec-tolength.

I'll be creating a `ToLength` C++ function, and exposing it to builtins as `@ToLength`, to correct this, along with adding tests.

[Jordan Harband]

Created attachment 251589 [details]
Patch

[Jordan Harband]

Benjamin raised a concern that replacing the `>>> 0` check with the ToLength call might have performance impacts.

My first priority was correctness - now that this patch is correct, I'd love to explore ways to determine if it's slow, and to speed it up if so.

[Build Bot]

Comment on attachment 251589 [details]
Patch

Attachment 251589 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5841617894244352

New failing tests:
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/built-in-function-calls-anonymous.html

[Build Bot]

Created attachment 251624 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 251589 [details]
Patch

Attachment 251589 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5553607386595328

New failing tests:
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/built-in-function-calls-anonymous.html

[Build Bot]

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

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

[Jordan Harband]

Created attachment 251724 [details]
Patch

[Build Bot]

Comment on attachment 251724 [details]
Patch

Attachment 251724 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5508203139825664

New failing tests:
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/built-in-function-calls-anonymous.html

[Build Bot]

Created attachment 251725 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 251724 [details]
Patch

Attachment 251724 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4687940286414848

New failing tests:
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/built-in-function-calls-anonymous.html

[Build Bot]

Created attachment 251726 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

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

[Yusuke Suzuki]

What do you think about implementing ToLength and ToInteger in JavaScript.
For example, the current `String.raw` and `Array.from` has its own ToLength/ToInteger implementation written in JS.

[Jordan Harband]

That'd be fine - although, this patch is ready to go except that I can't get the tests to pass :-) Please feel free to help, or take it over if you prefer!

[Yusuke Suzuki]

(In reply to comment #13)
> That'd be fine - although, this patch is ready to go except that I can't get
> the tests to pass :-) Please feel free to help, or take it over if you
> prefer!

Ah, ok. So based on your great patch, I'll change slightly to implement ToLength/ToInteger in JS.

[Yusuke Suzuki]

Created attachment 253286 [details]
Patch

[Build Bot]

Comment on attachment 253286 [details]
Patch

Attachment 253286 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5037208369102848

New failing tests:
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/built-in-function-calls-anonymous.html

[Build Bot]

Created attachment 253287 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 253286 [details]
Patch

Attachment 253286 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5369848754339840

New failing tests:
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/built-in-function-calls-anonymous.html

[Build Bot]

Created attachment 253290 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

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

[Yusuke Suzuki]

Created attachment 253291 [details]
Patch

[Yusuke Suzuki]

Could anyone take a look?

[Oliver Hunt]

Comment on attachment 253291 [details]
Patch

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

> Source/JavaScriptCore/builtins/GlobalObject.js:38
> +    return (numberValue > 0 ? 1 : -1) * @floor(@abs(numberValue));

Just as a sanity question, what is the expected behavior of 1/ToInteger(-0)?

> Source/JavaScriptCore/builtins/GlobalObject.js:47
> +    return length > 0 ? (length < maxSafeInteger ? length : maxSafeInteger) : 0;

ditto

[Yusuke Suzuki]

Comment on attachment 253291 [details]
Patch

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

Thank you for your review!

>> Source/JavaScriptCore/builtins/GlobalObject.js:38
>> +    return (numberValue > 0 ? 1 : -1) * @floor(@abs(numberValue));
> 
> Just as a sanity question, what is the expected behavior of 1/ToInteger(-0)?

When -0 comes, `numberValue` becomes `-0` and `numberValue === 0` becomes true. So `ToInteger` returns `-0`.
It conforms the spec. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-tointeger

"If number is +0, −0, +∞, or −∞, return number."

>> Source/JavaScriptCore/builtins/GlobalObject.js:47
>> +    return length > 0 ? (length < maxSafeInteger ? length : maxSafeInteger) : 0;
> 
> ditto

When `-0` comes, `length` becomes `-0` since `ToInteger(-0)` returns `-0`.
And after that, `length > 0` becomes `false`. So `ToLength(-0)` returns `+0`.
It conforms the spec. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-tolength

"If len ≤ +0, return +0."

[WebKit Commit Bot]

Comment on attachment 253291 [details]
Patch

Clearing flags on attachment: 253291

Committed r184582: <http://trac.webkit.org/changeset/184582>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.