Last Comment Bug 548185 - Crash on event.dataTransfer.setDragImage({},0,0) [@ nsINode::GetCurrentDoc() ] [@ PresShell::RenderNode(nsIDOMNode*, nsIRegion*, nsIntPoint&, nsIntRect*) ]
: Crash on event.dataTransfer.setDragImage({},0,0) [@ nsINode::GetCurrentDoc() ...
Status: RESOLVED FIXED
[sg:dos]
: crash, regression, testcase
Product: Core
Classification: Components
Component: Drag and Drop (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-23 19:29 PST by Dave Garrett
Modified: 2011-06-13 10:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
.4-fixed
-
.10-fixed


Attachments
testcase (437 bytes, text/html)
2010-02-23 19:29 PST, Dave Garrett
no flags Details
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not) (656 bytes, patch)
2010-02-28 02:51 PST, timeless
bugs: review+
mbeltzner: approval1.9.2.2+
mbeltzner: approval1.9.1.9+
Details | Diff | Splinter Review
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not) part 2/2 (701 bytes, patch)
2010-03-22 13:24 PDT, timeless
bugs: review-
Details | Diff | Splinter Review
patch (1.26 KB, patch)
2010-03-24 10:29 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
enndeakin: review+
mbeltzner: approval1.9.2.4+
mbeltzner: approval1.9.1.10+
Details | Diff | Splinter Review

Description Dave Garrett 2010-02-23 19:29:59 PST
Created attachment 428615 [details]
testcase

event.dataTransfer.setDragImage({},0,0) crashes under Firefox 3.5, 3.6, and latest Trunk, but not 3.0. See attached testcase.

I initially stumbled onto it from XUL but it crashes from HTML too and the element being dragged doesn't matter. 100% reproducible for me under both Linux and Windows XP.

bp-8e5d0035-68b5-49c6-aa83-c00802100223

bp-b69b8122-33c1-49b9-ad8b-490142100223
0  	xul.dll  	nsINode::GetCurrentDoc  	 obj-firefox/dist/include/nsINode.h:378
1 	xul.dll 	GetPresShellForContent 	widget/src/xpwidgets/nsBaseDragService.cpp:404
2 	xul.dll 	nsBaseDragService::DrawDrag 	widget/src/xpwidgets/nsBaseDragService.cpp:436
3 	xul.dll 	nsDragService::CreateDragImage 	widget/src/windows/nsDragService.cpp:117
4 	xul.dll 	nsDragService::InvokeDragSession 	widget/src/windows/nsDragService.cpp:258
5 	xul.dll 	nsBaseDragService::InvokeDragSessionWithImage 	widget/src/xpwidgets/nsBaseDragService.cpp:281
6 	xul.dll 	nsEventStateManager::DoDefaultDragStart 	content/events/src/nsEventStateManager.cpp:2233
7 	xul.dll 	nsEventStateManager::GenerateDragGesture 	
8 	xul.dll 	nsEventStateManager::PreHandleEvent 	content/events/src/nsEventStateManager.cpp:1123
9 	xul.dll 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6509
10 	xul.dll 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:6365
11 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6228
12 	xul.dll 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1139
13 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1118
14 	xul.dll 	HandleEvent 	view/src/nsView.cpp:160
15 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:3044
16 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:3067
17 	xul.dll 	nsWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:3450
18 	xul.dll 	ChildWindow::DispatchMouseEvent 	widget/src/windows/nsWindow.cpp:7273
Comment 1 Dave Garrett 2010-02-24 01:49:37 PST
By the way, passing null or undefined instead of {} does not crash.
Comment 2 Daniel Veditz [:dveditz] 2010-02-24 13:24:38 PST
Looks like a null deref which doesn't "block" on the old branches unless it's a high frequency crash, but once there's a fix we could look at landing it on the old branches.
Comment 3 timeless 2010-02-28 02:51:02 PST
Created attachment 429392 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-03 12:52:16 PST
Comment on attachment 429392 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)

a1922,1919=beltzner
Comment 8 Dave Garrett 2010-03-05 04:21:48 PST
Well, it probably wouldn't hurt to get it onto there too if anyone wanted to.
Comment 9 Al Billings [:abillings] 2010-03-22 12:16:20 PDT
This isn't fixed in 1.9.2 or 1.9.1. If I use the attached testcase on Firefox 3.6.2, build 3 or Firefox 3.5.9, I crash.

See http://crash-stats.mozilla.com/report/index/418b0311-0a16-493a-a893-7630f2100322 for 3.6.2.

See http://crash-stats.mozilla.com/report/index/bp-8480fb00-043c-4ca9-bf24-e9b9d2100322 for 3.5.9.

Both tested with clean profiles on a clean Windows XP virtual machine.
Comment 10 Dave Garrett 2010-03-22 12:54:36 PDT
Trunk crashes too. This is a new sig though. I think this was fixed and may have regressed again? Not sure.

Does setDragImage have some automated tests this could be added to?
Comment 11 timeless 2010-03-22 13:03:50 PDT
oops, my fault, thanks al.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000018
0x00000001003a4788 in nsINode::IsInDoc (this=0x0) at nsINode.h:370
370	    return mParentPtrBits & PARENT_BIT_INDOCUMENT;
(gdb) up
#1  0x0000000100418923 in PresShell::RenderNode (this=0x1194b5fc0, aNode=0x120b92cf0, aRegion=0x0, aPoint=@0x7fff5fbfd3e0, aScreenRect=0x7fff5fbfd950) at /Users/timeless/devel/mozilla.org/mozilla-central/layout/base/nsPresShell.cpp:5486
5486	  if (!node->IsInDoc())
(gdb) l
5481	  nsRect area;
5482	  nsTArray<nsAutoPtr<RangePaintInfo> > rangeItems;
5483	
5484	  // nothing to draw if the node isn't in a document
5485	  nsCOMPtr<nsINode> node = do_QueryInterface(aNode);

we need an extra guard here, i'm testing locally (including dragging the link...)

5486	  if (!node->IsInDoc())
5487	    return nsnull;

dave: dunno, but yeah, it definitely needs to go into one.
Comment 12 timeless 2010-03-22 13:24:52 PDT
Created attachment 434017 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not) part 2/2

ok, with this, the testcase no longer crashes.

i'm sick and don't have the energy to deal with producing a testcase. hopefully dave or smaug can produce one. i'm essentially unavailable for 3 weeks (one for health, two for travel).

i can understand not wanting to push this to branches w/o the testcase, although as al has shown, testing can verify the fix.
Comment 13 Dave Garrett 2010-03-22 14:21:41 PDT
Comment on attachment 429392 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)

The first patch isn't really obsolete. It did fix the first crash and landed and wasn't backed out.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-03-24 10:27:37 PDT
Comment on attachment 434017 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not) part 2/2

I'd like to prevent the crash earlier in the code.
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-03-24 10:29:12 PDT
Created attachment 434564 [details] [diff] [review]
patch

This way we don't need to patch all the cases when drag code
QI's nsIDOMElement to nsINode
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-03-25 13:51:29 PDT
http://hg.mozilla.org/mozilla-central/rev/be976867b094
Comment 17 Daniel Veditz [:dveditz] 2010-03-25 18:42:13 PDT
Are you going to want to land this follow-up patch on the same branches as the first patch?
Comment 18 timeless 2010-03-25 22:05:44 PDT
Comment on attachment 434564 [details] [diff] [review]
patch

we'd want them, but presumably he'd need approval.
Comment 19 Dave Garrett 2010-03-26 17:11:17 PDT
Latest Trunk now gives the following in the Error Console instead of a crash:

Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMDataTransfer.setDragImage]
Source file: https://bug548185.bugzilla.mozilla.org/attachment.cgi?id=428615
Line: 6

Looks good now.
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-29 10:56:37 PDT
Comment on attachment 434564 [details] [diff] [review]
patch

a=beltzner for 1.9.2.3 and 1.9.1.10

Note You need to log in before you can comment on or make changes to this bug.