Closed Bug 503992 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.1 --- .4-fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

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.
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.
Status: NEW → ASSIGNED
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.
Assignee: general → jwalden+bmo
Attachment #388729 - Flags: review?(jorendorff)
Summary: js_Int32ToId doesn't return JSVAL_INT ids when given negative numbers → js_Int32ToId doesn't return the right ids for negative !INT_FITS_IN_JSVAL() integers
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.
Attachment #388729 - Flags: review?(jorendorff) → review+
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.
Whiteboard: fixed-in-tracemonkey
Attached patch 191 branch patchSplinter Review
Attachment #388765 - Flags: approval1.9.1.1?
http://hg.mozilla.org/mozilla-central/rev/cea0f6a3e458
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Attachment #388765 - Flags: approval1.9.1.1? → approval1.9.1.3?
Attachment #388765 - Flags: approval1.9.1.3? → approval1.9.1.4+
Comment on attachment 388765 [details] [diff] [review]
191 branch patch

Approved for 1.9.1.4, a=dveditz for release-drivers
trace-test testInt32ToId|testOwnPropertyWithInOperator
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.