[FIX]Crash [@ nsCellMapColumnIterator::GetNextFrame] removing cell with rowspan

RESOLVED FIXED in mozilla1.9alpha1

Status

()

--
critical
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

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

Trunk
mozilla1.9alpha1
PowerPC
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Created attachment 252746 [details]
testcase (crashes Firefox when loaded)

Loading the testcase makes Firefox (Mac debug) assert and crash.

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../dist/include/xpcom/nsTArray.h, line 327

Crash [@ nsCellMapColumnIterator::GetNextFrame]
(Reporter)

Comment 1

12 years ago
I'm guessing this is a regression from bug 352461 or bug 367749.
(Assignee)

Comment 2

12 years ago
On the one hand, this is a regression from bug 352461 -- the crash wasn't there before.

But the underlying bug (an nsCellMap whose mRows.Count() is less than its mContentRowCount) was there ever since bug 344000 got fixed.  The patch just backs out one part of bug 344000, basically.  Not sure how one could trigger a crash with this setup before bug 352461 landed, though.  Bernd might know.

I checked, and this should not be an issue on branches.
(Assignee)

Comment 3

12 years ago
Created attachment 252788 [details] [diff] [review]
The comments should describe what the issue is...
Attachment #252788 - Flags: superreview?(roc)
Attachment #252788 - Flags: review?(bernd_mozilla)
(Assignee)

Updated

12 years ago
Assignee: nobody → bzbarsky
Flags: in-testsuite?
Summary: Crash [@ nsCellMapColumnIterator::GetNextFrame] removing cell with rowspan → [FIX]Crash [@ nsCellMapColumnIterator::GetNextFrame] removing cell with rowspan
Target Milestone: --- → mozilla1.9alpha

Comment 4

12 years ago
I am not confident that the patch is correct, so it will take more time than usual to r- it.

Did your cellmap iterator made assumptions about the relationship between

mContentRowCount and mRows.Length like

 mContentRowCount <= mRows.Length 

tThere is no such invariant in the cellmap

(Assignee)

Comment 5

12 years ago
Comment on attachment 252788 [details] [diff] [review]
The comments should describe what the issue is...

Yeah, I'm making that assumption.  OK, let me undo that.  ;)
Attachment #252788 - Flags: superreview?(roc)
Attachment #252788 - Flags: superreview-
Attachment #252788 - Flags: review?(bernd_mozilla)
Attachment #252788 - Flags: review-
(Assignee)

Updated

12 years ago
Blocks: 352461
(Assignee)

Comment 6

12 years ago
Created attachment 252828 [details] [diff] [review]
Better fix
Attachment #252788 - Attachment is obsolete: true
Attachment #252828 - Flags: superreview?(roc)
Attachment #252828 - Flags: review?(bernd_mozilla)

Comment 7

12 years ago
Comment on attachment 252828 [details] [diff] [review]
Better fix

+  // The number of "relevant" rows in mCurMap.  That is, the number of rows
+  // which might have an originating cell in them.  

This comment is wrong. mRows.Length is not the number of rows with originating cells but the number of rows with cellmap entries. If you have dead rowspan, then the entries for the rowspanned cells will be in rows that contribute to mRows.Length
Attachment #252828 - Flags: review?(bernd_mozilla) → review+
(Assignee)

Comment 8

12 years ago
> mRows.Length is not the number of rows with originating cells

Right.  Which is why the member in question is PR_MIN(mRows.Length(), mContentRowCount).

So I think I stand by the comment...
Attachment #252828 - Flags: superreview?(roc) → superreview+

Comment 9

12 years ago
I was talking about PR_MIN(mRows.Length(), mContentRowCount), but "relevant" in this context means relevant to the iterator and not to the cellmap, so your comment is correct, I didn't grasp it on first read.
(Assignee)

Comment 10

12 years ago
Fixed this morning.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

11 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCellMapColumnIterator::GetNextFrame]
You need to log in before you can comment on or make changes to this bug.