Closed
Bug 465241
Opened 16 years ago
Closed 16 years ago
TM: Wrong value for ("0" in [3])
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b2
People
(Reporter: jruderman, Assigned: gal)
References
Details
(Keywords: testcase, verified1.9.1)
Attachments
(1 file)
8.87 KB,
patch
|
dvander
:
review+
brendan
:
review-
|
Details | Diff | Splinter Review |
js> for (let j = 0; j < 5; ++j) print("" + ("0" in [3])); true true false false false
Assignee | ||
Updated•16 years ago
|
Assignee: general → gal
Assignee | ||
Comment 1•16 years ago
|
||
This is a substantial rewrite. The previous code seems totally bogus.
Attachment #348511 -
Flags: review?(danderson)
Assignee | ||
Comment 2•16 years ago
|
||
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+
Assignee | ||
Comment 4•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/acb5720b8c51
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 5•16 years ago
|
||
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 6•16 years ago
|
||
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-
Assignee | ||
Updated•16 years ago
|
Severity: normal → critical
Flags: blocking1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b2
Comment 7•16 years ago
|
||
reopening, marking blocking beta2, will close once landed on m-c.
Status: RESOLVED → REOPENED
Flags: blocking1.9.1? → blocking1.9.1+
Resolution: FIXED → ---
Comment 8•16 years ago
|
||
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 ago → 16 years ago
Resolution: --- → FIXED
Comment 9•16 years ago
|
||
testIn already in js1_8_1/trace/trace-test.js
Flags: in-testsuite+
Flags: in-litmus-
Comment 10•16 years ago
|
||
test landed http://hg.mozilla.org/mozilla-central/rev/190225e022e0 and cvs
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 11•15 years ago
|
||
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•