209593 - Moving mails via Drag'n'Drop crashes Mozilla [TB21092337G]

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

[Karsten Düsterloh]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030615
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030616

Moving several (at least two) mails per drag'n'drop from subfolder of POP3 inbox
into subfolder of Local Folders crashes Mozilla.

Talkback-ID TB21092337G:
Incident ID 21092337
Stack Signature 0x95e3fcdb 8d61909d
Email Address [e-mail]
Product ID MozillaTrunk
Build ID 2003061611
Trigger Time 2003-06-16 14:25:46
Platform Win32
Operating System Windows NT 5.0 build 2195
Module
URL visited
User Comments moving several mails per drag'n'drop from subfolder of
POP2-inbox into subfolder of Local Folders
Trigger Reason Access violation
Source File Name
Trigger Line No.
Stack Trace
0x95e3fcdb
0x01e2fa85
nsDragService::InvokeDragSession
[c:/builds/seamonkey/mozilla/widget/src/windows/nsDragService.cpp, line 131]
XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2019]
XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1270]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 845]
js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2856]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 861]
js_InternalInvoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 936]
JS_CallFunctionValue [c:/builds/seamonkey/mozilla/js/src/jsapi.c, line
3533]
nsJSContext::CallEventHandler
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1114]
nsJSEventListener::HandleEvent
[c:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp, line 183]
nsEventListenerManager::HandleEventSubType
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1192]
nsEventListenerManager::HandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp,
line 2017]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp,
line 3303]
nsEventStateManager::GenerateDragGesture
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp,
line 1527]

Comment 1

[Karsten Düsterloh]

The crash occurs in the patch for bug 203847 in line 174 of
/seamonkey/mozilla/widget/src/windows/nsDragService.cpp:

  aDataObj->SetData(&fmte, &medium, FALSE);

Comment 2

[Antony Mawer]

I'm seeing a similar problem with drag-and-drop between IMAP folders... Windows
XP, build 2003061508. I selected two messages I wanted to move to another folder
on the same IMAP account, dragged them to the corresponding folder, and Mozilla
mail crashed. This happens repeatably evey time...

STEPS TO REPRODUCE

1. Open Mozilla Mail
2. Go to the INBOX for an IMAP mail account
3. Select one or more emails to move
4. Attempt to drag the message(s) to another folder on the same account

ACTUAL RESULTS

1. Mozilla crashes

EXPECTED RESULTS

1. The mail messages are moved, and Mozilla does not crash

Relevant talkback IDs:

TB21102483G, TB2110256X, TB21102442X


WORKAROUND

Select the messages, and use Messages -> Move -> ... to move the folder does not
cause the crash to occur.

Comment 3

[Kathleen :Brade]

Is drag & drop within mail compose body also broken?

Comment 4

[Antony Mawer]

This doesn't appear to affect drag & drop within Mail Compose in any way. I can
drag text from the browser into a Compose window, as well as dragging and
dropping text around within the actual body itself. This works with both plain
text and HTML compose.

Additionally, dragging files from Windows Explorer to the attachment area works
without problems. Messages send without any crash.

Comment 5

[Antony Mawer]

It appears this does not happen with all emails; I was just able to drag and
drop to move one email successfully. I've managed to isolate two messages that
repeatedly DO cause the crash, and copied them to a new account that I have
setup for testing purposes. Using a new profile in conjunction with the messages
in the test account, I can still reproduce the problem every time.

I can provide access to the test account if it would be of assistance; please
email me privately for the details.

Comment 6

[Jason M. Abels]

I can also confirm that this happens when going between two IMAP folders. It
seems only to occur when I select more than one message, though.

Build is 2003061704 WinXP

Comment 7

[Frank Wein [:mcsmurf]]

Looks like a topcrasher, in
http://ftp.mozilla.org/pub/data/crash-data/Trunk-topcrashers.html you see
topcrash in nsDragService::StartInvokingDragSession, i think this is the same as
this bug. Can someone change the summary, so that you can see this Bug in the
topcrasher-list?

Comment 8

[kinmoz]

Ok I just looked into this, Nisheeth's fix for bug 203847 simply exposes an
existing problem in nsDragService::InvokeDragSession(). InvokeDragSession()
doesn't AddRef the IDataObject it allocates when multiple items are being
dragged, so when DoDragDrop(), called in 
nsDragService::StartInvokingDragSession(), releases all of it's references to
the object, it gets automatically deleted.

Also, I see that in the case wherer a single item is dragged, an IDataObject is
created and AddRef'd, but Release is never called, so we leak that object too.

Comment 9

[kinmoz]

Attachment: Patch Rev 1

Here's a patch that prevents the crash and leak by calling AddRef and Release
in InvokeDragSession().

Comment 10

[kinmoz]

Taking the bug. Cc'ing smontagu to keep him in the loop. Simon, you may want to
look at Patch Rev 1 above.

Comment 11

[Nisheeth Ranjan]

Comment on attachment 125825 [details] [diff] [review]
Patch Rev 1

Looks good!

Comment 12

[Andrew Schultz]

*** Bug 209678 has been marked as a duplicate of this bug. ***

Comment 13

[kinmoz]

Comment on attachment 125825 [details] [diff] [review]
Patch Rev 1

Requesting sr= from dbaron ... I think sspitzer is on vacation.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 125825 [details] [diff] [review]
Patch Rev 1

This would be much cleaner using smart pointers, probably like the following:

+#include "nsAutoPtr.h"

@@ -101,6 +101,9 @@
-  IDataObject* itemToDrag = nsnull;
+  nsRefPtr<IDataObject> itemToDrag;
   if ( numItemsToDrag > 1 ) {
     nsDataObjCollection * dataObjCollection = new nsDataObjCollection();
+    if (!dataObjCollection)
+      return NS_ERROR_OUT_OF_MEMORY;
+    itemToDrag = dataObjCollection;
     IDataObject* dataObj = nsnull;
     for ( PRUint32 i=0; i<numItemsToDrag; ++i ) {
       nsCOMPtr<nsISupports> supports;
@@ -111,8 +114,10 @@
	   dataObjCollection->AddDataObject(dataObj);
	   NS_IF_RELEASE(dataObj);
	 }
	 else
	   return NS_ERROR_FAILURE;
       }
     }
-    itemToDrag = NS_STATIC_CAST ( IDataObject*, dataObjCollection );
@@ -122,12 +127,17 @@
     anArrayTransferables->GetElementAt(0, getter_AddRefs(supports));
     nsCOMPtr<nsITransferable> trans(do_QueryInterface(supports));
     if ( trans ) {
-      if ( NS_FAILED(nsClipboard::CreateNativeDataObject(trans, &itemToDrag))
)
+      if ( NS_FAILED(nsClipboard::CreateNativeDataObject(trans,
getter_AddRefs(itemToDrag))) )
	 return NS_ERROR_FAILURE;
     }
   } // else dragging a single object

   return StartInvokingDragSession ( itemToDrag, aActionType );
 }

Comment 15

[kinmoz]

Attachment: Patch Rev 2 (Use nsRefPtr instead)

Ok here's the patch which uses nsRefPtr as in dbaron's comment above. I also
converted dataObj in the same method to be an nsRefPtr too.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 125929 [details] [diff] [review]
Patch Rev 2 (Use nsRefPtr instead)

sr=dbaron (although the |dataObj| variable might be better inside the loop and
the if).

Comment 17

[kinmoz]

Attachment: Patch Rev 2.1

Ok, this is the patch I'm checking in. This one just moves the dataObj
declaration into the |if| in the |for| loop.

Comment 18

[kinmoz]

Comment on attachment 125938 [details] [diff] [review]
Patch Rev 2.1

Rolling r=nisheeth and sr=dbaron forward to this patch.

Comment 19

[kinmoz]

Patch Rev 2.1 checked into the TRUNK:

    mozilla/widget/src/windows/nsDragService.cpp  revision 1.35

Comment 20

[Jason M. Abels]

verified fixed.