96725 - calling QI on XBL objects causes crash

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Alex Fritze]

QI'ing an xbl object from js and then calling a method on it leads to a stack 
overflow. The xpc wrapper seems to get messed up somewhat. This is the stack 
trace for the attached testcase:

[...]
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 1884 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x01248568, JSObject * 0x0117ad68, unsigned int 
0, long * 0x023f85c4, long * 0x00033adc) line 1252 + 14 bytes
js_Invoke(JSContext * 0x01248568, unsigned int 0, unsigned int 2) line 807 + 23 
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x02439dc8, 
nsXPCWrappedJS * 0x011990e0, unsigned short 4, const nsXPTMethodInfo * 
0x0247b334, nsXPTCMiniVariant * 0x0003406c) line 1023 + 21 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x011990e0, unsigned short 4, 
const nsXPTMethodInfo * 0x0247b334, nsXPTCMiniVariant * 0x0003406c) line 427
PrepareAndDispatch(nsXPTCStubBase * 0x011990e0, unsigned int 4, unsigned int * 
0x0003411c, unsigned int * 0x0003410c) line 100 + 31 bytes
SharedStub() line 124
XPTC_InvokeByIndex(nsISupports * 0x011990e0, unsigned int 4, unsigned int 0, 
nsXPTCVariant * 0x00034298) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 1884 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x01248568, JSObject * 0x0117ad68, unsigned int 
0, long * 0x023f85bc, long * 0x000344d0) line 1252 + 14 bytes
js_Invoke(JSContext * 0x01248568, unsigned int 0, unsigned int 2) line 807 + 23 
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x02439dc8, 
nsXPCWrappedJS * 0x011990e0, unsigned short 4, const nsXPTMethodInfo * 
0x0247b334, nsXPTCMiniVariant * 0x00034a60) line 1023 + 21 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x011990e0, unsigned short 4, 
const nsXPTMethodInfo * 0x0247b334, nsXPTCMiniVariant * 0x00034a60) line 427
PrepareAndDispatch(nsXPTCStubBase * 0x011990e0, unsigned int 4, unsigned int * 
0x00034b10, unsigned int * 0x00034b00) line 100 + 31 bytes
SharedStub() line 124
XPTC_InvokeByIndex(nsISupports * 0x011990e0, unsigned int 4, unsigned int 0, 
nsXPTCVariant * 0x00034c8c) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode 
CALL_METHOD) line 1884 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x01248568, JSObject * 0x0117ad68, unsigned int 
0, long * 0x023f85b4, long * 0x00034ec4) line 1252 + 14 bytes
[...]

Comment 1

[Alex Fritze]

Attachment: testcase (zipped xml/css/xbl)

Comment 2

[David Hyatt]

Argh!  People everywhere are trying to use this feature! :)

jband, here's another fun one.

Comment 3

[John Bandhauer]

OK. This can be fixed from the xpconnect side.

Oh boy, ascii art time!


 DOM Node             JSObject of WrappedNativeProto
    ^                        ^ 
    |                        |              <- JSObject proto chain
    |                  XBL proto JSObject
    |                        ^
    |                        | 
  WrappedNative <--- JSObject of WrappedNative  <- JS callers see this object
                        ^
                        |
                     WrappedJS built by XBL and QI aggregated to DOM node
                            

In the normal xpconnect case the wrappedNative methods are found on the 
(potentially) shared WrappedNativeProto's JSObject. This object is in the 
JS proto chain of the WrappedNative's own JSObject. This allows for reasonable 
factoring and also allows for shadowing those methods by inserting a new 
JSObject into the JS proto chain (as XBL does).

The WrappedNativeProto represents all the interfaces that all the objects of a 
given 'class' claim to implement. The 'class' is defined as those object that 
return the same nsIClassInfo object when asked. XPConnect gets the list of 
expected interface iids from the nsIClassInfo and builds a WrappedNativeProto to 
represent those interfaces. If you QI a wrapper for an iid that is a member of 
that interface set, then xpconnect will verify that the actual native object 
actually does implement that interface by calling QueryInterface on that object. 
But nothing really special happens.

However, if JS code QIs the wrapper for an interface that is not a member of 
that shared set, then xpconnect will QI the underlying native and if that 
succeeds then xpconnect will build a mutated interface set that is based on the 
set of its WrappedNativeProto, but adds the additional interface. This mutated 
set will be associated with that particular wrapper instance. XPConnect will 
*not* mutate the *shared* set simply because one instance was QI'd for an 
interface.

So, with a mutated set on the instance any method lookup for an interface that 
is not also in the shared set will have to return a property on the instance's 
JSObject - and not on the shared JSObject. And *that* method would then shadow 
any method in the xbl proto object.

The thing that goes wrong in this test case is caused by the fact that the QI 
must return the wrappedJS that was created by XBL and aggregated to the target 
DOM node. Nothing bad happens at that point. But when the method is actually 
called the flow goes from the WrappedNative to the WrappedJS which then does a 
lookup for the method on the same target JSObject and calls that method... This 
recurs until the stack overflows.

The fix is to detect at QI time that the interface pointer that the object 
wrapped by the wrappedNative (the DOM node in this case) returns in the QI call 
is in fact a wrappedJS and that *its* JSObject is the very same JSObject that 
belongs to our WrappedNative. When xpconnect sees this case, instead of mutating 
the wrapper instance's interface set, xpconnect just says "sure this wrapper 
implements that interface". Then later, when JS code tries to make a call and 
the engine does a property lookup for the method in question, xpconnect will 
*not* reflect a method on the target JSObject (because that method is not a part 
of its interface set). Instead, the property lookup will proceed up the JS 
proto chain to the XBL prototype as the XBL system expects it. This then gives 
the same behavior that would happen if QI had not been called from JS.

I have a patch for review. I'll attach it (and take this bug).

Comment 4

[John Bandhauer]

Attachment: proposed fix

Comment 5

[John Bandhauer]

My fix could *certainly* use additional testing to make sure it does not trip up 
any other legitimate double-wrapping sorts of cases.

Comment 6

[David Hyatt]

r/sr=hyatt.  Nice ASCII art. :)

Comment 7

[David Bradley]

psuedo diff:
        if(!mTearOff || mTearOff->GetInterface() != mInterface)
+       {
+            mTearOff = 0;
             return NS_FAILED(rv) ? rv : NS_ERROR_UNEXPECTED;
+       }

I'm wondering if this would be appropriate, since this would be the state in a
normal failure of CanCallNow.

Comment 8

[John Bandhauer]

That would be: mTearOff = nsnull;
Sure. I can make that change. I don't think it much matters in the current code 
since if CanCallNow fails no one is going to do much with mTearOff. But, I've 
added that change to my tree. Anything else?

Comment 9

[David Bradley]

yes, nsnull.

r=dbradley

Comment 10

[Brendan Eich [:brendan]]

s/implmenting/implementing/ in the comment.

I think we can take this.  It makes things better by not crashing, and I don't
see it making things worse in as severe a way as causing a crash.

/be

Comment 11

[John Bandhauer]

fix checked in