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]
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.
Created attachment 252788 [details] [diff] [review] The comments should describe what the issue is...
Assignee: nobody → bzbarsky
Summary: Crash [@ nsCellMapColumnIterator::GetNextFrame] removing cell with rowspan → [FIX]Crash [@ nsCellMapColumnIterator::GetNextFrame] removing cell with rowspan
Target Milestone: --- → mozilla1.9alpha
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 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. ;)
Created attachment 252828 [details] [diff] [review] Better fix
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+
> 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+
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.
Fixed this morning.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.