1124857 - Fix self-hosted Array methods to use ToLength for length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[m93a.cz]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150121004011

Steps to reproduce:

`Array.prototype.forEach.call( {length:-1}, anyFunction )`


Actual results:

stuck in infinite loop, then a "script not responding" dialog


Expected results:

1) nothing
2) an error thrown

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like ArrayForEach does this:

204     var len = TO_UINT32(O.length);

This is in fact what forEach did in ES5.  So in ES5, that call would call anyFunction 2^32-1 times, and the slow script dialog is in fact correct.

In ES6, looks like this has been changed to:

  4. Let len be ToLength(lenValue).

where ToLength does a ToInteger and then returns 0 if the integer is negative.  So in ES6 there should be 0 calls to anyFunction.

So the draft specification actually changes the behavior of the code here.  And as far as I can tell, this is basically a duplicate of bug 924058.

Other than possible web compat impact, what stops us from just making this change?

Comment 2

[ziyunfei]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Other than possible web compat impact, what stops us from just making this
> change?

I think we need a ToLength method here https://dxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#82

Comment 3

[Boris Zbarsky [:bzbarsky]]

You need it at http://mxr.mozilla.org/mozilla-central/source/js/src/builtin/Array.js?rev=26263767b141&force=1#203 for this particular bug, no?

Comment 4

[Jason Orendorff [:jorendorff]]

Yes, this is another report of bug 924058.

Comment 5

[Tom S]

Attachment: Use ToLength in self-hosted Array methods

Most of these tests exploit that TO_UINT32(Infinity) would be 0, while ToLength() will MAX_SAFE_INTEGER, thus in the first case our callback would not be executed at all, while in the later case potentially almost for ever.

Comment 6

[André Bargull [:anba]]

The comments [1,2] in ArrayFind and ArrayFindIndex should also be updated ( = removed).

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/tip/js/src/builtin/Array.js#l513
[2] http://hg.mozilla.org/integration/mozilla-inbound/file/tip/js/src/builtin/Array.js#l549

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8775693 [details] [diff] [review]
Use ToLength in self-hosted Array methods

Review of attachment 8775693 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you :D

::: js/src/tests/ecma_6/Array/to-length.js
@@ +2,5 @@
> +
> +assertEq(Array.prototype.indexOf.call({length: Infinity, [max - 1]: 'test'}, 'test', max - 3), max - 1);
> +assertEq(Array.prototype.lastIndexOf.call({length: Infinity, [max - 2]: 'test', [max - 1]: 'test2'}, 'test'), max - 2);
> +
> +// ToUint32 would truncate Infinity to zero, so the callback would never be invoked

This comment is misleading as it's saying about the before-patched code that doesn't exist anymore.
Can you rephrase it to saying about "ToLength shouldn't truncate Infinity to zero", or
add "var summary" or a comment with "Array methods should use ToLength instead of TO_UINT32 for length" or something, and bug number also, to clarify why "ToUint32" is mentioned here?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9eeb3adc7dd
Fix self-hosted Array methods to use ToLength for length. r=arai

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a9eeb3adc7dd