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

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Comment 1

[Jesse Ruderman]

I'm guessing this is a regression from bug 352461 or bug 367749.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: The comments should describe what the issue is...

Comment 4

[Bernd]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Better fix

Comment 7

[Bernd]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 9

[Bernd]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed this morning.

Comment 11

[Jesse Ruderman]

Crashtest checked in.