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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: fantasai)

Tracking

(Blocks: 1 bug, {assertion, testcase, verified1.9.0.15})

Trunk
assertion, testcase, verified1.9.0.15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.1 .8+, status1.9.1 .8-fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

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

Comment 3

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

Comment 4

8 years ago
A related problem would be, I'm not sure OC containers are currently getting pushed forward when their prev-in-flows shift down.
(Assignee)

Updated

8 years ago
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 5

8 years ago
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.
Attachment #376324 - Flags: superreview?(roc)
Attachment #376324 - Flags: review?(roc)
Comment on attachment 376324 [details] [diff] [review]
patch

need to add the testcase of course.
Attachment #376324 - Flags: superreview?(roc)
Attachment #376324 - Flags: superreview+
Attachment #376324 - Flags: review?(roc)
Attachment #376324 - Flags: review+
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
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.)
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 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]
Attachment #402644 - Flags: approval1.9.0.15?
OS: Mac OS X → All
Hardware: x86 → All
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].
Blocks: 503961
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!
Attachment #402644 - Flags: approval1.9.0.15? → approval1.9.0.15+
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
Keywords: fixed1.9.0.15
Verified for 1.9.0.15 with bug 503961.
Keywords: fixed1.9.0.15 → verified1.9.0.15
Comment on attachment 376324 [details] [diff] [review]
patch

It appears this never landed on the 1.9.1 branch
Attachment #376324 - Flags: approval1.9.1.7?

Updated

8 years ago
Attachment #376324 - Flags: approval1.9.1.7? → approval1.9.1.6?
Blocks: 472774
Attachment #376324 - Flags: approval1.9.1.6? → approval1.9.1.7?
blocking1.9.1: --- → .7+
status1.9.1: --- → wanted
Comment on attachment 376324 [details] [diff] [review]
patch

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #376324 - Flags: approval1.9.1.7? → approval1.9.1.7+
Landed for 1.9.1.7, with test:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6ae23801c1ba
status1.9.1: wanted → .7-fixed
You need to log in before you can comment on or make changes to this bug.