Last Comment Bug 370360 - Heap corruption in nsTableRowGroupFrame::CalculateRowHeights
: Heap corruption in nsTableRowGroupFrame::CalculateRowHeights
Status: RESOLVED FIXED
[sg:critical]
: crash, fixed1.8.0.12, fixed1.8.1.4, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Bernd
:
Mentors:
http://www.w3.org/TR/css3-page/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-14 00:36 PST by Eli Friedman
Modified: 2009-04-24 10:55 PDT (History)
4 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reduced testcase (1.09 KB, text/html)
2007-02-14 00:54 PST, Eli Friedman
no flags Details
fix for the assert (5.01 KB, patch)
2007-02-15 21:44 PST, Bernd
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
Patch to make the issue completely obvious (1.08 KB, patch)
2007-02-16 13:55 PST, Eli Friedman
no flags Details | Diff | Review
revised patch (8.07 KB, patch)
2007-02-17 07:17 PST, Bernd
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Review

Description Eli Friedman 2007-02-14 00:36:18 PST
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 2007-02-14 00:54:32 PST
Created attachment 255070 [details]
Reduced testcase

There, reduced it.
Comment 2 Bernd 2007-02-14 22:28:58 PST
rowinfo is they key we work past array bounds
Comment 3 Bernd 2007-02-15 21:44:11 PST
Created attachment 255305 [details] [diff] [review]
fix for the assert
Comment 4 Bernd 2007-02-16 12:27:24 PST
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 2007-02-16 13:55:40 PST
Created attachment 255384 [details] [diff] [review]
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 [:bz] 2007-02-16 14:45:40 PST
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 2007-02-16 22:06:43 PST
exactly
Comment 8 Bernd 2007-02-17 07:17:31 PST
Created attachment 255455 [details] [diff] [review]
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 [:bz] 2007-02-18 16:51:18 PST
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 2007-02-21 12:20:32 PST
fix checked in
Comment 11 Bernd 2007-02-21 12:24:21 PST
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] 2007-03-21 15:57:34 PDT
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 2007-04-06 01:12:40 PDT
I checked in the part from comment 11 the other stuff does not apply anyway.
Comment 14 Bob Clary [:bc:] 2009-04-24 10:55:31 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/8dc547dc339c

Note You need to log in before you can comment on or make changes to this bug.