Closed Bug 1124857 Opened 9 years ago Closed 8 years ago

Fix self-hosted Array methods to use ToLength for length

Categories

(Core :: JavaScript Engine, defect)

37 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: m93a.cz, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
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?
Depends on: 924058
Flags: needinfo?(jorendorff)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
Yes, this is another report of bug 924058.
Blocks: es6
Flags: needinfo?(jorendorff)
Summary: Array.prototype.forEach freeze for length less than zero → Fix self-hosted Array methods to use ToLength for length
Assignee: nobody → jorendorff
Assignee: jorendorff → evilpies
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.
Attachment #8775693 - Flags: review?(jorendorff)
Blocks: 924058
No longer depends on: 924058
Attachment #8775693 - Flags: review?(jorendorff) → review?(arai.unmht)
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?
Attachment #8775693 - Flags: review?(arai.unmht) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9eeb3adc7dd
Fix self-hosted Array methods to use ToLength for length. r=arai
https://hg.mozilla.org/mozilla-central/rev/a9eeb3adc7dd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: