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)
Core
Layout: Tables
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: martijn.martijn, Assigned: bernd_mozilla)
References
(Depends on 1 open bug)
Details
(Keywords: hang, regression, testcase)
Attachments
(8 files)
316 bytes,
application/xhtml+xml
|
Details | |
22.33 KB,
text/plain
|
Details | |
20.19 KB,
text/html
|
Details | |
4.24 KB,
patch
|
Details | Diff | Splinter Review | |
933 bytes,
patch
|
Details | Diff | Splinter Review | |
264 bytes,
application/xhtml+xml
|
Details | |
2.09 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.58 KB,
patch
|
roc
:
feedback+
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•16 years ago
|
||
Updated•16 years ago
|
Component: Layout → Layout: Tables
OS: Windows XP → All
QA Contact: layout → layout.tables
Hardware: x86 → All
Comment 2•16 years ago
|
||
The kid's next-in-flow isn't our child frame, pushing it is bad.
Comment 3•16 years ago
|
||
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)
Reporter | ||
Comment 4•15 years ago
|
||
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.
Comment 7•15 years ago
|
||
> 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
Comment 8•15 years ago
|
||
This testcase shows 1 page containing both rows with the "hack" patch. Shouldn't "row2" be on a second page?
Updated•15 years ago
|
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)
Assignee | ||
Comment 10•15 years ago
|
||
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 11•15 years ago
|
||
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+
Updated•15 years ago
|
Assignee: nobody → bernd_mozilla
Assignee | ||
Comment 12•15 years ago
|
||
note: this should be tested against visibility collapse in combination with print.
Reporter | ||
Updated•15 years ago
|
Flags: blocking1.9.2?
Comment 14•15 years ago
|
||
Bernd, what's the status of this patch?
Assignee | ||
Comment 15•15 years ago
|
||
comment 12 indicates that it creates issues with visibility collapse and print but bug 325292 may help with this.
Assignee | ||
Comment 16•15 years ago
|
||
I will look at this after bug 325292 lands
Flags: blocking1.9.2? → wanted1.9.2+
Comment 17•15 years ago
|
||
Bernd in #16
> I will look at this after bug 325292 lands
fixed :)
Comment 18•14 years ago
|
||
May be related to 470974
Reporter | ||
Comment 19•11 years ago
|
||
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
Reporter | ||
Comment 21•11 years ago
|
||
Attachment #751844 -
Flags: feedback?(roc)
Reporter | ||
Updated•11 years ago
|
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+
Comment 23•10 years ago
|
||
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.
Description
•