Closed Bug 470851 Opened 16 years ago Closed 11 years ago

Hang [@ nsPropertyTable::GetPropertyListFor] on print preview with table elements, page-break-after, position: relative and line-height

Categories

(Core :: Layout: Tables, defect)

defect
Not set
critical

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: martijn.martijn, Assigned: bernd_mozilla)

References

(Depends on 1 open bug)

Details

(Keywords: hang, regression, testcase)

Attachments

(8 files)

Attached file testcase
See testcase, which hangs on print preview in current trunk build and Firefox 3.
It doesn't hang in Firefox 2, I can look for a regression range, if wanted.

In a debug build, I see these assertions before the hang:
###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.:
'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file c:/mozilla-build-1.
3/src/layout/generic/nsBlockFrame.cpp, line 1413
###!!! ASSERTION: loop in frame list.  This will probably hang soon.: 'Error', f
ile c:/mozilla-build-1.3/src/layout/generic/nsFrameList.cpp, line 600

The stack of the hang looks like this:
>	gklayout.dll!nsPropertyTable::GetPropertyListFor(unsigned short aCategory=0, nsIAtom * aPropertyName=0x01d7ed68)  Line 295 + 0x11 bytes	C++
 	gklayout.dll!nsPropertyTable::GetPropertyInternal(nsPropertyOwner aObject={...}, unsigned short aCategory=0, nsIAtom * aPropertyName=0x01d7ed68, int aRemove=0, unsigned int * aResult=0x00000000)  Line 190 + 0x11 bytes	C++
 	gklayout.dll!nsPropertyTable::GetProperty(nsPropertyOwner aObject={...}, nsIAtom * aPropertyName=0x01d7ed68, unsigned int * aResult=0x00000000)  Line 114	C++
 	gklayout.dll!nsContainerFrame::GetOverflowFrames(nsPresContext * aPresContext=0x04eaa988, int aRemoveProperty=0)  Line 1164	C++
 	gklayout.dll!nsContainerFrame::GetFirstChild(nsIAtom * aListName=0x01d7c2c8)  Line 319 + 0x13 bytes	C++
 	gklayout.dll!nsBlockFrame::CollectFloats(nsIFrame * aFrame=0x04203710, nsFrameList & aList={...}, nsIFrame * * aTail=0x001284a8, int aFromOverflow=0, int aCollectSiblings=1)  Line 6709 + 0x21 bytes	C++
 	gklayout.dll!nsBlockFrame::CollectFloats(nsIFrame * aFrame=0x0420379c, nsFrameList & aList={...}, nsIFrame * * aTail=0x001284a8, int aFromOverflow=0, int aCollectSiblings=1)  Line 6709	C++
 	gklayout.dll!nsBlockFrame::CollectFloats(nsIFrame * aFrame=0x04203754, nsFrameList & aList={...}, nsIFrame * * aTail=0x001284a8, int aFromOverflow=0, int aCollectSiblings=1)  Line 6709	C++
 	gklayout.dll!nsBlockFrame::PushLines(nsBlockReflowState & aState={...}, nsLineList_iterator aLineBefore={...})  Line 4128	C++
 	gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x00128ad0)  Line 3099	C++
 	gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x00128ad0)  Line 2272 + 0x1b bytes	C++
 	gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...})  Line 1907 + 0x1b bytes	C++
 	gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x04eaa988, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 954 + 0xf bytes	C++
 	gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x04c98838, nsPresContext * aPresContext=0x04eaa988, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=60, int aY=60, unsigned int aFlags=16, unsigned int & aStatus=0, nsOverflowContinuationTracker * aTracker=0x00000000)  Line 793 + 0x21 bytes	C++
 	gklayout.dll!nsTableCellFrame::Reflow(nsPresContext * aPresContext=0x04eaa988, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0)  Line 937	C++
etc...
Component: Layout → Layout: Tables
OS: Windows XP → All
QA Contact: layout → layout.tables
Hardware: x86 → All
Attached file Frame dump
The kid's next-in-flow isn't our child frame, pushing it is bad.
Attached patch Patch rev. 1Splinter Review
The fix:
+          if (kidNextInFlow->GetParent() == this) {

Additionally:
 1. add an assertion in PushChildren that the child frames are found
 2. cleanup the use of kidNextInFlow/reorder variables
Assignee: nobody → mats.palmgren
Attachment #354310 - Flags: review?(bernd_mozilla)
Thanks for that patch, Mats, that seems to fix a lot of the hangs I see on print preview.
How does this patch address the first assertion ###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.:
'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file
c:/mozilla-build-1.3/src/layout/generic/nsBlockFrame.cpp, line 1413, which is usually the root of wrong next in flow relationships. According to the reflow log that preceedes this 
the 

      <tfoot>
        <tr style="page-break-after: left;"></tr>
        <tbody style="line-height: 999px;">m</tbody>
      </tfoot>

marks as incomplete also if the row group has infinite space to layout.
I applied the patch and the test case did still assert like hell, due to this.
> marks as incomplete also if the row group has infinite space to layout

Isn't that correct if there is a page break inside it?
AIUI, it should split and have a next-in-flow on the next page containing
the second row.  It doesn't look like which row to break after is
propagated to SplitRowGroup() though, so I'm probably missing something.
I'll leave you to it.
Assignee: mats.palmgren → nobody
Attached file Terstcase #2
This testcase shows 1 page containing both rows with the "hack" patch.
Shouldn't "row2" be on a second page?
Attachment #354310 - Flags: review?(bernd_mozilla)
the code from bug 24000 is simply wrong, but what would you expect from a thorough review (a=asa, sr=attinasi, r=alexsavulov)
Attached patch next revSplinter Review
This fixes the second testcase rendering. (it also fixes the rendering with respect to the current situation). I believe that during a unconstrained height reflow one should never create a next in flow. Nor should a frame mark as incomplete during this reflow.
Attachment #355105 - Flags: review?(mats.palmgren)
Comment on attachment 355105 [details] [diff] [review]
next rev

FYI, it looks like it now triggers a false alarm about data loss:
WARNING: data loss - complete row needed more height than available, on top of page: file layout/tables/nsTableRowGroupFrame.cpp, line 1166

r=mats but please include the assertion in my patch that
RemoveFrame() must find the frame.
Also, the 2nd and 3rd hunks makes the code more readable
and are worth taking IMO.
Attachment #355105 - Flags: review?(mats.palmgren) → review+
Assignee: nobody → bernd_mozilla
note: this should be tested against visibility collapse in combination with print.
Flags: blocking1.9.2?
Bernd, what's the status of this patch?
comment 12 indicates that it creates issues with visibility collapse and print but bug 325292
 may help with this.
I will look at this after bug 325292 lands
Flags: blocking1.9.2? → wanted1.9.2+
Bernd in #16
> I will look at this after bug 325292 lands

fixed :)
May be related to 470974
Depends on: 470974
The testcase doesn't seem to hang anymore on print preview. Can this bug be marked worksforme?
Yes!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
Attachment #751844 - Flags: feedback?(roc)
Flags: in-testsuite?
Comment on attachment 751844 [details] [diff] [review]
crashtest for checkin

Review of attachment 751844 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks!
Attachment #751844 - Flags: feedback?(roc) → feedback+
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3c4038c9e9
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.