Closed Bug 378371 Opened 17 years ago Closed 17 years ago

Crash [@ nsNativeDragTarget::ProcessDrag] when drag event handler and removing iframe

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: asmith16)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 2 obsolete files)

testcase
610 bytes, text/html
Details
fix using AddRef()/Release()
4.29 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
Attached file testcase — 
See testcase, current trunk Mozilla builds crash when dragging some text inside the iframe.

The iframe source consists of this:
<html><head></head>
<body>
When selecting and dragging (some of) this text, Mozilla should not crash
<script>
function doe() {
window.frameElement.parentNode.removeChild(window.frameElement);
}
document.body.addEventListener("drag", doe, false);
</script>
</body>
</html>

Probably this started happening as soon as the drag events were added, bug 375681.

Talkback ID: TB31431884G
nsNativeDragTarget::ProcessDrag  [mozilla/widget/src/windows/nsnativedragtarget.cpp, line 227]
nsNativeDragTarget::DragOver  [mozilla/widget/src/windows/nsnativedragtarget.cpp, line 301]
ole32.dll + 0x1180d6 (0x775b80d6)
ole32.dll + 0x1182c0 (0x775b82c0)
ole32.dll + 0xef347 (0x7758f347)
ole32.dll + 0xef7a1 (0x7758f7a1)
nsDragService::StartInvokingDragSession  [mozilla/widget/src/windows/nsdragservice.cpp, line 215]
nsDragService::InvokeDragSession  [mozilla/widget/src/windows/nsdragservice.cpp, line 162]
nsBaseDragService::InvokeDragSessionWithSelection  [mozilla/widget/src/xpwidgets/nsbasedragservice.cpp, line 293]
nsContentAreaDragDrop::DragGesture  [mozilla/content/base/src/nscontentareadragdrop.cpp, line 815]
No Crash
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/2007041104 Minefield/3.0a4pre

Crash
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/2007041207 Minefield/3.0a4pre

Regression Range

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-11+03&maxdate=2007-04-12+08&cvsroot=%2Fcvsroot

(Which bug 375681 is in)
Blocks: 375681
That testcase is a bit misleading though. The event that removes the child doesn't fire before 375681 was added. If the event is changed from 'drag' to 'dragover' it also crashes in builds before then.

This also crashes on Mac. I suspect that the window/widget is being deleted while the drag is occuring.
No longer blocks: 375681
OS: Windows XP → All
Actually, I should clarify that the test crashes on Mac before bug 375681 with 'dragover'. It's possible that the Windows crash is actually different.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Does this still crash for anyone? On my build from ~29Jul the iframe dissapears when I drag the text, but Firefox doesn't crash.

If I leave the page and come back, it's still not there. Reloading the page the iframe comes back and I can repeat the sequence.
Also when reloading the testcase page i get this:

###!!! ASSERTION: Adding child where we already have a child?  This will likely misbehave: 'Error', file /[...]/mozilla/docshell/shistory/src/nsSHEntry.cpp, line 536

I don't know if it has something to do with it.
This is still crashing for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081105 Minefield/3.0a8pre
I guess it doesn't crash on the mac anymore?

The assertion is probably covered by bug 307241/bug 314457/bug 344216.
I don't know about Macs. I tested on Windows and it does crash for me.

Comment #5 was Linux, sorry I forgot to mention it:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007081010 Minefield/3.0a7pre
This bug is fun. Taking it.

nsNativeDragTarget is destroyed while handling events in mDragService->FireDragEventAtSource(), called from nsNativeDragTarget::DragOver()
Status: NEW → ASSIGNED
Assignee: nobody → asmith15
Status: ASSIGNED → NEW
Attached patch fix using AddRef()/Release() (obsolete) — — Splinter Review 
What's causing the crash is a short chain of member function calls of an object that's getting destroyed.

The fix is doing an AddRef() to that object before the said chain of functions starts executing + releasing the object after the first call ends + breaking the chain if drag is canceled.

It no longer crashes with the patch. I don't know if the fix is ugly or beautiful. Will someone who knows this code comment please? And if it's good enough, ask someone for a review?
Ere maybe? Ere, see comment 9.
common practice for doing addref/release like that is to just do:

nsCOMPtr<IDropTarget> kungFuDeathGrip(this); before the call to FireDragEventAtSource rather than manually doing addref/release.
Comment on attachment 277505 [details] [diff] [review]
fix using AddRef()/Release()

Nice :) But Stuart, are you sure that will work here? I get an:

"error C2440: 'static_cast' : cannot convert from 'IDropTarget *' to 'nsISupports *'"

And I didn't try very hard to fix it cause I can't even find the definition for IDropTarget.

Anybody want to review this?
Attachment #277505 - Flags: review?
Yeah, this is a COM object, not an XPCOM object.

+  if (mDragCanceled) { /* |this| is likely ready to be destroyed */
+    this->Release();
+    return S_OK;
+  }
+  this->Release();
 
   // Now process the native drag state and then dispatch the event
   ProcessDrag(nsnull, NS_DRAGDROP_OVER, grfKeyState, pt, pdwEffect);

Why not just wrap "if (!mDragCanceled)" around the call to ProcessDrag?

Also cancelled has two 'l's.
Attached patch fix not using AddRef()/Release() (obsolete) — — Splinter Review 
> Why not just wrap "if (!mDragCanceled)" around the call to ProcessDrag?
Like this? Because I'm pretty sure nsNativeDragTarget::~nsNativeDragTarget() is called before mDragService->FireDragEventAtSource(NS_DRAGDROP_DRAG) is finished; and I figured that means mDragCanceled is unreliable. But it seems to work, so maybe I'm missing something.
(In reply to comment #13)
>Also cancelled has two 'l's.
Might be one of those American misspellings :-(
I haven't tried Andrew's patch, but just out of interest I swapped the order of the events (I stored the drag service in a temporary stack variable in case of object destruction) and I was able to complete the drag ;-)
I actually want the two patches combined: i.e., an "if (!mDragCancelled)" but with an AddRef and Release wrapped around the whole thing.

Also, you should never request review and leave the requestee field blank. Request review from someone you think might be appropriate (in this case, me) and if they don't think they should review, they'll tell you and hopefully suggest a better reviewer...
Ah, just in case I guess? Here it is.
Attachment #277505 - Attachment is obsolete: true
Attachment #278011 - Attachment is obsolete: true
Attachment #280407 - Flags: review?(roc)
Attachment #277505 - Flags: review?
you can cast MSCOM objects to nsISupports since xpcom objects are compatible and use nsCOMPtr.. *shrug*
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Checking in nsNativeDragTarget.h;
/cvsroot/mozilla/widget/src/windows/nsNativeDragTarget.h,v  <--  nsNativeDragTar
get.h
new revision: 1.12; previous revision: 1.11
done
Checking in nsNativeDragTarget.cpp;
/cvsroot/mozilla/widget/src/windows/nsNativeDragTarget.cpp,v  <--  nsNativeDragT
arget.cpp
new revision: 1.40; previous revision: 1.39
done
Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.711; previous revision: 3.710
done

Checked into trunk.
Thanks for fixing this, Andrew!
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092904 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsNativeDragTarget::ProcessDrag]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: