Closed Bug 510575 Opened 16 years ago Closed 16 years ago

Selecting a table row and pasting in the editor crashes Firefox

Categories

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

Product:
Core
Component:
DOM: Selection
Version:
1.9.2 Branch
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: sheppy, Assigned: graememcc)

References

Details

(Keywords: crash, regression)

Attachments

(2 files, 1 obsolete file)

Patch v2
35.22 KB, patch
roc
: review+
Details | Diff | Splinter Review
Branch patch
36.35 KB, patch
Details | Diff | Splinter Review
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
Windows XP crash report: bp-9e103573-3085-49b9-9ec9-436782090815 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
Regression from bug 348681.
Blocks: 348681
Flags: blocking1.9.2?
Keywords: regressionwindow-wanted
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
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Status: NEW → ASSIGNED
Component: Editor → Selection
QA Contact: editor → selection
Assignee: nobody → graememcc_firefox
Attached patch Patch v1 (obsolete) — — Splinter Review
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)
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.
Attached patch Patch v2 — — Splinter Review
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)
Tree and time willing, I'll land this on 1.9.2 tomorrow. http://hg.mozilla.org/mozilla-central/rev/2a772c89e07a
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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.
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
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!
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
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
Attached patch Branch patch — — Splinter Review
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: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/13c1a1135fa2
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: