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

RESOLVED FIXED in mozilla8



JavaScript Engine
7 years ago
7 years ago


(Reporter: Waldo, Assigned: Waldo)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: fixed-in-tracemonkey)


(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 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

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

Assignee: general → jwalden+bmo
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.)
Whiteboard: fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
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.