350524 - Crash [@ nsTableRowFrame::CollapseRowIfNecessary] with border-collapse, visibility:collapse, colspan=0

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Trunk debug stack trace from Mac OS X Crash Reporter:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000018

Thread 0 Crashed:
0  nsIFrame::GetStyleData(nsStyleStructID) const + 40 (nsIFrame.h:590)
1  nsIFrame::GetStyleVisibility() const + 36 (nsStyleStructList.h:103)
2  nsTableRowFrame::CollapseRowIfNecessary(int, int, int, int&) + 1584 (nsTableRowFrame.cpp:1623)
3  nsTableRowGroupFrame::CollapseRowGroupIfNecessary(int, int) + 216 (nsTableRowGroupFrame.cpp:820)
4  nsTableFrame::AdjustForCollapsingRowsCols(nsHTMLReflowMetrics&, nsMargin) + 308 (nsTableFrame.cpp:2392)
5  nsTableFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) + 2956 (nsTableFrame.cpp:2136)
6  nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned, unsigned&) + 280 (nsContainerFrame.cpp:738)
7  nsTableOuterFrame::OuterReflowChild(nsPresContext*, nsIFrame*, nsHTMLReflowState const&, nsHTMLReflowMetrics&, int, nsSize&, nsMargin&, nsMargin&, nsMargin&, nsReflowReason, unsigned&, int*) + 1304 (nsTableOuterFrame.cpp:1301)
8  nsTableOuterFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) + 1376 (nsTableOuterFrame.cpp:1946)
...

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Before the crash, I see some assertions:

###!!! ASSERTION: program error: 'colIndex < mTableCellMap->GetColCount()', file /Users/admin/trunk/mozilla/layout/tables/nsTableFrame.cpp, line 4974

###!!! ASSERTION: program error: '(i >= startIndex) && (i <= endIndex)', file /Users/admin/trunk/mozilla/layout/tables/nsTableFrame.cpp, line 5529

###!!! ASSERTION: CellIterator program error: 'PR_FALSE', file /Users/admin/trunk/mozilla/layout/tables/nsTableFrame.cpp, line 4816

Comment 3

[Bernd]

Attachment: patch

The core issue here is that we check for a cellmap hole without taking into account that the hole can be caused by not expanding a zero span. However AppendCell  does check for it. So we try to mark a zero spanned cell as dead but the append cell puts the dead cell at the end and increases the number of columns. After this the number of cols is wrong and some iterations later we crash. 

The real core is however that zerospan repair mechanism at all. I think that a mark and sweep would be better than these permanent repair attempts.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236119 [details] [diff] [review]
patch

Hey, whatever brings sanity to the cellmap would be good.  :(

Comment 5

[Bernd]

Comment on attachment 236119 [details] [diff] [review]
patch

with some baking it should go on branch

Comment 6

[Bernd]

fixed on trunk

Comment 7

[Mike Schroepfer]

Bz - can you comment on risk for the 1.8 branch here - would love a crash fix but concerned about our lovely but fragile table code :-).

Comment 8

[Bernd]

IMHO, the problem is less that the code is fragile but more that the code is old and has worked for 5 years and we do only today recognize how many holes it had. And that is indeed frightening. But there is light at the end of the tunnel see the last sentence at https://bugzilla.mozilla.org/show_bug.cgi?id=339128#c22

Comment 9

[Mike Schroepfer]

Comment on attachment 236119 [details] [diff] [review]
patch

a=schrep for drivers for crash fix.

Comment 10

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 11

[Bernd]

Comment on attachment 236119 [details] [diff] [review]
patch

we baked this on trunk and have it 1.8.1

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 236119 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 13

[Daniel Veditz [:dveditz]]

fix checed into MOZILLA_1_8_0_BRANCH

Comment 14

[Marcia Knous [:marcia]]

verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.8) Gecko/20061023 Firefox/1.5.0.8. Testcase in Comment 1 does not cause a crash. Adding verified keyword.

Comment 15

[Jesse Ruderman]

Crashtest checked in.