363370 - Crash [@ nsTArray_base::ShiftData] with testcase that appends table cells

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes on a 2006-12-10 build.
It doesn't crash in a 2006-12-06 build, so very likely a regression from the reflow branch landing.

Talkback ID: TB27142781X
MSVCR80.dll + 0x150aa (0x781450aa)
nsTArray_base::ShiftData  [mozilla\xpcom\build\nstarray.cpp, line 176]
nsTArray_base::InsertSlotsAt  [mozilla\xpcom\build\nstarray.cpp, line 196]
nsTArray<nsTPtrArray<CellData> >::InsertElementsAt<int>  [mozilla\dist\include\xpcom\nstarray.h, line 641]
nsCellMap::Grow  [mozilla\layout\tables\nscellmap.cpp, line 1235]
nsCellMap::ExpandWithRows  [mozilla\layout\tables\nscellmap.cpp, line 1696]
nsCellMap::InsertRows  [mozilla\layout\tables\nscellmap.cpp, line 1284]
nsTableCellMap::InsertRows  [mozilla\layout\tables\nscellmap.cpp, line 558]
nsTableFrame::InsertRows  [mozilla\layout\tables\nstableframe.cpp, line 1078]
nsTableFrame::AppendRows  [mozilla\layout\tables\nstableframe.cpp, line 1044]
nsCSSFrameConstructor::AppendFrames  [mozilla\layout\base\nscssframeconstructor.cpp, line 8149]
nsCSSFrameConstructor::ContentAppended  [mozilla\layout\base\nscssframeconstructor.cpp, line 8878]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Bernd]

Its unlikely that is a reflow branch issue, its probably a regression from bug 356335. Looks like we need a complete run of bug 339128 again :-(

Comment 3

[Martijn Wargers (dead)]

Yeah, you're right, probably a regression from the patch from bug 339128.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the issue here is that mRows is empty (has length 0) and mRowCount is 1.  Then nsTableCellMap::InsertRows is called with aFirstRowIndex=3.  This calls nsCellMap::InsertRows with aFirstRowIndex=3.  Since nsCellMap::InsertRows grows mRows based on mRowCount, not on mRows.Length(), we run into issues...

Should nsCellMap::InsertRows just use mRows.Length()?

Comment 5

[Bernd]

Both RebuildConsideringRows and ExpandWithRows have issues with the growing. We got more picky with the cellmap changes

Comment 6

[Boris Zbarsky [:bzbarsky]]

We could just go ahead and condition all appends/inserts to arrays to grow first as needed if we want... that should make things safe.

Comment 7

[Bernd]

I will rename mRowCount into mContentRowCount so that the mess that is written in those functions becomes obvious. I should have done so for a long time.

Further I propose to put an assert at http://lxr.mozilla.org/seamonkey/source/layout/tables/nsCellMap.cpp#1234
that startRowIndex <= mRows.Length();
and after that to put a PR_MIN(startRowIndex,mRows.Length()) to prevent these conditions to crash. 

Comment 8

[Bernd]

Attachment: patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 249086 [details] [diff] [review]
patch

>Index: nsCellMap.cpp
> nsCellMap::InsertRows(nsTableCellMap& aMap,
>                       nsVoidArray&    aRows,
>                       PRInt32         aFirstRowIndex,
...
>+  // update mContentRowCount, since non-empty rows will be added
>+  mContentRowCount = PR_MAX(aFirstRowIndex, mContentRowCount);

Why doesn't this depend on aRows.Count()?  I guess we'll increment for those rows when we actually expand, so this is equivalent to what the old code did, eh?

> nsCellMap::ExpandWithRows(nsTableCellMap& aMap,
>+  NS_ASSERTION(PRUint32(startRowIndex) <= mRows.Length(), "caller should have grown cellmap before");
>+  
>+

Take out one of the blank lines?

>@@ -1750,17 +1758,21 @@ void nsCellMap::ExpandWithCells(nsTableC

This hunk is the fix for bug 363239, right?  So needs to get sr from roc there...  I can't really sr it, since I wrote it.  ;)

r+sr=bzbarsky with that.  Again, thanks for handling this...

Comment 10

[Bernd]

fix checked in

Comment 11

[Bernd]

fix checked in