503992 - js_Int32ToId doesn't return the right ids for negative !INT_FITS_IN_JSVAL() integers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

If the integer passed to this method is a negative 31-bit integer, the id returned is a string id.  One caller checks for string index and dodges the bullet; I think this might only be noticeable via GetElement/SetElement and friends inside imacro code from trace.

Comment 1

[Jeff Walden [:Waldo]]

I miswrote my testcase, and this is an observable bug without needing special object classes or ops:

var x = { "-1073741828": 17 };
var j = -1073741819;
var a = [];
for (var i = 0; i < 20; i++)
{
  j--;
  a.push(j in x);
}
assertEq(a[8], true);

Patch later this evening.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch, testing both that over-negative properties are found correctly and that they don't alias other properties

Hey, two bugs for the price of one!  The second test within testInt32ToId not failing with current code was what led me to discover the need for the HasProperty change -- glad I decided to make the effort to test the second scenario even after writing the first test.

I could split this patch if truly desired, but it's small enough anyway I don't see an especially compelling reason to do so.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 388729 [details] [diff] [review]
Patch, testing both that over-negative properties are found correctly and that they don't alias other properties

>-    if (!js_LookupPropertyWithFlags(cx, obj, id, JSRESOLVE_QUALIFIED, &obj2, &prop))
>+    if (js_LookupPropertyWithFlags(cx, obj, id, JSRESOLVE_QUALIFIED, &obj2, &prop) < 0)

*jaw drops*

Nice catch.

Comment 4

[Jeff Walden [:Waldo]]

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

These fixes are only correctness/speed fixes, not stability fixes, but I think we may want these changes in 191.  The 'in' change in particular is going to break tracing of most loops with use of 'in' in their bodies.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 191 branch patch

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cea0f6a3e458

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 388765 [details] [diff] [review]
191 branch patch

Let's let this bake for a while; we'll get it in 3.5.3

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 388765 [details] [diff] [review]
191 branch patch

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 9

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4db402111361

Comment 10

[Bob Clary [:bc] (inactive)]

trace-test testInt32ToId|testOwnPropertyWithInOperator