581769 - "Assertion failure: JSID_IS_INT(id) || JSID_IS_ATOM(id)," with E4X

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

__defineGetter__(<x/>,function(){})
for(w in this){}

asserts js debug shell on TM tip without -j at Assertion failure: JSID_IS_INT(id) || JSID_IS_ATOM(id), at ../jsiter.cpp:206

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   47546:9c869e64ee26
user:        Luke Wagner
date:        Wed Jul 14 23:19:36 2010 -0700
summary:     Bug 549143 - fatvals

Comment 2

[Luke Wagner [:luke]]

The issue here is that fatvals is producing a JSID_IS_OBJECT id in __defineGetter__ whereas pre-fatvals is producing a JSID_IS_STRING (specifically, "").  The reason is that, when determining whether to stick an object in an id directly or use the generic js_ValueToStringId (which will turn <x/> to ""), fatvals uses the predicate (obj->isXML()) whereas pre-fatvals uses the predicate (clasp == &js_QNameClass || clasp == &js_AttributeNameClass || clasp == &js_AnyNameClass).  Thus <x/> matches the the first but not the second.  Note that js_InternNonIntElementId uses obj->isXML() to make the same determination.

So my question: is this on purpose?  That is, that JS_ValueToId uses a different predicate than js_InternNonIntElementId (and, hence, JSOP_GETELEM, JSOP_IN, JSOP_INCPROP, JSOP_SETELEM, JSOP_ENUMELEM, ...).  If so, I'm happy to fix.

Also, I'm not sure how one creates a QName, AttributeName, or AnyName, but it seems like if the testcase in comment 0 replace <x/> with one of those three, this would trigger the same assertion in Enumerate pre-fatval.  Regardless, I think the assertion in Enumerate is overzealous.

Comment 3

[Jeff Walden [:Waldo]]

js> <a b="2"/>.(t.__defineGetter__(@b, function(){}))

Look and weep.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix

So, two fixes:
 - put ValueToId back the way it was,
 - take out the assert at the top of Enumerate, since objects can flow in too and Enumerate isn't actually assuming that the id is only one of those two.

Comment 5

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/7ff4f93bddaa

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7ff4f93bddaa