548185 - Crash on event.dataTransfer.setDragImage({},0,0) [@ nsINode::GetCurrentDoc() ] [@ PresShell::RenderNode(nsIDOMNode*, nsIRegion*, nsIntPoint&, nsIntRect*) ]

Status: RESOLVED FIXED

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

Description

[Dave Garrett]

Attachment: 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]

By the way, passing null or undefined instead of {} does not crash.

Comment 2

[Daniel Veditz [:dveditz]]

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]

Attachment: not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)

Comment 4

[timeless]

http://hg.mozilla.org/mozilla-central/rev/8fdae602e9b0

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 429392 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)

a1922,1919=beltzner

Comment 6

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/78a9c6efb2f7

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/widget/src/xpwidgets/nsBaseDragService.cpp&mark=1.57

Comment 7

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/94920563c414

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/widget/src/xpwidgets/nsBaseDragService.cpp&mark=1.58

sorry, i confused branches

Comment 8

[Dave Garrett]

Well, it probably wouldn't hurt to get it onto there too if anyone wanted to.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

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]

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]

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]

Attachment: 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]

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][[e-mail]]]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/be976867b094

Comment 17

[Daniel Veditz [:dveditz]]

Are you going to want to land this follow-up patch on the same branches as the first patch?

Comment 18

[timeless]

Comment on attachment 434564 [details] [diff] [review]
patch

we'd want them, but presumably he'd need approval.

Comment 19

[Dave Garrett]

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]]

Comment on attachment 434564 [details] [diff] [review]
patch

a=beltzner for 1.9.2.3 and 1.9.1.10

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c184f28b91ff
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9963127ec7bf