Closed
Bug 428263
Opened 16 years ago
Closed 16 years ago
Crash [@ nsCellMapColumnIterator::GetNextFrame] with table root, -moz-column
Categories
(Core :: Layout: Block and Inline, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: roc)
References
Details
(Keywords: assertion, crash, testcase, Whiteboard: [sg:dos] null deref)
Crash Data
Attachments
(9 files)
424 bytes,
text/html
|
Details | |
1.20 KB,
patch
|
Details | Diff | Splinter Review | |
1.20 KB,
patch
|
Details | Diff | Splinter Review | |
13.56 KB,
text/plain
|
Details | |
168 bytes,
text/html
|
Details | |
181 bytes,
text/html
|
Details | |
254 bytes,
text/html
|
Details | |
1.16 KB,
text/plain
|
Details | |
3.24 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
Loading the testcase triggers: ###!!! ASSERTION: Shouldn't be incomplete if availableHeight is UNCONSTRAINED.: 'aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 1403 ###!!! ASSERTION: Bogus mOrigCells?: 'mCurMapRow < mCurMapRelevantRowCount', file /Users/jruderman/trunk/mozilla/layout/tables/nsCellMap.cpp, line 2928 Null deref [@ nsCellMapColumnIterator::GetNextFrame]. The crash seems similar to the crash in bug 393936, but the testcase is pretty different. Gary Kwong found this bug and I made the reduced testcase.
Comment 1•16 years ago
|
||
We're ending up with the anonymous cellframe incomplete (that's the first assert there), then we split the table... after that, things are more or less DOA, aren't they?
This does not fix the root cause of the columns asking strangely to split (what the first assert rightfully states). It rather hardens the table against a attack from below, we had this before from above. In other words ==> Floriani Principle.
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attachment #315047 -
Flags: superreview?(roc)
Attachment #315047 -
Flags: review?(roc)
Assignee | ||
Comment 4•16 years ago
|
||
Won't the split cellframe still cause problems, probably killing us in some testcases? If so, what purpose does this patch serve?
We do not create split cell frames in this case. The only way to split cell frames is to go through SplitRowGroup. SplitRowGroup is protected by the same check (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.408&mark=1358,1366#1356). So what I propose here is to adapt to the fact that the block frame below the cell frame sometimes thinks that it is incomplete while it has infinite height to lay out, what we do not expect ever to happen. This is certainly wallpaper. However I would like to deal with this crash in the 1.9 time frame and I am not confident when reading bug 414255 that the root cause will be fixed before 1.9. My intention is to reassign the bug to block layout once the table side is secured.
Comment on attachment 315047 [details] [diff] [review] 80 signs this is garbage, we should fix the block issue first. We have here a nice 0 deref, no security impact yet.
Attachment #315047 -
Flags: superreview?(roc)
Attachment #315047 -
Flags: review?(roc)
Below follows my first guess at columnset handling (see the reflow for the details) what happens here is that we start with a unconstrained height till we reduced the height down to 38 twips. On the next step we do not fit but we create a nextinflow of the second column as it is not complete. Then we increase again the height and recognize that we need to go back to 38. Now we pull back the continuation of the first column. This is the old second column. But now the second column has a nextinflow. We reflow it again but in contrast to the first run we have now 3 children of the columnsetframe the columnset should have only 2 columns so regardless what we do we will end with one column too much. And then the impossible happens while we have infinite space the bogus third column does not fit. I think what DrainOverflowColumns http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsColumnSetFrame.cpp&rev=3.51&mark=744-749#716 is doing is not sufficient. What should happen here is to get the nextinflow on the overflow list pushed back.
Assignee: bernd_mozilla → nobody
Status: ASSIGNED → NEW
Component: Layout → Layout: Block and Inline
QA Contact: layout → layout.block-and-inline
Assignee | ||
Comment 9•16 years ago
|
||
I didn't fully understand that. What should happen is that while things fit in a column, nsBlockFrame::ReflowDirtyLines should pull lines from the following columns into itself, and if there no more to pull it should mark itself complete which will lead to the destruction of all next-in-flows. Looking at the code in ReflowDirtyLines, I see if (!bifLineIter.Next() || !bifLineIter.GetLine()->IsDirty()) { that actaully looks wrong, I think it should be "if (bifLineIter.Next() && ...)". It looks like the current code is going to mark the current frame incomplete if there is no next line. Does fixing that help?
Comment 10•16 years ago
|
||
>Does fixing that help?
No we do not hit the code in this case, as aState.mNextInFlow is null,
and we see the following block reflow
block 04D7A8C8 Reflow a=1,38 c=1,UC dirty v-resize nif=04D7BE0C cnt=9759
Block(body)(1)@04D7A8C8: begin reflow availSize=1,38 computedSize=1,1073741824
Block(body)(1)@04D7A8C8: reflowing dirty lines computedWidth=1
line=04D7B814 mY=0 dirty=no oldBounds={0,0,1,37} oldCombinedArea={0,0,1,37} deltaY=0 mPrevBottomMargin=0 childCount=1
GetAvailableSpace: band=0,0,1,38 count=1
block 04D7B67C Reflow a=1,38 c=1,60 dirty v-resize nif=04D7BE64 cnt=9760
Block(div)(0)@04D7B67C: begin reflow availSize=1,38 computedSize=1,60
Block(div)(0)@04D7B67C: reflowing dirty lines computedWidth=1
Block(div)(0)@04D7B67C: done reflowing dirty lines (status=0)
Block(div)(0)@04D7B67C: status=1 (not complete) metrics=1,38 carriedMargin=0
block 04D7B67C Reflow d=1,38 status=0x1
line=04D7B814 mY=38 dirty=no oldBounds={0,0,1,38} oldCombinedArea={0,0,1,38} deltaY=0 mPrevBottomMargin=0 childCount=1
Block(body)(1)@04D7A8C8: done reflowing dirty lines (status=3)
Block(body)(1)@04D7A8C8: status=3 (not complete) metrics=1,38 carriedMargin=0
block 04D7A8C8 Reflow d=1,38 status=0x3
block 04D7BE0C Reflow a=1,38 c=1,UC dirty v-resize pif=04D7A8C8 nif=04D7BF64 cnt=9761
Block(body)(1)@04D7BE0C: begin reflow availSize=1,38 computedSize=1,1073741824
Block(body)(1)@04D7BE0C: reflowing dirty lines computedWidth=1
line=04D7BEBC mY=0 dirty=no oldBounds={0,0,1,23} oldCombinedArea={0,0,1,23} deltaY=0 mPrevBottomMargin=0 childCount=1
GetAvailableSpace: band=0,0,1,38 count=1
block 04D7BE64 Reflow a=1,38 c=1,60 dirty v-resize pif=04D7B67C cnt=9762
Block(div)(0)@04D7BE64: begin reflow availSize=1,38 computedSize=1,60
Block(div)(0)@04D7BE64: reflowing dirty lines computedWidth=1
Block(div)(0)@04D7BE64: done reflowing dirty lines (status=0)
Block(div)(0)@04D7BE64: status=0 (complete) metrics=1,22 carriedMargin=0
block 04D7BE64 Reflow d=1,22
line=04D7BEBC mY=22 dirty=no oldBounds={0,0,1,22} oldCombinedArea={0,0,1,22} deltaY=-1 mPrevBottomMargin=0 childCount=1
line=04D7B83C mY=22 dirty=yes oldBounds={0,0,0,0} oldCombinedArea={0,0,0,0} deltaY=-1 mPrevBottomMargin=0 childCount=1
GetAvailableSpace: band=0,22,1,16 count=1
block 04D7B7BC GetMinWidth
block 04D7B7BC GetMinWidth=0
block 04D7B7BC GetPrefWidth
block 04D7B7BC GetPrefWidth=0
block 04D7B7BC Reflow a=1,UC c=0,UC dirty v-resize cnt=9765
Block(span)(1)@04D7B7BC: begin reflow availSize=1,1073741824 computedSize=0,1073741824
Block(span)(1)@04D7B7BC: reflowing dirty lines computedWidth=0
Block(span)(1)@04D7B7BC: done reflowing dirty lines (status=0)
clear floats: in: aY=0(0)
clear floats: out: y=0(0)
Block(span)(1)@04D7B7BC: status=0 (complete) metrics=0,0 carriedMargin=0
block 04D7B7BC Reflow d=0,0
Line reflow status = LINE_REFLOW_OK
line=04D7B83C mY=22 dirty=yes oldBounds={0,0,0,0} oldCombinedArea={0,0,0,0} deltaY=-1 mPrevBottomMargin=0 childCount=1
Block(body)(1)@04D7BE0C: done reflowing dirty lines (status=1)
Block(body)(1)@04D7BE0C: status=3 (not complete) metrics=1,38 carriedMargin=0
block 04D7BE0C Reflow d=1,38 status=0x3
ColumnSet(body)(1) 02CB9C38 Reflow d=960,38 status=0x3o=(0,0) 962 x 38sto=(0,0) 962 x 38
line=04D7A9F4 mY=518 dirty=no oldBounds={480,480,960,38} oldCombinedArea={480,480,962,38} deltaY=0 mPrevBottomMargin=0 childCount=1
Block(html)(-1)@04D7A7A0: done reflowing dirty lines (status=3)
Assignee | ||
Comment 11•16 years ago
|
||
Block(body)(1)@04D7BE0C: status=3 (not complete) metrics=1,38 carriedMargin=0 So why did this return "not complete"? Its child line reflowed OK and was complete, so this columns should have been complete too.
Comment 12•16 years ago
|
||
no the line above states lock(body)(1)@04D7BE0C: done reflowing dirty lines (status=1) so this line is not complete and we have overflow lines so we http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.950&mark=1055-1057#1050 mark the block as not complete
Comment 13•16 years ago
|
||
Block(body)(1)@04E4597C: begin reflow availSize=1,38 computedSize=1,1073741824 Block(body)(1)@04E4597C: reflowing dirty lines computedWidth=1 line=04E45A2C mY=0 dirty=no oldBounds={0,0,1,23} oldCombinedArea={0,0,1,23} deltaY=0 mPrevBottomMargin=0 childCount=1 GetAvailableSpace: band=0,0,1,38 count=1 block 04E459D4 Reflow a=1,38 c=1,60 dirty v-resize pif=04E451EC cnt=9751 Block(div)(0)@04E459D4: begin reflow availSize=1,38 computedSize=1,60 Block(div)(0)@04E459D4: reflowing dirty lines computedWidth=1 Block(div)(0)@04E459D4: done reflowing dirty lines (status=0) Block(div)(0)@04E459D4: status=0 (complete) metrics=1,22 carriedMargin=0 block 04E459D4 Reflow d=1,22 line=04E45A2C mY=22 dirty=no oldBounds={0,0,1,22} oldCombinedArea={0,0,1,22} deltaY=-1 mPrevBottomMargin=0 childCount=1 line=04E453AC mY=22 dirty=yes oldBounds={0,0,0,0} oldCombinedArea={0,0,0,0} deltaY=-1 mPrevBottomMargin=0 childCount=1 GetAvailableSpace: band=0,22,1,16 count=1 block 04E4532C GetMinWidth block 04E4532C GetMinWidth=0 block 04E4532C GetPrefWidth block 04E4532C GetPrefWidth=0 block 04E4532C Reflow a=1,UC c=0,UC dirty v-resize cnt=9754 Block(span)(1)@04E4532C: begin reflow availSize=1,1073741824 computedSize=0,1073741824 Block(span)(1)@04E4532C: reflowing dirty lines computedWidth=0 Block(span)(1)@04E4532C: done reflowing dirty lines (status=0) clear floats: in: aY=0(0) clear floats: out: y=0(0) Block(span)(1)@04E4532C: status=0 (complete) metrics=0,0 carriedMargin=0 block 04E4532C Reflow d=0,0 Pushing this line line=04E453AC mY=22 dirty=no oldBounds={0,22,0,1200} oldCombinedArea={0,22,0,1200} deltaY=0 mPrevBottomMargin=0 childCount=1 Line reflow status = LINE_REFLOW_OK line=04E453AC mY=22 dirty=yes oldBounds={0,0,0,0} oldCombinedArea={0,0,0,0} deltaY=-1 mPrevBottomMargin=0 childCount=1 Block(body)(1)@04E4597C: done reflowing dirty lines (status=1) Block(body)(1)@04E4597C: status=3 (not complete) metrics=1,38 carriedMargin=0 block 04E4597C Reflow d=1,38 status=0x3 ColumnSet(body)(1) 04E10610 Reflow d=960,38 status=0x3o=(0,0) 962 x 38sto=(0,0) 962 x 38 line=048747EC mY=518 dirty=no oldBounds={480,480,960,38} oldCombinedArea={480,480,962,38} deltaY=0 mPrevBottomMargin=0 childCount=1 Block(html)(-1)@04874598: done reflowing dirty lines (status=3) I added at http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.950&mark=4030,4031#4025 printf("Pushing this line "); DumpLine(aState, aLine, 0, -1); to see what line gets pushed So to me it looks that the second line did get a 1200 twips height causing the incompleteness. (the log is done with GECKO_BLOCK_DEBUG_FLAGS=reflow,really-noisy-reflow)
Comment 14•16 years ago
|
||
I see many asserts like ###!!! ASSERTION: bad line list: 'count == frameCount', file d:/moz_src/mozilla/ layout/generic/nsBlockFrame.cpp, line 6954 ###!!! ASSERTION: bad line list: 'frame == line->mFirstChild', file d:/moz_src/m ozilla/layout/generic/nsBlockFrame.cpp, line 6967 if I specify GECKO_BLOCK_DEBUG_FLAGS=verify-lines Robert are those real asserts? It would fit the my feeling that the exotic document (frame) structure creates frame construction issues which then turn into reflow (completeness) issues which then cause bogus table split operations.
Comment 15•16 years ago
|
||
this is a minimal testcase, removing the doctype (making it quirks mode) or the height or the inline block on the span makes the assert go away.
Comment 16•16 years ago
|
||
Assignee | ||
Comment 17•16 years ago
|
||
I don't think I've ever used that in my life. But thanks for the testcase. Not sure what's going on without more digging.
Comment 18•16 years ago
|
||
>So to me it looks that the second line did get a 1200 twips height causing the >incompleteness. Thats the min line height that the block below the columnsetframe gets at http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.950&mark=922,923#915
Comment 19•16 years ago
|
||
just slowly resize the window horizontally and see how the line height changes, this should never happen.
Comment 20•16 years ago
|
||
This hack fixes the assert and the crash and makes the colored test case behave. Notice that with this patch the line height of the outer div in the colored case behaves like there is no moz-column defined on the div. Robert, IMHO this is now good enough debugged.
Comment 21•16 years ago
|
||
So now with some more context If one defines NOISY_VERTICAL_ALIGN in http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineLayout.cpp&rev=3.309&mark=74#65 then it becomes clear that the zeroEffectiveSpanBox toggles between the different reflow iterations leading to either 1200 or 0 line heights. This is bad as the columnset frame relies in its decisions on a certain persistence of the line block properties. The somehow arbitrary toggling is visualized as described in comment 19. The effect of this toggling is visible also in the reflow log block 04E48BC8 Reflow a=1,UC c=1,UC dirty dirty-children v-resize cnt=9791 block 04E4997C Reflow a=1,UC c=1,60 dirty v-resize cnt=9792 block 04E4997C Reflow d=1,60 block 04E49ABC GetMinWidth block 04E49ABC GetMinWidth=0 block 04E49ABC GetPrefWidth block 04E49ABC GetPrefWidth=0 block 04E49ABC Reflow a=1,UC c=0,UC dirty v-resize cnt=9795 block 04E49ABC Reflow d=0,0 block 04E48BC8 Reflow d=1,1260 block 04E48BC8 Reflow a=1,1230 c=1,UC dirty v-resize cnt=9796 block 04E4997C Reflow a=1,1230 c=1,60 dirty cnt=9797 block 04E4997C Reflow d=1,60 block 04E49ABC GetMinWidth block 04E49ABC GetMinWidth=0 block 04E49ABC GetPrefWidth block 04E49ABC GetPrefWidth=0 block 04E49ABC Reflow a=1,UC c=0,UC dirty v-resize cnt=9800 block 04E49ABC Reflow d=0,0 block 04E48BC8 Reflow d=1,1230 status=0x3 block 04E4A10C Reflow a=1,1230 c=1,UC dirty v-resize pif=04E48BC8 cnt=9801 block 04E49ABC GetMinWidth block 04E49ABC GetMinWidth=0 block 04E49ABC GetPrefWidth block 04E49ABC GetPrefWidth=0 block 04E49ABC Reflow a=1,UC c=0,UC dirty v-resize cnt=9804 block 04E49ABC Reflow d=0,0 block 04E4A10C Reflow d=1,0 during the unconstrained reflow we get a 1px div + 20 px line height ==> 1260 twips, then we reflow it with 1230, so the line with the inline-block gets pushed onto the second column but now it has all over sudden only a line height of 0. This analysis might be wrong if the second line should never be 1200 twips high. The question would then transform into what is the correct rendering if the -moz-column-count: 2 is removed from the outer div.
Comment 22•16 years ago
|
||
It looks to me that the behavior is a consequence of the following comment( http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineLayout.cpp&rev=3.309&mark=1637-1642#1630) // In general, if the document being processed is in full standards // mode then it should act normally (with one exception). The // exception case is when a span is continued and yet the span is // empty (e.g. compressed whitespace). For this kind of span we treat // it as if it were not there so that it doesn't impact the // line-height. This leads to the following behavior: (as the span is empty) if the span is not continued it has a line height of 1200. If it is has a line height of 0. Now the column reflow tries to push the content on the second page it will have either 1200 if the whole span moves or 0 height if the span continues. The column reflow does not expect this height toggling during its multiple reflow attempts.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → roc
Flags: wanted1.9.1?
Assignee | ||
Comment 23•16 years ago
|
||
mZeroEffectiveSpanBox was being set on the "root span", which is totally bogus. We were discovering the continuations of the block and treating them like inline continuations. Regardless of the goodness of this zeroEffectiveSpanBox hack, applying it to blocks makes no sense at all.
Attachment #339737 -
Flags: superreview?(dbaron)
Attachment #339737 -
Flags: review?(dbaron)
Assignee | ||
Updated•16 years ago
|
Flags: wanted1.9.1? → wanted1.9.1+
Comment on attachment 339737 [details] [diff] [review] fix r+sr=dbaron (In reply to comment #23) > mZeroEffectiveSpanBox was being set on the "root span", which is totally bogus. > We were discovering the continuations of the block and treating them like > inline continuations. Regardless of the goodness of this zeroEffectiveSpanBox > hack, applying it to blocks makes no sense at all. Your comment mislead me into thinking this patch was likely to be incorrect -- we actually often apply zeroEffectiveSpanBox to the root span in quirks mode (as we need to). But all you were changing is whether it was triggered in standards mode, by the "emptyContinuation" variable.
Attachment #339737 -
Flags: superreview?(dbaron)
Attachment #339737 -
Flags: superreview+
Attachment #339737 -
Flags: review?(dbaron)
Attachment #339737 -
Flags: review+
Assignee | ||
Comment 25•16 years ago
|
||
Pushed b1d18924f0f5
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: blocking1.9.0.11?
Updated•15 years ago
|
Flags: blocking1.9.0.11? → wanted1.9.0.x+
Whiteboard: [sg:dos] null deref
Updated•13 years ago
|
Crash Signature: [@ nsCellMapColumnIterator::GetNextFrame]
You need to log in
before you can comment on or make changes to this bug.
Description
•