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)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:dos] null deref)

Crash Data

Attachments

(9 files)

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.
Depends on: 393936
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?
Attached patch patchSplinter Review
don't let columns disturb us
Attached patch 80 signs Splinter Review
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)
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)
Attached file reflow log
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
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?
>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)

        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.
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
     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)
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.
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.
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.
>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
just slowly resize the window horizontally and see how the line height changes, this should never happen.
Attached file hack
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.
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.
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: nobody → roc
Flags: wanted1.9.1?
OS: Mac OS X → All
Attached patch fixSplinter Review
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)
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+
Pushed b1d18924f0f5
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: blocking1.9.0.11?
Flags: blocking1.9.0.11? → wanted1.9.0.x+
Whiteboard: [sg:dos] null deref
Crash Signature: [@ nsCellMapColumnIterator::GetNextFrame]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: