Closed Bug 465241 Opened 16 years ago Closed 16 years ago

TM: Wrong value for ("0" in [3])

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: jruderman, Assigned: gal)

References

Details

(Keywords: testcase, verified1.9.1)

Attachments

(1 file)

js> for (let j = 0; j < 5; ++j) print("" + ("0" in [3]));
true
true
false
false
false
Assignee: general → gal
Attached patch patchSplinter Review
This is a substantial rewrite. The previous code seems totally bogus.
Attachment #348511 - Flags: review?(danderson)
Comment on attachment 348511 [details] [diff] [review]
patch

Brendan, please double check.
Attachment #348511 - Flags: review?(brendan)
Comment on attachment 348511 [details] [diff] [review]
patch

Looks a lot cleaner - hopefully no one is using "in" on dense arrays.
Attachment #348511 - Flags: review?(danderson) → review+
http://hg.mozilla.org/tracemonkey/rev/acb5720b8c51
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: arithfuzz
Comment on attachment 348511 [details] [diff] [review]
patch

>+static inline JSBool
>+js_Int32ToId(JSContext* cx, int32 index, jsid* id)
>+{
>+    if (unsigned(index) <= JSVAL_INT_MAX) {
>+        *id = INT_TO_JSID(index);
>+        return JS_TRUE;

else after return here:

>+    } else {
>+        JSString* str = js_NumberToString(cx, index);
>+        if (!str)
>+            return JS_FALSE;
>+        return js_ValueToStringId(cx, STRING_TO_JSVAL(str), id);
>+    }

I just pushed the fix.

> bool
> TraceRecorder::record_JSOP_IN()
> {
[snip]
>+    } else if (JSVAL_IS_STRING(lval)) {
>         if (!js_ValueToStringId(cx, lval, &id))
>+            ABORT_TRACE("left operand of JSOP_IN didn't convert to a string-id");

False return from js_ValueToStringId is an error, should just return false -- we need something to notice in the TRACE/RECORD macrology.

>@@ -2433,16 +2433,32 @@ function testStringToInt32() {
[snip]
>+function testIn() {
>+    var array = [3];
>+    var obj = { "-1": 5, "1.7": 3, "foo": 5, "1": 7 };
>+    var a = [];
>+    for (let j = 0; j < 5; ++j) {
>+        a.push("0" in array);
>+        a.push(-1 in obj);
>+        a.push(1.7 in obj);
>+        a.push("foo" in obj);
>+        a.push(1 in obj);
>+    }
>+    return a.join(",");
>+}
>+testIn.expected = "true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true,true";
>+test(testIn);

We had inObjectTest and inArrayTest, maybe they should live next to this (and have give all these better testFoo names).

/be
Attachment #348511 - Flags: review?(brendan) → review+
Comment on attachment 348511 [details] [diff] [review]
patch

>+static inline JSBool
>+js_Int32ToId(JSContext* cx, int32 index, jsid* id)
>+{
>+    if (unsigned(index) <= JSVAL_INT_MAX) {
>+        *id = INT_TO_JSID(index);
>+        return JS_TRUE;
>+    } else {
>+        JSString* str = js_NumberToString(cx, index);
>+        if (!str)
>+            return JS_FALSE;
>+        return js_ValueToStringId(cx, STRING_TO_JSVAL(str), id);
>+    }
> }

This (with else after return elimination) is now the same function as js_IndexToId (from jsarray.cpp). So that should be used, provided it's ok to cast the index to (jsuint) before calling. But is that ok?

If someone indexes into an array by -1, they are not accessing index 0xffffffff. So this is actually broken.

What's more, if someone indexes using "42", then we could recognize that and convert to 42. See CHECK_FOR_STRING_INDEX in jsobj.h, which calls js_CheckForStringIndex.

I'll file a followup, which needs to be pushed and taken for b2.

/be
Attachment #348511 - Flags: review+ → review-
Depends on: 465347
Severity: normal → critical
Flags: blocking1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b2
reopening, marking blocking beta2, will close once landed on m-c.
Status: RESOLVED → REOPENED
Flags: blocking1.9.1? → blocking1.9.1+
Resolution: FIXED → ---
Fixed in the merge pushed by vlad on Nov 18 14:11:14 2008 -0800:
http://hg.mozilla.org/mozilla-central/rev/e8ed5d4bf531
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
testIn already in js1_8_1/trace/trace-test.js
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: