The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
--
critical
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mats, Assigned: roc)

Tracking

({fixed1.9.0.20, testcase})

unspecified
fixed1.9.0.20, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .7+, status1.9.2 .7-fixed, status1.9.1 .11-fixed)

Details

(Whiteboard: [sg:critical][critsmash:resolved], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
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?
(Reporter)

Comment 2

7 years ago
Created attachment 438924 [details] [diff] [review]
wip1

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 5

7 years ago
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.
(Reporter)

Comment 6

7 years ago
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.

Updated

7 years ago
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.
Created attachment 446063 [details] [diff] [review]
fix

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-
Created attachment 446084 [details] [diff] [review]
fix v2

Yes, this does work. Thanks.
Attachment #446063 - Attachment is obsolete: true
Attachment #446084 - Flags: review?(dbaron)
Comment on attachment 446084 [details] [diff] [review]
fix v2

r=dbaron
Attachment #446084 - Flags: review?(dbaron) → review+
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
Last Resolved: 7 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

Updated

7 years ago
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:resolved]
(Reporter)

Comment 21

7 years ago
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?
(Reporter)

Updated

7 years ago
Blocks: 537631

Updated

7 years ago
blocking1.9.2: --- → .6+
status1.9.2: --- → wanted

Comment 22

7 years ago
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?
Please do!
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
(Reporter)

Comment 27

4 years ago
(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.