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

RESOLVED FIXED

Status

()

Core
Drag and Drop
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Dave Garrett, Assigned: smaug)

Tracking

({crash, regression, testcase})

Trunk
x86
All
crash, regression, testcase
Points:
---

Firefox Tracking Flags

(blocking1.9.2 -, status1.9.2 .4-fixed, blocking1.9.1 -, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:dos], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
By the way, passing null or undefined instead of {} does not crash.
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.
blocking1.9.1: ? → -
blocking1.9.2: ? → -
Keywords: crash
Whiteboard: [sg:dos]
Keywords: testcase

Comment 3

7 years ago
Created attachment 429392 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #429392 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Attachment #429392 - Flags: review?(Olli.Pettay) → review+

Comment 4

7 years ago
http://hg.mozilla.org/mozilla-central/rev/8fdae602e9b0
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Version: 1.9.1 Branch → Trunk

Updated

7 years ago
Attachment #429392 - Flags: approval1.9.2.2?
Attachment #429392 - Flags: approval1.9.1.9?
Attachment #429392 - Flags: approval1.9.2.2?
Attachment #429392 - Flags: approval1.9.2.2+
Attachment #429392 - Flags: approval1.9.1.9?
Attachment #429392 - Flags: approval1.9.1.9+
Comment on attachment 429392 [details] [diff] [review]
not all nsIDOMNodes are nsIContents (and all scriptable emulating objects are not)

a1922,1919=beltzner

Comment 6

7 years ago
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
status1.9.1: --- → .9-fixed
status1.9.2: --- → .2-fixed

Comment 7

7 years ago
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
(Reporter)

Comment 8

7 years ago
Well, it probably wouldn't hurt to get it onto there too if anyone wanted to.
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.
status1.9.1: .9-fixed → ---
status1.9.2: .2-fixed → ---
(Reporter)

Comment 10

7 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Crash on event.dataTransfer.setDragImage({},0,0) [@ nsINode::GetCurrentDoc() ] → Crash on event.dataTransfer.setDragImage({},0,0) [@ nsINode::GetCurrentDoc() ] [@ PresShell::RenderNode(nsIDOMNode*, nsIRegion*, nsIntPoint&, nsIntRect*) ]

Comment 11

7 years ago
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

7 years ago
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.
Attachment #429392 - Attachment is obsolete: true
Attachment #434017 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 13

7 years ago
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.
Attachment #429392 - Attachment is obsolete: false
(Assignee)

Comment 14

7 years ago
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.
Attachment #434017 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 15

7 years ago
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
Attachment #434564 - Flags: review?(enndeakin)

Updated

7 years ago
Attachment #434564 - Flags: review?(enndeakin) → review+

Updated

7 years ago
Assignee: timeless → Olli.Pettay
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/mozilla-central/rev/be976867b094
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Are you going to want to land this follow-up patch on the same branches as the first patch?

Comment 18

7 years ago
Comment on attachment 434564 [details] [diff] [review]
patch

we'd want them, but presumably he'd need approval.
Attachment #434564 - Flags: approval1.9.2.3?
Attachment #434564 - Flags: approval1.9.1.10?
(Reporter)

Comment 19

7 years ago
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.
(Reporter)

Updated

7 years ago
Attachment #434017 - Attachment is obsolete: true
Comment on attachment 434564 [details] [diff] [review]
patch

a=beltzner for 1.9.2.3 and 1.9.1.10
Attachment #434564 - Flags: approval1.9.2.3?
Attachment #434564 - Flags: approval1.9.2.3+
Attachment #434564 - Flags: approval1.9.1.10?
Attachment #434564 - Flags: approval1.9.1.10+
(Assignee)

Comment 21

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c184f28b91ff
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9963127ec7bf
blocking2.0: ? → ---
status1.9.1: --- → .10-fixed
status1.9.2: --- → .4-fixed
Crash Signature: [@ nsINode::GetCurrentDoc() ] [@ PresShell::RenderNode(nsIDOMNode*, nsIRegion*, nsIntPoint&, nsIntRect*) ]
You need to log in before you can comment on or make changes to this bug.