Closed Bug 605013 Opened 9 years ago Closed 9 years ago

"Assertion failure: JSID_IS_INT(id),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: gkw, Assigned: luke)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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),
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.
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).
blocking2.0: ? → betaN+
Assignee: general → lw
Reduced test case:

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

This is just obj_keys forgetting about 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.
Attachment #489370 - Flags: review?(jwalden+bmo)
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.
Attachment #489370 - Flags: review?(jwalden+bmo) → review+
Duh, thanks!
http://hg.mozilla.org/tracemonkey/rev/b9eac30071aa
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/b9eac30071aa
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.