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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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

[Josh Matthews [:jdm]]

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.

Comment 2

[Tom S [:evilpie]]

The problems is in ValueToLength. 
We would need to call ValueToECMAUint32 and then compare this with the result of ValueToNumber.

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jeff Walden [:Waldo]]

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

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 6

[David Mandelin [: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)|.

Comment 7

[Jeff Walden [:Waldo]]

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

Comment 8

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/f25365ded0a9

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f25365ded0a9