66559 - plugin crash because nsJSUtils::nsGetNativeThis can return non-DOM node

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[John Bandhauer]

I saw a report in the plugin newsgroup about a crash and investigated. The core 
of the problem I found is that callers to nsJSUtils::nsGetNativeThis do a 
plain C cast to their nsIDOM derivitive a choice. But nsJSUtils::nsGetNativeThis 
as written only verifies that the object returned is an nsISupports. In the 
plugin case we have code in nsHTMLEmbedElement::GetScriptObject that will insert 
an xpconnect wrapped native as the JSObject and set the original DOM wrapper as 
that object's proto. nsJSUtils::nsGetNativeThis looks for a native nsISupports 
and finds it... but the native object is the xpconnect wrapper not a DOM node. 
So, nsIDOMNode calls on the object cause a crash because they call the wrong 
methods on the wrong interface.

This could be fixed (perhaps) by doing a QI check in the loop in 
nsJSUtils::nsGetNativeThis and only stopping at something that implements 
nsIDOMNode. This is still iffy since we are not returning *that* addref'd 
pointer, so we could get bit by aggregation cases, etc. The reality is that the 
callers to nsJSUtils::nsGetNativeThis should not be doing a cast. They should 
ask for an addreff'd pointer and nsJSUtils::nsGetNativeThis should QI for 
*something* nsIDOM-ish.

Inasmuch as we are working to get rid of the callers to 
nsJSUtils::nsGetNativeThis we may just want to live with this bug as is for now.

Additional info...

The info on steps to reproduce and a stack (somewhat different from mine) comes 
from the mozilla plugins newgroup:
news://news.mozilla.org/94ocvo%24dk92%40secnews.netscape.com
news://news.mozilla.org/3A6FEEA0.3080401%40superscape.com

The stack I see is:

nsXPCWrappedNativeClass::Release(nsXPCWrappedNativeClass * const 0x04882080) 
line 61
ElementGetAttribute(JSContext * 0x027a0e70, JSObject * 0x013a0b10, unsigned int 
0x00000001, long * 0x0132dfbc, long * 0x0012dd40) line 206 + 32 bytes
js_Invoke(JSContext * 0x027a0e70, unsigned int 0x00000001, unsigned int 
0x00000000) line 784 + 23 bytes
js_Interpret(JSContext * 0x027a0e70, long * 0x0012ea8c) line 2614 + 15 bytes
js_Invoke(JSContext * 0x027a0e70, unsigned int 0x00000001, unsigned int 
0x00000002) line 801 + 13 bytes
js_InternalInvoke(JSContext * 0x027a0e70, JSObject * 0x01319808, long 
0x01319820, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x0012ec24, 
long * 0x0012ebb4) line 873 + 20 bytes
JS_CallFunctionValue(JSContext * 0x027a0e70, JSObject * 0x01319808, long 
0x01319820, unsigned int 0x00000001, long * 0x0012ec24, long * 0x0012ebb4) line 
3268 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0279f060, void * 0x01319808, 
void * 0x01319820, unsigned int 0x00000001, void * 0x0012ec24, int * 0x0012ec20, 
int 0x00000000) line 931 + 33 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x041b3184) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x034a1bb0, 
nsIDOMEvent * 0x041b3184, nsIDOMEventTarget * 0x034a1d08, unsigned int 
0x00000001, unsigned int 0x00000007) line 838 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x0285aab0, nsEvent * 
0x0012f4d0, nsIDOMEvent * * 0x0012f45c, nsIDOMEventTarget * 0x034a1d08, unsigned 
int 0x00000007, nsEventStatus * 0x0012f514) line 1720 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x034a1d00, nsIPresContext * 
0x0285aab0, nsEvent * 0x0012f4d0, nsIDOMEvent * * 0x0012f45c, unsigned int 
0x00000001, nsEventStatus * 0x0012f514) line 3455
PresShell::HandleDOMEventWithTarget(PresShell * const 0x0285b760, nsIContent * 
0x034a1d00, nsEvent * 0x0012f4d0, nsEventStatus * 0x0012f514) line 4961 + 39 
bytes
nsPopupSetFrame::OnCreate(nsIContent * 0x034a1d00) line 635 + 38 bytes
nsPopupSetFrame::CreatePopup(nsPopupSetFrame * const 0x011af750, nsIContent * 
0x0354f360, nsIContent * 0x034a1d00, int 0x000001cb, int 0x00000137, const 
nsString & {...}, const nsString & {...}, const nsString & {...}) line 459 + 15 
bytes
nsPopupSetBoxObject::CreatePopup(nsPopupSetBoxObject * const 0x041a67a0, 
nsIDOMElement * 0x0354f364, nsIDOMElement * 0x034a1d04, int 0x000001cb, int 
0x00000137, const unsigned short * 0x0012fb4c, const unsigned short * 
0x0012f9d0, const unsigned short * 0x0012f924) line 139 + 109 bytes
XULPopupListenerImpl::LaunchPopup(int 0x000001cb, int 0x00000137) line 798
XULPopupListenerImpl::sTooltipCallback(nsITimer * 0x041ef130, void * 0x0354f2f0) 
line 837
nsTimer::Fire() line 194 + 17 bytes
FireTimeout(HWND__ * 0x00000000, unsigned int 0x00000113, unsigned int 
0x00004e9b, unsigned long 0x0251f630) line 78
USER32! 77e7185c()
nsAppShellService::Run(nsAppShellService * const 0x00ae5350) line 408
main1(int 0x00000001, char * * 0x00a25fb0, nsISupports * 0x00000000) line 978 + 
32 bytes
main(int 0x00000001, char * * 0x00a25fb0) line 1272 + 37 bytes
mainCRTStartup() line 338 + 17 bytes


It goes wrong where ElementGetAttribute is trying to call a method on an 
nsIDOMNode interface and ends up calling the wrong method on an 
xpcwrappernative.

I suspect that the crash point of the stack in the newgroup posting is the 
result of memory corruption from a previous call to the wrong method due to 
wrong interface pointers. Of course, it might be something else :)

The specific place where we are hitting this is when a tooltip window tries to 
popup over a node that is a scriptable plugin. The core problem might bite 
elsewhere too.

[cc [e-mail] because he may have (potentially) unexplained similar bugs 
on his list]

Comment 1

[Johnny Stenback (:jst)]

I see only generated glue code ever calling nsJSUtils::nsGetNativeThis() and all
that code is going away (and so could nsJSUtils::nsGetNativeThis) so I won't
spend any time fixing this now, moving to Future to keep this on my list so that
I can verify that this doesn't happen after the glue code is gone.

Comment 2

[Johnny Stenback (:jst)]

Fixed by the xpcdom landing.

Comment 3

[Johnny Stenback (:jst)]

... marking fixed.

Comment 4

[Prashant Desale]

John [jband], would you please like to verify this one ?

Comment 5

[John Bandhauer]

verified