351628 - [reflow branch] Crash [@ nsTableColFrame::ResetMinCoord]

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we have:

(gdb) frame
#0  0xb727eda2 in nsTableColFrame::ResetMinCoord (this=0x0)
    at ../../../mozilla/layout/tables/nsTableColFrame.h:141
141         mMinCoord = 0;
(gdb) frame 1
#1  0xb727dba7 in BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths (
    this=0x8aab680, aRenderingContext=0x89b8878)
    at ../../../mozilla/layout/tables/BasicTableLayoutStrategy.cpp:215
215             colFrame->ResetMinCoord();
(gdb) list
210         // Loop over the columns to consider the columns and cells *without*
211         // a colspan.
212         PRInt32 col, col_end;
213         for (col = 0, col_end = cellMap->GetColCount(); col < col_end; ++col) {
214             nsTableColFrame *colFrame = tableFrame->GetColFrame(col);
215             colFrame->ResetMinCoord();
216             colFrame->ResetPrefCoord();
217             colFrame->ResetPrefPercent();
218             colFrame->ResetSpanMinCoord();
219             colFrame->ResetSpanPrefCoord();
(gdb) p cellMap->GetColCount()
$1 = 1
(gdb) p col_end
$2 = 1
(gdb) p colFrame
$3 = (class nsTableColFrame *) 0x0
(gdb) p col
$4 = 0

Is the cellmap somehow out of date?

Comment 3

[Bernd]

does 
-cellMap->GetColCount()
+mTableFrame->GetColCount()

fix it?

Comment 4

[Boris Zbarsky [:bzbarsky]]

(gdb) p tableFrame->GetColCount()
$1 = 1

So no.

Comment 5

[Bernd]

 for (col = 0, col_end = cellMap->GetColCount(); col < col_end;
++col) {
214             nsTableColFrame *colFrame = tableFrame->GetColFrame(col);

whoooaaaa, the friends of extreme sport :-)
No risk no fun, 

GetColFrame can return null,  the assert is commented out..... since 2001-03-12 22:38

If you look at basictablelayoutstrategy on trunk then you will find that *every* get colframe is followed by a null check, guess why.

I would predict that the reflow branch will not survive the layout regression tests with that.

At least you need the MatchCellMapToColCache stuff to survive without a nullcheck.

Comment 6

[Bernd]

the assert is covered by bug 351691

Comment 7

[Bernd]

s/I would predict that the reflow branch will not survive the layout regression
tests with that.//

the assert that I have seen came from  bug 351691

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What does not having a col frame at a valid index mean?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So it seems to me that there are probably two bugs here:  bug 351691, and a frame construction bug leading to not enough frames (for anonymous table objects) being destroyed when the inner td is removed.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, and a third bug leading to the wrong col count being reported.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> does 
> -cellMap->GetColCount()
> +mTableFrame->GetColCount()

nsTableFrame::GetColCount just forwards the call to the cell map.

It also has a comment that says:

  /* return the col count including dead cols */

Does this mean "including columns that no longer exist"?  Why would anyone want the function to do such a thing?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> er, and a third bug leading to the wrong col count being reported.

That seems most likely to be because either (1) nsCellMap::RemoveRows wasn't called when it should have been or (2) it doesn't remove as much as it should.  But presumably nsTableFrame::RemoveCol was called as a result of the cell being deleted.

(For what it's worth, the anonymous row, row group, and table were all NOT deleted, although they should have been.)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, when the cell frame is removed from the row, I'm seeing the col correctly removed from the cellmap, and the cell map's mCols having a size of zero.  But then something later adds the col back.

It's removed (correctly) here:

#0  nsTableCellMap::RemoveColsAtEnd (this=0x99414a8)
    at /builds/reflow/mozilla/layout/tables/nsCellMap.cpp:479
#1  0x0127891b in nsCellMap::ShrinkWithoutCell (this=0x9a35f60,
    aMap=@0x99414a8, aCellFrame=@0x9a77f00, aRowIndex=0, aColIndex=0,
    aDamageArea=@0xbfd38f10)
    at /builds/reflow/mozilla/layout/tables/nsCellMap.cpp:2022
#2  0x01278b89 in nsCellMap::RemoveCell (this=0x9a35f60, aMap=@0x99414a8,
    aCellFrame=0x9a77f00, aRowIndex=0, aDamageArea=@0xbfd38f10)
    at /builds/reflow/mozilla/layout/tables/nsCellMap.cpp:2239
#3  0x01278c54 in nsTableCellMap::RemoveCell (this=0x99414a8,
    aCellFrame=0x9a77f00, aRowIndex=0, aDamageArea=@0xbfd38f10)
    at /builds/reflow/mozilla/layout/tables/nsCellMap.cpp:699
#4  0x0128d1e3 in nsTableFrame::RemoveCell (this=0x9a77cd4,
    aCellFrame=0x9a77f00, aRowIndex=0)
    at /builds/reflow/mozilla/layout/tables/nsTableFrame.cpp:992
#5  0x0129703a in nsTableRowFrame::RemoveFrame (this=0x9a77ea4, aListName=0x0,
    aOldFrame=0x9a77f00)
    at /builds/reflow/mozilla/layout/tables/nsTableRowFrame.cpp:260

I need to figure out what adds it back...

Comment 14

[Bernd]

just merge my patches from trunk that fixes it, the bc part bug 350906

Comment 15

[Bernd]

http://lxr.mozilla.org/mozilla/source/layout/tables/nsTableFrame.cpp#4840 is the cause with a empty table (no columns) we add a dead cell that expands the cellmap but does not create a new col frame. This will change in a near future upto now there is a entry protection.

Comment 16

[Bernd]

More serious than comment 5. I am so used to think that GetColFrame can return zero that I didn't even question it. It happened so often due to the mismatch between cellmap and frame construction that the assert got never enabled. If one looks on my patches from the previous months one will see that they fixed various places that were responsible for this mismatch. I can't tell how many corps are still buried but bug 339128 has shown that there are ways to circumvent the first line of null check defenses and bring the cellmap out of sync. Activating the assert in bug 351691 will catch more of those errors. So there are two options:
1. zero check cols in BasicTableLayoutStrategy on branch and fix slowly the asserts that popup. My slow speed is known, but one fine day the asserts will be gone.
2. Let it crash and fix the core issues.
2. requires more committed resources but will definetely yield a better product. However I can't promise the necessary resources to go with that.

So David, thats a management decision whether you would like to spend resources on this or you have other/better places to apply the resources.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Added null checks + assertions.

Comment 18

[Jesse Ruderman]

Crashtest checked in.