Last Comment Bug 642088 - Crash while printing or print-previewing [@ nsFrameList::RemoveFrame(nsIFrame*) ] - [@ nsTableFrame::PushChildren]
: Crash while printing or print-previewing [@ nsFrameList::RemoveFrame(nsIFrame...
Status: VERIFIED FIXED
[sg:dos][inbound]
: crash, regression, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla8
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
http://www.open.edu.au/public/student...
: 665257 690055 (view as bug list)
Depends on:
Blocks: 665257
  Show dependency treegraph
 
Reported: 2011-03-16 02:06 PDT by Scoobidiver (away)
Modified: 2013-12-27 14:19 PST (History)
16 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
testcase (512 bytes, text/html)
2011-03-27 15:09 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
initial patch no reftests just to fix the crash (1.28 KB, patch)
2011-04-10 11:19 PDT, Bernd
no flags Details | Diff | Splinter Review
next rev (11.57 KB, patch)
2011-04-19 00:03 PDT, Bernd
no flags Details | Diff | Splinter Review
wip (25.97 KB, patch)
2011-06-19 11:49 PDT, Bernd
no flags Details | Diff | Splinter Review
fix (26.29 KB, patch)
2011-07-19 18:06 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v2 (26.34 KB, patch)
2011-07-19 22:46 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2011-03-16 02:06:27 PDT
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, NI for questions) 2011-03-27 06:34:31 PDT
*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 2011-03-27 07:32:17 PDT
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 [:mwargers] (not working for Mozilla) 2011-03-27 15:09:55 PDT
Created attachment 522251 [details]
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 [:bz] (still a bit busy) 2011-04-05 19:37:32 PDT
> 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 [:bz] (still a bit busy) 2011-04-05 20:06:25 PDT
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 2011-04-09 00:31:21 PDT
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 2011-04-10 11:19:44 PDT
Created attachment 524976 [details] [diff] [review]
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. 2011-04-14 11:19:26 PDT
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] 2011-04-15 11:09:39 PDT
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 2011-04-19 00:03:17 PDT
Created attachment 526940 [details] [diff] [review]
next rev

update with a few reftests
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-06-17 21:53:15 PDT
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 2011-06-19 11:49:44 PDT
Created attachment 540337 [details] [diff] [review]
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 2011-06-19 11:50:17 PDT
this is also not fixed in the wip
Comment 14 Bernd 2011-06-19 11:51:34 PDT
please notice that most of the test cases cover situations which we did not cover before.
Comment 15 Bernd 2011-06-19 12:31:53 PDT
(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] 2011-07-18 11:38:15 PDT
This is pretty critical. Some Finnish sites crash because of this.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-07-18 11:44:08 PDT
I think this bug need a new owner for the moment....
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-07-18 11:44:26 PDT
ccing Mats too.
Comment 19 Asa Dotzler [:asa] 2011-07-18 14:50:53 PDT
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 20 christian 2011-07-18 15:33:53 PDT
*** Bug 665257 has been marked as a duplicate of this bug. ***
Comment 21 Mats Palmgren (:mats) 2011-07-18 17:21:19 PDT
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 (:mats) 2011-07-19 18:06:05 PDT
Created attachment 546957 [details] [diff] [review]
fix

I filed bug 672654 for the remaining work (described in comment 12).
Comment 23 Mats Palmgren (:mats) 2011-07-19 22:46:15 PDT
Created attachment 546995 [details] [diff] [review]
fix v2
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-07-21 13:57:35 PDT
Comment on attachment 546995 [details] [diff] [review]
fix v2

r=me.  Thank you!
Comment 26 Marco Bonardo [::mak] 2011-07-27 03:26:39 PDT
http://hg.mozilla.org/mozilla-central/rev/2676a94cee76
Comment 27 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-29 00:40:36 PDT
Verified fixed, using Nightly 8.0a1 (2011-07-28)
Comment 28 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-31 02:38:42 PDT
I filed bug 675490 for the new crash.
Comment 29 Bernd 2011-08-07 05:01:02 PDT
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 christian 2011-09-06 14:56:18 PDT
We're going to stop tracking this for Firefox 7. It will be fixed when it comes up naturally through the release process.
Comment 31 Benjamin Smedberg [:bsmedberg] 2011-09-28 13:35:07 PDT
*** Bug 690055 has been marked as a duplicate of this bug. ***

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