Closed Bug 391747 Opened 16 years ago Closed 16 years ago

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


(Core :: DOM: Selection, defect)

Not set





(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)



(Keywords: crash)

Crash Data


(2 files, 1 obsolete file)

I have a testcase that crashes with the following stacktrace on current trunk builds:
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
Attached patch patch? (obsolete) — Splinter Review
According to the stack, this would fix it, I would think. Is this patch any good?
Attachment #276186 - Flags: review?(Olli.Pettay)
What is the test case?
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.
(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
Attachment #276186 - Attachment is obsolete: true
Attachment #276186 - Flags: review?(Olli.Pettay)
Attachment #276216 - Flags: review?(Olli.Pettay)
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...
Yeah, I get a slow script warning after a while, so it's not a 'real' hang.
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 on attachment 276216 [details] [diff] [review]
Alternative patch

Nit, maybe NS_ENSURE_STATE(parentNode);
Attachment #276216 - Flags: review?(Olli.Pettay) → review+
> Nit, maybe NS_ENSURE_STATE(parentNode);

Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Attachment #276216 - Flags: superreview?(roc)
Attachment #276216 - Flags: superreview?(roc)
Attachment #276216 - Flags: superreview+
Attachment #276216 - Flags: approval1.9+
Attached patch MochitestSplinter Review
Not sure how we usually add crashers to the test suite, does this look ok?
Attachment #276945 - Flags: review?(martijn.martijn)
Flags: in-testsuite?
mozilla/layout/generic/nsSelection.cpp 	3.290 

Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
Comment on attachment 276945 [details] [diff] [review]

> 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 ;)
Attachment #276945 - Flags: review?(martijn.martijn) → review+
Flags: in-testsuite? → in-testsuite+
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.
Crash Signature: [@ nsFrameSelection::GetFirstCellNodeInRange]
You need to log in before you can comment on or make changes to this bug.