Closed Bug 363370 Opened 14 years ago Closed 14 years ago

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


(Core :: Layout: Tables, defect)

Not set





(Reporter: martijn.martijn, Assigned: bernd_mozilla)


(Blocks 1 open bug)


(Keywords: crash, regression, testcase)

Crash Data


(2 files)

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]
Attached file testcase
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 :-(
Summary: [reflow branch] Crash [@ nsTArray_base::ShiftData] with testcase that appends table cells → Crash [@ nsTArray_base::ShiftData] with testcase that appends table cells
Yeah, you're right, probably a regression from the patch from bug 339128.
Blocks: 356335
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()?
Both RebuildConsideringRows and ExpandWithRows have issues with the growing. We got more picky with the cellmap changes
Assignee: nobody → bernd_mozilla
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.
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
that startRowIndex <= mRows.Length();
and after that to put a PR_MIN(startRowIndex,mRows.Length()) to prevent these conditions to crash. 
OS: Windows XP → All
Hardware: PC → All
Attached patch patchSplinter Review
Attachment #249086 - Flags: superreview?(bzbarsky)
Attachment #249086 - Flags: review?(bzbarsky)
Comment on attachment 249086 [details] [diff] [review]

>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...
Attachment #249086 - Flags: superreview?(bzbarsky)
Attachment #249086 - Flags: superreview+
Attachment #249086 - Flags: review?(bzbarsky)
Attachment #249086 - Flags: review+
fix checked in
Assignee: bernd_mozilla → nobody
Flags: in-testsuite?
fix checked in
Closed: 14 years ago
Resolution: --- → FIXED
Assignee: nobody → bernd_mozilla
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsTArray_base::ShiftData]
You need to log in before you can comment on or make changes to this bug.