Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with table, padding-bottom and height > line-height

REOPENED
Unassigned

Status

()

P3
critical
REOPENED
10 years ago
2 years ago

People

(Reporter: martijn.martijn, Unassigned)

Tracking

({hang, regression, testcase})

Trunk
hang, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 353899 [details]
testcase

See testcase, which hangs current trunk build on print preview.

It doesn't hang in a 2008-09-07 build, but it does hang in a 2008-09-08 build:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-07+04%3A00%3A00&enddate=2008-09-08+11%3A00%3A00
I guess a regression from bug 243519 somehow.


In a debug build this assertion keeps coming by:
###!!! ASSERTION: Someone forgot a REFLOW_NEXTINFLOW flag: 'frameStatus & NS_FRA
ME_REFLOW_NEXTINFLOW', file c:/mozilla-build-1.3/src/layout/generic/nsContainerF
rame.cpp, line 1022

Stack of hang from debug build:
	gklayout.dll!nsSplittableFrame::IsInPrevContinuationChain(nsIFrame * aFrame1=0x05ff1474, nsIFrame * aFrame2=0x060c5504)  Line 137 + 0x10 bytes	C++
 	gklayout.dll!nsSplittableFrame::SetPrevInFlow(nsIFrame * aFrame=0x060c5224)  Line 161 + 0xd bytes	C++
 	gklayout.dll!nsSplittableFrame::Init(nsIContent * aContent=0x00000000, nsIFrame * aParent=0x060c54c0, nsIFrame * aPrevInFlow=0x060c5224)  Line 58	C++
 	gklayout.dll!nsContainerFrame::Init(nsIContent * aContent=0x00000000, nsIFrame * aParent=0x060c54c0, nsIFrame * aPrevInFlow=0x060c5224)  Line 92 + 0x15 bytes	C++
 	gklayout.dll!ViewportFrame::Init(nsIContent * aContent=0x00000000, nsIFrame * aParent=0x060c54c0, nsIFrame * aPrevInFlow=0x060c5224)  Line 62	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructPageFrame(nsIPresShell * aPresShell=0x04b3c598, nsPresContext * aPresContext=0x05ba84c8, nsIFrame * aParentFrame=0x04c1b688, nsIFrame * aPrevPageFrame=0x060c51e0, nsIFrame * & aPageFrame=0x060c54c0, nsIFrame * & aCanvasFrame=0x012750ac)  Line 4679	C++
 	gklayout.dll!nsCSSFrameConstructor::CreateContinuingFrame(nsPresContext * aPresContext=0x05ba84c8, nsIFrame * aFrame=0x060c51e0, nsIFrame * aParentFrame=0x04c1b688, nsIFrame * * aContinuingFrame=0x0012cf5c, int aIsFluid=1)  Line 10428 + 0x20 bytes	C++
 	gklayout.dll!nsSimplePageSequenceFrame::CreateContinuingPageFrame(nsPresContext * aPresContext=0x05ba84c8, nsIFrame * aPageFrame=0x060c51e0, nsIFrame * * aContinuingPage=0x0012cf5c)  Line 163	C++
 	gklayout.dll!nsSimplePageSequenceFrame::Reflow(nsPresContext * aPresContext=0x05ba84c8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 315 + 0x1a bytes	C++
 	gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x04c1b688, nsPresContext * aPresContext=0x05ba84c8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0, int aY=0, unsigned int aFlags=3, unsigned int & aStatus=0, nsOverflowContinuationTracker * aTracker=0x00000000)  Line 793 + 0x21 bytes	C++
 	gklayout.dll!nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState * aState=0x0012d450, int aAssumeHScroll=0, int aAssumeVScroll=1, nsHTMLReflowMetrics * aMetrics=0x0012d3ac, int aFirstPass=1)  Line 528 + 0x30 bytes	C++
 	gklayout.dll!nsHTMLScrollFrame::ReflowContents(ScrollReflowState * aState=0x0012d450, const nsHTMLReflowMetrics & aDesiredSize={...})  Line 622 + 0x35 bytes	C++
 	gklayout.dll!nsHTMLScrollFrame::Reflow(nsPresContext * aPresContext=0x05ba84c8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 823 + 0x13 bytes	C++
 	gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x04c1b884, nsPresContext * aPresContext=0x05ba84c8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0, int aY=0, unsigned int aFlags=0, unsigned int & aStatus=0, nsOverflowContinuationTracker * aTracker=0x00000000)  Line 793 + 0x21 bytes	C++
 	gklayout.dll!ViewportFrame::Reflow(nsPresContext * aPresContext=0x05ba84c8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 283 + 0x2d bytes	C++
 	gklayout.dll!PresShell::DoReflow(nsIFrame * target=0x04c1b5f0)  Line 6384	C++
 	gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=0)  Line 6485	C++
 	gklayout.dll!PresShell::DoFlushPendingNotifications(mozFlushType aType=Flush_Layout, int aInterruptibleReflow=0)  Line 4593	C++
 	gklayout.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Layout)  Line 4522	C++
 	gklayout.dll!nsPrintEngine::ReflowPrintObject(nsPrintObject * aPO=0x05d5bba8)  Line 1979	C++
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
I get hang & "Someone forgot" assertion on my Linux debug build, too.
 Platform --> All/All
Assignee: nobody → dholbert
OS: Windows XP → All
Hardware: x86 → All
Summary: Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with position: absolute, display: table, padding-bottom and height → Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with position: absolute, display: table, padding-bottom and percent height
Created attachment 355876 [details]
testcase 2

This testcase is modified a bit, with the style separated out for easy tweaking.
(In reply to comment #0)
> It doesn't hang in a 2008-09-07 build, but it does hang in a 2008-09-08 build:
> http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-07+04%3A00%3A00&enddate=2008-09-08+11%3A00%3A00
> I guess a regression from bug 243519 somehow.

Adding regression keyword.
Keywords: regression
Summary: Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with position: absolute, display: table, padding-bottom and percent height → Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with position: absolute, display: table, padding-bottom and height > line-height
The second testcase actually has an earlier regression range, between these builds:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008040804 Minefield/3.0pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre

Bonsai link, for 'layout' subdirectory: http://tinyurl.com/7qfh96
Possibly bug 411585?
(In reply to comment #4)
> Possibly bug 411585?

Nope -- from doing a few CVS checkouts to investigate, it looks like this was caused by bug 422678's checkin.
Blocks: 422678
The hang is in the main loop of nsSimplePageSequence::Reflow [ url: http://tinyurl.com/83qtbr ] -- we never exit that loop.

The gist of that loop is this (with many lines deleted):
273   for (nsIFrame* kidFrame = mFrames.FirstChild(); nsnull != kidFrame; ) {
289     ReflowChild(kidFrame, aPresContext, kidSize, kidReflowState, x, y, 0, status);
300     if (NS_FRAME_IS_FULLY_COMPLETE(status)) {
301       NS_ASSERTION(nsnull == kidNextInFlow, "bad child flow list");
302     } else if (nsnull == kidNextInFlow) {
306       nsresult rv = CreateContinuingPageFrame(aPresContext, kidFrame,
307                                               &continuingPage);
314     }
317     kidFrame = kidFrame->GetNextSibling();
318   }

Basically, ReflowChild always returns with "status" = NS_OVERFLOW_INCOMPLETE, and so we always create a continuing page frame and try again. This leaves us creating continuing page frames forever.
(In reply to comment #6)
> Basically, ReflowChild always returns with "status" = NS_OVERFLOW_INCOMPLETE
(when print-previewing testcase 2)
Created attachment 356995 [details]
testcase 3

Turns out we don't need the 'position: absolute' in the testcase.  Here's a testcase without that.
Created attachment 357005 [details] [diff] [review]
workaround: comment out line that regressed this

This patch comments out a "NS_FRAME_SET_INCOMPLETE" statement that was added in the regressing changeset (bug 422678).

This patch fixes the hang on all the testcases.  (probably not the right fix; just a demonstration of the root of the regression)
So in mozilla-central, we hit the aforementioned NS_FRAME_SET_INCOMPLETE over and over.  The first time we hit it, we have:
  aReflowState.availableHeight = 0
  cellMaxHeight = 1800 = CSS 'line-height' value
  styleHeight   = 1860 = CSS 'height' value

which results in
  aDesiredSize.height = 1800

Every subsequent time we hit that line, we have:
   aReflowState.availableHeight = 0
   cellMaxHeight =  0
   styleHeight =   60 = CSS 'height' value - CSS 'line-height' value

which results in
  aDesiredSize.height = 0

which is bad, because it means we never actually give ourselves that last 60px of height, and we spin forever.

I think the 0-valued cellMaxHeight might be the root of the problem... If I manually set cellMaxHeight to 60 (which crucially ends up making aDesiredSize.height = 60), then the hang goes away.
For reference, here's a MXR link for the chunk I talk about above:
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowFrame.cpp#1015
Created attachment 357089 [details] [diff] [review]
workaround #2: use GetSize to calculate cellMaxHeight 

I'm not actually sure why we use desiredSize rather than the cell's actual size (GetSize()) to compute cellMaxHeight...  If we used GetSize() instead, would that prevent us from splitting cells across page boundaries, or something?

In any case, just for fun, this workaround uses GetSize() to compute cellMaxHeight.
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Summary: Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with position: absolute, display: table, padding-bottom and height > line-height → Hang [@ nsSplittableFrame::IsInPrevContinuationChain] on print preview with table, padding-bottom and height > line-height
Created attachment 362622 [details]
testcase 4: using <table> & <td>

For simplicity, this testcase uses a <table> instead of a div with "display:table".
Created attachment 362817 [details] [diff] [review]
fix v1: if availHeight == cellMaxHeight == 0, request full styleHeight to avoid infinite loop

I think this is the right fix. I'll give more details / justification in a few minutes.
Created attachment 362818 [details] [diff] [review]
fix v1w (no-whitespace-changes version)

here's the "diff -w" version of this patch
Attachment #362818 - Attachment description: fix v1 (no-whitespace-changes version) → fix v1w (no-whitespace-changes version)
So, to explain the patch -- here's the problematic chunk of code:

1015   else { // constrained height, paginated
1016     // Compute the height we should have from style (subtracting the
1017     // height from our prev-in-flows from the style height)
1018     nscoord styleHeight = CalcHeightFromUnpaginatedHeight(aPresContext, *this);
1019     if (styleHeight > aReflowState.availableHeight) {
1020       styleHeight = aReflowState.availableHeight;
1021       NS_FRAME_SET_INCOMPLETE(aStatus);
1022     }
1023     aDesiredSize.height = PR_MAX(cellMaxHeight, styleHeight);
1024   }
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowFrame.cpp#1015

That chunk basically says "If we've got more specified-height left for this row (& its continuations), then give as much height as we can to this row.  And if we don't use up all the remaining specified height, request another page."

But that logic gets us into trouble when the table's margin and/or padding occupy the full page height -- when that's the case, aReflowState.availableHeight will always be 0 at this point in the code.  When cellMaxHeight is 0 as well (which it is when this page's continuation-cell has no content), then desiredHeight ends up as 0, and we loop forever. (Case in point: see comment 10)

This patch simply adds a clause for this edge case, making us bail out (with a warning) and simply use all the remaining styleHeight immediately, rather than looping forever.
Attachment #362818 - Flags: superreview?(roc)
Attachment #362818 - Flags: review?(roc)
Are you saying we put the table's margin-top and padding-top on every page containing part of the table?
(In reply to comment #17)
> Are you saying we put the table's margin-top and padding-top on every page
> containing part of the table?

Yes, that's correct. (It's correct for margin-bottom & padding-bottom, too -- i.e. we also put those on every page containing part of the table.)

Maybe that's the real problem that we should fix here.  If we only ever allow ourselves to use one full copy of the margin & padding, I think that would fix this problem in all cases except for a pathological case with a 0-height page-content area (which users might accidentally run into if they make a typo while setting up their print settings, e.g. entering in 15in instead of 1.5in for one of the page margins).
Yeah, we should stop doing that. We don't do it for blocks; block top-padding and top-margin only appears once, on the first page containing the block.
Attachment #362818 - Flags: superreview?(roc)
Attachment #362818 - Flags: review?(roc)
Created attachment 363626 [details] [diff] [review]
reftests to test margin-/padding-, bottom/top

Here are some new reftests that more pointedly test the issue raised in comment 17 - comment 19.  The only difference between each testcase & reference case is the "display: table" declaration.

I've also included the crashtest from my previously posted patch.
Created attachment 363627 [details]
broken reftest output

Here's the failing output of the reftests I've just attached, from an up-to-date mozilla-central build.
Created attachment 363797 [details] [diff] [review]
reftests v2 (testing margin-/padding-/border- bottom/top)

As might be expected, we actually have this same problem for border-top and border-bottom, too.  i.e. we reserve space for border-top/border-bottom on every page that contains a piece of the table (though we don't actually _draw_ the top/bottom border on every page -- we just reserve empty space).

Here's an updated reftest-patch with some border tests included as 470495-5.html and 470495-6.html.
Attachment #363626 - Attachment is obsolete: true
So here's the current state of the code for handling "margin-top" / "margin-bottom" on paginated tables vs. paginated divs.

For tables, we subtract the top & bottom margins from the availableHeight before reflowing the table.  (We do this on every page). That happens in nsTableOuterFrame.cpp, here:
 1056       availHeight -= margin.top;
 [snip]
 1059       availHeight -= margin.bottom;
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableOuterFrame.cpp#1046


For divs, on the other hand, we subtract *just the top margin* from the availableHeight, here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockReflowContext.cpp#267
  267       aFrameRS.availableHeight -= mTopMargin.get() + aClearance;

We don't apply any margin on the div's continuations, though, because "mTopMargin" ends up being zero for all subsequent continuations.  That's because of how we set "applyTopMargin" here:
  2803   PRBool applyTopMargin =
  2804     !frame->GetPrevInFlow() && ShouldApplyTopMargin(aState, aLine);
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#2799

That "applyTopMargin" boolean ends up controlling whether we put anything in aState.mPrevBottomMargin, and that's what mTopMargin is based on.

For divs, we don't really bother reserving *any* space for the *bottom* margin.  If it fits on the page (after reflowing the div into the available space), then that's great.  If it doesn't, we let the margin run off the page, and we place the next piece of content at the top of the next page.
Created attachment 366383 [details] [diff] [review]
fix v2 wip

Here's a WIP version of a better fix for this.

The strategy is to make tables match divs, in how we handle margin/border/padding & splitting across pages.

In particular, when we're reflowing a table that may be split across pages, I think this is the desirable behavior:
MARGIN HANDLING:
 - Subtract margin-top from available height, but _only on the first page_.
 - Never subtract margin-bottom from available height.
 - After table is reflowed, place the margin-bottom on the last page that contains a chunk of the table.  If margin-bottom extends past the end of that page, just ignore any chunk of it that falls off.

BORDERPADDING HANDLING:
 - Subtract margin-top from available height, but _only on the first page_.
 - After we've reflowed the table, try adding the bottom borderpadding to its desired height to see if that fits in the available height.
    * If it fits, cool. We're done.
    * If it doesn't fit, then push the bottom borderpadding to the next page.
    * If the borderpadding-bottom doesn't fit (by itself) on the next page (i.e. if the borderpadding is > 1 page tall), I'm not sure what we do for divs right now... perhaps we should align the border with the bottom of the page, and let any excess border/padding get clipped off the top of that page.

In this attached patch, I haven't finished off the borderpadding-bottom handling yet (and there are a few XXX comments) but I think everything else is basically correct.
Attachment #362817 - Attachment is obsolete: true
Attachment #362818 - Attachment is obsolete: true
Attachment #363797 - Attachment is obsolete: true
(In reply to comment #24)
> BORDERPADDING HANDLING:
>  - Subtract margin-top from available height, but _only on the first page_.

(oops, make that "Subtract borderpadding-top...")
Sorry I haven't updated this bug for a little while -- I've put this on the back-burner because I don't think we should land a comprehensive fix for this until post-1.9.1.

To completely fix this bug (as outlined in comment 19 through this comment), we'd need to handle a wider range of cases than I'd originally thought.  "fix v2" just attempts to handle margin/border/padding on the <table> element itself, but a complete fix would really need to handle those properties on cells, rows, rowgroups, and captions, as well as handling the effects of the "border-spacing" and "border-collapse" properties.  That's a scary amount of new table behavior to introduce into the final(?) 1.9.1 beta, so I think this should wait until post-1.9.1.

As for the original infinite-loop hang on this bug, though -- that itself is a regression, and fix v1 is a good workaround for it, I think.  Roc, would you be open to taking fix v1 as a band-aid for the hang, with the addition of an XXX comment noting that it should be removed when we've cleaned up multipage table margin/border/padding?
(fix v1 is attachment 362817 [details] [diff] [review])
Unless this is found to occur in a real Web page, I think we should just not fix it for 1.9.1.
Unassigning, as I haven't been actively working on this for a while.
Assignee: dholbert → nobody

Comment 29

7 years ago
re comment 23

the code moved slightly to 
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableOuterFrame.cpp#862

I think if the either the bottom border padding or the margin do not fit we need to create necessary continuations for the table frames to handle them inside table code and not to rely on a opportunity to this later "probably after parent's "brc.PlaceBlock" call" at a higher level.
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.