Closed Bug 614070 Opened 9 years ago Closed 9 years ago

Array.prototype.unshift does not set new length if argc == 0

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Test case:
---
var a = {};
a.length = 4294967296;
Array.prototype.unshift.call(a);
assertEq(a.length, 0);
---
Easy fix, patch tomorrow (don't have time to run tests atm)
Is this bug 612260, or different?
Blocks: 612260
Attached patch Fix (obsolete) — Splinter Review
Assignee: general → jandemooij
Status: NEW → ASSIGNED
Attachment #492735 - Flags: review?(jwalden+bmo)
Blocks: sputnik
(In reply to comment #1)
> Is this bug 612260, or different?

No this is only an edge case in unshift.
No longer blocks: 612260
Comment on attachment 492735 [details] [diff] [review]
Fix

This seems generally reasonable, but I admit I found the test behavior a bit surprising, until I remembered the 32-bit cast in ToUint32 -- plus, your test neglects testing the other mode of failure.  Could you also add a test that checks for correct behavior using a getter and setter for the length property?  With that all should be good.
Attachment #492735 - Flags: review?(jwalden+bmo) → review-
Attached patch Patch v2Splinter Review
Good idea, added more tests using getters/setters.
Attachment #492735 - Attachment is obsolete: true
Attachment #493118 - Flags: review?(jwalden+bmo)
Comment on attachment 493118 [details] [diff] [review]
Patch v2

Excellent.

(I'm queueing up your patches for a push in a bit -- probably not today but most likely before Monday, or Monday at latest -- just FYI.)
Attachment #493118 - Flags: review?(jwalden+bmo) → review+
(In reply to comment #6)
> (I'm queueing up your patches for a push in a bit -- probably not today but
> most likely before Monday, or Monday at latest -- just FYI.)

Thanks, most of these bugs have gone unnoticed for years (spec nitpickery), so another week shouldn't hurt ;)
http://hg.mozilla.org/tracemonkey/rev/2000c21b7910

Oh, also, I noticed the |if (argc > 0)| guard on reassigning existing elements of this isn't in the spec -- sent mail to es5-discuss about it:

https://mail.mozilla.org/pipermail/es5-discuss/2010-November/003836.html
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/2000c21b7910
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.