642088 - Crash while printing or print-previewing [@ nsFrameList::RemoveFrame(nsIFrame*) ] - [@ nsTableFrame::PushChildren]

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Scoobidiver (away)]

It is a new crash signature in the trunk.
It is #110 top crasher in 4.0RC1.

Comments say:
"Tried printing a dynamically created webpage containing several 100% width tables."
"crashes when print preview is selected"
"Firefox crashed when trying to print a page from Open Universities Australia"

Signature	nsFrameList::RemoveFrame(nsIFrame*)
UUID	f6b8cb10-0805-4863-9713-88ed62110313
Time 	2011-03-13 12:57:10.20991
Uptime	15
Last Crash	24 seconds before submission
Install Age	268220 seconds (3.1 days) since version was first installed.
Product	Firefox
Version	4.0
Build ID	20110303194838
Branch	2.0
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	AuthenticAMD family 17 model 3 stepping 1
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x20
User Comments	will einfach nur drucken ... Absturtz!
App Notes 	AdapterVendorID: 1002, AdapterDeviceID: 9612, AdapterDriverVersion: 8.632.1.2000
D3D10 Layers? D3D10 Layers-
D3D9 Layers? D3D9 Layers-

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsFrameList::RemoveFrame 	layout/generic/nsFrameList.cpp:133
1 	xul.dll 	nsTableFrame::PushChildren 	layout/tables/nsTableFrame.cpp:1958
2 	xul.dll 	nsTableFrame::ReflowChildren 	
3 	xul.dll 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1900
4 	xul.dll 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1804
5 	xul.dll 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:740
6 	xul.dll 	nsTableOuterFrame::Reflow 	layout/tables/nsTableOuterFrame.cpp:1089
7 	xul.dll 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:297
8 	xul.dll 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3184
9 	xul.dll 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2506
10 	xul.dll 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1999
11 	xul.dll 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:1080
12 	xul.dll 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:297
13 	xul.dll 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3184
14 	xul.dll 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2506
15 	xul.dll 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1999
...

More reports at:
https://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=nsFrameList%3A%3ARemoveFrame%28nsIFrame*%29

Comment 1

[Wayne Mery (:wsmwk)]

*not* a recent regression afaict. going back to at least 4.0b1
bp-2d7e4a80-15b1-46ff-bb13-fe7d52100715
EXCEPTION_ACCESS_VIOLATION
0x20
0	xul.dll	nsFrameList::RemoveFrame	layout/generic/nsFrameList.cpp:133
1	xul.dll	nsTableFrame::PushChildren	layout/tables/nsTableFrame.cpp:1941
2	xul.dll	nsTableFrame::ReflowChildren	
3	xul.dll	nsTableFrame::ReflowTable	layout/tables/nsTableFrame.cpp:1883
4	xul.dll	nsTableFrame::Reflow	layout/tables/nsTableFrame.cpp:1787 


#100 crash for 4.0

Comment 2

[timeless]

Bug 512336. Make frame lists doubly-linked. r=roc,fantasai 

seems to assert prevsibling is non null, i believe it's null.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

https://crash-stats.mozilla.com/report/index/bp-ed2ab83d-f31e-4d1a-93d5-78fa02110327
0 	xul.dll 	nsFrameList::RemoveFrame 	layout/generic/nsFrameList.cpp:133
1 	xul.dll 	nsTableFrame::PushChildren 	layout/tables/nsTableFrame.cpp:1958
2 	xul.dll 	nsTableFrame::ReflowChildren 	
3 	xul.dll 	nsTableFrame::ReflowTable 	layout/tables/nsTableFrame.cpp:1900
4 	xul.dll 	nsTableFrame::Reflow 	layout/tables/nsTableFrame.cpp:1804
5 	xul.dll 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:740
6 	xul.dll 	nsTableOuterFrame::Reflow 	layout/tables/nsTableOuterFrame.cpp:1089
7 	xul.dll 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:297
8 	xul.dll 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3184
9 	xul.dll 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2506
10 	xul.dll 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:1999
etc...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> i believe it's null.

Yes, so it is.

What's happening is that nsTableFrame::PushChildren is being called with aRowGroups containing a single frame twice.  The first instance of it is removed; then the second instance fails the assertions and crashes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The problem is that in nsTableFrame::ReflowChildren when we end up with an incomplete rowgroup that already has a next-in-flow we _still_ insert the next-in-flow into rowGroups.  Even if it's already there.  

The code to do that was added in bug 347367.

It looks like at the time this code was added OrderRowGroups had this block in its mFrames traversal:

3165   // Get the next sibling but skip it if it's also the next-in-flow, since
3166   // a next-in-flow will not be part of the current table.
3167   while (kidFrame) {
3168     nsIFrame* nif = kidFrame->GetNextInFlow();
3169     kidFrame = kidFrame->GetNextSibling();
3170     if (kidFrame != nif) 
3171       break;
3172   }

Bug 347367 was working around the fact that that comment was false, as far as I can tell.  That block is still there, but the problem here is that the child list of the table looks like this: A, B, C where A->mNextContinuation == C (!).

A and C are frames for <tbody> while B is a frame for <tfoot>.  This seems ... broken.

Comment 6

[Bernd]

At first glance it looks to me that 

we reflow the outer table frame
it reflows the caption
it reflows the inner table frame without reducing the available height by the space taken by the caption
the inner table splits, creates the next inflows and reports that it is incomplete
the outer table frame reports incomplete and frame truncated as the height of the table + caption exceeds the available height
we reflow the same outer table on the second page as it was truncated, now the inner table tries to reflow the splitted row group => boom.

Comment 7

[Bernd]

Attachment: initial patch no reftests just to fix the crash

my last comment is basically saying that one should fix bug 294991 and its sibling bug 292124. The test cases in both bugs are fixed by this patch.

What remains to do is:
1. verify that the patch decently works for caption side: top bottom and
NS_STYLE_CAPTION_SIDE_TOP_OUTSIDE 
NS_STYLE_CAPTION_SIDE_BOTTOM_OUTSIDE 
by creating the reftests
2. stress test  with a large caption that covers more than one page.
3. apply all sorts of vertical margins to the caption and the table.

Comment 8

[Arthur K. (he/him)]

You got a typo on line 1080:

Is..
// now that we now the height of the caption reduce the available height for

Should be..
// now that we know the height of the caption reduce the available height for

Comment 9

[Daniel Veditz [:dveditz]]

If this is a topcrash we should get it into FF5, but otherwise we aren't planning another 4.0.x release other than chemspills

Comment 10

[Bernd]

Attachment: next rev

update with a few reftests

Comment 11

[Boris Zbarsky [:bzbarsky]]

Bernd, are you going to have a chance to finish this up and drive it in?  Or would you prefer that someone else take it over?

Comment 12

[Bernd]

Attachment: wip

The problem with attachment 526940 [details] [diff] [review] is that handles the cases table-caption-splitaftercaption-8.html till table-caption-splitaftercaption-11.html wrong where a large caption causes a push to the next page.

Comment 13

[Bernd]

this is also not fixed in the wip

Comment 14

[Bernd]

please notice that most of the test cases cover situations which we did not cover before.

Comment 15

[Bernd]

(In reply to comment #11)
> Bernd, are you going to have a chance to finish this up and drive it in?  Or
> would you prefer that someone else take it over?

It looks to me, that this will not be done in a few weeks time frame from my side.
So whoever want's to continue please feel free to take the code.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

This is pretty critical. Some Finnish sites crash because of this.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I think this bug need a new owner for the moment....

Comment 18

[Boris Zbarsky [:bzbarsky]]

ccing Mats too.

Comment 19

[Asa Dotzler [:asa]]

Tracking this because while the total crashes might be lower than other crashes, printing could be a low enough volume activity that this could be a more serious problem. Probably too late for 6, but we should try to get a handle on this for 7.

Comment 21

[Mats Palmgren (inactive)]

I think we should take the patch to fix the crash and file a follow-up bug
for the remaining cases.  Fixing those correctly seems hard at first glance
since there's no support for splitting caption frames and doing a
not-quite-correct fix that splits the inner table and pushes all its children
seems risky and not worth it.

I would add:
  innerRS->availableHeight = NS_MAX(0, innerRS->availableHeight)
at the end though (it becomes negative in several of the tests).

Comment 22

[Mats Palmgren (inactive)]

Attachment: fix

I filed bug 672654 for the remaining work (described in comment 12).

Comment 23

[Mats Palmgren (inactive)]

Attachment: fix v2

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 546995 [details] [diff] [review]
fix v2

r=me.  Thank you!

Comment 25

[Mats Palmgren (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2676a94cee76

Comment 26

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2676a94cee76

Comment 27

[Martijn Wargers (dead)]

Verified fixed, using Nightly 8.0a1 (2011-07-28)

Comment 28

[Martijn Wargers (dead)]

I filed bug 675490 for the new crash.

Comment 29

[Bernd]

This and bug 675490 have the same crash signature, during my tests for this bug I had seen the layout going wrong due to the reasons outlined in bug 675490, and that wrong layout was the reason why I was so hesitant to push this patch but I did not recognize at that time what the root cause of the problem was.
So when pushing this to aurora or beta the fix for bug 675490 needs to be pushed too, to silence this crash signature.

Comment 30

[LegNeato]

We're going to stop tracking this for Firefox 7. It will be fixed when it comes up naturally through the release process.