391747 - Crash [@ nsFrameSelection::GetFirstCellNodeInRange] with mathml ctrl-clicking, etc

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Martijn Wargers (dead)]

I have a testcase that crashes with the following stacktrace on current trunk builds:
http://crash-stats.mozilla.com/report/index/4973008a-477f-11dc-9a31-001a4bd43e5c
0  	nsFrameSelection::GetFirstCellNodeInRange(nsIDOMRange*, nsIDOMNode**)  	 e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsselection.cpp:3464
1 	nsFrameSelection::GetFirstSelectedCellAndRange(nsIDOMNode**, nsIDOMRange**) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsselection.cpp:3497
2 	nsFrameSelection::HandleTableSelection(nsIContent*, int, int, nsMouseEvent*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsselection.cpp:3023
3 	nsFrame::HandlePress(nsPresContext*, nsGUIEvent*, nsEventStatus*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsframe.cpp:1985
4 	nsFrame::HandleEvent(nsPresContext*, nsGUIEvent*, nsEventStatus*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsframe.cpp:1680
5 	nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nspresshell.cpp:1210
6 	nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\events\src\nseventdispatcher.cpp:303
7 	nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\content\events\src\nseventdispatcher.cpp:473
8 	PresShell::HandleEventInternal(nsEvent*, nsIView*, nsEventStatus*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nspresshell.cpp:5731
9 	PresShell::HandlePositionedEvent(nsIView*, nsIFrame*, nsGUIEvent*, nsEventStatus*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nspresshell.cpp:5620
10 	PresShell::HandleEvent(nsIView*, nsGUIEvent*, nsEventStatus*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nspresshell.cpp:5456
11 	nsViewManager::HandleEvent(nsView*, nsPoint, nsGUIEvent*, int) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\view\src\nsviewmanager.cpp:1292
12 	nsViewManager::DispatchEvent(nsGUIEvent*, nsEventStatus*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\view\src\nsviewmanager.cpp:1248
13 	HandleEvent 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\view\src\nsview.cpp:168
14 	nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\widget\src\windows\nswindow.cpp:1090
15 	nsDOMWindowUtils::SendMouseEvent(nsAString_internal const&, int, int, int, int, int) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\dom\src\base\nsdomwindowutils.cpp:212
16 	NS_InvokeByIndex_P 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp:101
17 	JS_SuspendRequest 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\js\src\jsapi.c:942
18 	@0x324e827

Comment 2

[Martijn Wargers (dead)]

Attachment: patch?

According to the stack, this would fix it, I would think. Is this patch any good?

Comment 3

[Ray Kiddy]

What is the test case?

Comment 4

[Martijn Wargers (dead)]

I've attached it to the bug, but it has to remain security sensitive, atm, and unfortunately I haven't been able to make a minimized testcase yet.

Comment 5

[Mats Palmgren (inactive)]

(In reply to comment #2)
> According to the stack, this would fix it, I would think. Is this patch any
> good?

The QI fails because we have a document node as the range start node
and that is a valid start node AFAIK.  I think the right fix is to
QI to nsINode instead (that would make GetFirstCellNodeInRange()
succeed in the degenerate case of a <td> as root content with a
document node in the range object.)

(gdb) p startParent
$2 = {mRawPtr = 0x14f2408}
(gdb) p *startParent.mRawPtr
$3 = {<nsISupports> = {_vptr.nsISupports = 0x2aaaaf8d35f0}, <No data fields>}
(gdb) x/aw 0x2aaaaf8d35f0
0x2aaaaf8d35f0 <_ZTV14nsHTMLDocument+2544>:     0xffffffffaf16cf18

Comment 6

[Mats Palmgren (inactive)]

Attachment: Alternative patch

Comment 7

[Mats Palmgren (inactive)]

It fixes the crash, but I still see the hang so I think that's an unrelated
problem. Interrupting the hang in a debugger doesn't seem to reveal any
recurring pattern - it looks like it's doing useful work...

Comment 8

[Martijn Wargers (dead)]

Yeah, I get a slow script warning after a while, so it's not a 'real' hang.

Comment 9

[Mats Palmgren (inactive)]

FYI, calling DumpJSStack() in the debugger a few times give the
location swapattributes.js:11 and that 'temp1' contains thousands
of objects and it grows each time...

Comment 10

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

Comment on attachment 276216 [details] [diff] [review]
Alternative patch



>+  NS_ENSURE_TRUE(parentNode, NS_ERROR_FAILURE);
Nit, maybe NS_ENSURE_STATE(parentNode);

Comment 11

[Mats Palmgren (inactive)]

> Nit, maybe NS_ENSURE_STATE(parentNode);

Sure.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Mochitest

Not sure how we usually add crashers to the test suite, does this look ok?

Comment 13

[Mats Palmgren (inactive)]

mozilla/layout/generic/nsSelection.cpp 	3.290 

-> FIXED

Comment 14

[Martijn Wargers (dead)]

Comment on attachment 276945 [details] [diff] [review]
Mochitest

> Not sure how we usually add crashers to the test suite, does this look ok?

I'm not sure either, I guess mochitests are the best way for that, currently.
The test looks fine, it crashes nicely in my debug build ;)

Comment 15

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082005 Minefield/3.0a8pre

The zipped, unminimised testcase doesn't crash anymore, just hangs, because the script is swapping tons of attributes.