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)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: jorendorff)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Blocks: 453825
Attached patch v1 (obsolete) — Splinter Review
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)
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 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 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 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+
Attached patch v2Splinter Review
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)
Attachment #339865 - Flags: review?(brendan) → review+
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 on attachment 339865 [details] [diff] [review]
v2

sr=bzbarsky on the dom changes
Attachment #339865 - Flags: superreview+
(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
Depends on: 457663
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: