566914 - Crash [@ PropertyDescriptor::initialize] or "Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at ../jsapi.h"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

function f(code) {
    code.replace(/s/, "")
    eval(code)
}
__defineGetter__("x", /x/)
f("function a() {\
    x = Proxy.createFunction((function () {\
        return {\
            defineProperty: function (name, desc) {\
                Object.defineProperty(x, name, desc)\
            },\
            has: function () {},\
            get: function (r, name) {\
                return x[name]\
            }\
        }\
    })(), Object.defineProperties).__defineGetter__(\"\",(Function(\"\")))};\
a()\
")

crashes js opt shell on TM tip without -j at PropertyDescriptor::initialize and asserts js debug shell on TM tip without -j at Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at ../jsapi.h:204

Assuming related to bug harmony:proxies. Tested on 64-bit Ubuntu Linux 10.04.

Console stdout:

Program received signal SIGSEGV, Segmentation fault.
0x000000000047012c in PropertyDescriptor::initialize(JSContext*, long, long) ()
(gdb) bt
#0  0x000000000047012c in PropertyDescriptor::initialize(JSContext*, long, long) ()
#1  0x0000000000471775 in obj_defineProperty(JSContext*, unsigned int, long*) ()
#2  0x000000000054d6f2 in js_Interpret ()
#3  0x0000000000458ea8 in js_Invoke ()
#4  0x0000000000459682 in js_InternalInvoke ()
#5  0x00000000004a2615 in JSProxy::defineProperty(JSContext*, JSObject*, long, JSPropertyDescriptor*) ()
#6  0x00000000004a27b5 in proxy_DefineProperty(JSContext*, JSObject*, long, long, int (*)(JSContext*, JSObject*, long, long*), int (*)(JSContext*, JSObject*, long, long*), unsigned int) ()
#7  0x00000000004650bc in js_obj_defineGetter ()
#8  0x000000000054d6f2 in js_Interpret ()
#9  0x0000000000458391 in js_Execute ()
#10 0x000000000046f6ce in obj_eval(JSContext*, unsigned int, long*) ()
#11 0x000000000054d6f2 in js_Interpret ()
#12 0x0000000000458391 in js_Execute ()
#13 0x000000000040b646 in JS_ExecuteScript ()
#14 0x00000000004069e5 in Process(JSContext*, JSObject*, char*, int) ()
#15 0x0000000000407269 in main ()
(gdb) x/i $rip
=> 0x47012c <_ZN18PropertyDescriptor10initializeEP9JSContextll+1036>:	mov    (%rax),%rax
(gdb) x/b $rax
0x2e66c300000001b8:	Cannot access memory at address 0x2e66c300000001b8

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Patch with test case.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 446290 [details] [diff] [review]
patch

Pre-existing nits below.

>@@ -100,33 +100,33 @@ JSProxyHandler::get(JSContext *cx, JSObj
> bool
> JSProxyHandler::set(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, jsval *vp)
> {
>     AutoDescriptor desc(cx);
>     if (!getOwnPropertyDescriptor(cx, proxy, id, &desc))
>         return false;
>     if (desc.obj) {
>         if (desc.setter) {
>-            if (desc.attrs & JSPROP_GETTER)
>+            if (desc.attrs & JSPROP_SETTER)
>                 return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.setter), JSACC_READ, 0, 0, vp);
..1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

You are over the limit here :-P -- brace and wrap.

>             return desc.setter(cx, proxy, (desc.attrs & JSPROP_SHORTID)
>                                           ? INT_TO_JSID(desc.shortid)
>                                           : id, vp);

This is kinda right-heavy and ugly -- usually we break line after the, and the (desc.attrs & JSPROP_SHORTID) etc. underhangs the first arg.

>                 return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.setter), JSACC_READ, 0, 0, vp);
>             return desc.setter(cx, proxy, (desc.attrs & JSPROP_SHORTID)
>                                           ? INT_TO_JSID(desc.shortid)
>                                           : id, vp);

Ditto twice.

>+    return js_NewPropertyDescriptorObject(cx, id, attrs,
>+                                          (attrs & JSPROP_GETTER) ? CastAsObjectJSVal(desc->getter) : JSVAL_VOID,
>+                                          (attrs & JSPROP_SETTER) ? CastAsObjectJSVal(desc->setter) : JSVAL_VOID,

Ditto the 100 column limit nit.

/be

Comment 4

[Andreas Gal :gal]

Attachment: patch

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 446309 [details] [diff] [review]
patch

>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>--- a/js/src/jsproxy.cpp
>+++ b/js/src/jsproxy.cpp
>@@ -85,52 +85,58 @@ JSProxyHandler::get(JSContext *cx, JSObj
>     if (!desc.obj) {
>         *vp = JSVAL_VOID;
>         return true;
>     }
>     if (!desc.getter) {
>         *vp = desc.value;
>         return true;
>     }

Nice how early return means the remaining code is less indented:

>-    if (desc.attrs & JSPROP_GETTER)
>-        return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.getter), JSACC_READ, 0, 0, vp);
>-    return desc.getter(cx, proxy, (desc.attrs & JSPROP_SHORTID)
>-                                  ? INT_TO_JSID(desc.shortid)
>-                                  : id, vp);
>+    if (desc.attrs & JSPROP_GETTER) {
>+        return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.getter), JSACC_READ,
>+                                   0, 0, vp);
>+    }
>+    if (desc.attrs & JSPROP_SHORTID)
>+        id = INT_TO_JSID(desc.shortid);
>+    return desc.getter(cx, proxy, id, vp);

Cool.

> }
> 
> bool
> JSProxyHandler::set(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, jsval *vp)
> {
>     AutoDescriptor desc(cx);
>     if (!getOwnPropertyDescriptor(cx, proxy, id, &desc))
>         return false;
>     if (desc.obj) {
>         if (desc.setter) {
>-            if (desc.attrs & JSPROP_SETTER)
>-                return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.setter), JSACC_READ, 0, 0, vp);
>-            return desc.setter(cx, proxy, (desc.attrs & JSPROP_SHORTID)
>-                                          ? INT_TO_JSID(desc.shortid)
>-                                          : id, vp);
>+            if (desc.attrs & JSPROP_SETTER) {
>+                return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.setter), JSACC_READ,
>+                                           0, 0, vp);
>+            }
>+            if (desc.attrs & JSPROP_SHORTID)
>+                id = INT_TO_JSID(desc.shortid);
>+            return desc.setter(cx, proxy, id, vp);
>         } else {
>             if (desc.attrs & JSPROP_READONLY)
>                 return true;
>             desc.value = *vp;
>             return defineProperty(cx, proxy, id, &desc);
>         }
>     }

Looks like you can use similar early return structure in set to get, to avoid overindenting and still exceeding column 100!

/be

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 446312 [details] [diff] [review]
patch

>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>--- a/js/src/jsproxy.cpp
>+++ b/js/src/jsproxy.cpp
>@@ -85,57 +85,61 @@ JSProxyHandler::get(JSContext *cx, JSObj
>     if (!desc.obj) {
>         *vp = JSVAL_VOID;
>         return true;
>     }
>     if (!desc.getter) {
>         *vp = desc.value;
>         return true;
>     }
>+    if (desc.attrs & JSPROP_GETTER) {
>+        return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.getter), JSACC_READ,
>+                                   0, 0, vp);
>+    }
>+    if (desc.attrs & JSPROP_SHORTID)
>+        id = INT_TO_JSID(desc.shortid);
>+    return desc.getter(cx, proxy, id, vp);
> }

Case analysis in get is if (nosuchprop [!desc.obj){...return} if (no getter){...return} if (JSPROP_GETTER) {return...} etc.

> JSProxyHandler::set(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, jsval *vp)
> {
>     AutoDescriptor desc(cx);
>     if (!getOwnPropertyDescriptor(cx, proxy, id, &desc))
>         return false;
>     if (desc.obj) {
>         if (desc.setter) {
>+            if (desc.attrs & JSPROP_SETTER) {
>+                return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.setter), JSACC_READ,
..1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

D'oh!

>+                                           0, 0, vp);
>+            }
>+            if (desc.attrs & JSPROP_SHORTID)
>+                id = INT_TO_JSID(desc.shortid);
>+            return desc.setter(cx, proxy, id, vp);
>         }
>+        if (desc.attrs & JSPROP_READONLY)
>+            return true;
>+        desc.value = *vp;
>+        return defineProperty(cx, proxy, id, &desc);
>     }

Here the case analysis is less kind on indentation levels, but I guess you have to follow the spec and do this if (!desc.obj):

>     if (!getPropertyDescriptor(cx, proxy, id, &desc))
>         return false;
>     if (desc.obj) {
>         if (desc.setter) {
>+            if (desc.attrs & JSPROP_SETTER) {
>+                return js_InternalGetOrSet(cx, proxy, id, CastAsObjectJSVal(desc.setter), JSACC_READ,
>+                                           0, 0, vp);
>+            }
>+            if (desc.attrs & JSPROP_SHORTID)
>+                id = INT_TO_JSID(desc.shortid);
>+            return desc.setter(cx, proxy, id, vp);
>         }
>+        if (desc.attrs & JSPROP_READONLY)
>+            return true;
>+        /* fall through */
>     }

But why not test if (!desc.setter) and early return on the value case, as get does?

/be

Comment 8

[Andreas Gal :gal]

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

Comment 9

[Brendan Eich [:brendan]]

Andreas cited the spec's control flow, and the fall through required at the bottom of the repeated-looking section in set. All good as landed.

/be

Comment 10

[Robert Sayre]

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

Comment 11

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-566914.js.

Comment 12

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

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.