57370 - xpconnect needs support for wrapJSAggregatedToNative

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P3)

Description

[John Bandhauer]

See hyatt's plan for use of this in XBL: 
news://news.mozilla.org/39EEBCCE.8090800%40netscape.com

An email exchange...

John Bandhauer wrote:
>
> David Hyatt wrote:
> >
> > Suppose we have an element that impls interfaces A and B.  The XBL
> > implements C.  Let's say someone asks for C, so I do a WrapJS on it and
> > make a wrapper and hand that back.
> >
> > My question then is this.  If C inherits from the other interface B that
> > my native object already implements, will the wrapper return itself when
> > B is asked for because I already did a WrapJS for C?  In other words,
> > when you call WrapJS on an object, does it add only C to its list of
> > known interfaces, or will it also add all interfaces C inherits from?
> >
> > I'm worrying about a problem where you could have a B ptr handed back by
> > the native XUL element, you QI to C, which gives you back a ptr to the
> > wrapper, and you then QI back to B and still get the wrapper.
> >
> > Tell me I'm ok! :)
> >
> > dave
> > (hyatt@netscape
> 
> Yeah that is a problem. The wrapper has a schecme to walk its own
> inheritence tree looking for matches. I noticed last night that
> this was a bit broken and I worked on fixing it. Current code is
> at:
> 
> 
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.c
pp#303
> 
> I have a plan. xpcwrappedJS will get a new method called
> aggregatedQueryInterface.
> 
> the wrapper's QI will figure out up front if it is part of an
> aggregation with you. If so it will delegate to you right off,
> else it does its usual thing. If you decide in your QI to
> delegate to it then you call aggregatedQI rather than QI and good
> stuff happens. So, your QI always gets first shot and no call
> cycles.
> 
> I thought more about the whole scheme and I think we're going to
> be OK. I'll add a nsIXPConnect::wrapJSAggregatedToNative
> (or somesuch) and I'll mark the wrapper with a flag (rather than
> go look at the JSObject's class flags each time I use it). The
> wrapping process will be changed as we decided. The autowrap
> process will not need to change at all because when autowrap
> calls your QI on the native you can build a wrapper on the
> JSObject and return it as appropriate. I think it will all just
> work.
> 
> There is the issue that these wrappers will not be doublewrapped
> when passed back into JS. So, some xpconnect magic on the JS side
> won't work; e.g. 'foo instanceof Components.interfaces.nsIFoo'.
> That is the curse of the DOM object in xpconnect.

Comment 1

[John Bandhauer]

I have the xpconnect end mostly coded. I'm thinking about some things I may not 
have done 'right'...

I set it up so that the wrapper does not need an owning reference on the native 
object. It gets that from the JSObject private when it needs to call it. I'm not 
sure I like that. The owning reference might be bad, but as it is the 
wrapper has to go through a bunch of stuff to get the native when it needs it. 
The wrapper chain is rooting the JSObject anyway. Would it be bad to have it 
also have an owning reference to the native object? This gives us yet another 
cycle in the 'live' case. But, when the native wants to die it can break the 
cycle by doing what it will already need to do: releasing its reference to the 
root wrapper. I'm pretty much convinced that the owning reference in the 
wrapper on the native is a good thing. I'm inclined to code that. Thoughts?

Also, what I have so far assumes that you are never going to call 
nsIXPConnect::wrapJSAggregatedToNative for a JSObject that might also have been 
previously wrapped with a 'plain' wrapper. This should be OK since xpconnect 
autowrap stuff won't actually build a wrapper around a JSObject with a native 
private (i.e. the special flag set in the JSClass).

Comment 2

[John Bandhauer]

Attachment: first draft of a fix

Comment 3

[John Bandhauer]

Attachment: second draft of fix (though of a stupid bug while writing post!)

Comment 4

[John Bandhauer]

I attached a draft patch. This is a first draft for hyatt to experiment with. 
I'm sure it needs cleanup.

Make an aggregating wrapper with nsIXPConnect:wrapJSAggregatedToNative and pass 
in the nsISupports* of the native 'outer'. That outer will be called via 
delagation for all QI calls on the wrapper. The exception is that the native can 
call QI on the returned wrapper to ask for the nsIXPConnectWrappedJS interface 
without the wrapper delegating to the native (this allows the native to create 
the wrapper and *then* get the interface on the wrapper that it needs to use 
for aggregatedQueryInterface). The native can delegate QI calls to the wrapper's 
aggregatedQueryInterface method. If the wrapper's aggregatedQI method is asked 
for the nsISupports iface then it will return a pointer to the wrapper root. The 
native might find this useful. The native should not pass that on as the 
identify object for the aggregation; i.e. the native's normal QI handing should 
filter out requests for nsISupports (and other interfaces the native does 
natively) before delegating to the wrapper's aggregatedQueryInterface.

This patch has the root wrapper of a wrapper chain holding an owning reference 
on the native 'outer'.

Any attempt to autowrap a JSObject that is already part of one of these 
aggregations will follow the old path of detecting that the JSObject has an 
nsISupports private and just return the result of QI on that private. Since the 
native's QI is capable of determining which ifaces are supposed to result in 
wrapper creation then it can decide to call wrapJSAggregatedToNative or return 
NO_INTERFACE or whatever. We wind in a little deeper, but it works I think.

Let me know how this works.

Comment 5

[John Bandhauer]

Attachment: third draft of fix

Comment 6

[John Bandhauer]

The third draft is the same as the second except it moves the new method on 
nsIXPConnect to the end of the interface rather than the middle so that the 
world does not need to be recompiled. I actually ran mozilla this time :)

Comment 7

[David Hyatt]

Ok, cool!  I'll play with this and let you know how it works.

Comment 8

[David Hyatt]

I applied the patch (just now getting back to working on this) and it doesn't
compile.

xpcwrappedjs.cpp
C:\gecko\mozilla\js\src\xpconnect\src\xpcwrappedjs.cpp(348) : error C2039:
'IsIID' : is not a member of 'nsDerivedSafe<class nsIInterfaceInfo>'
  

Comment 9

[David Hyatt]

Never mind.  I'm an idiot.  It does compile.

Comment 10

[David Hyatt]

a=hyatt.  I'm ready for this, so get an r= when you can, and land this puppy. :)

Comment 11

[Mike McCabe]

r=mccabe.

Comment 12

[John Bandhauer]

fix checked in.

Comment 13

[Phil Schwartau]

Marking Verified -