13125 - using wrapped native as a __proto__ is funky

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[John Bandhauer]

Christine Begle wrote:
>
> what should happen in this case?  weird stuff happening -- settting
> a JS object's __proto__ to an xpconnect object creates an object
> whose functions and attributes are created but not initialized.
>
> js> Enumerate(child)
> Properties of object [xpconnect wrapped nsIXPCTestChild2]
> QueryInterface: function
> ChildAttribute: xpcTestChild2 attribute
> ChildMethod: function
> js> c = {}
> [object Object]
>
> js> c.__proto__ = child
> [xpconnect wrapped nsIXPCTestChild2]
> js> Enumerate(c)
> Properties of object [object Object]
> QueryInterface: undefined
> ChildAttribute: undefined
> ChildMethod: undefined

Well, Liveconnect crashes when trying to do roughly the same thing:

js> o = new java.lang.String("foo")
foo
js> o.hashCode()
143856
js> c = new Object()
[object Object]
js> c.__proto__ = o
foo
js> c.hashCode()

[crash here]

This looks releated to http://bugzilla.mozilla.org/show_bug.cgi?id=12367

Though...

in liveconnect:

JSClass JavaObject_class = {
    "JavaObject", JSCLASS_HAS_PRIVATE,
    NULL, NULL, NULL, NULL,
    NULL, NULL, JavaObject_convert, JavaObject_finalize,
    JavaObject_getObjectOps,
};

in xpconnect:

JSClass WrappedNative_class = {
    "XPCWrappedNative", JSCLASS_HAS_PRIVATE,
    JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,
    JS_EnumerateStub, JS_ResolveStub,
    WrappedNative_Convert,
    WrappedNative_Finalize,
    /* Optionally non-null members start here. */
    WrappedNative_getObjectOps,         /* getObjectOps */
    nsnull,                             /* checkAccess  */
    nsnull,                             /* call         */
    nsnull,                             /* construct    */
    nsnull,                             /* xdrObject    */
    WrappedNative_ClassHasInstance      /* hasInstance  */
};

Should liveconnect be setting the stubs just in case?

I have a fix for the engine here where is assumes that clasp->resolve != NULL:

resolve = clasp->resolve;
if (resolve != JS_ResolveStub) {
...becomes...
if (resolve && resolve != JS_ResolveStub) {

I'll check that in for safety sake.

Nevertheless, This looks like another collision of jsclass and JSOps.
js_GetProperty is looking for the property in the objects it finds in the target
object's proto chain. It uses _js_LookupProperty to do this and that uses
clasp->resolve. I suppose that I need to figure out this resolve stuff and
implement it in xpconnect rather then letting the JS_ResolveStub try to handle
it.

Should the engine try to use the JSOp LookupProperty rather than clasp->resolve
in this case?

Comment 1

[Scott Furman]

I added a lengthy comment when this bug was first reported,
but Bugzilla seems to have hiccupped, since I don't see it there.  What I
said was something like this:

    The JSObjectOps for native objects (js_ObjectOps) are
    implemented on top of the JSClass-based callbacks.  The
    layering is not strict, but I think that the only exceptions to this
    generalization are the conversion and finalization JSClass
    callbacks.  In the case of LiveConnect'ed host objects, which
    can not be decorated with user properties, the JSObjectOps
    are wholly replaced with new versions that should never call
    the JSClass methods on one of these host objects.

    Since none of the JSClass getter/setter/resolver, etc.
    callbacks should ever be called on a JavaObject instance, I
    regard the NULL placeholders in the JavaObject JSClass as a
    sort of assertion that LiveConnect'ed objects are not being
    treated by engine code as if they were native objects, as was
    recently discovered in bug 12367. So, I would rather that the
    NULL-check did not get added.

Comment 2

[leger]

Moving all XPConnect QA contact to rginda

Comment 3

[John Bandhauer]

Currently the native JS functions for the wrappers just try to get the "native 
this" by looking at the JSObject's private. I can add code to do some JSClass 
checking and walk the proto chain to find the correct "native this" if the 
wrapper is used as a proto for some other object. I'll investigate. I think 
we'll need this for the DOM conversion.

Comment 4

[Brendan Eich [:brendan]]

The old libmocha/lm_win.c code had to walk the proto-chain looking for the right 
class of private data, too.  It's a recurring pattern.

/be

Comment 5

[John Bandhauer]

Attachment: proposed fix

Comment 6

[Brendan Eich [:brendan]]

Patch looks good to me, r=[e-mail].  Does it cause extra properties 
to be created in the typical XPConnect use cases (where the xpc object is not a 
prototype)?

/be

Comment 7

[John Bandhauer]

fix checked in.

No, The resolve hook does not get called at all in our regular non-__proto__ 
cases. I had not understood this before.

Thanks!

Comment 8

[Brendan Eich [:brendan]]

That's because you have your own OBJ_LOOKUP_PROPERTY as well as OBJ_GET_PROPERTY
ops implementations.  So when dealing directly (not via a prototype) with an xpc
object, control vectors directly to those ops.

But if you get into one of the js_ObjectOps ("native") ops, say via jso.foo
where jso is a vanilla JS object whose __proto__ == xpc, *and* you've contrived
to make xpc's object-ops look "native" according to OBJ_IS_NATIVE, then
js_LookupProperty at least is a "roach motel" -- control flows in, but never
back out to your OBJ_LOOKUP_PROPERTY impl.

That's not unreasonable in light of what OBJ_IS_NATIVE originally meant: that
obj->map->ops == &js_ObjectOps.  It allows js_LookupProperty to iterate rather
than do a tail-recursive search up an all-native prototype chain.

This is what I was trying to say on the phone last night, but I got distracted!

Anyway, glad this is fixed.  Beard, go nuts.

/be