81341 - Crash from infinite recursion/ stack overflow

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Aaron Leventhal]

Mozilla crashes in the following infinite loop. 

The stack:
---------
Here's my stack, the calls start at the bottom and move up.

.
.
.
nsXULElement::QueryInterface
XPCConvert::JSObject2NativeInterface
nsXPConnect::WrapJSAggregatedToNative
nsBindingManager::GetBindingImplementation
nsXULElement::QueryInterface
XPCConvert::JSObject2NativeInterface
nsXPConnect::WrapJSAggregatedToNative
nsBindingManager::GetBindingImplementation
nsXULElement::QueryInterface
XPCConvert::JSObject2NativeInterface
nsXPConnect::WrapJSAggregatedToNative
nsBindingManager::GetBindingImplementation
nsXULElement::QueryInterface
nsQueryInterface::operator()
nsCOMPtr<nsIAccessible>::assign_from_helper
nsCOMPtr<nsIAccessible>::operator=(const nsQueryInterface & {...}) line 615
nsDOMTreeWalker::GetAccessible() line 662
nsDOMTreeWalker::GetFirstChild() line 543 + 8 bytes


What we were doing before crash:
-------------------------------
We're trying to QueryInterface a nsIDOMNode to an nsIAccessible.
The DOM node is for a XUL button. This worked before we merged our branch in.

What happens:
-------------
nsXULElement::QueryInterface falls through until: if (mDocument) {...  }
There it calls GetBindingImplementation.
This calls WrapJSAggregatedToNative
Which always calls JSObjectToNativeInterface
There, inside if (wrappedNative), it calls QI all over again with the same node.


How to duplicate:
----------------
1. You must download the MSAA SDK, and install it. It's at 
http://www.microsoft.com/enable/download/msaa/msaasdk.exe
2. Run Mozilla.
3. Run the Inspect tool that's part of the SDK.
4. Move the mouse cursor over a link in Mozilla. It should crash as above.


Where to set a breakpoint
-------------------------
To see the start of the infinite loop:
nsXPConnect::WrapJSAggregatedToNative seems to be a good place to set a 
breakpoint, before the stack gets filled up.
It seems that doesn't get called until we do our QI for an nsIAccessible.

Comment 1

[John Bandhauer]

arronl: I *really* don't want to install this MSAA SDK. Is there any chance you 
could create some simpler test case that reproduces the problem?

It looks like it's only recurring thorugh four frames. Could you please attach a 
copy of the stack snippet that includes the line numbers in those frames? Visual 
inspection of the code might reveal the problem, but I'd like to know better 
where to look. 

It would also help if you could tell me the interface type it is QIing for if 
you can tell.

Thanks.

Comment 2

[Aaron Leventhal]

Attachment: Here it is, it keeps going after this until it eventually crashes

Comment 3

[Aaron Leventhal]

Longer version of stack attached.

jst, I tried to make a simpler test case, but I think it's no so bad installing 
the SDK. It's not large, and you can uninstall it when you're done.

Comment 4

[Aaron Leventhal]

We're trying to QueryInterface a nsIDOMNode to an nsIAccessible.
The DOM node in question is for a XUL button.

Comment 5

[Stephen Anderson]

I'm getting stack overflows all over the place.  Do you think bug 80883 and this
are the same bug?

Comment 6

[David Bradley]

I think they are different. What I've found so far is that XPConnect finds a 
wrapper but the wrapper's IID is not the one we're looking for, which then 
causes it to QI which starts the recursion. jband, you come up with anything 
more. I'm going to push further, if nothing else it will further my education.

Comment 7

[John Bandhauer]

Gez, You guys told me that aggregation was working. It had to be completely 
busted as far as reflecting the bindings into native code is concerned. Hyatt 
and I worked this out on his blackboard one day. It *required* that we detect 
the 'outer' case when looking for a native to reflect a JSObject and *force* the 
building of a wrappedJS. It *used* to be that we needed to avoid returning the 
native underlying DOM object we could find via its 'nsISupport-ness'. With the 
DOM using xpconnect we need to extend that forcing case to the lookup of 
wrappedNatives.

Some XUL hacker should be able to build a pretty simple native test case. All 
you need to do is use XBL to bind interface nsIFoo to some XULNode and then use 
some native code to QI the XULNode to nsIFoo.

I'll attach a patch that I *expect* will fix the problem. I have not tested it.

Comment 8

[John Bandhauer]

Attachment: proposed fix to force creation of wrappedJS if aggregating

Comment 9

[John Bandhauer]

Silence. Reassign to aaronl for testing/comment/review.

Comment 10

[Aaron Leventhal]

I have applied jband's patch. It indeed fixes my problem. Great!

I don't know that I'm the right person to r=

Comment 11

[John Bandhauer]

aaronl: thanks for testing.

Comment 12

[Johnny Stenback (:jst)]

I suspect that the check if the JSObject has 'nsISupportness' really isn't
neccessary now since only xpinstall uses the old nsIScriptObjectOwner interface,
and I doubt we care if aggregation works with those objects, but leaving the
check in the code doesn't really hurt anything so we might as well leave it in
there for now.

r/sr=jst

Comment 13

[David Bradley]

=r dbradley

Comment 14

[John Bandhauer]

fix checked in

Comment 15

[Phil Schwartau]

Based on Aaron's comment at 2001-05-20 01:24 above, marking VERIFIED -