Closed Bug 559241 Opened 14 years ago Closed 14 years ago

###!!! ASSERTION: this type of frame can't have overflow containers

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .7+
status1.9.2 --- .7-fixed
status1.9.1 --- .11-fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: roc)

References

(
URL
)

Details

(Keywords: fixed1.9.0.20, testcase, Whiteboard: [sg:critical][critsmash:resolved])

Attachments

(1 file, 2 obsolete files)

fix v2
2.19 KB, patch
dbaron
: review+
christian
: approval1.9.2.7+
christian
: approval1.9.1.11+
christian
: approval1.9.0.next+
Details | Diff | Splinter Review 
From bug 537631 comment 2:

========= Jesse Ruderman      2010-04-13 17:43:58 PDT

Also triggers an assertion recently added in bug 509839:

###!!! ASSERTION: this type of frame can't have overflow containers:
'(aProperty != nsContainerFrame::OverflowContainersProperty() && aProperty !=
nsContainerFrame::ExcessOverflowContainersProperty()) ||
IsFrameOfType(nsIFrame::eCanContainOverflowContainers)', file
/Users/jruderman/mozilla-central/layout/generic/nsContainerFrame.cpp, line 1212
I suspect it can lead to a crash when destroying the shell, as in bug 509839.
The container frame is a ViewPortFrame, and we're adding a child frame
to the ExcessOverflowContainersProperty frame list on line 173:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsAbsoluteContainingBlock.cpp#169
(The root cause of the problem is most likely what triggers the
assertion in bug 537631.)

We could wallpaper the problem the same way as in bug 509839
by adding the eCanContainOverflowContainers bit to ViewPortFrame.
Anyone got a better idea?
Attached patch wip1 (obsolete) — — Splinter Review 
nsPageContentFrame inherits ViewportFrame so we can remove it there
without breaking bug 509839.
Assignee: nobody → matspal
Attachment #438924 - Flags: review?(fantasai.bugs)
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
Can we get a call as to whether or not this is really an sg:crit?
I'm not sure, but since we have a patch, no point in agonizing over it.
Whiteboard: [sg:critical?][needs review] → [sg:critical][needs review]
Comment on attachment 438924 [details] [diff] [review]
wip1

r=fantasai for branch, but not for trunk, because, if I'm understanding correctly, viewport frames shouldn't be causing splits in the first place and that's what should be fixed.
Ok, but it needs to bake on trunk before landing on 1.9.2 and for that
I need r+.  I could back it out from trunk once the baking is done,
but how would we remember to apply this patch if we make new branches
from trunk?  I think it makes more sense to leave it on trunk and
file a new bug on "viewport frames causing splits".
It would be easy to add an assertion for that (checking frame type) in
nsContainerFrame::SetPropTableFrames.
How about we just fix the underlying problem here instead of worrying about where to take a wallpaper fix?

It's probably a bug in nsColumnSetFrame. We should not be creating an overflow container for the nsColumnSetFrame.
Whiteboard: [sg:critical][needs review] → [sg:critical][needs review][critsmash:investigating]
Taking
Assignee: matspal → roc
Attachment #438924 - Flags: review?(fantasai.bugs)
Whiteboard: [sg:critical][needs review][critsmash:investigating] → [sg:critical][critsmash:investigating]
Roc promises to look at this before next CritSmash meeting, w/ Vlad.
Using the testcase
<body style="position: fixed; -moz-column-count: 2;"><div style="position: absolute; height: 100px;"><br><br></div></body>
the column reflow log as we balance the columns narrows down the problem:

*** Doing column reflow pass: mLastBalanceHeight=1073741824, mColMaxHeight=3600, RTL=0
, mBalanceColCount=2, mColWidth=1, mColGap=960
*** Reflowing child #0 0x20011c70: availHeight=3600
*** Reflowed child #0 0x20011c70: status = 6, desiredSize=1,0
*** NEXT CHILD ORIGIN.x = 961
*** Reflowing child #1 0x151f138: availHeight=3600
*** Reflowed child #1 0x151f138: status = 0, desiredSize=1,0
*** DONE PASS feasible=1

...

*** Doing column reflow pass: mLastBalanceHeight=2700, mColMaxHeight=3599, RTL=0
, mBalanceColCount=2, mColWidth=1, mColGap=960
*** Reflowing child #0 0x20011c70: availHeight=3599
*** Reflowed child #0 0x20011c70: status = 4, desiredSize=1,0
*** NEXT CHILD ORIGIN.x = 961
*** Reflowing child #1 0x151f138: availHeight=3599
*** Reflowed child #1 0x151f138: status = 4, desiredSize=1,0
*** DONE PASS feasible=0

*** Doing column reflow pass: mLastBalanceHeight=3599, mColMaxHeight=3600, RTL=0
, mBalanceColCount=2, mColWidth=1, mColGap=960
*** Reflowing child #0 0x20011c70: availHeight=3600
*** Reflowed child #0 0x20011c70: status = 4, desiredSize=1,0
*** NEXT CHILD ORIGIN.x = 961
*** Reflowing child #1 0x151f138: availHeight=1073741824
*** Reflowed child #1 0x151f138: status = 4, desiredSize=1,0
*** DONE PASS feasible=0

We reflow once with column height 60px and the content fits (as it should). Later we try height 60px - epsilon and it doesn't fit, which is wrong, it should fit (in this case I believe the correct balanced column height would be 50px). Then we decide that height 60px must be the optimal height and do one last reflow with that height, and we still think the content doesn't fit and we start creating overflow frames.
Attached patch fix (obsolete) — — Splinter Review 
Really simple and safe fix.

We just neglected to reflow our overflow-container-children if they weren't dirty but the available vertical space had changed. Fixing that makes balancing work properly.
Attachment #438924 - Attachment is obsolete: true
Attachment #446063 - Flags: review?(dbaron)
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:investigating][needs review]
Would calling nsHTMLReflowState::ShouldReflowAllKids outside the loop be sufficient?  It seems better not to reinvent that function one piece at a time.
i.e., calling it outside the loop and making the condition
  shouldReflowAllKids || NS_SUBTREE_DIRTY(frame)
Comment on attachment 446063 [details] [diff] [review]
fix

Additionally, I think mVResize isn't meant to be used other than in combination with the way ShouldReflowAllKids uses it.  So if ShouldReflowAllKids works, then we should really use that instead.
Attachment #446063 - Flags: review?(dbaron) → review-
Attached patch fix v2 — — Splinter Review 
Yes, this does work. Thanks.
Attachment #446063 - Attachment is obsolete: true
Attachment #446084 - Flags: review?(dbaron)
Whiteboard: [sg:critical][critsmash:investigating][needs review] → [sg:critical][critsmash:investigating][needs landing]
Blocking 1.9.3 final as it's an sg:crit.
blocking2.0: --- → final+
http://hg.mozilla.org/mozilla-central/rev/a55765a1c2f5
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical][critsmash:investigating][needs landing] → [sg:critical][critsmash:investigating]
This patch fixed one assertion, on layout/generic/crashtests/514800-1.html.
The assertion on that test was:

###!!! ASSERTION: StealFrame: can't find aChild: 'removed', file /builds/slave/mozilla-central-linux-debug/build/layout/generic/nsContainerFrame.cpp, line 1043
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:resolved]
Comment on attachment 446084 [details] [diff] [review]
fix v2

The patch applies cleanly to 1.9.0/1/2 branches.
I think we need it there.
The testcase in bug 537631 crashes on 1.9.0 in a way that looks exploitable (the patch fixes it).
Attachment #446084 - Flags: approval1.9.2.5?
Attachment #446084 - Flags: approval1.9.1.11?
Attachment #446084 - Flags: approval1.9.0.next?
Blocks: 537631
blocking1.9.2: --- → .6+
status1.9.2: --- → wanted
Comment on attachment 446084 [details] [diff] [review]
fix v2

a=LegNeato for 1.9.2.6 and 1.9.1.11. Please land this on mozilla-1.9.2 default and mozilla-1.9.1 default.

Also approving for 1.9.0, though you don't really need to check it in there as we won't be doing an update off that branch.
Attachment #446084 - Flags: approval1.9.2.6+
Attachment #446084 - Flags: approval1.9.2.5?
Attachment #446084 - Flags: approval1.9.1.11?
Attachment #446084 - Flags: approval1.9.1.11+
Attachment #446084 - Flags: approval1.9.0.next?
Attachment #446084 - Flags: approval1.9.0.next+
1.9.1: a1e407b24260
1.9.2: 781a027428c7
status1.9.1: --- → .11-fixed
status1.9.2: wanted → .6-fixed
(In reply to comment #22)
> Also approving for 1.9.0, though you don't really need to check it in there as
> we won't be doing an update off that branch.

roc, would you mind if I checked this in for you on 1.9.0, given comment 21 and that the patch already has approval for 1.9.0.next?
Checking in layout/generic/nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.56; previous revision: 3.55
done
Checking in layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.317; previous revision: 1.316
done

Thanks!
Keywords: fixed1.9.0.20
Group: core-security
(test added in bug 537631)
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: