470565 - Hang [@ nsSplittableFrame::GetPrevInFlow] on print preview with visibility: collapse, line-height, word-spacing and tbody

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which hangs in print preview in current trunk build.
It also hangs in Firefox 3, but not in Firefox 2, I can look for a regression range, if wanted.

Stack from hang in a debug build:
>	gklayout.dll!nsSplittableFrame::GetPrevInFlow()  Line 155 + 0x8 bytes	C++
 	gklayout.dll!nsSplittableFrame::GetFirstInFlow()  Line 186 + 0x8 bytes	C++
 	gklayout.dll!nsTableRowFrame::CollapseRowIfNecessary(int aRowOffset=0, int aWidth=0, int aCollapseGroup=1, int & aDidCollapse=0)  Line 1152 + 0x2b bytes	C++
 	gklayout.dll!nsTableRowGroupFrame::CollapseRowGroupIfNecessary(int aYTotalOffset=0, int aWidth=0)  Line 877 + 0x18 bytes	C++
 	gklayout.dll!nsTableFrame::AdjustForCollapsingRowsCols(nsHTMLReflowMetrics & aDesiredSize={...}, nsMargin aBorderPadding={...})  Line 2191 + 0x10 bytes	C++
 	gklayout.dll!nsTableFrame::Reflow(nsPresContext * aPresContext=0x01c816b0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 1996	C++
 	gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x064eb10c, nsPresContext * aPresContext=0x01c816b0, 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!nsTableOuterFrame::OuterDoReflowChild(nsPresContext * aPresContext=0x01c816b0, nsIFrame * aChildFrame=0x064eb10c, const nsHTMLReflowState & aChildRS={...}, nsHTMLReflowMetrics & aMetrics={...}, unsigned int & aStatus=0)  Line 1109	C++
 	gklayout.dll!nsTableOuterFrame::Reflow(nsPresContext * aPresContext=0x01c816b0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aOuterRS={...}, unsigned int & aStatus=0)  Line 1250 + 0x22 bytes	C++

Comment 1

[Martijn Wargers (dead)]

Attachment: complete stack of hang in a debug build

Comment 2

[Bernd]

brilliant test case

Comment 3

[Bernd]

Attachment: patch

the hang occurs due to stack overflow that occurs due to a infinite number of splitted rowgroups that we create.

Comment 4

[Bernd]

securing  the bug

Comment 5

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

+  PRBool hasOneColumn;

Unused

+  PRBool hasOneColumn = PR_FALSE;

Wouldn't this be better as, say, "PRInt32 columnIndex = 0;"? "hasOneColumn" sounds like a property of the table but in fact it's just telling us whether our loop over the columns is currently at the first column.

-  /** return the width of the table taking into account visibility collapse
-    * on columns and colgroups
-    * @param aBorderPadding  the border and padding of the table
-    */
-  nscoord GetCollapsedWidth(nsMargin aBorderPadding);
+  

Trailing whitespace

+  /** return the width of the table row groups taking into account visibility
+    * collapse on columns and colgroups
+    */
+  nscoord GetCollapsedRowGroupWidth();
+  

Here too

+  nscoord collapseOffset = 0;

Unused

Comment 6

[Bernd]

Comment on attachment 354065 [details] [diff] [review]
patch

this will be fixed by the patch in bug 325292

Comment 7

[Bernd]

fixed by bug 325292

Comment 8

[Bernd]

it still hangs my nightly....

Comment 9

[Jesse Ruderman]

(In reply to comment #3)
> the hang occurs due to stack overflow that occurs due to a infinite number of
> splitted rowgroups that we create.

A stack buffer overflow, or running out of stack space?  Only the former is a security hole.

Comment 10

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

We probably need to re-fix this bug. Mats, will you be able to look at it anytime soon?

Comment 11

[Mats Palmgren (inactive)]

This doesn't look sg:critical to me.  nsSimplePageSequenceFrame::Reflow
loops forever, creating new continuation frames since the Reflow never
becomes fully complete.  Eventually we should get an OOM crash.

Here's what happens:
The "top:10%" style makes 'dependsOnCBHeight' true here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#397
which adds the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit on the table
(ancestor) at the end of the method.

This bit propagates to 'needToInitiateSpecialReflow' here:
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1785
the frame is complete after the 1st reflow so we do the 2nd where
it becomes incomplete.

The "visibility:collapse" style has made NeedToCollapse() true, so we do a
AdjustForCollapsingRowsCols() on line 1833.  Doing that on an incomplete
frame seems odd.

Comment 12

[Mats Palmgren (inactive)]

Attachment: wip1

CSS top/bottom doesn't apply unless we have a positioned frame,
so we shouldn't have set 'dependsOnCBHeight' true in the first place.
This patch should make the bug less likely to occur at least.

Comment 13

[Mats Palmgren (inactive)]

Attachment: wip3 (no good)

An attempt to fix the real bug.

Comment 14

[Mats Palmgren (inactive)]

Attachment: wip5

Don't split collapsed rows or row-groups.

Comment 15

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

+     (nsGkAtoms::positionedInlineFrame == frame->GetType() ||
+      ((frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
+       mStyleDisplay->IsAbsolutelyPositioned()))) ||

Why not just check mStyleDisplay->IsPositioned()?

Comment 16

[Mats Palmgren (inactive)]

I wanted to check the actual frame type, because sometimes we ignore
the specified 'position' during frame construction.  I'll have to check if
mStyleDisplay->IsPositioned() is good enough in this case.

Either way, isn't it a bug in the style system that
mOffset.GetTopUnit() has the specified value (Percent) in this case?
CSS 2.1 says that the computed value for 'top' etc is 'auto'
when 'position' is 'static' (which it is in this case).
http://www.w3.org/TR/CSS21/visuren.html#propdef-top
Should the style system have set it to eStyleUnit_Auto?

Comment 17

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

It doesn't really matter if we sometimes think a frame is positioned here when it really isn't. This is an optimization.

Comment 18

[Mats Palmgren (inactive)]

Ok, I understand, but it's essential to check for a positioned frame type
for the "wip1" wallpaper to work, ie avoid the troublesome table code.
I just tried mStyleDisplay->IsPositioned() and it hangs with just a
minor change to the testcase (adding position:relative).

If we are confident "wip5" will fix/avoid the real bug then we can
just skip "wip1" though.

Comment 19

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

How about "disp->IsPositioned() && !IsTableRelated(frame->GetType())". That's what nsCSSFrameConstructor::GetAbsoluteContainingBlock does for this.

Comment 20

[fantasai]

Comment on attachment 437351 [details] [diff] [review]
wip5

-    // See if the row fits on this page
-    if (rowRect.YMost() > availHeight) {
+    // See if the row doesn't fit on this page; a collapsed row always fits.
+    if (rowRect.YMost() > availHeight &&
+        rowFrame->GetStyleVisibility()->mVisible != 

There's a couple other uses of rowRect.YMost() towards the end of that method. It seems to me

     aDesiredSize.height = rowRect.YMost();

should be under an "if row is not collapsed" clause and

     isTopOfPage = isTopOfPage && rowRect.YMost() == 0;

should be (rowRect.YMost() == 0 || rowIsCollapsed). No?

   // See if all the frames fit. Do not try to split anything if we're
   // not paginated ... we can't split across columns yet.
-  if (aReflowState.mFlags.mTableIsSplittable &&
+  if (!collapseGroup &&
+      aReflowState.mFlags.mTableIsSplittable &&

Can be folded into one line.

Comment 21

[Martijn Wargers (dead)]

The testcase is wfm in current trunk build. Mark this bug wfm?

Comment 22

[fantasai]

Comment on attachment 437351 [details] [diff] [review]
wip5

Marking r- due to unanswered questions.

Comment 23

[Martijn Wargers (dead)]

The testcase is wfm, but I might be hitting this hang on other cases.

Comment 24

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

Comment on attachment 437130 [details] [diff] [review]
wip1

I presume this won't help because the testcase can just use position:relative to actually be positioned.

Comment 25

[Mats Palmgren (inactive)]

dveditz, I don't think this bug needs to be hidden.  It's just an infinite
stack recursion crash (DoS).  Please open it if you agree.

Comment 26

[Martijn Wargers (dead)]

Can this be marked worksforme?

Comment 27

[Guilherme Lima]

Can't reproduce on latest Nightly on Win7.