Closed
Bug 566914
Opened 14 years ago
Closed 14 years ago
Crash [@ PropertyDescriptor::initialize] or "Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at ../jsapi.h"
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: gkw, Assigned: gal)
References
Details
(4 keywords, Whiteboard: fixed-in-tracemonkey)
Crash Data
Attachments
(2 files, 2 obsolete files)
3.90 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
4.62 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•14 years ago
|
Assignee: general → gal
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #446287 -
Flags: review?(brendan)
Assignee | ||
Comment 2•14 years ago
|
||
Patch with test case.
Attachment #446287 -
Attachment is obsolete: true
Attachment #446290 -
Flags: review?(brendan)
Attachment #446287 -
Flags: review?(brendan)
Comment 3•14 years ago
|
||
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
Attachment #446290 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 4•14 years ago
|
||
Comment 5•14 years ago
|
||
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
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #446290 -
Attachment is obsolete: true
Attachment #446309 -
Attachment is obsolete: true
Attachment #446312 -
Flags: review?(brendan)
Comment 7•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Attachment #446290 -
Attachment is obsolete: false
Assignee | ||
Updated•14 years ago
|
Attachment #446312 -
Flags: review?(brendan)
Assignee | ||
Comment 8•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/a6b91d5d9c89
Whiteboard: fixed-in-tracemonkey
Comment 9•14 years ago
|
||
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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a6b91d5d9c89
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ PropertyDescriptor::initialize]
Comment 11•12 years ago
|
||
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-566914.js.
Flags: in-testsuite+
Reporter | ||
Comment 12•11 years ago
|
||
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•