400793 - Need JS_AlreadyHasOwnProperty (UCProperty, Element)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 400705 comment 9 and bug 400705 comment 10.

Comment 1

[Brendan Eich [:brendan]]

Attachment: WIP, handing off to crowder

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Can we start allowing JSAPI functions to take jsvals instead of creating *{Property,Element} functions?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Can we start allowing JSAPI functions to take jsvals instead of creating
> *{Property,Element} functions?

That would hurt XBL because it keeps a const PRUnichar *mName around to pass. See the other bug.

I think we're not ready to force that divergence into the JS API. The cost is pretty small (someone measure), carry the torch a bit longer.

/be

Comment 4

[Brian Crowder]

Attachment: v2, adds header declarations and finishes !OBJ_IS_NATIVE case

bz: does this fit your use-case?  Can you apply it and see if it does what you require?

Comment 5

[Brian Crowder]

Attachment: cleanup patch-file

Just tidying up the patchfile a bit.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 286047 [details] [diff] [review]
cleanup patch-file

>Index: jsapi.c
>+static JSBool
>+AlreadyHasOwnPropertyHelper(JSContext *cx, JSObject *obj, jsid id,
>+                            JSBool *foundp)
...
>+        ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);

This should just be:

if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop))
    return JS_FALSE;

>+        *foundp = (prop != NULL);

...or you could be using an uninitialized prop here. Also, I think this could just be:

*foundp = obj == obj2;

Also, you need:

if (prop)
    OBJ_DROP_PROPERTY(cx, obj2, prop);

Comment 7

[Boris Zbarsky [:bzbarsky]]

Assuming that XPCWrappedNatives are OBJ_IS_NATIVE, that looks like it'll do what I want.

Comment 8

[Brian Crowder]

Attachment: v2.1, addressing mrbkap's feedback

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 286049 [details] [diff] [review]
v2.1, addressing mrbkap's feedback

Nit: order of prototypes in jsapi.h does not match corresponding definition order in jsapi.c. I put the JS_AlreadyHas* before the JS_Has* jazz in jsapi.c, FWIW.

>+        if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop))
>+            return JS_FALSE;
>+        *foundp = obj == obj2;

Nit: house style overparenthesizes bitwise, equality, and relational expressions on the RHS of assignment expressions.

>+

Nit: no need for a blank line here, really.

>+        if (prop)
>+            OBJ_DROP_PROPERTY(cx, obj2, prop);
>+        return JS_TRUE;
>+    }
>+
>+    scope = OBJ_SCOPE(obj);
>+    JS_LOCK_SCOPE(cx, scope);

Not sure who suggested this change from my WIP patch, but you need JS_LOCK_OBJ(cx, obj); scope = OBJ_SCOPE(obj) in that order -- see jslock.c:js_LockObj.

/be

Comment 10

[Brian Crowder]

Attachment: v2.2, addressing brendan's feedback

> >+    scope = OBJ_SCOPE(obj);
> >+    JS_LOCK_SCOPE(cx, scope);
> 
> Not sure who suggested this change from my WIP patch, but you need
> JS_LOCK_OBJ(cx, obj); scope = OBJ_SCOPE(obj) in that order -- see
> jslock.c:js_LockObj.

Original WIP patch did:

+    JS_LOCK_SCOPE(cx, scope); /* uninited scope */
+    scope = OBJ_SCOPE(obj);

So the suggestion was my own.  The old AlreadyHasOwnProperty() routine (any concern about the naming here, for the new functions?  Especially the *Helper function that doesn't, in fact, help this old routine?) in jsapi.c did this:

    JS_LOCK_OBJ(cx, obj);
    scope = OBJ_SCOPE(obj);
    sprop = SCOPE_GET_PROPERTY(scope, ATOM_TO_JSID(atom));
    JS_UNLOCK_SCOPE(cx, scope);

Is that correct?  It looks unbalanced to me, but I will assume it is correct and do similar in the attached revision.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> Is that correct?  It looks unbalanced to me, but I will assume it is correct
> and do similar in the attached revision.

JS_UNLOCK_OBJ calls js_UnlockObj, which is a thin wrapper around js_UnlockScope(cx, OBJ_SCOPE(obj)).

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 286178 [details] [diff] [review]
v2.2, addressing brendan's feedback

Thanks, r/a=me (after M9). sorry about the WIP glitch with SCOPE instead of OBJ on the front side. Yeah, as we discussed on IRC, SM uses the lower layer JS_UNLOCK_SCOPE to unlock when there's a scope = OBJ_SCOPE(obj) following a JS_LOCK_OBJ, just to avoid reloading scope.

/be

Comment 13

[Reed Loden [:reed]]

Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.364; previous revision: 3.363
done
Checking in js/src/jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.164; previous revision: 3.163
done