Last Comment Bug 559241 - ###!!! ASSERTION: this type of frame can't have overflow containers
: ###!!! ASSERTION: this type of frame can't have overflow containers
Status: RESOLVED FIXED
[sg:critical][critsmash:resolved]
: fixed1.9.0.20, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
https://bugzilla.mozilla.org/attachme...
Depends on:
Blocks: 537631
  Show dependency treegraph
 
Reported: 2010-04-13 18:17 PDT by Mats Palmgren (vacation)
Modified: 2013-05-18 03:20 PDT (History)
9 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.7+
.7-fixed
.11-fixed


Attachments
wip1 (1.70 KB, patch)
2010-04-13 18:53 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
fix (2.04 KB, patch)
2010-05-18 14:47 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review-
Details | Diff | Splinter Review
fix v2 (2.19 KB, patch)
2010-05-18 15:57 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
christian: approval1.9.2.7+
christian: approval1.9.1.11+
christian: approval1.9.0.next+
Details | Diff | Splinter Review

Description Mats Palmgren (vacation) 2010-04-13 18:17:00 PDT
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
Comment 1 Mats Palmgren (vacation) 2010-04-13 18:37:05 PDT
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?
Comment 2 Mats Palmgren (vacation) 2010-04-13 18:53:22 PDT
Created attachment 438924 [details] [diff] [review]
wip1

nsPageContentFrame inherits ViewportFrame so we can remove it there
without breaking bug 509839.
Comment 3 Damon Sicore (:damons) 2010-04-20 13:47:30 PDT
Can we get a call as to whether or not this is really an sg:crit?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-04-20 14:12:38 PDT
I'm not sure, but since we have a patch, no point in agonizing over it.
Comment 5 fantasai 2010-04-20 14:37:22 PDT
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.
Comment 6 Mats Palmgren (vacation) 2010-04-21 10:09:47 PDT
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.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-04-21 14:00:20 PDT
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.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-04 12:39:33 PDT
Taking
Comment 9 Damon Sicore (:damons) 2010-05-11 13:26:30 PDT
Roc promises to look at this before next CritSmash meeting, w/ Vlad.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-18 13:50:37 PDT
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.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-18 14:47:32 PDT
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.
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-05-18 15:30:51 PDT
Would calling nsHTMLReflowState::ShouldReflowAllKids outside the loop be sufficient?  It seems better not to reinvent that function one piece at a time.
Comment 13 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-05-18 15:31:17 PDT
i.e., calling it outside the loop and making the condition
  shouldReflowAllKids || NS_SUBTREE_DIRTY(frame)
Comment 14 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-05-18 15:51:07 PDT
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.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-18 15:57:48 PDT
Created attachment 446084 [details] [diff] [review]
fix v2

Yes, this does work. Thanks.
Comment 16 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-05-18 16:20:21 PDT
Comment on attachment 446084 [details] [diff] [review]
fix v2

r=dbaron
Comment 17 Damon Sicore (:damons) 2010-05-18 17:55:37 PDT
Blocking 1.9.3 final as it's an sg:crit.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-18 22:53:41 PDT
http://hg.mozilla.org/mozilla-central/rev/a55765a1c2f5
Comment 19 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-05-19 07:40:04 PDT
This patch fixed one assertion, on layout/generic/crashtests/514800-1.html.
Comment 20 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-05-19 07:42:55 PDT
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
Comment 21 Mats Palmgren (vacation) 2010-05-21 07:33:21 PDT
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).
Comment 22 christian 2010-06-11 15:51:48 PDT
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.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-23 22:32:57 PDT
1.9.1: a1e407b24260
1.9.2: 781a027428c7
Comment 24 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-16 11:27:04 PDT
(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?
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-18 16:12:33 PDT
Please do!
Comment 26 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-18 21:48:50 PDT
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!
Comment 27 Mats Palmgren (vacation) 2013-05-18 03:20:21 PDT
(test added in bug 537631)

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