456041 - Crash [@ nsCellMapColumnIterator::GetNextFrame] with contenteditable, generated content on table and double tbody

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build.
This seems to have regressed between 2008-08-15 and 2008-08-18.

Perhaps this is the same as bug 393936, but this has a different regression range.


Some assertions, prior to the crash from my debug build:
###!!! ASSERTION: expected only one child frame: '!aFrameList->GetNextSibling()'
, file c:/mozilla-build-1.3/mozilla-central/layout/tables/nsTableFrame.cpp, line
 2298
++WEBSHELL 05E26F70 == 6
++DOMWINDOW == 10 (08B7516C) [serial = 13] [outer = 00000000]
###!!! ASSERTION: Bogus mOrigCells?: 'mCurMapRow < mCurMapRelevantRowCount', fil
e c:/mozilla-build-1.3/mozilla-central/layout/tables/nsCellMap.cpp, line 2928


Stack of the crash:
>	xpcom_core.dll!nsTArray_base::Length()  Line 66 + 0x3 bytes	C++
 	gklayout.dll!nsTArray<nsTPtrArray<CellData> >::ElementAt(unsigned int i=0)  Line 326 + 0x9 bytes	C++
 	gklayout.dll!nsTArray<nsTPtrArray<CellData> >::operator[](unsigned int i=0)  Line 356	C++
 	gklayout.dll!nsCellMapColumnIterator::GetNextFrame(int * aRow=0x0012d0ec, int * aColSpan=0x0012d0f4)  Line 2932 + 0x12 bytes	C++
 	gklayout.dll!BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths(nsIRenderingContext * aRenderingContext=0x05d97af0)  Line 292 + 0x16 bytes	C++
 	gklayout.dll!BasicTableLayoutStrategy::ComputeIntrinsicWidths(nsIRenderingContext * aRenderingContext=0x05d97af0)  Line 410	C++
 	gklayout.dll!BasicTableLayoutStrategy::GetMinWidth(nsIRenderingContext * aRenderingContext=0x05d97af0)  Line 70	C++
 	gklayout.dll!nsTableFrame::GetMinWidth(nsIRenderingContext * aRenderingContext=0x05d97af0)  Line 1633	C++
 	gklayout.dll!nsTableFrame::TableShrinkWidthToFit(nsIRenderingContext * aRenderingContext=0x05d97af0, int aWidthInCB=83880)  Line 1687 + 0x14 bytes	C++
 	gklayout.dll!nsTableFrame::ComputeAutoSize(nsIRenderingContext * aRenderingContext=0x05d97af0, nsSize aCBSize={...}, int aAvailableWidth=83880, nsSize aMargin={...}, nsSize aBorder={...}, nsSize aPadding={...}, int aShrinkWrap=1)  Line 1719 + 0x15 bytes	C++
 	gklayout.dll!nsFrame::ComputeSize(nsIRenderingContext * aRenderingContext=0x05d97af0, nsSize aCBSize={...}, int aAvailableWidth=83880, nsSize aMargin={...}, nsSize aBorder={...}, nsSize aPadding={...}, int aShrinkWrap=1)  Line 3118	C++
 	gklayout.dll!nsTableFrame::ComputeSize(nsIRenderingContext * aRenderingContext=0x05d97af0, nsSize aCBSize={...}, int aAvailableWidth=83880, nsSize aMargin={...}, nsSize aBorder={...}, nsSize aPadding={...}, int aShrinkWrap=1)  Line 1675	C++
 	gklayout.dll!ChildShrinkWrapWidth(nsIRenderingContext * aRenderingContext=0x05d97af0, nsIFrame * aChildFrame=0x05e288fc, nsSize aCBSize={...}, int aAvailableWidth=83880, int * aMarginResult=0x00000000)  Line 609	C++
etc..

Comment 1

[Martijn Wargers (dead)]

Attachment: complete stack of debug build

Comment 2

[Bernd]

the assertion on top is just to the point

http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableFrame.cpp#2290

2294   // Asssume there's only one frame being inserted. The problem is that
2295   // row group frames and col group frames go in separate child lists and
2296   // so if there's more than one this gets messy...
2297   // XXX The frame construction code should be separating out child frames
2298   // based on the type, bug 343048.
2299   NS_PRECONDITION(!aFrameList->GetNextSibling(), "expected only one child frame");

Comment 3

[Bernd]

> This seems to have regressed between 2008-08-15 and 2008-08-18.

bug 238072 would fit especially the checkin comment:

Make generated content take the normal frame construction path. Relanding with changes so that counter and quote nodes are initialized after their frames are available, more like the old code. r+sr=dbaron,bzbarsky

Comment 4

[Bernd]

Attachment: patch that shows the root of the issue

Comment 5

[Martijn Wargers (dead)]

Ah, right, probably a regression from bug 238072.

Comment 6

[Bernd]

Attachment: patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

So why is that <style> with all the mmm's in it needed?

Why are the two tbodys needed, for that matter?  I guess the point is that we're ending up with multiple rowgroups?  But then why is the ::after style relevant for that?

"heterogenous" or "heterogeneous" are OK spellings: need an 'o' after the 'r'.

Comment 8

[Bernd]

> So why is that <style> with all the mmm's in it needed?
> Why are the two tbodys needed, for that matter?
> But then why is the ::after style relevant for that?

Because it will otherwise not crash. This is minimized. And frankly I do not care, what is in the testcase as long as I see code that first assumes that there is a list and then uses InsertFrame

2378     nsFrameList newList(aFrameList);
2379     nsIFrame* lastSibling = newList.LastChild();  <=====
2380     // Insert the frames in the sibling chain
2381     mFrames.InsertFrame(nsnull, aPrevFrame, aFrameList); <=====

I can remove the testcase from the patch if you like. 

The story is that Martijn found a way that will send more than one rowgroup to this function. I am not sure that he/one will be able to get a mixed list down that path, that is why I added the assert. I guess if dbaron fixes that content insert for pseudos this patch will be necessary anyway.

Comment 9

[Boris Zbarsky [:bzbarsky]]

No, I mean why is this crashing?  Why are we ending up with a list in this case but not others?  I'd really like to understand how this is failing so I can figure out whether the patch is the right way to fix it, basically...  Does replacing that huge style block with <script>document.body.offsetWidth</script> still reproduce the crash, for example?

Comment 10

[Martijn Wargers (dead)]

Attachment: testcase2

(In reply to comment #9)
> Does replacing that huge style block with <script>document.body.offsetWidth</script>
> still reproduce the crash, for example?

Yes, it does. (oops)

Comment 11

[Boris Zbarsky [:bzbarsky]]

And at that point do you still need the generated content?

Comment 12

[Martijn Wargers (dead)]

Yes, the generated content is needed for the crash.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky with the spelling fixed and the big style block replaced with a layout flush.

Comment 14

[Bernd]

re comment 9 Boris the code was blatant wrong to me that I that I just thought this should never have been written like this. Do you know why do not assert that 
aNewFrame->GetNextSibling() is null at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generi/nsFrameList.cpp&rev=3.54&mark=190#169
I mean if this happens bad things should be expected.

Comment 15

[Bernd]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameList.cpp&rev=3.54&mark=190#169

Comment 16

[Boris Zbarsky [:bzbarsky]]

Adding an assert there sounds good to me.

Comment 17

[Bernd]

Attachment: patch that got checked in

Comment 18

[Bernd]

Boris, I added the assert and the testcase from bug 457115 and of course did what you asked for.

Comment 19

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080928204134 Minefield/3.1b1pre