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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Martijn Wargers (dead)]

(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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind's explanation of the problem

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, we also really shouldn't have a pushed floats list lying around after reflow is complete.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: destroy the pushed floats

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: split floats when they're overflow incomplete

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/f12e2c14fbe5
http://hg.mozilla.org/mozilla-central/rev/fe74a63c16b6

Comment 9

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100822 Minefield/4.0b5pre