Closed Bug 585598 Opened 9 years ago Closed 9 years ago

Crash [@ nsContainerFrame::DestroyFrom] on print preview with select, float and page-break

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- final+

People

(Reporter: martijn.martijn, Assigned: dbaron)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files)

Attached file testcase
See testcase, which crashes current trunk build on print preview. It doesn't crash in a 2010-08-04 trunk build, so I guess it's a regression from bug 563584.

http://crash-stats.mozilla.com/report/index/bp-4755f15f-5db0-4293-9ad7-882a32100809
0  	xul.dll  	nsContainerFrame::DestroyFrom  	 layout/generic/nsContainerFrame.cpp:268
1 	xul.dll 	nsListControlFrame::DestroyFrom 	layout/forms/nsListControlFrame.cpp:219
2 	xul.dll 	nsFrameList::DestroyFramesFrom 	layout/generic/nsFrameList.cpp:98
3 	xul.dll 	nsComboboxControlFrame::DestroyFrom 	layout/forms/nsComboboxControlFrame.cpp:1218
4 	xul.dll 	nsIFrame::Destroy 	layout/generic/nsIFrame.h:531
5 	xul.dll 	nsFrameList::DestroyFrames 	layout/generic/nsFrameList.cpp:87
6 	xul.dll 	nsFrameList::Destroy 	layout/generic/nsFrameList.cpp:70
7 	xul.dll 	nsContainerFrame::DestroyFrameList 	layout/generic/nsContainerFrame.h:334
8 	xul.dll 	mozilla::FramePropertyTable::DeleteEnumerator 	layout/base/FramePropertyTable.cpp:248
9 	xul.dll 	nsTHashtable<nsIdentifierMapEntry>::s_EnumStub 	obj-firefox/dist/include/nsTHashtable.h:420
10 	xul.dll 	PL_DHashTableEnumerate 	obj-firefox/xpcom/build/pldhash.c:754
11 	xul.dll 	PresShell::Destroy 	layout/base/nsPresShell.cpp:1968
12 	xul.dll 	DocumentViewerImpl::DestroyPresShell 	layout/base/nsDocumentViewer.cpp:4319
13 	xul.dll 	DocumentViewerImpl::SetIsPrintPreview 	layout/base/nsDocumentViewer.cpp:4145
14 	xul.dll 	DocumentViewerImpl::ReturnToGalleyPresentation 	layout/base/nsDocumentViewer.cpp:4178
15 	xul.dll 	DocumentViewerImpl::ExitPrintPreview 	layout/base/nsDocumentViewer.cpp:3939
16 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
(In reply to comment #0)
> See testcase, which crashes current trunk build on print preview. It doesn't

Actually, it's crashing when closing print preview.
blocking2.0: --- → final+
Assignee: nobody → dbaron
So the basic problem here is that we have a view on something inside the float continuations list, but nsBlockFrame doesn't destroy its float continuations list in its DestroyFrom method; it instead waits for the property table destruction to happen.  But by this point the frame for the view's parent view has been destroyed... and, along with it, the view itself.  So when we try to destroy the property table we delete the float continuations list and its contents and end up trying to destroy the view a second time.

This actually isn't a problem in the dynamic case since PresShell::NotifyDestroyingFrame destroys the properties at the same time as the frame destruction, via its call to mPresContext->NotifyDestroyingFrame(aFrame).  But when mIgnoreFrameDestruction is true, as it is during teardown, it's a problem.

The obvious fix is to make sure nsBlockFrame deletes its float continuations list in its destroy method.

I should figure out whether the reordering done in bug 399994 was also done because of the float continuations list not being destroyed, or whether there is another list we need to fix.
That said, we also really shouldn't have a pushed floats list lying around after reflow is complete.
Additional bad things happening:
 * the outer float tries to fit on the first page, despite the page break frame before it, since it's zero height.  This might be fixed if we made the page break frames take up one extra appunit.
 * the outer float doesn't split despite that the inner float is truncated and pushed to the float continuations list -- this despite returning a reflow status of NS_FRAME_REFLOW_NEXTINFLOW | NS_FRAME_OVERFLOW_INCOMPLETE (this would be the cause of having a PushedFloats list still lying around)
This fixes the crash, but not the layout bug.  We should take it anyway; it's good crash defense even if there shouldn't really be any of these lists hanging around at frame destruction time.  (We already do the same for overflow lines, which are the same way.)

The crashtest doesn't crash in the harness even without the patch; I'm not sure why, but I'm not going to bother investigating.
Attachment #467060 - Flags: review?(roc)
This also (without patch 1) fixes the crash.  It also fixes the layout of the testcase.  We should split a float when it's overflow-incomplete.
Attachment #467061 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/f12e2c14fbe5
http://hg.mozilla.org/mozilla-central/rev/fe74a63c16b6
Status: NEW → RESOLVED
Closed: 9 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Verified fixed, using:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100822 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsContainerFrame::DestroyFrom]
You need to log in before you can comment on or make changes to this bug.