Setting an array's length doesn't convert to number twice

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

No, really.

js> var c = 0; [].length = { valueOf: function() { c++; return 1; } }; assertEq(c, 2)
typein:1: Error: Assertion failed: got 1, expected 2

Comment 1

7 years ago
Jeff, do you have a pointer to a spec that lays out how this should be behaving? I read through array_length_setter, and it seemed fairly reasonable to me - we convert the value to a number, then set the array length.
The problems is in ValueToLength. 
We would need to call ValueToECMAUint32 and then compare this with the result of ValueToNumber.
See ES5 15.4.5.1 step 3, substeps b-d:

b. Let newLenDesc be a copy of Desc.
c. Let newLen be ToUint32(Desc.[[Value]]).
d. If newLen is not equal to ToNumber( Desc.[[Value]]), throw a RangeError
   exception.

Instead of performing step c then step d, we perform this:

c. Let v be ToNumber(Desc.[[Value]]).
d. Let newLen be ToUint32(v).
e. If newLen is not equal to v, throw a RangeError exception.

Looking more at how ValueToLength is used, I'm not convinced it's a good abstraction.  The callers are so few, and their circumstances different enough, that it seems preferable to me to just encode the ToUint32 == ToNumber check directly, and win from more readable code.
There are actually a number of different problems with this code, some touching upon test262 results -- this one at least:

S15.4.5.1_A1.3_T2	Uint32 use ToNumber and ToPrimitve	fail

Taking...
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla7
Created attachment 542357 [details] [diff] [review]
Patch with tests

I think this covers everything here.  (I'll give it a try run before pushing, since this is pretty heavily used functionality.)

Regarding JS_HasArrayLength, its semantics are not clearly described in the JSAPI reference, and both jorendorff and I are mystified about the use case it serves, so needing to modify it in order to remove the ValueToLength method seemed a good reason to just remove it.
Attachment #542357 - Flags: review?(dmandelin)
Comment on attachment 542357 [details] [diff] [review]
Patch with tests

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

Looks good. One small note about an assertion.

::: js/src/jsarray.cpp
@@ +2518,3 @@
>                      return false;
> +                JS_ASSERT(js_DoubleToInteger(len.toNumber()) == len.toNumber());
> +                uint32 alength = uint32(len.toNumber());

I'm not sure if the assertion is necessary, but if so, it looks too weak to me: I think it would be better to assert |len.toNumber() == uint32(len.toNumber)|.
Attachment #542357 - Flags: review?(dmandelin) → review+
(In reply to comment #6)
> I'm not sure if the assertion is necessary

Actually, it's much easier to just replace this with js_GetLengthProperty -- don't know why that didn't occur to me when reading this.

(As far as assertion weakness/strength, when I wrote the patch I was being particularly wary of the anti-pattern in bug 667739, trying to write something which didn't spread that problem any further.  Reusing an existing method is clearly the better approach, of course.)
http://hg.mozilla.org/tracemonkey/rev/f25365ded0a9
Whiteboard: fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f25365ded0a9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla7 → mozilla8
You need to log in before you can comment on or make changes to this bug.