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

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: bernd_mozilla)

Tracking

(Blocks 1 bug, {crash, regression, testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

Reporter

Description

13 years ago
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]
Reporter

Comment 1

13 years ago
Posted file testcase
Assignee

Comment 2

13 years ago
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
Reporter

Comment 3

13 years ago
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()?
Assignee

Comment 5

13 years ago
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.
Assignee

Comment 7

13 years ago
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. 

Updated

13 years ago
OS: Windows XP → All
Hardware: PC → All
Assignee

Comment 8

13 years ago
Posted patch patchSplinter Review
Attachment #249086 - Flags: superreview?(bzbarsky)
Attachment #249086 - Flags: review?(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...
Attachment #249086 - Flags: superreview?(bzbarsky)
Attachment #249086 - Flags: superreview+
Attachment #249086 - Flags: review?(bzbarsky)
Attachment #249086 - Flags: review+
Assignee

Comment 10

13 years ago
fix checked in
Assignee: bernd_mozilla → nobody
Flags: in-testsuite?
Assignee

Comment 11

13 years ago
fix checked in
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee: nobody → bernd_mozilla
Assignee

Updated

12 years ago
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.