Closed Bug 553972 Opened 15 years ago Closed 15 years ago

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

Categories

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

Product:
Core
Component:
Layout: Tables
Platform:
x86
All
Type:
defect

Tracking

()

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

People

(Reporter: chofmann, Unassigned)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(8 files)

page that demonstrates the crash
307.37 KB, text/html
Details
crashtest
407 bytes, text/html
Details
test case that works with print preview
435 bytes, text/html
Details
revised test case
437 bytes, text/html
Details
more reduced testcase
366 bytes, text/html
Details
minimal testcase
316 bytes, text/html
Details
revised crashtest from bug 554376
297 bytes, application/xhtml+xml
Details
fix
1.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
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
Keywords: regressionwindow-wanted
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 fix — — Splinter 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: 15 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: ? → ---
status1.9.2: --- → wanted
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.

Attachment

General

Creator:
Created:
Updated:
Size: