Closed
Bug 389034
Opened 17 years ago
Closed 16 years ago
JS_SetProperty() ends up resolving w/o JSRESOLVE_ASSIGNING
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jst, Assigned: jorendorff)
References
Details
Attachments
(1 file, 1 obsolete file)
32.06 KB,
patch
|
brendan
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
If I call JS_SetProperty() on an object whose class uses new resolve, I get into the resolve hook as I'd expect, but the flag JSRESOLVE_ASSIGNING is not set. Seems it should be, as I'm setting a property, we know it's a set and the resolve hook flags should reflect that.
Comment 1•17 years ago
|
||
The problem here is that js_LookupPropertyWithFlags decides to set JSRESOLVE_ASSIGNING based solely on *cx->fp->pc, so if we get here from a JSAPI entry point, cx->fp->pc might be entirely unrelated to what's going on.
Assignee | ||
Comment 2•16 years ago
|
||
Add cx->resolveFlags. It would have been more direct to add a resolveFlags argument to each of JSObjectOps::{get,set,define,delete}Property, but that would touch a lot more code and change more behavior. Also could be slow. When defining a property, the prevailing resolveFlags are relevant only if the JSPROP_GETTER or SETTER attrs are present. Because of the change, when nsWindowSH forwards NewResolve, it must explicitly forward the flags too. I add a new API to support this.
Assignee: general → jorendorff
Attachment #339025 -
Flags: review?(mrbkap)
Assignee | ||
Comment 3•16 years ago
|
||
Note- this doesn't go as far as it could. There are also places where the JS engine itself looks up properties where it doesn't make sense to infer the resolve flags--e.g. JSOP_LENGTH, o.__noSuchMethod__, fn.prototype, o.toString, o.valueOf... I didn't see any actual bugs though, at a glance, so I left them all alone.
Comment 4•16 years ago
|
||
Comment on attachment 339025 [details] [diff] [review] v1 I like this. I think brendan should have the final say on the JS_LookupPropertyById API (which I've wanted for a long time), though.
Attachment #339025 -
Flags: review?(mrbkap)
Attachment #339025 -
Flags: review?(brendan)
Attachment #339025 -
Flags: review+
Comment 5•16 years ago
|
||
Comment on attachment 339025 [details] [diff] [review] v1 /me is sad mrbkap didn't scratch his itch a while ago Nice patch, the API design flaw is 12+ years old here: >@@ -5904,30 +5904,27 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp > wrapper->GetJSObject(&realObj); > if (realObj == obj && > innerWin && (innerObj = innerWin->GetGlobalJSObject())) { > #ifdef DEBUG_SH_FORWARDING > printf(" --- Forwarding resolve to inner window %p\n", (void *)innerWin); > #endif > > jsid interned_id; >- JSObject *pobj; >- JSProperty *prop = nsnull; >+ JSObject *pobj = NULL; >+ jsval val; > > *_retval = (::JS_ValueToId(cx, id, &interned_id) && >- OBJ_LOOKUP_PROPERTY(cx, innerObj, interned_id, &pobj, >- &prop)); >- >- if (*_retval && prop) { >+ ::JS_LookupPropertyByIdWithFlags(cx, innerObj, interned_id, >+ flags, &pobj, &val)); >+ >+ if (*_retval && pobj) { > #ifdef DEBUG_SH_FORWARDING > printf(" --- Resolve on inner window found property.\n"); > #endif >- >- OBJ_DROP_PROPERTY(cx, pobj, prop); >- > *objp = pobj; How is it ok to leave pobj locked here? In general the OBJ_DROP_PROPERTY API allows for both pobj and prop to need some mandatory release or unlock computation, but prop is not returned via an out parameter, so that's a warning sign that this OBJ_DROP_PROPERTY can't go away. Or else I'm missing something big! >@@ -6405,31 +6402,30 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp > (JSOp)*cx->fp->regs->pc != JSOP_BINDNAME && win->IsInnerWindow()) { > JSObject *realObj; > wrapper->GetJSObject(&realObj); > > if (obj == realObj) { > JSObject *proto = STOBJ_GET_PROTO(obj); > if (proto) { > jsid interned_id; >- JSProperty *prop = nsnull; >+ JSObject *pobj = NULL; >+ jsval val; > > if (!::JS_ValueToId(cx, id, &interned_id) || >- !OBJ_LOOKUP_PROPERTY(cx, proto, interned_id, objp, &prop)) { >+ !::JS_LookupPropertyByIdWithFlags(cx, proto, interned_id, flags, >+ &pobj, &val)) { > *_retval = JS_FALSE; > > return NS_OK; > } > >- if (prop) { >- // A property was found on the prototype chain, and *objp is >- // already set to point to the prototype where the property >- // was found. >- OBJ_DROP_PROPERTY(cx, proto, prop); >- >+ if (pobj) { >+ // A property was found on the prototype chain. >+ *objp = pobj; Same here. >+LookupPropertyById(JSContext *cx, JSObject *obj, jsid id, uintN flags, >+ JSObject **objp, JSProperty **propp) >+{ >+ JSAutoResolveFlags rf(cx, flags); >+ >+ return OBJ_LOOKUP_PROPERTY(cx, obj, id, objp, propp); Could cuddle vertically, since a few other places do. Usual C-style was to have one blank line separating declarations from statements, but we're past that, especially with this constructor-call-style initializer. > JS_LookupPropertyWithFlags(JSContext *cx, JSObject *obj, const char *name, > uintN flags, jsval *vp) > { > JSAtom *atom; >- JSBool ok; >- JSObject *obj2; >- JSProperty *prop; >- >- CHECK_REQUEST(cx); >- atom = js_Atomize(cx, name, strlen(name), 0); >- if (!atom) >- return JS_FALSE; >+ JSObject *obj2; >+ >+ atom = js_Atomize(cx, name, strlen(name), 0); >+ return atom && >+ JS_LookupPropertyByIdWithFlags(cx, obj, ATOM_TO_JSID(atom), flags, >+ &obj2, vp); Indent overflow lines to underhang left operand of && -- to start in same column as 'a' in 'atom &&' (with any overflow args of course adjusted to underhang first arg). > JS_GetProperty(JSContext *cx, JSObject *obj, const char *name, jsval *vp) > { > JSAtom *atom; > > CHECK_REQUEST(cx); > atom = js_Atomize(cx, name, strlen(name), 0); > if (!atom) > return JS_FALSE; >+ >+ JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED); >+ > return OBJ_GET_PROPERTY(cx, obj, ATOM_TO_JSID(atom), vp); This would read ok and look better to my eyes if you get rid of the blank line between rf's declaration and the return. It would match the following: > JS_SetProperty(JSContext *cx, JSObject *obj, const char *name, jsval *vp) > { > JSAtom *atom; > > CHECK_REQUEST(cx); > atom = js_Atomize(cx, name, strlen(name), 0); > if (!atom) > return JS_FALSE; >+ >+ JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED | JSRESOLVE_ASSIGNING); > return OBJ_SET_PROPERTY(cx, obj, ATOM_TO_JSID(atom), vp); Which looks good to me, and which repeats later. > JS_GetElement(JSContext *cx, JSObject *obj, jsint index, jsval *vp) > { >+ JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED); >+ > CHECK_REQUEST(cx); > return OBJ_GET_PROPERTY(cx, obj, INT_TO_JSID(index), vp); OTOH here a blank line helps. >@@ -301,16 +301,18 @@ js_NewContext(JSRuntime *rt, size_t stac > JS_INIT_ARENA_POOL(&cx->regexpPool, "regexp", > 12 * 1024 - 40, /* FIXME: bug 421435 */ > sizeof(void *), &cx->scriptStackQuota); > > if (!js_InitRegExpStatics(cx, &cx->regExpStatics)) { > js_DestroyContext(cx, JSDCM_NEW_FAILED); > return NULL; > } >+ >+ cx->resolveFlags = 0; Not needed, cx is zeroed on creation. I wonder if you couldn't initialize with JSRESOLVE_INFER and then make a new rf only in js_Execute and js_InternalInvoke, to keep the js_Interpret path a tiny bit faster. Other calls to js_Interpret from elsewhere in the engine are from known-native call sites, not nesting on top of APIs (I think -- double check me here). The rf at the top of js_Interpret is safer for sure, so mainly I'm asking here, in case you surveyed and found even tougher optimization sledding. /be
Comment 6•16 years ago
|
||
Comment on attachment 339025 [details] [diff] [review] v1 (In reply to comment #5) > >+ JSObject *pobj = NULL; > >+ jsval val; > > > > *_retval = (::JS_ValueToId(cx, id, &interned_id) && > >- OBJ_LOOKUP_PROPERTY(cx, innerObj, interned_id, &pobj, > >- &prop)); > >- > >- if (*_retval && prop) { > >+ ::JS_LookupPropertyByIdWithFlags(cx, innerObj, interned_id, > >+ flags, &pobj, &val)); Jason pointed out my blindness on IRC -- no prop result here, pobj is unlocked. The final thought about minimizing JSRESOLVE_INFER defaulting was just a question but it doesn't need answering with a code change in the patch. With nits-picked, r=me. /be
Attachment #339025 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 7•16 years ago
|
||
Picked nits. I looked at putting the assignment outside of js_Interpret. There are a couple cases where it would be nice to make that path faster (Array.sort), but even so, I doubt the JSAutoResolveFlags is a measurable cost, relative to the rest of it. I will test to make sure.
Attachment #339025 -
Attachment is obsolete: true
Attachment #339865 -
Flags: review?(brendan)
Updated•16 years ago
|
Attachment #339865 -
Flags: review?(brendan) → review+
Comment 8•16 years ago
|
||
Comment on attachment 339865 [details] [diff] [review] v2 >+ if ((flags & (JSRESOLVE_ASSIGNING)) && >+ !(cx->fp && cx->fp->regs && (JSOp)*cx->fp->regs->pc == JSOP_BINDNAME) && Missed this -- if you are called from the interpreter then you're guaranteed to have non-null cx->fp and cx->fp->regs. If not, then you need to test. Just confirming (I think). r=me again -- no need to over-optimize the rf in js_Interpret, we can let it pop out when profiling (but if you have already proved it's noise, even better ;-). /be
Comment 9•16 years ago
|
||
Comment on attachment 339865 [details] [diff] [review] v2 sr=bzbarsky on the dom changes
Attachment #339865 -
Flags: superreview+
Assignee | ||
Comment 10•16 years ago
|
||
(In reply to comment #8) > Missed this -- if you are called from the interpreter then you're guaranteed to > have non-null cx->fp and cx->fp->regs. If not, then you need to test. Just > confirming (I think). Yes. When called from outside the interpreter, cx->fp can be null. When called from a JSNative, cx->fp->regs can be null. Pushed http://hg.mozilla.org/mozilla-central/rev/17c60e5a30c1
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•