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)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: m93a.cz, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
12.74 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•9 years ago
|
||
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)
Updated•9 years ago
|
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
Comment 3•9 years ago
|
||
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•9 years ago
|
||
Yes, this is another report of bug 924058.
Blocks: es6
Flags: needinfo?(jorendorff)
Updated•8 years ago
|
Summary: Array.prototype.forEach freeze for length less than zero → Fix self-hosted Array methods to use ToLength for length
Updated•8 years ago
|
Assignee: nobody → jorendorff
Assignee | ||
Updated•8 years ago
|
Assignee: jorendorff → evilpies
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Comment 6•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8775693 -
Flags: review?(jorendorff) → review?(arai.unmht)
Comment 7•8 years ago
|
||
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
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9eeb3adc7dd
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•