399412 - "ASSERTION: overflow containers must be zero-height" with -moz-column, overflowing height

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: 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]

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)]

> 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]

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]

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]

Attachment: 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)]

Comment on attachment 376324 [details] [diff] [review]
patch

need to add the testcase of course.

Comment 7

[Daniel Holbert [:dholbert]]

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]]

Attachment: 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]]

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]]

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]]

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]]

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 - ex-MoCo]]

Verified for 1.9.0.15 with bug 503961.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 376324 [details] [diff] [review]
patch

It appears this never landed on the 1.9.1 branch

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 376324 [details] [diff] [review]
patch

Approved for 1.9.1.7, a=dveditz for release-drivers

Comment 17

[Mats Palmgren (inactive)]

Landed for 1.9.1.7, with test:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6ae23801c1ba