220536 - crash when i modify (by JS) colSpan value

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Vincent Ricard]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030908 Debian/1.4-4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030908 Debian/1.4-4

Mozilla crash when i modify colspan value of the third th element. If i invert
the line with the next one, that's ok. (The next one changes the style.display
property).

Reproducible: Always

Steps to Reproduce:
1. Go to the URL
2. Click on the 'eeee' check box
3. Click on the 'ffff' check box

Actual Results:  
Big crash :-/

Expected Results:  
it shouldn't crash :P
maybe should it throw a javascript exception if these steps are forbidden (?).

Comment 1

[Bernd]

nsCellMap::GetCellInfoAt(nsTableCellMap & {...}, int 0, int 2, int * 0x0012cde4,
int * 0x0012cde8) line 2353 + 16 bytes
nsTableCellMap::GetCellInfoAt(int 1, int 2, int * 0x0012cde4, int * 0x0012cde8)
line 735 + 28 bytes
nsTableFrame::GetCellInfoAt(int 1, int 2, int * 0x0012cde4, int * 0x0012cde8)
line 4541
BasicTableLayoutStrategy::AssignNonPctColumnWidths(nsIPresContext * 0x02577c50,
int 7152, const nsHTMLReflowState & {...}, float 12.0000) line 1009 + 30 bytes
BasicTableLayoutStrategy::Initialize(nsIPresContext * 0x02577c50, const
nsHTMLReflowState & {...}) line 131 + 27 bytes
nsTableFrame::ReflowTable(nsIPresContext * 0x02577c50, nsHTMLReflowMetrics &
{...}, const nsHTMLReflowState & {...}, int 1073741824, nsReflowReason
eReflowReason_Resize, nsIFrame * & 0x00000000, int & 0, int & 0, unsigned int &
0) line 2185
nsTableFrame::Reflow(nsTableFrame * const 0x025b0734, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 2047
nsContainerFrame::ReflowChild(nsIFrame * 0x025b0734, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0,
int 0, unsigned int 3, unsigned int & 0) line 925 + 31 bytes
nsTableOuterFrame::OuterReflowChild(nsTableOuterFrame * const 0x025b05c4,
nsIPresContext * 0x02577c50, nsIFrame * 0x025b0734, const nsHTMLReflowState &
{...}, nsHTMLReflowMetrics & {...}, int 7152, nsSize & {...}, nsMargin & {...},
nsMargin & {...}, nsMargin & {...}, nsReflowReason eReflowReason_Incremental,
unsigned int & 0) line 1334 + 47 bytes
nsTableOuterFrame::IR_InnerTableReflow(nsTableOuterFrame * const 0x025b05c4,
nsIPresContext * 0x02577c50, nsHTMLReflowMetrics & {...}, const
nsHTMLReflowState & {...}, unsigned int & 0) line 1686 + 71 bytes
nsTableOuterFrame::IR_TargetIsInnerTableFrame(nsTableOuterFrame * const
0x025b05c4, nsIPresContext * 0x02577c50, nsHTMLReflowMetrics & {...}, const
nsHTMLReflowState & {...}, unsigned int & 0) line 1447 + 31 bytes
nsTableOuterFrame::IR_TargetIsChild(nsTableOuterFrame * const 0x025b05c4,
nsIPresContext * 0x02577c50, nsHTMLReflowMetrics & {...}, const
nsHTMLReflowState & {...}, unsigned int & 0, nsIFrame * 0x025b0734) line 1420 +
31 bytes
nsTableOuterFrame::IncrementalReflow(nsTableOuterFrame * const 0x025b05c4,
nsIPresContext * 0x02577c50, nsHTMLReflowMetrics & {...}, const
nsHTMLReflowState & {...}, unsigned int & 0) line 1400 + 42 bytes
nsTableOuterFrame::Reflow(nsTableOuterFrame * const 0x025b05c4, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 1931 + 31 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 1,
nsCollapsingMargin & {...}, int 0, nsMargin & {...}, nsHTMLReflowState & {...},
unsigned int & 0) line 533 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012dc98) line 3188 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012dc98, int 1) line 2397 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2180 + 31 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x025a2c90, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 849 + 15 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 1,
nsCollapsingMargin & {...}, int 1, nsMargin & {...}, nsHTMLReflowState & {...},
unsigned int & 0) line 533 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012e7ec) line 3188 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012e7ec, int 1) line 2397 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2180 + 31 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x025a2a7c, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 849 + 15 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x025a2a7c, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0,
int 0, unsigned int 0, unsigned int & 0) line 925 + 31 bytes
CanvasFrame::Reflow(CanvasFrame * const 0x0113d568, nsIPresContext * 0x02577c50,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0)
line 570
nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0, int 0, int 0, int 7344, int 3456, int 1) line 888
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x025a29ec,
nsBoxLayoutState & {...}) line 634 + 46 bytes
nsBox::Layout(nsBox * const 0x025a29ec, nsBoxLayoutState & {...}) line 1000
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x0113d92c, nsBoxLayoutState
& {...}) line 339
nsBox::Layout(nsBox * const 0x0113d92c, nsBoxLayoutState & {...}) line 1000
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x0113d92c,
const nsRect & {...}) line 650 + 16 bytes
nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x0113d92c,
const nsRect & {...}) line 1196 + 17 bytes
nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1346
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x0113d80c, nsBoxLayoutState
& {...}) line 1204 + 15 bytes
nsBox::Layout(nsBox * const 0x0113d80c, nsBoxLayoutState & {...}) line 1000
nsBoxFrame::Reflow(nsBoxFrame * const 0x0113d7d4, nsIPresContext * 0x02577c50,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0)
line 883
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x0113d7d4, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 833 + 25 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x0113d7d4, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0,
int 0, unsigned int 0, unsigned int & 0) line 925 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x0113d458, nsIPresContext *
0x02577c50, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 262 + 43 bytes
IncrementalReflow::Dispatch(nsIPresContext * 0x02577c50, nsHTMLReflowMetrics &
{...}, const nsSize & {...}, nsIRenderingContext & {...}) line 916
PresShell::ProcessReflowCommands(int 1) line 6492
ReflowEvent::HandleEvent() line 6334
HandlePLEvent(ReflowEvent * 0x025775c8) line 6348
PL_HandleEvent(PLEvent * 0x025775c8) line 671 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x0116a1f8) line 606 + 9 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x0116a130) line
391 + 12 bytes
nsWindow::DispatchPendingEvents() line 3618
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 3735567, long *
0x0012fe10) line 3983
nsWindow::WindowProc(HWND__ * 0x00170580, unsigned int 514, unsigned int 0, long
3735567) line 1333 + 27 bytes
USER32! 77d37ad7()
USER32! 77d3ccd4()
USER32! 77d14455()
USER32! 77d14d58()
main(int 1, char * * 0x00377ac0) line 155 + 11 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e614c7()

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.. is the cellmap munging in nsTableFrame::AttributeChangedFor done correctly?

Comment 3

[Bernd]

Attachment: minimized testcase (crashes after 1000 msec)

Comment 4

[Adam Hauner]

Crash using 2003092304/trunk/W2K -> TB23981472W

Comment 5

[Bernd]

taking

Comment 6

[Bernd]

Attachment: patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky

Comment 8

[Bernd]

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

the patch breaks
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/dom/insertCells
Rebuild1.html
(the test needs to be run locally as lxr does not give the necessary js when
displaying the page

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** Bug 224068 has been marked as a duplicate of this bug. ***

Comment 10

[Bernd]

Attachment: patch

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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

+  NS_ASSERTION(PR_FALSE, "nsTableCellMap::RemoveCell - could not remove
cell");

use NS_ERROR

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

What biesi said, and r=bzbarsky.  Does this testcase trigger that assert?

Comment 13

[Bernd]

Hmm, some people on vacation are faster than I could provide some explanations
(hints) for review.

1. crashes with BasicTableLayoutStrategy::AssignNonPctColumnWidths in a
prominent place in the stack trace are usually caused by cellmap problems.
Cellmap manipulation very often require a reinitialization of the layout
strategy and then the whole cellmap is queried for all cells.

2. The cellmap knows a concept of dead rows. Cells at the last row that have
rowspans > 1 require dead rows below the last row. The cellmap has row entries
for those dead rows. The number of "true" rows is counted by mRowCount. The
amount of rows in a cellmap is mrows.Count(). Usually they are identical but
when read rows are present the differ mRowCount < mrows.Count() in this case.

3. The cellmap returns mRowCount for the row count: (from nsCellMap.h)

472 inline PRInt32 nsCellMap::GetRowCount(PRBool aConsiderDeadRowSpanRows) const
473 { 
474   PRInt32 rowCount = (aConsiderDeadRowSpanRows) ? mRows.Count() : mRowCount;
475   return rowCount; 
476 }

aConsiderDeadRowSpanRows is always false ... ( I believe so)
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsCellMap.h#305

4. As rowspans can not cross row group boundaries dead rows can be at the end of
every row group.

5. Rows have theire own mechanism of knowing where they are in the table.
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableRowFrame.h#329

329 inline PRInt32 nsTableRowFrame::GetRowIndex() const
330 {
331   return PRInt32(mBits.mRowIndex);
332 }

6. It would be cool if for a cell the rowindex from the parent row and the
rowindex in the cellmap coincide. Otherwise it will certainly crash when the
cell will be removed, as the row parent will remove the cell but the cellmap
will not be able to remove the reference to previously existing cell. (this is
what causes the crash here).

7. When cells are removed from the cellmap they could empty a previously non
empty row causing a transition from a true row to a dead row. Due to 6. this is
not allowed. So even if the row is empty but as it exists in the DOM it will be
treated like a true row.

8. When cells are inserted into the cellmap they cannot shrink the cellmap. True
rows should not become dead rows. This is what this patch tries to do.

 

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky

Comment 15

[Bernd]

fix checked in

Comment 16

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1