Closed
Bug 642088
Opened 14 years ago
Closed 13 years ago
Crash while printing or print-previewing [@ nsFrameList::RemoveFrame(nsIFrame*) ] - [@ nsTableFrame::PushChildren]
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
VERIFIED
FIXED
mozilla8
People
(Reporter: scoobidiver, Assigned: MatsPalmgren_bugz)
References
()
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:dos][inbound])
Crash Data
Attachments
(2 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
*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
Summary: Crash while printing or print-previewing [@ nsFrameList::RemoveFrame(nsIFrame*) ] → Crash while printing or print-previewing [@ nsFrameList::RemoveFrame(nsIFrame*) ] - [@ nsTableFrame::PushChildren]
Bug 512336. Make frame lists doubly-linked. r=roc,fantasai seems to assert prevsibling is non null, i believe it's null.
Comment 3•14 years ago
|
||
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...
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
Whiteboard: [sg:dos]
Comment 4•14 years ago
|
||
> 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•14 years ago
|
||
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.
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.
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.
Attachment #524976 -
Attachment is patch: true
Comment 8•14 years ago
|
||
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•14 years ago
|
||
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
blocking2.0: ? → -
Comment 10•14 years ago
|
||
update with a few reftests
Attachment #524976 -
Attachment is obsolete: true
Updated•13 years ago
|
Crash Signature: [@ nsFrameList::RemoveFrame(nsIFrame*) ]
[@ nsTableFrame::PushChildren]
Comment 11•13 years ago
|
||
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?
Crash Signature: [@ nsFrameList::RemoveFrame(nsIFrame*) ]
[@ nsTableFrame::PushChildren] → [@ nsFrameList::RemoveFrame(nsIFrame*) ]
[@ nsTableFrame::PushChildren]
Comment 12•13 years ago
|
||
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.
Attachment #526940 -
Attachment is obsolete: true
Comment 14•13 years ago
|
||
please notice that most of the test cases cover situations which we did not cover before.
Comment 15•13 years ago
|
||
(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.
Updated•13 years ago
|
OS: Windows 7 → All
Updated•13 years ago
|
Comment 19•13 years ago
|
||
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.
Updated•13 years ago
|
Crash Signature: [@ nsFrameList::RemoveFrame(nsIFrame*) ]
[@ nsTableFrame::PushChildren] → [@ nsFrameList::RemoveFrame(nsIFrame*) ]
[@ nsTableFrame::PushChildren]
[@ nsFrameList::RemoveFrame]
Assignee | ||
Comment 21•13 years ago
|
||
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).
Assignee: bernd_mozilla → matspal
Assignee | ||
Comment 22•13 years ago
|
||
I filed bug 672654 for the remaining work (described in comment 12).
Attachment #540337 -
Attachment is obsolete: true
Attachment #546957 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #546957 -
Attachment is obsolete: true
Attachment #546957 -
Flags: review?(bzbarsky)
Attachment #546995 -
Flags: review?(bzbarsky)
Comment 24•13 years ago
|
||
Comment on attachment 546995 [details] [diff] [review] fix v2 r=me. Thank you!
Attachment #546995 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 25•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/2676a94cee76
Flags: in-testsuite+
Hardware: x86 → All
Whiteboard: [sg:dos] → [sg:dos][inbound]
Target Milestone: --- → mozilla8
Version: Trunk → unspecified
Comment 26•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2676a94cee76
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 27•13 years ago
|
||
Verified fixed, using Nightly 8.0a1 (2011-07-28)
Status: RESOLVED → VERIFIED
Comment 29•13 years ago
|
||
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•13 years ago
|
||
We're going to stop tracking this for Firefox 7. It will be fixed when it comes up naturally through the release process.
tracking-firefox7:
+ → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•