510575 - Selecting a table row and pasting in the editor crashes Firefox

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Eric Shepherd [:sheppy]]

This is happening in the latest nightly of 1.9.2 for me. I'm editing a table on devmo; when I select a row, copy it, then select another row and paste, Firefox immediately crashes.

See http://crash-stats.mozilla.com/report/index/bp-c43a67d9-3b54-49d1-8ce4-af6492090814

Comment 1

[Michael Ryan]

Windows XP crash report:
bp-9e103573-3085-49b9-9ec9-436782090815

Looking for regression window now...

Comment 2

[Michael Ryan]

regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=baf06abf0af4&tochange=ff250122fa99

Caused by bug 348681?

Various crash reports (many different call stacks):
bp-d72b0f26-712d-4e4b-a61d-341102090815
bp-141b27e0-8164-42fc-a783-258e92090815
bp-4df9bc55-a635-437e-9bbc-11c3a2090815
bp-d063e0e6-0fb2-40a6-a724-1b98f2090815
bp-63d4e5e0-4a20-406e-be46-1c5392090815
bp-b92430b1-26bd-49c1-ab27-11f672090815
bp-95b7bb75-3038-4059-9327-bc5882090815
bp-e7dd9496-a315-42b1-a761-b7eff2090815
bp-5afb2acd-bebe-4cd3-a743-b080f2090815
bp-c686dccb-ed90-479b-bbfe-b29352090815
bp-aeef47f7-1d13-497d-8d4d-a91f02090815
bp-9f5986b9-645e-4c1f-8209-2306b2090815
bp-1778e39e-b2fe-44ee-8f06-e21292090815
bp-b92c485f-2d59-4d77-90b5-b391d2090815
bp-9e103573-3085-49b9-9ec9-436782090815

Comment 3

[Michael Ryan]

Regression from bug 348681.

Comment 4

[timeless]

most of your crashes that have stacks look like this:
Signature	nsCOMPtr_base::~nsCOMPtr_base()
UUID	95b7bb75-3038-4059-9327-bc5882090815
Time 	2009-08-15 11:22:11.796118
Uptime	30
Last Crash	251 seconds before submission
Product	Firefox
Version	3.6a1pre
Build ID	20090724045715
Branch	1.9.2
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x78000000
User Comments	
Processor Notes 	
Related Bugs
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsCOMPtr_base::~nsCOMPtr_base 	obj-firefox/xpcom/build/nsCOMPtr.cpp:81
1 	xul.dll 	RangeData::`scalar deleting destructor' 	
2 	xul.dll 	nsTArrayElementTraits<nsCOMPtr<nsIStyleRule> >::Destruct 	obj-firefox/dist/include/nsTArray.h:197
3 	xul.dll 	nsTArray<RangeData>::DestructRange 	obj-firefox/dist/include/nsTArray.h:862
4 	xul.dll 	nsTArray<RangeData>::RemoveElementsAt 	obj-firefox/dist/include/nsTArray.h:663
5 	xul.dll 	xul.dll@0x390a4f 	

the one that doesn't looks like this;
Signature	__negvdi2
UUID	c43a67d9-3b54-49d1-8ce4-af6492090814
Time 	2009-08-14 13:29:29.745593
Uptime	672
Last Crash	692 seconds before submission
Product	Firefox
Version	3.6a2pre
Build ID	20090814032440
OS	Mac OS X
OS Version	10.5.8 9L30
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 10
Crash Reason	EXC_BAD_INSTRUCTION / EXC_I386_INVOP
Crash Address	0xe857
User Comments	
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	libplds4.dylib 	__negvdi2 	libgcc2.c:220
1 	XUL 	nsTypedSelection::AddRange 	layout/generic/nsSelection.cpp:4986
2 	XUL 	nsTypedSelection::AddRange 	layout/generic/nsSelection.cpp:4969
3 	XUL 	nsSelectionState::RestoreSelection 	editor/libeditor/base/nsSelectionState.cpp:129
4 	XUL 	nsEditor::RestorePreservedSelection 	editor/libeditor/base/nsEditor.cpp:1963
5 	XUL 	XUL@0x61c470 	
6 	XUL 	nsHTMLEditor::InsertHTMLWithContext 	editor/libeditor/html/nsHTMLDataTransfer.cpp:403
7 	XUL 	nsHTMLEditor::InsertFromTransferable 	editor/libeditor/html/nsHTMLDataTransfer.cpp:1334
8 	XUL 	nsHTMLEditor::Paste 	editor/libeditor/html/nsHTMLDataTransfer.cpp:1929
9 	XUL 	nsPasteCommand::DoCommand 	editor/libeditor/base/nsEditorCommands.cpp:418
10 	XUL 	nsControllerCommandTable::DoCommand 	embedding/components/commandhandler/src/nsControllerCommandTable.cpp:191
11 	XUL 	nsBaseCommandController::DoCommand 	embedding/components/commandhandler/src/nsBaseCommandController.cpp:169
12 	XUL 	nsXBLPrototypeHandler::DispatchXBLCommand 	content/xbl/src/nsXBLPrototypeHandler.cpp:512
13 	XUL 	nsXBLPrototypeHandler::ExecuteHandler 	content/xbl/src/nsXBLPrototypeHandler.cpp:252
14 	XUL 	nsXBLWindowKeyHandler::WalkHandlersAndExecute 	content/xbl/src/nsXBLWindowKeyHandler.cpp:584
15 	XUL 	nsXBLWindowKeyHandler::WalkHandlersInternal 	content/xbl/src/nsXBLWindowKeyHandler.cpp:500
16 	XUL 	nsXBLWindowKeyHandler::WalkHandlers 	content/xbl/src/nsXBLWindowKeyHandler.cpp:361
17 	XUL 	nsXBLWindowKeyHandler::KeyPress 	content/xbl/src/nsXBLWindowKeyHandler.cpp:418
18 	XUL 	nsEventListenerManager::HandleEvent 	content/events/src/nsEventListenerManager.cpp:169
19 	XUL 	nsEventTargetChainItem::HandleEvent 	content/events/src/nsEventDispatcher.cpp:244
20 	XUL 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:330
21 	XUL 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:360
22 	XUL 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:539
23 	XUL 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6424
24 	XUL 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6194
25 	XUL 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6005
26 	XUL 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1210
27 	XUL 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1189
28 	XUL 	HandleEvent 	view/src/nsView.cpp:167
29 	XUL 	nsChildView::DispatchEvent 	widget/src/cocoa/nsChildView.mm:1785
30 	XUL 	nsChildView::DispatchWindowEvent 	widget/src/cocoa/nsChildView.mm:1797
31 	XUL 	-[ChildView processKeyDownEvent:keyEquiv:] 	widget/src/cocoa/nsChildView.mm:5145
32 	XUL 	-[ChildView performKeyEquivalent:] 	widget/src/cocoa/nsChildView.mm:5450
33 	XUL 	-[ChildView performKeyEquivalent:] 	widget/src/cocoa/nsChildView.mm:5386
34 	AppKit 	-[NSView performKeyEquivalent:] 	
35 	AppKit 	-[NSView performKeyEquivalent:] 	
36 	AppKit 	-[NSWindow performKeyEquivalent:] 	
37 	XUL 	-[ToolbarWindow performKeyEquivalent:] 	widget/src/cocoa/nsCocoaWindow.mm:1948
38 	AppKit 	-[NSApplication _handleKeyEquivalent:] 	
39 	AppKit 	-[NSApplication sendEvent:] 	
40 	AppKit 	-[NSApplication run] 	
41 	XUL 	nsAppShell::Run 	widget/src/cocoa/nsAppShell.mm:766
42 	XUL 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:193
43 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3392
44 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:156
45 	firefox-bin 	firefox-bin@0x1541 	
46 	firefox-bin 	firefox-bin@0x1468 	
47 		@0x0

Comment 5

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

Nothing to do with editor per se: the collapsed selections created by editor code exposed the real bug though. Collapsed ranges aren't always correctly handled when they are added at either extreme edge of the array.

There are two problems:
> 3757   if (endIndex == -1) {
> 3758     // All ranges start after the given range. We can insert our range at
> 3759     // position 0, knowing there are no overlaps (handled below)
That's generally true, however it is untrue if the range at position 0 is a collapsed range, and we're trying to add the same collapsed range. In that case, we should silently succeed. Thus the range equality test needs to be earlier. Currently, this code allows the collapsed range to be repeatedly added, which breaks all the invariants and eventually causes the second problem to exhibit itself.

Secondly, if the last range in the array is a collapsed range, (with at least one other range lying before it), and we try and add the same collapsed range, GetIndicesForInterval will return startIndex and endIndex such that startIndex = endIndex+1 (it would first have calculated startIndex and endIndex to both be the index of the last range, but the adjacency avoiding code increments startIndex). In particular, startIndex > mRanges.Length; coupled with startIndex != endIndex, this causes the overlap code to execute, which attempts to fetch the range at index startIndex, causing a read beyond the array bounds, and hence a crash.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 401639 [details] [diff] [review]
Patch v1

Thanks!

Comment 7

[Graeme McCutcheon [:graememcc]]

I'm holding off on pushing this for the moment - I've had some further thoughts (sadly after Robert had taken the time for the review - apologies).

Essentially, I'm thinking that GetRangesForIntervalCOMArray is going to similarly fail to return collapsed ranges at the extreme ends of the range array when passed a collapsed range to check (although note it wouldn't have worked correctly in the pre-348681 code either). 

I think it might be best to fix up GetIndices... to take into account collapsed ranges when calculating the indices, and catch this problem too.

Comment 8

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2

OK, this is the better fix. Fixes the same root problems as before, but does so by making GetIndicesForInterval smarter, which has the benefits of removing the need for special-casing collapsed ranges in AddItem, and fixes the equivalent bug for GetRangesForInterval, which would have still been incorrect with my first attempt. To show off, it fixes a couple of cases where GetRangesForInterval has never worked correctly.

The tests are greatly expanded to give both AddItem and GetIndicesForInterval a comprehensive shakedown.

Comment 9

[Graeme McCutcheon [:graememcc]]

Tree and time willing, I'll land this on 1.9.2 tomorrow.
http://hg.mozilla.org/mozilla-central/rev/2a772c89e07a

Comment 10

[Graeme McCutcheon [:graememcc]]

I pushed this to 1.9.2 () but had to backout () as everything-else unit tests went orange on win and linux with two "unexpected pass" errors:

2768 ERROR TEST-UNEXPECTED-PASS | chrome://mochikit/content/a11y/accessible/test_table_sels.html | no cell selected - got 0, expected 0
2769 ERROR TEST-UNEXPECTED-PASS | chrome://mochikit/content/a11y/accessible/test_table_sels.html | no cell selected - got 0, expected 0

Haven't had time to look at this in any detail. The relevant file is significantly different on trunk vs 1.9.2 branch which would explain why I didn't hit this before on trunk.

Do any of the accessibility guys know what's happening here? This patch ensures that duplicate ranges aren't added to the selection.

Comment 11

[Graeme McCutcheon [:graememcc]]

Er, meant to link to the changesets in those empty brackets.

Initial push: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2f61617809c2
Backout: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23f26d78b6f9

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To be honest, given it's an UNEXPECTED-PASS I'd be tempted to just mark those tests as passing (assuming they already pass or aren't run on Mac) and move on...

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Yeah, Graeme, feel free to edit those test_table_sels.html tests to expect passes on 1.9.2; thanks!

Comment 14

[Graeme McCutcheon [:graememcc]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3c91edd10b31 with the two-line fix to mark those tests as passing, and got 198 a11y failures for my troubles...

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1254252202.1254254576.1243.gz

Comment 15

[Graeme McCutcheon [:graememcc]]

Actually, I may have just been particularly unlucky and have ran in to the more extreme case of bug 508766. 

Not sure when I'm next going to have the cycles to try and get this in again.

Comment 16

[Graeme McCutcheon [:graememcc]]

Attachment: Branch patch

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You could just push it to the try servers (yes, you can push a branch to the try servers, and the right things happen) ... that would help you tell if the failures are random.

Comment 18

[Mats Palmgren (inactive)]

I pushed the 1.9.2 patch to the try server and it passed unit tests
on all platforms except Linux where there was an unrelated failure
(I believe), bug 519592.  So I pushed it to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/13c1a1135fa2