Last Comment Bug 399412 - "ASSERTION: overflow containers must be zero-height" with -moz-column, overflowing height
: "ASSERTION: overflow containers must be zero-height" with -moz-column, overfl...
Status: RESOLVED FIXED
: assertion, testcase, verified1.9.0.15
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: fantasai
:
:
Mentors:
Depends on:
Blocks: stirdom 472774 503961
  Show dependency treegraph
 
Reported: 2007-10-11 00:39 PDT by Jesse Ruderman
Modified: 2009-12-01 10:47 PST (History)
9 users (show)
fantasai.bugs: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.8+
.8-fixed


Attachments
testcase (460 bytes, text/html)
2007-10-11 00:39 PDT, Jesse Ruderman
no flags Details
patch (744 bytes, patch)
2009-05-07 15:33 PDT, fantasai
roc: review+
roc: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review
patch as-landed (with testcase), backported to 1.9.0.x (2.35 KB, patch)
2009-09-24 13:09 PDT, Daniel Holbert [:dholbert]
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-10-11 00:39:14 PDT
Created attachment 284427 [details]
testcase

Loading the testcase triggers:

###!!! ASSERTION: overflow containers must be zero-height: 'aMetrics.height == 0', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 1370
Comment 1 fantasai 2007-10-17 15:01:04 PDT
So, I think what's happening is this:
  The first frame for each div.container is a normal frame. The following frames are all overflow containers.
  When the height restriction is removed, we reflow the first div.container, converting its continuations to normal frames, pulling them off the overflow container list and onto the main child list. This has the effect of pushing back the second div.container's first-in-flow. But that div's overflow containers are still in the overflow containers lists of earlier frames, and they get reflowed on the overflow container list before their first-in-flow gets reflowed and therefore before they converted to normal frames. The frame tree is fine at the end of reflow, but the overflow containers do get reflowed twice and that first premature reflow is confusing things and triggering that assert.
  I don't know what's the right way to fix this bug yet. Ideally we'd want to avoid that premature reflow, but I don't know how to detect when that's the case.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-10-17 15:32:40 PDT
> This has the effect of pushing
> back the second div.container's first-in-flow. But that div's overflow
> containers are still in the overflow containers lists of earlier frames, and
> they get reflowed on the overflow container list before their first-in-flow
> gets reflowed

This is wrong, I think. Frames always should be reflowed in flow order.
Comment 3 fantasai 2009-04-29 16:48:30 PDT
Hm, maybe this could be fixed by checking that an overflow continuation's prev-in-flow's parent is the OC's parent's prev-in-flow. If not, the OC gets skipped, assuming it will get reflowed later.
Comment 4 fantasai 2009-05-07 14:05:21 PDT
A related problem would be, I'm not sure OC containers are currently getting pushed forward when their prev-in-flows shift down.
Comment 5 fantasai 2009-05-07 15:33:53 PDT
Created attachment 376324 [details] [diff] [review]
patch

The OverflowContinuationTracker takes care of everything, we just need to skip reflow when the OC frame hasn't been moved yet.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-07 16:09:46 PDT
Comment on attachment 376324 [details] [diff] [review]
patch

need to add the testcase of course.
Comment 7 Daniel Holbert [:dholbert] 2009-09-24 12:54:27 PDT
For the record, the landing for this bug was:
http://hg.mozilla.org/mozilla-central/rev/056faf1793c7

Also: it looks like 1.9.0.x branch needs this bug's patch, in order to complete the fix for bug 503961. (see bug 503961 comment 38.)
Comment 8 Daniel Holbert [:dholbert] 2009-09-24 13:09:24 PDT
Created attachment 402644 [details] [diff] [review]
patch as-landed (with testcase), backported to 1.9.0.x

Here's the patch with testcase (as landed on m-c), with bitrot fixed so it applies on 1.9.0.x without any fuzz.
Comment 9 Daniel Holbert [:dholbert] 2009-09-24 13:17:15 PDT
Comment on attachment 402644 [details] [diff] [review]
patch as-landed (with testcase), backported to 1.9.0.x

Requesting approval to land this for 1.9.0.15, to finish fixing bug 503961.  

It just fixes a case where we reflow an overflow continuation frame too early, when we know we'll end up coming back and reflowing it again later anyway. [see comment 2, comment 3]
Comment 10 Daniel Holbert [:dholbert] 2009-09-24 13:42:22 PDT
BTW, I've confirmed that this bug's testcase's "overflow containers must be zero-height" assertion becomes fixed on the 1.9.0.x branch, when I apply attachment 402644 [details] [diff] [review].
Comment 11 Daniel Veditz [:dveditz] 2009-09-25 11:12:27 PDT
Comment on attachment 402644 [details] [diff] [review]
patch as-landed (with testcase), backported to 1.9.0.x

Approved for 1.9.0.15, a=dveditz for release-drivers -- please land ASAP!
Comment 12 Daniel Holbert [:dholbert] 2009-09-25 11:46:57 PDT
Landed on CVS trunk for 1.9.0.15:

Checking in layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.315; previous revision: 1.314
done
RCS file: /cvsroot/mozilla/layout/generic/crashtests/399412-1.html,v
done
Checking in layout/generic/crashtests/399412-1.html;
/cvsroot/mozilla/layout/generic/crashtests/399412-1.html,v  <--  399412-1.html
initial revision: 1.1
done
Checking in layout/generic/crashtests/crashtests.list;
/cvsroot/mozilla/layout/generic/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.129; previous revision: 1.128
done
Comment 13 Al Billings [:abillings] 2009-09-28 16:05:33 PDT
Verified for 1.9.0.15 with bug 503961.
Comment 14 Mats Palmgren (:mats) 2009-11-19 20:08:18 PST
Comment on attachment 376324 [details] [diff] [review]
patch

It appears this never landed on the 1.9.1 branch
Comment 16 Daniel Veditz [:dveditz] 2009-11-30 15:00:26 PST
Comment on attachment 376324 [details] [diff] [review]
patch

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 17 Mats Palmgren (:mats) 2009-12-01 10:47:46 PST
Landed for 1.9.1.7, with test:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6ae23801c1ba

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