192281 - Error calling IDispatch.setCallback on active-x control

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[Ashish Bhatt]

tested with 20030207

calling winampx.setCallback( ) on the spinner radio active x control throws up
error 
 Error: uncaught exception: [Exception... "Success arg 0
[IDispatch.setCallback]"  nsresult: "0x0 (NS_OK)"  location: "JS frame ::
file:///D:/testcases/Modified_Content/nullsoft/ActiveX/ampxtest.html :: init ::
line 86"  data: no]

calling any other method after this error crashes mozilla

Comment 1

[Adam Lock]

I've seen this myself. CC'ing David since it sounds like a XPC thing.

Comment 2

[David Bradley]

Pretty sure this is mine, I'll take a look. Odd the error result is NS_OK

Comment 3

[Adam Lock]

This is a stack trace for me, from my own version of ampxtest.html which has
been hacked more or less the same as the one in the URL to remove the gecko test
at the top.

OLEAUT32! 77123d9b()
XPCDispObject::Dispatch(XPCCallContext & {...}, IDispatch * 0x012a6848, long
0x0000001a, XPCDispObject::CallMode CALL_METHOD, XPCDispParams * 0x011ee040,
long * 0x0012d454, XPCDispInterface::Member * 0x0129fa94, XPCJSRuntime *
0x00bd9cc0) line 196 + 30 bytes
XPCDispObject::Invoke(XPCCallContext & {...}, XPCDispObject::CallMode
CALL_METHOD) line 380 + 42 bytes
XPC_IDispatch_CallMethod(JSContext * 0x0116e7e8, JSObject * 0x02e08b48, unsigned
int 0x0000000c, long * 0x02e2afd8, long * 0x0012d5e8) line 463 + 14 bytes
js_Invoke(JSContext * 0x0116e7e8, unsigned int 0x0000000c, unsigned int
0x00000000) line 839 + 23 bytes
js_Interpret(JSContext * 0x0116e7e8, long * 0x0012e430) line 2803 + 15 bytes
js_Invoke(JSContext * 0x0116e7e8, unsigned int 0x00000001, unsigned int
0x00000002) line 856 + 13 bytes
js_InternalInvoke(JSContext * 0x0116e7e8, JSObject * 0x02e09100, long
0x02e09108, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x0012e68c,
long * 0x0012e55c) line 931 + 20 bytes
JS_CallFunctionValue(JSContext * 0x0116e7e8, JSObject * 0x02e09100, long
0x02e09108, unsigned int 0x00000001, long * 0x0012e68c, long * 0x0012e55c) line
3431 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0116e770, void * 0x02e09100,
void * 0x02e09108, unsigned int 0x00000001, void * 0x0012e68c, int * 0x0012e690,
int 0x00000000) line 1040 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x02de3558, nsIDOMEvent
* 0x02e4e6d8) line 181 + 77 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x02de3638,
nsIDOMEvent * 0x02e4e6d8, nsIDOMEventTarget * 0x02e41618, unsigned int
0x00000004, unsigned int 0x00000007) line 1216 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x02de34e0,
nsIPresContext * 0x02de68f8, nsEvent * 0x0012f364, nsIDOMEvent * * 0x0012ee50,
nsIDOMEventTarget * 0x02e41618, unsigned int 0x00000007, nsEventStatus *
0x0012f6ec) line 1385 + 36 bytes
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x02dcb9d8,
nsIPresContext * 0x02de68f8, nsEvent * 0x0012f364, nsIDOMEvent * * 0x0012ee50,
unsigned int 0x00000007, nsEventStatus * 0x0012f6ec) line 1930
nsHTMLInputElement::HandleDOMEvent(nsHTMLInputElement * const 0x02dcb9d8,
nsIPresContext * 0x02de68f8, nsEvent * 0x0012f364, nsIDOMEvent * * 0x00000000,
unsigned int 0x00000001, nsEventStatus * 0x0012f6ec) line 1453 + 29 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f364, nsIView * 0x00000000,
unsigned int 0x00000001, nsEventStatus * 0x0012f6ec) line 6211 + 47 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x02df9440, nsEvent *
0x0012f364, nsIFrame * 0x02e3a154, nsIContent * 0x02dcb9d8, unsigned int
0x00000001, nsEventStatus * 0x0012f6ec) line 6180 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsIPresContext * 0x02de68f8,
nsMouseEvent * 0x0012f8f4, nsEventStatus * 0x0012f6ec) line 2866 + 66 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x02de5ba0,
nsIPresContext * 0x02de68f8, nsEvent * 0x0012f8f4, nsIFrame * 0x02e3a154,
nsEventStatus * 0x0012f6ec, nsIView * 0x012bbf98) line 1847 + 23 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f8f4, nsIView * 0x012bbf98,
unsigned int 0x00000001, nsEventStatus * 0x0012f6ec) line 6247 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x02df9444, nsIView * 0x012bbf98,
nsGUIEvent * 0x0012f8f4, nsEventStatus * 0x0012f6ec, int 0x00000000, int &
0x00000001) line 6134 + 25 bytes
nsViewManager::HandleEvent(nsView * 0x011c6d80, nsGUIEvent * 0x0012f8f4, int
0x00000000) line 2210
nsView::HandleEvent(nsViewManager * 0x012d7958, nsGUIEvent * 0x0012f8f4, int
0x00000000) line 304
nsViewManager::DispatchEvent(nsViewManager * const 0x012d7958, nsGUIEvent *
0x0012f8f4, nsEventStatus * 0x0012f7f0) line 1942 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f8f4) line 83
nsWindow::DispatchEvent(nsWindow * const 0x011c6e4c, nsGUIEvent * 0x0012f8f4,
nsEventStatus & nsEventStatus_eIgnore) line 1115 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f8f4) line 1136
nsWindow::DispatchMouseEvent(unsigned int 0x0000012d, unsigned int 0x00000000,
nsPoint * 0x00000000) line 5376 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 0x0000012d, unsigned int
0x00000000, nsPoint * 0x00000000) line 5633
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long
0x009701d4, long * 0x0012fd88) line 4087 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x000f05f2, unsigned int 0x00000202, unsigned int
0x00000000, long 0x009701d4) line 1402 + 27 bytes
USER32! 77d67ad7()
USER32! 77d6ccd4()
USER32! 77d44455()
USER32! 77d44d58()
CWinThread::Run() line 487 + 11 bytes
CWinApp::Run() line 400
AfxWinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00000000, char *
0x00141f14, int 0x00000001) line 49 + 11 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00000000, char * 0x00141f14,
int 0x00000001) line 30
WinMainCRTStartup() line 330 + 54 bytes
KERNEL32! 77e814c7()

Comment 4

[David Bradley]

Hmm, odd, for me this control fails the IObjectSafety test. This control doesn't
support the IObjectSafety interface on my system.

Comment 5

[David Bradley]

I was really hoping we could avoid this issue, but looks like I'm going to have
to bite the bullet. This is due to the fact that currently the IDispatch
wrapping of JSObject is turned off. The code below creates a JS object and then
passes it to a function that takes an IDispatch pointer. It's easy enough to
turn this back on, but the security issue raised in bug 183223 will need to be
addressed.

events = new mozIAmpXEvents();

events.WinampMsgEvent = onWinampMSGEvent;
events.WinampStatus = onWinampStatus;
events.WinampMetaData = onWinampMetaData;

winampx.setCallback( events );

Comment 6

[David Bradley]

Attachment: Fixes several issues for this control

Ok, this opens up the logic to wrap JS Objects with an IDispatch interface. It
also fixes a couple of bugs down stream that occur as well. This doesn't
address the security concerns, though.

Comment 7

[David Bradley]

Comment on attachment 114207 [details] [diff] [review]
Fixes several issues for this control

I want to go ahead and get this patch checked in. There are still security
issues to be fleshed out, but this will get these controls unblocked. We can
reign in things after we determine the proper method of restrictions. I did
check the context being used and it is the proper one. However, I suspect this
may not be the case the control fired and event on a different thread.

Comment 8

[Johnny Stenback (:jst)]

If a control ever ends up executing JS (or touching Mozilla in almost any way)
on a thread other than the main thread, they're out on *really* rugged water to
begin with. Little of what they might do is even remotely thread safe...

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 114207 [details] [diff] [review]
Fixes several issues for this control

+	     IUnknown * pUnknown = nsnull;
+	     if (!XPCConvert::JSObject2NativeInterface(
+		 ccx, 
+		 (void**)&pUnknown, 
+		 obj, 
+		 &NSID_IDISPATCH,
+		 nsnull, 
+		 &err))
	     {
		 // Avoid cleaning up garabage
		 varDest->vt = VT_EMPTY;
		 return JS_FALSE;
	     }
+	     varDest->vt = VT_DISPATCH;
+	     pUnknown->QueryInterface(IID_IDispatch, 
+				      NS_REINTERPRET_CAST(void**,
+							  &varDest->pdispVal));
	 }

Doesn't this leave pUnknown's refcount unbalanced?

Comment 10

[Adam Lock]

Comment on attachment 114207 [details] [diff] [review]
Fixes several issues for this control

Does XPCConvert::JSObject2NativeInterface refcount the returned pUnknown? If
not it needs to be released.

Otherwise r=adamlock

Comment 11

[David Bradley]

Attachment: Same as before but takes care of the leaked referenced

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 114845 [details] [diff] [review]
Same as before but takes care of the leaked referenced

+	     IUnknown * pUnknown = nsnull;
+	     if (!XPCConvert::JSObject2NativeInterface(
+		 ccx, 
+		 (void**)&pUnknown, 
+		 obj, 
+		 &NSID_IDISPATCH,
+		 nsnull, 
+		 &err))
	     {
		 // Avoid cleaning up garabage
		 varDest->vt = VT_EMPTY;
		 return JS_FALSE;
	     }
+	     varDest->vt = VT_DISPATCH;
+	     pUnknown->QueryInterface(IID_IDispatch, 
+				      NS_REINTERPRET_CAST(void**,
+							  &varDest->pdispVal));
+	     NS_IF_RELEASE(varDest->pdispVal);

Um, didn't you mean to release pUnknown there? If not, the QI to IID_IDispatch
into varDest->pdispVal is pointless...

sr=jst if you fix that.

Comment 13

[David Bradley]

Too bad compilers can't compile what you meant and not what you wrote. Thanks,
I'll get that fixed on checkin

Comment 14

[Adam Lock]

Comment on attachment 114845 [details] [diff] [review]
Same as before but takes care of the leaked referenced

r=adamlock

Comment 15

[David Bradley]

Comment on attachment 114845 [details] [diff] [review]
Same as before but takes care of the leaked referenced

Looking for 1.3 approval. These code changes are in files that are not part of
the build. Need to get these in so we can continue further testing IDispatch.

Comment 16

[David Bradley]

Patch checked in.