100773 - infinite loop in profile manager when 2 XBL defined interfaces are used

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[John Gaunt (redfive)]

I am seeing a loop that eventually causes an unhandled memeory exception in the
profile manager. I have tracked it down to a call in
nsAccessibilityService::GetAccessibleFor(), but haven't had luck in determining
why it is looping in the JS. The problem _may_ be in some JS or XUL
implementation dealing with the QI of a XUL object to an interface defined in
XBL. Need to talk with Hyatt and one of the JS folks.

Comment 1

[John Gaunt (redfive)]

Stack trace. Repeats to the top of the buffer.

NTDLL! 77fb330a()
NTDLL! 77fa6b46()
NTDLL! 77fcac04()
_heap_alloc_base(unsigned int 80) line 200
_heap_alloc_dbg(unsigned int 39, int 1, const char * 0x00000000, int 0) line 378
+ 9 bytes
_nh_malloc_dbg(unsigned int 39, int 0, int 1, const char * 0x00000000, int 0)
line 248 + 21 bytes
malloc(unsigned int 39) line 130 + 21 bytes
PR_Malloc(unsigned int 39) line 54 + 10 bytes
nsID::ToString() line 114 + 8 bytes
xpc_NewIDObject(JSContext * 0x0111edc8, JSObject * 0x029af698, const nsID &
{...}) line 969 + 9 bytes
nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(XPCCallContext & {...},
JSObject * 0x029af698, const nsID & {...}) line 266 + 17 bytes
nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJSClass * const
0x02a51240, nsXPCWrappedJS * 0x02a530e8, const nsID & {...}, void * *
0x00031950) line 395 + 25 bytes
nsXPCWrappedJS::AggregatedQueryInterface(nsXPCWrappedJS * const 0x02a530ec,
const nsID & {...}, void * * 0x00031950) line 63
nsBindingManager::GetBindingImplementation(nsBindingManager * const 0x027dcc78,
nsIContent * 0x028980f0, const nsID & {...}, void * * 0x00031950) line 1411 + 40
bytes
nsXULElement::QueryInterface(nsXULElement * const 0x028980f0, const nsID &
{...}, void * * 0x00031950) line 804 + 47 bytes
XPTC_InvokeByIndex(nsISupports * 0x028980f0, unsigned int 0, unsigned int 2,
nsXPTCVariant * 0x00031940) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 1952 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x0111edc8, JSObject * 0x029af698, unsigned int 1,
long * 0x02b19104, long * 0x00031b78) line 1254 + 14 bytes
js_Invoke(JSContext * 0x0111edc8, unsigned int 1, unsigned int 2) line 807 + 23
bytes
js_InternalInvoke(JSContext * 0x0111edc8, JSObject * 0x029af698, long 44940056,
unsigned int 0, unsigned int 1, long * 0x00031cc4, long * 0x00031ce0) line 899 +
20 bytes
JS_CallFunctionValue(JSContext * 0x0111edc8, JSObject * 0x029af698, long
44940056, unsigned int 1, long * 0x00031cc4, long * 0x00031ce0) line 3362 + 31 bytes
nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(XPCCallContext & {...},
JSObject * 0x029af698, const nsID & {...}) line 270 + 28 bytes
nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJSClass * const
0x02a51240, nsXPCWrappedJS * 0x02a530e8, const nsID & {...}, void * *
0x000320e0) line 395 + 25 bytes
nsXPCWrappedJS::AggregatedQueryInterface(nsXPCWrappedJS * const 0x02a530ec,
const nsID & {...}, void * * 0x000320e0) line 63
nsBindingManager::GetBindingImplementation(nsBindingManager * const 0x027dcc78,
nsIContent * 0x028980f0, const nsID & {...}, void * * 0x000320e0) line 1411 + 40
bytes
nsXULElement::QueryInterface(nsXULElement * const 0x028980f0, const nsID &
{...}, void * * 0x000320e0) line 804 + 47 bytes
XPTC_InvokeByIndex(nsISupports * 0x028980f0, unsigned int 0, unsigned int 2,
nsXPTCVariant * 0x000320d0) line 139
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 1952 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x0111edc8, JSObject * 0x029af698, unsigned int 1,
long * 0x02b190f8, long * 0x00032308) line 1254 + 14 bytes
js_Invoke(JSContext * 0x0111edc8, unsigned int 1, unsigned int 2) line 807 + 23
bytes
js_InternalInvoke(JSContext * 0x0111edc8, JSObject * 0x029af698, long 44940056,
unsigned int 0, unsigned int 1, long * 0x00032454, long * 0x00032470) line 899 +
20 bytes
JS_CallFunctionValue(JSContext * 0x0111edc8, JSObject * 0x029af698, long
44940056, unsigned int 1, long * 0x00032454, long * 0x00032470) line 3362 + 31 bytes
...

Comment 2

[John Gaunt (redfive)]

cc-ing jband. I'm told you have handled these infinite QI problems with XUL
before ( or something similar ).

To reproduce I only have to have the MSAA inspect tool open and then start the
browser -- I have to accounts incidentally. So the Profile Manager opens and
withing a second or two it gives an unhandled exception ( when run in the
debugger  otherwise it just sits there chewing up all of the processor ).

The MSAA tools are available in a self extracting installer off of my publish
directory in the accessibility folder ( took me forever to find them on the MS
website ). Or go to http://www.microsoft.com/downloads/searchdl.asp? and search
for Accessiblity.

Comment 3

[Aaron Leventhal]

You don't need to go to all that trouble to get the bug to occur.
All you need to do is apply the following patch, which does uses QueryInterface
to get to both interfaces defined for checkbox. This code should work, but is
causing some deep problems. The problem is most likely because of the fix
introduced with bug 96610 "Multiple XBL interface implementations not allowed by
binding manager".

Index: xbl/src/nsXBLPrototypeHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,v
retrieving revision 1.34
diff -u -r1.34 nsXBLPrototypeHandler.cpp
--- nsXBLPrototypeHandler.cpp	2001/08/17 08:13:13	1.34
+++ nsXBLPrototypeHandler.cpp	2001/09/21 08:23:25
@@ -189,6 +189,10 @@
   }
 }
 
+#include "nsIDOMXULCheckboxElement.h"
+#include "nsIAccessibleProvider.h"
+#include "nsIAccessible.h"
+
 NS_IMETHODIMP
 nsXBLPrototypeHandler::ExecuteHandler(nsIDOMEventReceiver* aReceiver,
                                       nsIDOMEvent* aEvent)
@@ -205,6 +209,15 @@
   nsCOMPtr<nsIContent> content(do_QueryInterface(aReceiver));
   if (isXULKey && content && content.get() != mHandlerElement)
     isReceiverCommandElement = PR_TRUE;
+
+  if (content) {
+    nsCOMPtr<nsIDOMXULCheckboxElement> checkboxElement(do_QueryInterface(content));
+    if (checkboxElement)
+      printf("\nit's a checkbox!\n");
+    nsCOMPtr<nsIAccessibleProvider> accProv(do_QueryInterface(content));
+    if (accProv)  
+      printf("\nit's an accessible provider!\n"); 
+  }
 
   // This is a special-case optimization to make command handling fast.
   // It isn't really a part of XBL, but it helps speed things up.

Comment 4

[Aaron Leventhal]

To clarify, if you use the patch above, you don't need to run the inspect tool
to see the bug. Apply the patch, and then try to use the profile manager's checkbox.

Comment 5

[Aaron Leventhal]

What I don't understand is why clicking on checkboxes only crashes in the
profile manager. Any other checkbox works.

Comment 6

[John Bandhauer]

I think this is independent of bug 96610. I think the cause is that hyatt and I 
are idiots. I'd expect this bug to show up more. I suggest that the place to 
break the cycle is in nsXULElement::QueryInterface's call to 
nsBindingManager::GetBindingImplementation. I think we need to catch recursion 
there - checking for nested calls on the same object for the same iid. The JS 
engine does something similar to protect itself from Resolve recursion. It is 
easier here since we are on one thread and there is only one code block to deal 
with. I can write a patch in the morning.

Comment 7

[Aaron Leventhal]

Perhaps there are several bugs here, but the corruption that occurs seems to
cause a different kind of crash each time for me. The infinite loop variety is
only one of many possibilities. Have you tried to run with my bug-revealing
patch in Visual Studio's debugger?

Comment 8

[John Bandhauer]

Attachment: proposed fix

Comment 9

[John Bandhauer]

What corruption do you see? Where elese does it crash? Blowing the stack might 
have a s tack endpoint in any of many areas - its the stuff further up the stack 
that matters.

The patch I attached works for me. I had to spend some time figuring out why 
this was not a problem in more sorts of cases.

The normal cases are:

1) if QI'd for something we don't implement then ImplementsInterface will fail 
and that's that.

2) if QI'd for something we support but binding not yet made then 
ImplementsInterface succeeds but GetWrappedJS fails because we haven't built it 
yet. We build it and all is well.

3) if QI'd for something we support and the binding is made then we get through 
the above and get to AggregatedQueryInterface. XPConnect finds the wrapper 
without calling methods on the JSObject. All is well.

4) The case that was failing... if QI'd for something we support but have not 
yet made a binding |but the wrappedJS aready exists (for another interface 
binding)|, then we get into the AggregatedQueryInterface call but xpconnect does 
not yet have a wrapper for that *specific* interface and it ends up calling into 
the wrappedJS's QI method - as it should. This recurs back into 
nsXULElement::QueryInterface and so on.

The fix is to protect from such recursion before calling 
GetBindingImplementation.

Right?

Comment 10

[Aaron Leventhal]

the patch fixes my and jgaunt's problems.

Comment 11

[John Gaunt (redfive)]

Yeah, your explanation makes sense and it definitely sounds like that is what
was happening. And as Aaron said, it fixes my problem. When you are in the
office next maybe you can swing by and explain to me how your fix solves that
problem. I almost get it, but there is something I am missing.

Do you want me to assign the bug to you? Or should I just go get r/sr and check
it in?
Thanks

Comment 12

[John Bandhauer]

reassign to me. How about some an r= or two? I'll bug hyatt for an sr=.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 50286 [details] [diff] [review]
proposed fix

Funky struct bracing must go!  Else after return spotted at six o'clock! Otherwise r/sr=[e-mail]

Comment 14

[John Bandhauer]

> Else after return spotted at six o'clock!

Yeah, I get wierd about compacting inline stuff sometimes.

whitespace and reorder change to:

        struct AntiRecursionData {
            nsXULElement* element; 
            REFNSIID iid; 
            AntiRecursionData* next;

            AntiRecursionData(nsXULElement* aElement, 
                              REFNSIID aIID, 
                              AntiRecursionData* aNext)
                : element(aElement), iid(aIID), next(aNext) {};
        };

> Else after return spotted at six o'clock!

I'm not seeing that. The else is legit given that many of the "else if" above do 
not return.

I'll consider brendan's sr= in effect and hold for a r= or two.

Comment 15

[David Hyatt]

It seems like it would be better to do the AntiRecursion stuff inside the
binding manager itself, since GetBindingImplementation is eventually going to be
called by all elements (HTML, XML, and XUL), and that way you don't have to
duplicate this AntiRecursion stuff in each element class.

Also, you can optimize a little more by moving it inside
GetBindingImplementation, since you only need to worry about a recursion guard
if you actually have to call QI.  For cases where there is no binding
implementation, or where the binding implementation does not support the IID,
you don't need the recursion guard, so you can put the check after those cases.

Comment 16

[John Bandhauer]

That's why you make the big bucks Dave! I'll attach a better path. Thanks.

Comment 17

[John Bandhauer]

Attachment: better fix. in binding manager (thanks hyatt)

Comment 18

[David Hyatt]

Comment on attachment 50328 [details] [diff] [review]
better fix. in binding manager (thanks hyatt)

sr=hyatt

Comment 19

[Brendan Eich [:brendan]]

+        //            ... When we detect that we return NS_NOINTERFACE in

+        // the inner call and the outer call will fall through (see below)

+        // and build the new chained wrappedJS for the new iid.

This sentence is not well-formed: either replace the "and" with ", " or strike
"When" and change "and the outer call will fall through" to "and cause the outer
call to fall through".  r=[e-mail] with that nit picked.

/be

Comment 20

[John Bandhauer]

Fix checked in - with improved comment. I probably added some new awkward 
grammatical gyration for which brendan will roast me. But *I* think it is 
clearer. Thanks all.

Comment 21

[David Hyatt]

heheheh.  Thanks for fixing this, jband.

Comment 22

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381