459648 - Crash [@ nsNativeDragTarget::DragLeave] while hovering over marquee and using ondragenter display none

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

See testcase, it uses enhanced privileges so you need to download it to your computer.
Move the mouse over the marquee area (with the scrollbars), you should get the crash within a few reloads.

http://crash-stats.mozilla.com/report/index/0636b45e-9857-11dd-8da6-001cc45a2ce4?p=1
0  	xul.dll  	nsNativeDragTarget::DragLeave  	 widget/src/windows/nsNativeDragTarget.cpp:373
1 	ole32.dll 	ole32.dll@0x118f09 	
2 	ole32.dll 	ole32.dll@0x119107 	
3 	ole32.dll 	ole32.dll@0xefc90 	
4 	ole32.dll 	ole32.dll@0xefb86 	
5 	xul.dll 	nsDragService::StartInvokingDragSession 	widget/src/windows/nsDragService.cpp:315
6 	xul.dll 	nsDragService::InvokeDragSession 	widget/src/windows/nsDragService.cpp:263
7 	xul.dll 	nsBaseDragService::InvokeDragSessionWithSelection 	widget/src/xpwidgets/nsBaseDragService.cpp:305
8 	xul.dll 	nsEventStateManager::DoDefaultDragStart 	content/events/src/nsEventStateManager.cpp:2287
9 	xul.dll 	nsEventStateManager::GenerateDragGesture 	content/events/src/nsEventStateManager.cpp:2075
10 	xul.dll 	xul.dll@0x332812 	
11 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:5894
12 	xul.dll 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:5792
13 	xul.dll 	xul.dll@0x33b530 	
14 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1389
15 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1348
16 	xul.dll 	HandleEvent 	view/src/nsView.cpp:167
17 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:955
18 	xul.dll 	nsDOMWindowUtils::SendMouseEvent 	dom/src/base/nsDOMWindowUtils.cpp:248
19 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
20 	xul.dll 	XPCCallContext::XPCCallContext 	js/src/xpconnect/src/xpccallcontext.cpp:160
21 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2405
22 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1477
23 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1306
24 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:5001
25 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1324
26 	js3250.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5136
27 	xul.dll 	nsJSContext::CallEventHandler 	dom/src/base/nsJSEnvironment.cpp:2000
28 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/src/base/nsGlobalWindow.cpp:7721
29 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/src/base/nsGlobalWindow.cpp:8053
30 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:420
31 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:512
32 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
33 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
34 	nspr4.dll 	PR_GetEnv 	
35 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:87
36 	firefox.exe 	firefox.exe@0x2197 	
37 	kernel32.dll 	BaseProcessStart

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So at that crash, |this| looks to be bogus -- I'm guessing the nsNativeDragTarget was already deleted.  mDragService is null which is what causes the crash directly, but mCanMove is 35431396, mDragCancelled is -bignum, etc.

The stack with the ole32 symbols is:

>	gkwidget.dll!nsNativeDragTarget::DragLeave()  Line 373 + 0x32 bytes	C++
 	ole32.dll!CInterfaceFromWindowProp::PrivDragDrop()  + 0x111 bytes	
 	ole32.dll!PrivDragDrop()  + 0x97 bytes	
 	ole32.dll!CDropTarget::DragLeave()  + 0x2e bytes	
 	ole32.dll!CDragOperation::CompleteDrop()  + 0x55 bytes	
 	ole32.dll!_DoDragDrop@16()  + 0xab bytes	
 	gkwidget.dll!nsDragService::StartInvokingDragSession(IDataObject * aDataObj=0x0641bbc0, unsigned int aActionType=7)  Line 315 + 0x19 bytes	C++
 	gkwidget.dll!nsDragService::InvokeDragSession(nsIDOMNode * aDOMNode=0x06525a80, nsISupportsArray * anArrayTransferables=0x064adbc8, nsIScriptableRegion * aRegion=0x00000000, unsigned int aActionType=7)  Line 263 + 0x1b bytes	C++

I don't know enough about drag & drop to know how this nsNativeDragTarget would get deleted; it gets deleted at the start of StartInvokingDragSession before a new one is created, there's an explicit AddRef in nsDragService::InvokeDragSession (it's a COM object, not an XPCOM object).  But |this| in the crash spot isn't the same as mNativeDragSrc that's passed to DoDragDrop; could it be an earlier one?

Neil, do you have any thoughts?

Comment 2

[Neil Deakin]

Looks like the nativedragtarget is created in nsWindow (the mNativeDragTarget doesn't seem to be used) and likely gets destroyed when the window reloads.

May be a similar cause as bug 378371.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, indeed -- the window is getting destroyed after the DoDragDrop call:

[060B19D0] nsWindow
[060B19D0] nsWindow::EnableDragDrop 1 00000000
[060B1AD8] nsNativeDragTarget (wnd: 060B19D4)
[06155FA8] nsWindow::EnableDragDrop 0 061560B0
[061560B0] ~nsNativeDragTarget
[06155FA8] nsWindow::EnableDragDrop 0 00000000
[06155FA8] nsWindow::Destroy
DoDragDrop: src: 0384F798
* [060B19D0] nsWindow::EnableDragDrop 0 060B1AD8
    -- dragtarget is being destroyed here
* [060B1AD8] ~nsNativeDragTarget
* [060B19D0] nsWindow::EnableDragDrop 0 00000000
* [060B19D0] nsWindow::Destroy
* -- 060B1AD8  DragLeave called on already-destroyed drag target

The *'s are the DoDragDrop in progress.

I don't know the right way to fix this -- one way would be to have the nsNativeDragTarget hold a ref to the nsWindow; the circular ref would be broken when we call EnableDragDrop with FALSE and the nsNativeDragTarget is destroyed.

Another way would be for the nsNativeDragTarget to AddRef() itself in DragEnter, and to Release in DragLeave and Drop.  I don't know much about the win32 D&D APIs though, so I'm not sure if that would be enough, or if there are any cases when DragEnter would be called without a corresponding DragLeave or Drop.  (In particular, what happens when the DragEnter returns failure?  Is DragLeave still called?)

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: potential fix?

This fixes this testcase for me; I see the following dump output with my tracing for the case that would have previously crashed, as expected:

DoDragDrop: src: 04520DB8
[0622B9F8] nsWindow::EnableDragDrop 0 0622BB00
[0622B9F8] nsWindow::EnableDragDrop 0 00000000
[0622B9F8] nsWindow::Destroy
[0622B9F8] ~nsWindow (mNativeDragTarget: 00000000)
[0622BB00] ~nsNativeDragTarget
DoDragDrop end: src: 04520DB8

But, not knowing the win32 dnd code, I don't know if this might cause leaks of NativeDragTargets ever.  If it does, they won't be active ones, because RevokeDragDrop will still have been called by EnableDragDrop with FALSE, so at worst it would be just a leak of those objects.

Comment 5

[Neil Deakin]

I'm not fully sure of all the event ordering here either. What happens if the user cancels the drag during this?

One problem with this patch is when dragging from inside Mozilla to outside. DragLeave will be called when there wasn't a corresponding DragEnter. Or, if there was, the release will happen again and the end of InvokeDragSession.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It shouldn't -- note that we're only dealing with nsNativeDragTarget, not nsNativeDragSource.  I was kind of confused at first there since I didn't realize that both existed, but the problem is purely in the NativeDragTarget code.  InvokeDragSession will release the Source only, so that should be fine; for the target, I don't know how we'd get a DragLeave without a DragEnter.

I'm really surprised that the win32 d&d code doesn't take a ref to the IDropTarget that it's interacting with... maybe I should instrument AddRef/Release, make sure we don't have some weird unbalance somewhere?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm, I could set a flag when I take the ref in DragEnter and check/clear that flag in the other two places to ensure we don't get a bogus Release we didn't take; would that help?

Comment 8

[Neil Deakin]

(In reply to comment #6)
>I don't know how we'd get a DragLeave without a DragEnter.
 
I meant does DragEnter get called when a drag begins on the window that started the drag?

And, after a DragEnter occurs, does DragLeave get called when a user cancels a drag (by pressing Escape)?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: slightly better patch

Slightly better patch.  I traced addrefs/releases, everything seemed to get cleaned up correctly and these assertions never fired.

(In reply to comment #8)
> (In reply to comment #6)
> >I don't know how we'd get a DragLeave without a DragEnter.
> 
> I meant does DragEnter get called when a drag begins on the window that started
> the drag?

I'd assume so -- if the window is both a drag source and a drag target, the normal process would apply.  DragEnter has to get called to start the target process.

> And, after a DragEnter occurs, does DragLeave get called when a user cancels a
> drag (by pressing Escape)?

Yep, according to docs.

Comment 10

[Neil Deakin]

Comment on attachment 353250 [details] [diff] [review]
slightly better patch

I tested this a bit and I think this is probably a suitable solution.

> {
>   NS_RELEASE(mDragService);
>+  mDragService = nsnull;
>+

NS_RELEASE clears the pointer already.

>+  NS_ASSERTION(!mTookOwnRef, "own ref already taken!");
>+  this->AddRef();

'this->' isn't really needed, no? There's also NS_ADDREF_THIS/NS_RELEASE_THIS if you want this just to be clearer as to what it's doing.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ah, will get rid of the bogus = null.

For the AddRef, just wanted to be explicit about what's being ref'd.. also I didn't really want to use the macros because it's a COM class and not XPCOM; the other similar uses use bare AddRef/Release in other parts of d&d code.  I can change it if you'd rather I use the macros, though.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in.

Comment 13

[Martijn Wargers (dead)]

I couldn't reproduce this already anymore with a build before the patch, so I can't really verify that the patch fixed the crash.

Comment 14

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre and  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre. No crash with the testcase.