Selecting a table row and pasting in the editor crashes Firefox

RESOLVED FIXED in mozilla1.9.3a1



10 years ago
9 years ago


(Reporter: sheppy, Assigned: graememcc)


({crash, regression})

1.9.2 Branch
crash, regression
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)



(2 attachments, 1 obsolete attachment)



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


Comment 1

10 years ago
Windows XP crash report:

Looking for regression window now...
Severity: normal → critical
Keywords: crash, regression, regressionwindow-wanted
OS: Mac OS X → All
Version: 1.9.1 Branch → 1.9.2 Branch

Comment 3

10 years ago
Regression from bug 348681.
Blocks: 348681
Flags: blocking1.9.2?
Keywords: regressionwindow-wanted

Comment 4

10 years ago
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 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 Version	10.5.8 9L30
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 10
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/
30 	XUL 	nsChildView::DispatchWindowEvent 	widget/src/cocoa/
31 	XUL 	-[ChildView processKeyDownEvent:keyEquiv:] 	widget/src/cocoa/
32 	XUL 	-[ChildView performKeyEquivalent:] 	widget/src/cocoa/
33 	XUL 	-[ChildView performKeyEquivalent:] 	widget/src/cocoa/
34 	AppKit 	-[NSView performKeyEquivalent:] 	
35 	AppKit 	-[NSView performKeyEquivalent:] 	
36 	AppKit 	-[NSWindow performKeyEquivalent:] 	
37 	XUL 	-[ToolbarWindow performKeyEquivalent:] 	widget/src/cocoa/
38 	AppKit 	-[NSApplication _handleKeyEquivalent:] 	
39 	AppKit 	-[NSApplication sendEvent:] 	
40 	AppKit 	-[NSApplication run] 	
41 	XUL 	nsAppShell::Run 	widget/src/cocoa/
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
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2


9 years ago
Component: Editor → Selection
QA Contact: editor → selection


9 years ago
Assignee: nobody → graememcc_firefox

Comment 5

9 years ago
Created attachment 401639 [details] [diff] [review]
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.
Attachment #401639 - Flags: review?(roc)

Comment 7

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

9 years ago
Created attachment 402677 [details] [diff] [review]
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.
Attachment #401639 - Attachment is obsolete: true
Attachment #402677 - Flags: review?(roc)

Comment 9

9 years ago
Tree and time willing, I'll land this on 1.9.2 tomorrow.
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1

Comment 10

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

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

Initial push:
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...
Yeah, Graeme, feel free to edit those test_table_sels.html tests to expect passes on 1.9.2; thanks!

Comment 14

9 years ago
Pushed with the two-line fix to mark those tests as passing, and got 198 a11y failures for my troubles...

Comment 15

9 years ago
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.
Keywords: checkin-needed
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.
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:
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Flags: in-testsuite+
Duplicate of this bug: 517968
You need to log in before you can comment on or make changes to this bug.