605013 - "Assertion failure: JSID_IS_INT(id),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

x = /x/
Function("Object.defineProperty(x,new AttributeName,({e:true,enumerable:true}))")()
{
  throw (Object.keys)(x, /x/)
}

asserts js debug shell on TM changeset 47a8311cf0bb without -m or -j at Assertion failure: JSID_IS_INT(id),

Comment 1

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

x = /x/
Function("\
  Object.defineProperty(\
    x,\
    new AttributeName,\
    ({e:true,enumerable:true})\
  )\
")()
{
  throw (Object.keys)(x, /x/)
}


is a better testcase due to bugzilla wrapping long lines.

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   41832:b15fd8b568e4
user:        Andreas Gal
date:        Fri May 07 17:52:52 2010 -0700
summary:     fast object iteration (558754, r=brendan, CLOSED TREE).

Comment 3

[Luke Wagner [:luke]]

Reduced test case:

x = {};
Object.defineProperty(x, new AttributeName, { enumerable:true });
Object.keys(x);

This is just obj_keys forgetting about object ids.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix: ignore object ids

Waldo's suggestion is to ignore object ids.  This makes sense since 15.2.4.14 Step 5 says "For each own enumerable property of O whose name **String** is P" and there is no stringification k of (new AttributeName) where o[k] will refer to the same property.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 489370 [details] [diff] [review]
fix: ignore object ids

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -1784,27 +1784,28 @@ obj_keys(JSContext *cx, uintN argc, Valu
>     if (!GetFirstArgumentAsObject(cx, argc, vp, "Object.keys", &obj))
>         return JS_FALSE;
> 
>     AutoIdVector props(cx);
>     if (!GetPropertyNames(cx, obj, JSITER_OWNONLY, &props))
>         return JS_FALSE;
> 
>     AutoValueVector vals(cx);
>-    vals.resize(props.length());
>+    vals.reserve(props.length());  /* avoid oom check below */

Shouldn't this check for OOM?


>     for (size_t i = 0, len = props.length(); i < len; i++) {
>         jsid id = props[i];
>         if (JSID_IS_STRING(id)) {
>-            vals[i].setString(JSID_TO_STRING(id));
>-        } else {
>-            JS_ASSERT(JSID_IS_INT(id));
>+            vals.append(StringValue(JSID_TO_STRING(id)));
>+        } else if (JSID_IS_INT(id)) {
>             JSString *str = js_IntToString(cx, JSID_TO_INT(id));
>             if (!str)
>                 return JS_FALSE;
>-            vals[i].setString(str);
>+            vals.append(StringValue(str));
>+        } else {
>+            JS_ASSERT(JSID_IS_OBJECT(id));
>         }
>     }

These should use JS_ALWAYS_OK to verify OOM-safeness.

Comment 6

[Luke Wagner [:luke]]

Duh, thanks!

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Robert Sayre]

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

Comment 9

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929