370360 - Heap corruption in nsTableRowGroupFrame::CalculateRowHeights

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Eli Friedman]

When loading http://www.w3.org/TR/css3-page/ in print preview, I get a C runtime assertion complaining about heap corruption.

(If it doesn't crash for you, try turning up your scaling level.)

Trace:
WARNING: Stack unwind information not available. Following frames may be wrong.
MSVCR80D!free_dbg+0x2df
MSVCR80D!free_dbg+0x4e
MSVCR80D!operator delete+0xb7
MSVCR80D!operator delete[]+0xc
gklayout!nsTableRowGroupFrame::CalculateRowHeights(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b28c, struct nsHTMLReflowState * aReflowState = 0x0000b388)+0x7ce [c:\mozilla\mozilla\layout\tables\nstablerowgroupframe.cpp @ 821]
gklayout!nsTableRowGroupFrame::ReflowChildren(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b28c, struct nsRowGroupReflowState * aReflowState = 0x00000001, unsigned int * aStatus = 0x0012ba10, int * aPageBreakBeforeEnd = 0x0012b0ec)+0x345 [c:\mozilla\mozilla\layout\tables\nstablerowgroupframe.cpp @ 486]
gklayout!nsTableRowGroupFrame::Reflow(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x049db8a0, struct nsHTMLReflowState * aReflowState = 0x00000000, unsigned int * aStatus = 0x0012ba10)+0xbc [c:\mozilla\mozilla\layout\tables\nstablerowgroupframe.cpp @ 1280]
gklayout!nsContainerFrame::ReflowChild(class nsIFrame * aKidFrame = 0x04c25a14, class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b28c, struct nsHTMLReflowState * aReflowState = 0x0012b1e0, int aX = 120, int aY = 120, unsigned int aFlags = 0, unsigned int * aStatus = 0x0012ba10)+0x74 [c:\mozilla\mozilla\layout\generic\nscontainerframe.cpp @ 954]
gklayout!nsTableFrame::ReflowChildren(struct nsTableReflowState * aReflowState = 0x0012b38c, unsigned int * aStatus = 0x0012ba10, class nsIFrame ** aLastChildReflowed = 0x0012b430, struct nsRect * aOverflowArea = 0x0012b654)+0x2b6 [c:\mozilla\mozilla\layout\tables\nstableframe.cpp @ 2828]
gklayout!nsTableFrame::ReflowTable(struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b62c, struct nsHTMLReflowState * aReflowState = 0x0012b4d4, int aAvailHeight = 27840, class nsIFrame ** aLastChildReflowed = 0x0012b430, unsigned int * aStatus = 0x0012ba10)+0x60 [c:\mozilla\mozilla\layout\tables\nstableframe.cpp @ 2040]
gklayout!nsTableFrame::Reflow(class nsPresContext * aPresContext = 0x00000000, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b62c, struct nsHTMLReflowState * aReflowState = 0x00000000, unsigned int * aStatus = 0x0012ba10)+0x161 [c:\mozilla\mozilla\layout\tables\nstableframe.cpp @ 1933]
gklayout!nsContainerFrame::ReflowChild(class nsIFrame * aKidFrame = 0x049db8a0, class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b62c, struct nsHTMLReflowState * aReflowState = 0x0012b4d4, int aX = 0, int aY = 0, unsigned int aFlags = 3, unsigned int * aStatus = 0x0012ba10)+0x74 [c:\mozilla\mozilla\layout\generic\nscontainerframe.cpp @ 954]
gklayout!nsTableOuterFrame::OuterReflowChild(class nsPresContext * aPresContext = 0x04a54db0, class nsIFrame * aChildFrame = 0x049db8a0, struct nsHTMLReflowState * aOuterRS = 0x00006cc0, void * aChildRSSpace = 0x0012b4d4, struct nsHTMLReflowMetrics * aMetrics = 0x0012b62c, int aAvailWidth = 15060, struct nsSize * aDesiredSize = 0x0012b6cc, struct nsMargin * aMargin = 0x0012b6bc, unsigned int * aStatus = 0x0012ba10)+0x12b [c:\mozilla\mozilla\layout\tables\nstableouterframe.cpp @ 1112]
gklayout!nsTableOuterFrame::Reflow(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012b858, struct nsHTMLReflowState * aOuterRS = 0x00000000, unsigned int * aStatus = 0x0012ba10)+0x1a8 [c:\mozilla\mozilla\layout\tables\nstableouterframe.cpp @ 1217]
gklayout!nsBlockReflowContext::ReflowBlock(struct nsRect * aSpace = 0x0012b9dc, int aApplyTopMargin = 0, struct nsCollapsingMargin * aPrevMargin = 0x0012bc98, int aClearance = 0, int aIsAdjacentWithTop = 1, struct nsMargin * aComputedOffsets = 0x00000000, struct nsHTMLReflowState * aFrameRS = 0x0012b89c, unsigned int * aFrameReflowStatus = 0x0012ba10)+0x1e0 [c:\mozilla\mozilla\layout\generic\nsblockreflowcontext.cpp @ 372]
gklayout!nsBlockFrame::ReflowBlockFrame(class nsBlockReflowState * aState = 0x00000000, class nsLineList_iterator aLine = class nsLineList_iterator, int * aKeepReflowGoing = 0x0012bba0)+0x3cb [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 2873]
gklayout!nsBlockFrame::ReflowLine(class nsBlockReflowState * aState = 0x0012bc2c, class nsLineList_iterator aLine = class nsLineList_iterator, int * aKeepReflowGoing = 0x0012bba0)+0xb6 [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 2115]
gklayout!nsBlockFrame::ReflowDirtyLines(class nsBlockReflowState * aState = 0x00000000)+0x376 [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 1779]
gklayout!nsBlockFrame::Reflow(class nsPresContext * aPresContext = 0x04a54db0, struct nsHTMLReflowMetrics * aMetrics = 0x0012c0f8, struct nsHTMLReflowState * aReflowState = 0x0012c13c, unsigned int * aStatus = 0x0012c2b0)+0x21c [c:\mozilla\mozilla\layout\generic\nsblockframe.cpp @ 903]
gklayout!nsBlockReflowContext::ReflowBlock(struct nsRect * aSpace = 0x0012c27c, int aApplyTopMargin = 0, struct nsCollapsingMargin * aPrevMargin = 0x0012c538, int aClearance = 0, int aIsAdjacentWithTop = 1, struct nsMargin * aComputedOffsets = 0x00000870, struct nsHTMLReflowState * aFrameRS = 0x0012c13c, unsigned int * aFrameReflowStatus = 0x0012c2b0)+0x1e0 [c:\mozilla\mozilla\layout\generic\nsblockreflowcontext.cpp @ 372]

I've also gotten all sorts of wonderful assertions.

Not reduced, at least at the moment.

Comment 1

[Eli Friedman]

Attachment: Reduced testcase

There, reduced it.

Comment 2

[Bernd]

rowinfo is they key we work past array bounds

Comment 3

[Bernd]

Attachment: fix for the assert

Comment 4

[Bernd]

Comment on attachment 255305 [details] [diff] [review]
fix for the assert

The patch fixes the assert and cells will not use negative heights, but then we crash seemingly randomly (my bet is painting). It might be that the assert makes us crash in a single place rather than randomly

Comment 5

[Eli Friedman]

Attachment: Patch to make the issue completely obvious

The issue should become very clear if you apply this patch and try the testcase.  It should have been clear anyway from the trace and the title of the bug, but here you go.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 255305 [details] [diff] [review]
fix for the assert

So this makes NS_FRAME_SET_TRUNCATION do the right thing, basically?

Comment 7

[Bernd]

exactly

Comment 8

[Bernd]

Attachment: revised patch

The patch includes the fixup that is already reviewed, switches the rowinfo to Tarray and fixes the access beyond the array boundaries.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 255455 [details] [diff] [review]
revised patch

>Index: nsTableRowGroupFrame.cpp

>+  nsTArray<RowInfo> rowInfo;
>+  rowInfo.AppendElements(numRows);

Bail out if AppendElements fails?

r+sr=bzbarsky with that

Comment 10

[Bernd]

fix checked in

Comment 11

[Bernd]

Comment on attachment 255455 [details] [diff] [review]
revised patch

at least +          if ((rowIndex + rowSpan) > numRows) {
+            // there might be rows pushed already to the nextInFlow
+            rowSpan = numRows - rowIndex;
+          }

should be go branches

I would prefer to have the rest too minus the Tarray part.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 255455 [details] [diff] [review]
revised patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 13

[Bernd]

I checked in the part from comment 11 the other stuff does not apply anyway.

Comment 14

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/8dc547dc339c