597339 - Crash [@ CDragOperation::UpdateTarget]

Status: REOPENED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

Attachment: unminimized testcase, using enhanced privileges

(Sorry for the unminimized testcase, but I have no time to minimize)
The testcase is using enhanced privileges.
I crash when mousing down on the textarea, wait for the document to reload (takes mostly 2s), then release the mouse.

My guess is that this could be a regression from the bremoval patch.

http://crash-stats.mozilla.com/report/index/3d6d7edc-cc2f-4af1-b428-321cc2100917
0  	ole32.dll  	CDragOperation::UpdateTarget  	
1 	ole32.dll 	DoDragDrop 	
2 	xul.dll 	nsDragService::StartInvokingDragSession 	widget/src/windows/nsDragService.cpp:319
3 	xul.dll 	nsDragService::InvokeDragSession 	widget/src/windows/nsDragService.cpp:266
4 	xul.dll 	nsBaseDragService::InvokeDragSessionWithSelection 	widget/src/xpwidgets/nsBaseDragService.cpp:318
5 	xul.dll 	nsPlaintextEditor::DoDrag 	editor/libeditor/text/nsPlaintextDataTransfer.cpp:399
6 	xul.dll 	nsEditorEventListener::DragGesture 	editor/libeditor/base/nsEditorEventListener.cpp:525
7 	xul.dll 	nsEditorEventListener::HandleEvent 	editor/libeditor/base/nsEditorEventListener.cpp:281
8 	xul.dll 	xul.dll@0xc023fb

Comment 1

[(no longer active)]

Have you tried to see if crash happens on builds before bug 240933?

Comment 2

[Martijn Wargers (dead)]

Sorry, you're right, it's also crash Firefox 3.6, so I guess you couldn't even call it a regression.
http://crash-stats.mozilla.com/report/index/e2e54aff-44ce-44f2-aad1-4d46d2100917

Comment 3

[(no longer active)]

I tried to reproduce the crash but I was unsuccessful.  I should note that I tried opening the test case both from bugzilla and as a local file, but I didn't get the privilege request dialog either way...

Comment 4

[Martijn Wargers (dead)]

It could be that the enablePrivilege method is not working anymore in current trunk build. There might be an extension somewhere that adds it back or something.
Or perhaps enablePrivilege is denied by default now, for local files.

Comment 5

[Martijn Wargers (dead)]

Sorry, never mind. It still works for me on the latest nightly trunk build. And it still crashes for me.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Sorry, never mind. It still works for me on the latest nightly trunk build. And
> it still crashes for me.

So can you please post an itemized list of the exact things that I need to do in order to reproduce it?  Make it fool-proof, as I'm probably missing something very obvious.  :-)

Comment 7

[Martijn Wargers (dead)]

Steps to reproduce:
- Use a flavor of Windows (I suspect it's a Windows only crash)
- Download the testcase to your computer
- Allow the testcase all the enhanced privileges it would like to have
- Mouse down over the textarea
- Release the mouse after the document has reloaded

Comment 8

[(no longer active)]

Ah, OK.  What I was missing was the Windows-only nature of this bug.

I could reproduce the crash, and I think I should focus on fixing it after 2.0.

Note to myself:  we also get this assertion before the crash:

###!!! ASSERTION: own ref already taken!: '!mTookOwnRef', file c:/Users/ehsan/mo
z/src/widget/src/windows/nsNativeDragTarget.cpp, line 251

Which might be relevant.

Comment 9

[Martijn Wargers (dead)]

Fixing it after 2.0? What is 2.0? Firefox 2 was already released when you made that comment.

Comment 10

[(no longer active)]

I was talking about Gecko 2.0 (Firefox 4).

kaze: can you look into this, please?

Comment 11

[Fabien Cazenave [:kaze]]

ehsan: sure. Setting up a Windows VM atm.

Comment 12

[Fabien Cazenave [:kaze]]

Attachment: testcase (almost minimized)

Bug confirmed on Windows XP with Firefox 5 and Nightly.

Same steps to reproduce:
1. download the testcase to your computer
2. allow the testcase to use enhanced privileges
3. mouse down over the textarea
4. release the mouse after the document has reloaded

Comment 13

[(no longer active)]

Did you make any progress in determining why the crash happens?

Comment 14

[Fabien Cazenave [:kaze]]

After way too much time spent on this, I’m still not sure to know *why* the crash happens but I think I know where it does — in `nsDragService::StartInvokingDragSession`, line 317:
: // Call the native D&D method
: HRESULT res = ::DoDragDrop(aDataObj, mNativeDragSrc, effects, &winDropRes);

As the crash comes from the OLE component we can't easily see where our code is at fault. As you mentioned earlier, we do get an assertion before the crash (and only before the crash) which lets me think that the problem is that a OLE D&D operation is initialized in a window that doesn't exist any more.  A way to work around this bug would be to cancel the D&D operation when this assertion is hit in `nsNativeDragTarget::DragEnter` — but I couldn't make it work so far.

This assertion and the `mTookOwnRef` property have been coded by Vlad for bug 459648:
https://hg.mozilla.org/mozilla-central/rev/d2f133096be4

I’ve tried to see if we could prevent the crash in the `nsPlaintextEditor::DoDrag` caller but I haven’t found any relevant condition we could use there. At this point, I tend to think the problem isn’t in the editor core…

Comment 15

[(no longer active)]

It seems to me that the problem is that nsNativeDragTarget assumes that a call to DragEnter is followed by a call to DragLeave or Drop, but DragEnter is called two times in a row.  Can you please verify this, and note the arguments and callstack in each call?

Are we calling nsDragService::StartInvokingDragSession two times in a row, too?  If yes, can you please post the arguments and callstack in each call?  Maybe we're releasing the first mNativeDragSrc object, and then later on COM tries to call something on it, which results in accessing freed memory?  This might be kind of similar to bug 459648.

Comment 16

[(no longer active)]

Another thing that you can try is to modify the destructor for these objects to fill the memory contained by the object with a known bogus value (like a sequence of 0xdeadbeef's for example, and then instruments the member functions to DebugBreak if they detect that the memory has been modified like that.  This will indicate that an object has been freed too soon, and can help you determine what needs to be done to fix the bug.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Since the crash volume is low (less than 5 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.