278983 - [FIX] Print Preview crashes: table+thead+page-break-before:always

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Gyuris Gellért]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; hu-HU; rv:1.7.5) Gecko/20041108 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; hu-HU; rv:1.7.5) Gecko/20041108 Firefox/1.0

If I want to see the Print Preview of a page which 
- contains not only a tbody but a thead or a tfoot tag as well and
- tbody's css setting is the following: "page-break-before:always;
page-break-after:always; page-break-inside:avoid"
then the program does not respond any more. If there is only a tbody without
thead and tfoot then the problem does not appear. 

Reproducible: Always

Steps to Reproduce:
To reproduce the bug do the following steps:
1. open the attached file #1
2. choose the File => Print Preview menupoint.




The problem is rather important because if I have more Firefox instances running
in several separate windows then all of them will hang up.

Comment 1

[Gyuris Gellért]

Attachment: reproduces the bug because the head is in it

Comment 2

[Gyuris Gellért]

Attachment: does not show the bug because the head is NOT in it

Comment 3

[Mats Palmgren (inactive)]

I have a fix for row-groups, need to check other elements...
We could have the same problem with 'display:none' here as in bug 277035 too.

Comment 4

[Mats Palmgren (inactive)]

'display:none' is not a problem since table page-breaks are done in reflow.
'visibility:collapse' should apply page breaks as far as I read the spec, which
we do, so no problem there either.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 8

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days at least to get to this....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172225 [details] [diff] [review]
Patch rev. 1

> nsTableFrame::PageBreakAfter(nsIFrame& aSourceFrame,
>                              nsIFrame* aNextFrame)
>+  // don't allow a page break after a repeated element ...
>+  if (display->mBreakAfter &&
>+      !(aSourceFrame.GetStateBits() & NS_REPEATED_ROW_OR_ROWGROUP)) {
>+    return !(aNextFrame &&
>+             (aNextFrame->GetStateBits() & NS_REPEATED_ROW_OR_ROWGROUP)); // or before

Are we guaranteed that aSourceFrame and aNextFrame are "table elements" of some
sort?  If so, please assert that here (assert on the frame type?).

r+sr=bzbarsky if we are guaranteed this.

Comment 10

[Mats Palmgren (inactive)]

Good question, I'm not sure. 'nsTableFrame::PageBreakAfter' can only be
reached through 'nsTableFrame::ReflowChildren', 'nsTableRowGroupFrame::Reflow'
or 'nsTableRowGroupFrame::ReflowChildren'. Looking at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.604&root=/cvsroot&mark=3176-3177#3149

It looks like 'childX' should be a 'nsTableRowGroupFrame' - looks ok to me.

Looking at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.338&root=/cvsroot&mark=329,331,339#302
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.338&root=/cvsroot&mark=437,441#436

Here it looks like there is a path to 'nsTableFrame::PageBreakAfter' for
non-row children.

I think I need to add a test on the frame type before looking at those bits.

Bernd, do you know for sure the possible types of frames on a row-group
child list?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, with a frame type check this should definitely be good.

Comment 12

[Bernd]

>Bernd, do you know for sure the possible types of frames on a row-group
child list?
expect anything, I dont think that it is a really good idea to check for a frame
state bit without beeing sure about the frame type.

I would rather prefer to see a helper function

IsRepeatedFrame(nsIFrame* kidFrame) or something like this
which checks first for the display type and then for the frame state bit and
returns a boolean

Comment 13

[Bernd]

Mats could you roll this in?

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

1. Check frame type before looking at bits.
2. Added IsRepeatedFrame() as suggested by Bernd.
(only layout/tables/nsTableFrame.cpp has changed compared to Patch rev. 1)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173432 [details] [diff] [review]
Patch rev. 2

>Index: layout/tables/nsTableFrame.cpp
> nsTableFrame::PageBreakAfter(nsIFrame& aSourceFrame,
>                              nsIFrame* aNextFrame)
> {

So... wouldn't the logic be simpler (though equivalent) as:

if (IsRepeatedFrame(&aSourceFrame)) {
  return PR_FALSE;
}

if (aNextFrame && IsRepeatedFrame(aNextFrame)) {
  return PR_FALSE;
}

// check display values here, and don't worry about repeating
// frames

r=bzbarsky either way...

Comment 16

[Mats Palmgren (inactive)]

I kept my version with the lame excuse it's slightly more efficient,
since mBreakAfter/mBreakBefore are almost always false IsRepeatedFrame is rarely
called.

Checked in 2005-02-04 20:23 PDT

-> FIXED