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

Status: REOPENED

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

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

Comment 1

[Daniel Holbert [:dholbert]]

I get hang & "Someone forgot" assertion on my Linux debug build, too.
 Platform --> All/All

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

This testcase is modified a bit, with the style separated out for easy tweaking.

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Daniel Holbert [:dholbert]]

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?

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> Basically, ReflowChild always returns with "status" = NS_OVERFLOW_INCOMPLETE
(when print-previewing testcase 2)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Turns out we don't need the 'position: absolute' in the testcase.  Here's a testcase without that.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Daniel Holbert [:dholbert]]

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.

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: testcase 4: using <table> & <td>

For simplicity, this testcase uses a <table> instead of a div with "display:table".

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v1w (no-whitespace-changes version)

here's the "diff -w" version of this patch

Comment 16

[Daniel Holbert [:dholbert]]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are you saying we put the table's margin-top and padding-top on every page containing part of the table?

Comment 18

[Daniel Holbert [:dholbert]]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: broken reftest output

Here's the failing output of the reftests I've just attached, from an up-to-date mozilla-central build.

Comment 22

[Daniel Holbert [:dholbert]]

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

Comment 23

[Daniel Holbert [:dholbert]]

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.

Comment 24

[Daniel Holbert [:dholbert]]

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

Comment 25

[Daniel Holbert [:dholbert]]

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

Comment 26

[Daniel Holbert [:dholbert]]

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])

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Unless this is found to occur in a real Web page, I think we should just not fix it for 1.9.1.

Comment 28

[Daniel Holbert [:dholbert]]

Unassigning, as I haven't been actively working on this for a while.

Comment 29

[Bernd]

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.

Comment 30

[Andrei Purice]

This issue is still reproducible for me on the latest versions of Firefox beta 86.0 and release 85.0.2 on Windows 10, but not on Nightly 87.0a1 (2021-02-16).
Updating the flags accordingly.

Comment 31

[Daniel Holbert [:dholbert]]

Testcases 2, 3, and 4 render fine for me Nightly print-preview now. (They also render fine in Nightly 2020-06-01, so they've been fixed for a while.)

But I can still reproduce a hang using the original testcase here, using Nightly 88 and Beta 87. So I suspect comment 30 may've been mistaken about 87 being unaffected; perhaps Andrei was using testcases 2, 3, or 4.

In any case, I don't think we can call this "critical" severity at this point; let's classify it as S3.

Comment 32

[Daniel Holbert [:dholbert]]

Yeah, mozregression shows that testcases 2/3/4 were fixed ~10 years ago with this pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb4b93331e4f&tochange=b5b082d183d0

In that range, the fix would likely have been from...

Bug 659828 - Part 1: Apply table margins to the outer table frame instead of the inner table frame (also fixes bug 87277)
...given that the testcases here all have large margins on the table.

The original testcase has large padding instead of large margin, and that still does seem to cause the hang.