Closed Bug 553972 Opened 10 years ago Closed 10 years ago

Firefox 3.7a3pre Crash Report [@ XUL@0x2b2b97 ]

Categories

(Core :: Layout: Tables, defect, P2, critical)

x86
All
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status1.9.2 --- wanted

People

(Reporter: chofmann, Unassigned)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(8 files)

folks at University of Sao Paulo found reproducible crash/hang bug when trying to print or print preview a 621 Kbyte html document that is a report with with about 50 tables.  Test case coming up when we can sanitize the page and remove any personally identifiable information.

The stack trace looks like below and its reported to crash or ang both Mac and Linux .  Untested so far on windows.

http://crash-stats.mozilla.com/report/index/bp-2391203a-62b3-4267-b557-181b02100321

Frame  	Module  	Signature [Expand]  	Source
0 	XUL 	XUL@0x2b2b97 	
1 	XUL 	nsTableFrame::PushChildren 	layout/tables/nsTableFrame.cpp:1947
2 	XUL 	nsTableFrame::ReflowChildren 	layout/tables/nsTableFrame.cpp:2827
3 	XUL 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1889
4 	XUL 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1793
5 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
6 	XUL 	nsTableOuterFrame::Reflow 	layout/tables/nsTableOuterFrame.cpp:1010
7 	XUL 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:310
8 	XUL 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3085
9 	XUL 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2386
10 	XUL 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1887
11 	XUL 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:995
12 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
13 	XUL 	nsTableCellFrame::Reflow 	layout/tables/nsTableCellFrame.cpp:912
14 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
15 	XUL 	nsTableRowFrame::ReflowChildren 	layout/tables/nsTableRowFrame.cpp:911
16 	XUL 	nsTableRowFrame::Reflow 	layout/tables/nsTableRowFrame.cpp:1065
17 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
18 	XUL 	nsTableRowGroupFrame::ReflowChildren 	layout/tables/nsTableRowGroupFrame.cpp:420
19 	XUL 	nsTableRowGroupFrame::Reflow 	layout/tables/nsTableRowGroupFrame.cpp:1320
20 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
21 	XUL 	nsTableFrame::ReflowChildren 	layout/tables/nsTableFrame.cpp:2738
22 	XUL 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1889
23 	XUL 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1793
24 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
25 	XUL 	nsTableOuterFrame::Reflow 	layout/tables/nsTableOuterFrame.cpp:1010
26 	XUL 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:310
27 	XUL 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3085
28 	XUL 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2386
29 	XUL 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1887
30 	XUL 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:995
31 	XUL 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:310
32 	XUL 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3085
33 	XUL 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2386
34 	XUL 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1887
35 	XUL 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:995
36 	XUL 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:310
37 	XUL 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3085
38 	XUL 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2386
39 	XUL 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1887
40 	XUL 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:995
41 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
42 	XUL 	nsTableCellFrame::Reflow 	layout/tables/nsTableCellFrame.cpp:912
43 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
44 	XUL 	nsTableRowFrame::ReflowChildren 	layout/tables/nsTableRowFrame.cpp:911
45 	XUL 	nsTableRowFrame::Reflow 	layout/tables/nsTableRowFrame.cpp:1065
46 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
47 	XUL 	nsTableRowGroupFrame::ReflowChildren 	layout/tables/nsTableRowGroupFrame.cpp:420
48 	XUL 	nsTableRowGroupFrame::Reflow 	layout/tables/nsTableRowGroupFrame.cpp:1320
49 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
50 	XUL 	nsTableFrame::ReflowChildren 	layout/tables/nsTableFrame.cpp:2738
51 	XUL 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1889
52 	XUL 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1793
53 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
54 	XUL 	nsTableOuterFrame::Reflow 	layout/tables/nsTableOuterFrame.cpp:1010
55 	XUL 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:310
56 	XUL 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3085
57 	XUL 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2386
58 	XUL 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1887
59 	XUL 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:995
60 	XUL 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:310
61 	XUL 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3085
62 	XUL 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2386
63 	XUL 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1887
64 	XUL 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:995
65 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
66 	XUL 	nsCanvasFrame::Reflow 	layout/generic/nsCanvasFrame.cpp:505
67 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
68 	XUL 	nsPageContentFrame::Reflow 	layout/generic/nsPageContentFrame.cpp:103
69 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
70 	XUL 	nsPageFrame::Reflow 	layout/generic/nsPageFrame.cpp:139
71 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
72 	XUL 	nsSimplePageSequenceFrame::Reflow 	layout/generic/nsSimplePageSequence.cpp:280
73 	XUL 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:756
74 	XUL 	ViewportFrame::Reflow 	layout/generic/nsViewportFrame.cpp:285
75 	XUL 	PresShell::DoReflow 	layout/base/nsPresShell.cpp:7347
76 	XUL 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:7480
77 	XUL 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4811
78 	XUL 	nsPrintEngine::ReflowPrintObject 	layout/printing/nsPrintEngine.cpp:2037
79 	XUL 	nsPrintEngine::ReflowDocList 	layout/printing/nsPrintEngine.cpp:1866
80 	XUL 	nsPrintEngine::SetupToPrintContent 	layout/printing/nsPrintEngine.cpp:1675
81 	XUL 	nsPrintEngine::DoCommonPrint 	layout/printing/nsPrintEngine.cpp:1507
82 	XUL 	nsPrintEngine::CommonPrint 	layout/printing/nsPrintEngine.cpp:446
83 	XUL 	nsPrintEngine::Print 	layout/printing/nsPrintEngine.cpp:764
84 	XUL 	DocumentViewerImpl::Print 	layout/base/nsDocumentViewer.cpp:3676
85 	XUL 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
86 	XUL 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2727
87 	XUL 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1770
88 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1370
89 	libmozjs.dylib 	js_InternalInvoke 	js/src/jsinterp.cpp:1435
90 	libmozjs.dylib 	JS_CallFunctionValue 	js/src/jsapi.cpp:4951
91 	XUL 	XPC_NW_FunctionWrapper 	js/src/xpconnect/src/XPCNativeWrapper.cpp:553
92 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1370
93 	libmozjs.dylib 	js_Interpret 	js/src/jsops.cpp:2277
94 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1378
95 	libmozjs.dylib 	js_InternalInvoke 	js/src/jsinterp.cpp:1435
96 	libmozjs.dylib 	JS_CallFunctionValue 	js/src/jsapi.cpp:4951
97 	XUL 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2161
98 	XUL 	nsJSEventListener::HandleEvent 	dom/src/events/nsJSEventListener.cpp:228
99 	XUL 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1082
100 	XUL 	nsEventListenerManager::HandleEvent 	content/events/src/nsEventListenerManager.cpp:1198
101 	XUL 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:201
102 	XUL 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:601
103 	XUL 	nsEventDispatcher::DispatchDOMEvent 	content/events/src/nsEventDispatcher.cpp:664
104 	XUL 	nsEventListenerManager::DispatchEvent 	content/events/src/nsEventListenerManager.cpp:1325
105 	XUL 	nsDOMEventRTTearoff::DispatchEvent 	content/base/src/nsGenericElement.cpp:1622
106 	XUL 	nsContentUtils::DispatchXULCommand 	content/base/src/nsContentUtils.cpp:5305
107 	XUL 	nsXULElement::PreHandleEvent 	content/xul/content/src/nsXULElement.cpp:1640
108 	XUL 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:257
109 	XUL 	nsEventDispatcher::DispatchDOMEvent 	content/events/src/nsEventDispatcher.cpp:664
110 	XUL 	nsEventListenerManager::DispatchEvent 	content/events/src/nsEventListenerManager.cpp:1325
111 	XUL 	nsDOMEventRTTearoff::DispatchEvent 	content/base/src/nsGenericElement.cpp:1622
112 	XUL 	nsMenuUtilsX::DispatchCommandTo 	widget/src/cocoa/nsMenuUtilsX.mm:82
113 	XUL 	-[NativeMenuItemTarget menuItemHit:] 	widget/src/cocoa/nsMenuBarX.mm:857
114 	AppKit 	-[NSApplication sendAction:to:from:] 	
115 	AppKit 	-[NSMenu performActionForItemAtIndex:] 	
116 	AppKit 	-[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:] 	
117 	AppKit 	AppKitMenuEventHandler 	
118 	HIToolbox 	DispatchEventToHandlers 	
119 	HIToolbox 	SendEventToEventTargetInternal 	
120 	HIToolbox 	SendEventToEventTarget 	
121 	HIToolbox 	SendHICommandEvent 	
122 	HIToolbox 	SendMenuCommandWithContextAndModifiers 	
123 	HIToolbox 	SendMenuItemSelectedEvent 	
124 	HIToolbox 	FinishMenuSelection 	
125 	HIToolbox 	MenuSelectCore 	
126 	HIToolbox 	_HandleMenuSelection2 	
127 	HIToolbox 	_HandleMenuSelection 	
128 	AppKit 	_NSHandleCarbonMenuEvent 	
129 	AppKit 	_DPSNextEvent 	
130 	AppKit 	-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] 	
131 	AppKit 	-[NSApplication run] 	
132 	XUL 	nsAppShell::Run 	widget/src/cocoa/nsAppShell.mm:863
133 	XUL 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:182
134 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3548
135 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:158
136 	firefox-bin 	firefox-bin@0xbf5 	
137 		@0x0
Severity: normal → critical
Keywords: crash
OS: Mac OS X → All
1. open the attached page
2. print | preview

crash
Keywords: testcase
I get tons of assertion-spam before the crash.  Here are the assertions I got, in increasing order of count-per-assertion.  The top two assertions (with count=1) are the final ones before the crash.

> 1    ###!!! ASSERTION: Broken frame linkage: 'prevSibling && prevSibling->GetNextSibling() == aFrame', file layout/generic/nsFrameList.cpp, line 132
> 1    ###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', file layout/base/../generic/nsIFrame.h, line 906
> 4    ###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.: 'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file layout/generic/nsBlockFrame.cpp, line 1368
> 900  ###!!! ASSERTION: How did that happen?: '!mContent->GetPrimaryFrame()', file layout/base/nsCSSFrameConstructor.cpp, line 11334
> 7316 ###!!! ASSERTION: Why did we get called?: '!aChildContent->GetPrimaryFrame() || aChildContent->GetPrimaryFrame()->GetContent() != aChildContent', file layout/base/nsCSSFrameConstructor.cpp, line 2171
> 8252 ###!!! ASSERTION: Losing track of existing primary frame: '!aFrame || !mPrimaryFrame || aFrame == mPrimaryFrame', file ../../dist/include/nsIContent.h, line 886
A more-reduced testcase would be helpful in tracking down what's going on.

NOTE: I just tested this bug's testcase in Firefox3.0, 3.5, and 3.6 builds, and it just hangs on print-preview in all of those. (I let them hang for at least 5 min before giving up on them)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.19pre) Gecko/2010032604 GranParadiso/3.0.19pre
In Firefox 3.5
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3pre) Gecko/20100326 Namoroka/3.6.3pre

So, the change from hang to crash is a regression since the 3.6 branch. (not that a hang is much better, from a user's perspective.)

A reduced testcase and a regression window on the hang --> crash change would both be helpful here.
Keywords: qawanted
wfm on windows with DIN-A4 pages
Attached file crashtest
this is a crashtest  so it needs a reftest manifest and a debug build to trigger the asserts
the height of the div might require adjustment for letter size pages
Attached file revised test case
the thead is essential, the tfoot can been replaced by a tbody.
Attached file more reduced testcase
it looks like a thead during pagination is enough to trigger 

NS_PRECONDITION(!aChildContent->GetPrimaryFrame() ||
                 aChildContent->GetPrimaryFrame()->GetContent() != aChildContent,
                 "Why did we get called?");
Attached file minimal testcase
If my reading of mxr is correct then the testcase reveals that we do not have reftest-print tests that test <thead>
Boris, it looks that the thead copying is fundamentally wrong, could you please shed some light on the issue.
The test case does cause the frequent asserts from comment 2 but not the lethal ones. However the frequent asserts are not covered by a already file bug and look like a serious frame construction issue.
Hmm.  The primary frame asserts are almost certainly a regression from bug 500882.  The thead/tfoot replication actually goes through the normal frame construction codepaths for the thead/tfoot.  That means that in the old world it clobbered the primary frame mapping for non-table-internal 
(and non-text and maybe non-inline) nodes with the descendants of the last thead/tfoot.  In the new world that clobbering asserts.

The bad news is that if I fix the assert in the obvious way, so by not clobbering, I still get the null-deref crash.  We should spin off a separate bug on the asserts.

The real cause for that null-deref crash is that in nsTableFrame::PushChildren the rgFrame is not in mFrames.  And that happens because the aRowGroups passed to PushChildren contains a particular frame twice:

(gdb) p aRowGroups[0]
$13 = (nsTableRowGroupFrame * const&) @0xbfff2c10: 0x1b712df0
(gdb) p aRowGroups[1] 
$14 = (nsTableRowGroupFrame * const&) @0xbfff2c14: 0x1b5963c0
(gdb) p aRowGroups[2]
$15 = (nsTableRowGroupFrame * const&) @0xbfff2c18: 0x1b5963c0
(gdb) p aRowGroups[3]
$16 = (nsTableRowGroupFrame * const&) @0xbfff2c1c: 0x1b718b88
(gdb) p aRowGroups[4]
$17 = (nsTableRowGroupFrame * const&) @0xbfff2c20: 0x1b7159c8

Note [1] and [2].
In particular a <thead> ends up in aRowGroups twice.
I tested with the testcase in comment 1.  I let the hangs sit for 5 minutes before killing them.

Regression range 1:

works - print preview opens properly:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9a1) Gecko/20060920 Minefield/3.0a1
2006-09-20-04-trunk

broken - hang:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9a1) Gecko/20060921 Minefield/3.0a1
2006-09-21-04-trunk

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-20+03%3A00%3A00&maxdate=2006-09-21+05%3A00%3A00&cvsroot=%2Fcvsroot


Regression range 2:

broken - hang:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091002 Minefield/3.7a1pre
http://hg.mozilla.org/mozilla-central/rev/0aeac787a86d

broken - crash:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091003 Minefield/3.7a1pre
http://hg.mozilla.org/mozilla-central/rev/0a7dd88dbe67

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0aeac787a86d&tochange=0a7dd88dbe67
Right.  Going from hang to crash happened due to bug 512336 landing; after that the attempt to remove a frame from a framelist the frame is not on crashes.

Going from ok to hang is almost certainly a regression from bug 347367, given the regression range given in comment 15...
Its certainly bug 347367, although before we did probably  not  mark as incomplete and just did not paginate and lost content.
OK, in nsTableFrame::ReflowChildren we have this code:

      // Put the nextinflow so that it will get pushed
      rowGroups.InsertElementAt(childX + 1,
                           static_cast <nsTableRowGroupFrame*>(kidNextInFlow));

But we're hitting that code when kidNextInFlow is already in rowGroups sometimes.

On a probably-unrelated note, I see a number of asserts like this:

###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.: 'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsBlockFrame.cpp, line 1382

on this testcase as well...
That said, sometimes we get to this line when kidFrame already had a next-in-flow (and that next-in-flow is in mFrames), but the next-in-flow is NOT in rowGroups...  I don't see how that can happen yet.
Oh, I see.  OrderRowGroups leaves next-in-flows out of the array it generates.  But in that case I'm not sure how we can end up with next-in-flows in the array to start with....
(gdb) p mFrames.mFirstChild
$12 = (nsTableRowGroupFrame *) 0x21e751f0
(gdb) p mFrames.mFirstChild->GetNextSibling()
$13 = (nsTableRowGroupFrame *) 0x21e77dc8
(gdb) p mFrames.mFirstChild->GetNextSibling()->GetNextSibling()
$14 = (nsTableRowGroupFrame *) 0x17bc9c0
(gdb) p mFrames.mFirstChild->GetNextSibling()->GetNextSibling()->GetNextSibling()
$15 = (nsTableRowGroupFrame *) 0x21e7af88
(gdb) p mFrames.mFirstChild->GetNextSibling()->GetNextSibling()->GetNextSibling()->GetNextSibling()
$16 = (Cannot access memory at address 0x0
(gdb) p mFrames.mFirstChild->GetNextInFlow()
$17 = (nsTableRowGroupFrame *) 0x17bc9c0
(gdb) p mFrames.mFirstChild->mContent->Tag()->mString
$19 = (PRUnichar *) 0x135bcdf8
(gdb) pu $19
$20 = 0x1bf7f9a0 "thead"

So the next-in-flow of the thead is NOT its next sibling, and as a result we end up not skipping over it during OrderRowGroups.  We could fix OrderRowGroups to deal, but is this "next-in-flow is not next sibling" deal ok?  The actual next-sibling is a tfoot.
The bogus framelist is created at this point:

#6  0x124f85ed in nsIFrame::SetNextSibling (this=0x2177a3c8, aNextSibling=0x215bc9c0) at nsIFrame.h:945
#7  0x125cdd1b in nsFrameList::InsertFrames (this=0x217777a0, aParent=0x0, aPrevSibling=0x2177a3c8, aFrameList=@0x1c6e5b60) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsFrameList.cpp:228
#8  0x12588059 in nsFrameList::AppendFrames (this=0x217777a0, aParent=0x0, aFrameList=@0x1c6e5b60) at nsFrameList.h:126
#9  0x125aae51 in nsContainerFrame::MoveOverflowToChildList (this=0x21777768, aPresContext=0x1232c00) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsContainerFrame.cpp:1293
#10 0x12779f30 in nsTableFrame::Reflow (this=0x21777768, aPresContext=0x1232c00, aDesiredSize=@0xbfff2d54, aReflowState=@0xbfff2c04, aStatus=@0xbfff3184) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/tables/nsTableFrame.cpp:1740

This is grabbing the overflow frames from the next-in-flow.  At this point, mFrames has two frames in it: a <thead> and a <tfoot>.  The |overflowFrames| that's being pulled also has two frames in it: a <thead> (whose prev continuation is our existing thead) and a <tbody>.

So given the whole model where the thead might overflow while the tfoot does not, it seems like for tables pulling from the overflow list back into mFrames needs to be a lot smarter, or something.
blocking2.0: --- → ?
(In reply to comment #21)
> is this "next-in-flow is not next sibling" deal ok?  The actual
> next-sibling is a tfoot.

In general that is possible, yes.

(In reply to comment #23)
> This is grabbing the overflow frames from the next-in-flow.  At this point,
> mFrames has two frames in it: a <thead> and a <tfoot>.  The |overflowFrames|
> that's being pulled also has two frames in it: a <thead> (whose prev
> continuation is our existing thead) and a <tbody>.
> 
> So given the whole model where the thead might overflow while the tfoot does
> not, it seems like for tables pulling from the overflow list back into mFrames
> needs to be a lot smarter, or something.

We probably should pull back everything and then immediately push any next-in-flows of existing children back to the overflow list.

In general I don't trust our table pagination code to handle more than one reflow of a given frame, especially when header/footer replication is going on. That's why we don't allow table breaking inside columns. Why are we re-reflowing a table in this case?
https://bugzilla.mozilla.org/attachment.cgi?id=434277 is a reduced testcase for the lethal assertions and the crash
That testcase is in bug 554376.
Blocks: 554376
I removed any float references from the test case and replaced table pseudo frame construction by content based frame construction. The position of thead behind tbody is essential for crashing.
> Why are we re-reflowing a table in this case?
We create next-in-flows during unconstrained height reflows 
see http://mxr.mozilla.org/mozilla-central/ident?i=isPaginated
for a starting pointer and  http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowGroupFrame.cpp#364 for comment on the point. This is the cause for the unconstrained height asserts.
I filed bug 558575 for the issue in comment 10
(In reply to comment #28)
> > Why are we re-reflowing a table in this case?
> We create next-in-flows during unconstrained height reflows 
> see http://mxr.mozilla.org/mozilla-central/ident?i=isPaginated
> for a starting pointer and 
> http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowGroupFrame.cpp#364
> for comment on the point. This is the cause for the unconstrained height
> asserts.

I'm not completely sure what you mean. Do you mean that all those isPaginated checks should be checking for constrained height instead?

I still don't understand why we've created next-in-flows for a table and then we're reflowing it again and needing to pull back those next-in-flows.
During a unconstrained height reflow we look if the pres context is paginated and if so we honor the CSS page-break properties and create a next inflow. Tables handle the page-break properties different from block layout not during CSS frame construction but during reflow. The code is a wild mix of pres-context pagination checks and checks for unconstrained height just a short snippet from nsTableFrame.cpp:

    * line 1760 --  if (isPaginated && !GetPrevInFlow() && (NS_UNCONSTRAINEDSIZE != aReflowState.availableHeight)) {
    * line 2655 -- if (isPaginated) {
    * line 2676 -- (isPaginated || (kidFrame->GetStateBits() &
    * line 2687 -- if (isPaginated && (NS_UNCONSTRAINEDSIZE != kidAvailSize.height)) {
    * line 2747 -- if (NS_FRAME_IS_COMPLETE(aStatus) && isPaginated &&
    * line 2779 -- if (NS_FRAME_IS_COMPLETE(aStatus) && isPaginated &&
OK, that's exciting. I suspect we should only honor the page-break properties if the height is constrained.

I'm still unclear on the last part of comment 30.
Attached patch fixSplinter Review
this probably does not answer 
>I'm still unclear on the last part of comment 30.
However it fixes the test case from bug 554376. I did never see the crash on the initial test case here. The funny thing is to reftest this (aka bug 558575)
Attachment #438358 - Attachment is patch: true
Is the patch ready to be checked in?
>Is the patch ready to be checked in?

I dunno.

This part of the code is not covered by a reftest, so although it seems logical it might cause regressions, its like coding in 2001. When trying to write reftests  bug 558575) I found two bugs (bug 558574) so it might take long to get this code really covered.
1319   // Reflow the existing frames. 
1320   PRBool splitDueToPageBreak = PR_FALSE;
1321   rv = ReflowChildren(aPresContext, aDesiredSize, state, aStatus,
1322                       &splitDueToPageBreak);
1323 
1324   // See if all the frames fit. Do not try to split anything if we're
1325   // not paginated ... we can't split across columns yet.
1326   if (aReflowState.mFlags.mTableIsSplittable &&
1327       (NS_FRAME_NOT_COMPLETE == aStatus || splitDueToPageBreak || 
1328        (NS_UNCONSTRAINEDSIZE != aReflowState.availableHeight &&
1329        aDesiredSize.height > aReflowState.availableHeight))) {
1330     // Nope, find a place to split the row group 

The code logic that the patch changes is broken also with the patch, the splitDueToPageBreak variable will report only page breaks that occur at the edges of rows and row groups, however if the page break occurs inside a cell it will not get noticed (see https://bugzilla.mozilla.org/attachment.cgi?id=439756)
The code will not reflow the row group content with a constrained height if the unconstrained reflow reports that it would fit on the page, which violates a invariant that during a constrained height reflow all content is reflowed with a constrained height to give it a opportunity to split.
blocking2.0: ? → final+
Priority: -- → P2
 bug 558575 and bug 558574 are now scheduled for review, and I pushed this bug together with the patches in  bug 558575 and bug 558574 over the try server
I'm going to land the assert fix I mention in comment 13 as part of the fix for bug 563837.
I will land this bug and  bug 558575 and bug 558574 on saturday
http://hg.mozilla.org/mozilla-central/rev/f0206483a592
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
would it make sense to land this on 1.9.2 at some point if it shakes out well on the trunk, or is it too risky or difficult to backport?
blocking1.9.2: --- → ?
The backport is fairly easy, the risk is that we hork printing tables.
After this bakes on trunk please nominate for approval.
blocking1.9.2: ? → ---
Crash Signature: [@ XUL@0x2b2b97 ]
Issue is resolved - clearing old keywords - qa-wanted clean-up
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.