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.
Created attachment 251589 [details] Patch
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.
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
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
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
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
Created attachment 251724 [details] Patch
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
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
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
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
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.
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!
(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.
Created attachment 253286 [details] Patch
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
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
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
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
Created attachment 253291 [details] Patch
Could anyone take a look?
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
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."
Comment on attachment 253291 [details] Patch Clearing flags on attachment: 253291 Committed r184582: <http://trac.webkit.org/changeset/184582>
All reviewed patches have been landed. Closing bug.