Closed Bug 503961 Opened 10 years ago Closed 10 years ago

"ASSERTION: overflow containers out of order" with -moz-column


(Core :: Layout, defect, P2)




Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- unaffected


(Reporter: jruderman, Assigned: dholbert)


(Blocks 1 open bug)


(4 keywords)


(7 files, 5 obsolete files)

Attached file testcase
###!!! ASSERTION: overflow containers out of order: '!(mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont))', file /Users/jruderman/central/layout/generic/nsContainerFrame.cpp, line 1504
Confirmed on a linux m-c build from today.  Platform --> All
OS: Mac OS X → All
Hardware: x86 → All
I can reproduce this on CVS Trunk, too, but not on 1.9.1.  Possibly a regression from Bug 502017 (/ Bug 67752) or bug 465651...
Appears to be regression from bug 465651.  If I revert that bug's patch on CVS Trunk, it fixes the assertion on the testcase posted here.
Blocks: 465651
Here's a further-reduced testcase.
Attached file testcase 3 (obsolete) —
reduced a tad more, and switched to basic HTML (rather than xhtml).
Attachment #388521 - Attachment is obsolete: true
Comment on attachment 388521 [details]
testcase 3

d'oh, testcase 3 doesn't reproduce the assertion failure for me after all... I must've made a mistake when reducing. --> obsolete
Also, testcase 2 actually has a slightly different assertion:
###!!! ASSERTION: overflow container must not have computedHeightLeftOver: '!( IS_TRUE_OVERFLOW_CONTAINER(this) && computedHeightLeftOver )', file mozilla/layout/generic/nsBlockFrame.cpp, line 1319

However, I think it's the same root cause, because it becomes fixed if I revert bug 465651's patch. (like the original testcase here & its assertion)
Keywords: regression
Here's a more-reduced testcase, with a chunk of added style for better visualization.  It triggers the "overflow container must not have computedHeightLeftOver" assertion.
Testcase 4 doesn't trigger any assertions for me on Mac.  Testcase 2 still does.
Comment on attachment 399578 [details]
testcase 4 (linux-only; computedHeightLeftOver assertion)

Interesting -- both #2 and #4 definitely trigger the computedHeightLeftOver assertion for me on Linux.

I tuned the values in testcase 4 such that they're basically as small as possible while still triggering the assertion on my machine -- perhaps the math works out slightly differently on other platforms and avoids the assertion, though.
Attachment #399578 - Attachment description: testcase 4 → testcase 4 (linux-only; computedHeightLeftOver assertion)
Here's a reduced testcase that reproduces the original "overflow containers out of order" assertion on my Linux box.
So, it looks like these assertions are effectively warning us that layout is reflowing frames in a different order from what we expect.  We end up hitting a "nsBlockFrame::Reflow" call for the 3rd column, and we have a continuation chain F->F'->F'', where F' is in Column 2's overflow-lines list and F'' is in Column 3's overflow-containers-list.  During our reflow of column 3, we *first* reflow F'' (inside of ReflowOverflowContainerChildren) *before* we end up draining F' from column 2's overflow-lines to column 3 (in DrainOverflowLines).

This is backwards -- we should be draining our prev-in-flow's overflow lines *before* we reflow our overflow container.

So, this should be a trivial fix -- we just need to shift the ReflowOverflowContainerChildren call to after DrainOverflowLines.  I've already verified that this fixes the problem -- after I test a little more, I'll post a patch.
Assignee: nobody → dholbert
Flags: in-testsuite?
Attached patch m-c fix v1 (obsolete) — Splinter Review
This patch passes reftests locally, and it's currently going through try-server for extra bulletproofing.

fantasai, do you think this fix makes sense?
Attachment #399846 - Flags: review?
Attachment #399846 - Flags: review? → review?(fantasai.bugs)
Attached patch 1.9.0.x fix v1 (obsolete) — Splinter Review
Here's the fix for CVS trunk -- it's just the backported m-c fix, plus a variable-renaming (s/overflowContainerBounds/ocBounds/) to match m-c in the modified code.
(In reply to comment #15)
> This patch passes reftests locally

Just kidding -- looks like this patch breaks "abspos-overflow-01.xhtml" on try-server.  That test failed in my local run too, but I overlooked it amongst the noise of the reftests that always fail on my machine.

In that testcase, the third page ends up missing the left half of its green line.  Looking into it.
Attachment #399846 - Flags: review?(fantasai.bugs)
Attached patch m-c fix v2 (obsolete) — Splinter Review
Talked with fantasai on IRC a bit, and I think we've got this.  So, the problem with the old fix is that ReflowOverflowContainerChildren can mess with the overflow containers lists, and if it does that after |state| (and its tracker) is already initialized, then that tracker is left with stale data.

So, one solution is to just initialize |state|'s tracker later on, closer to where it's actually used -- in ReflowDirtyLines.  This patch does that, and it appears to fix the problem on "abspos-overflow-01.xhtml".  I'm running it through reftests right now.
Attachment #399846 - Attachment is obsolete: true
Attachment #399849 - Attachment is obsolete: true
Attached patch m-c fix v3Splinter Review
At fantasai's suggestion, I ran the last patch through the pagination reftests with "#define DISABLE_FLOAT_BREAKING_IN_COLUMNS" commented out in nsBlockFrame.cpp, and found a crash on float-continuations-000.html!

It turns out that "ReflowFloatContinuations" can end up indirectly calling nsBlockReflowContext::ReflowBlock, which calls aState.mOverflowTracker->Finish(). And that crashes if the tracker's not initialized yet. :)

This new patch avoids that problem by initializing the tracker one level higher, in nsBlockFrame::Reflow, before the call to ReflowFloatContinuations. (but crucially *after* the ReflowOverflowContainerChildren call, to fix the original bug here :))

(I've also used the "-d" diff option to make this patch more concise than the previous one.  In my previous patch, |hg diff| picked a kind of roundabout way to represent "move the ReflowOverflowContainerChildren chunk down a few lines", but this one uses a smarter representation.)
Attachment #400145 - Attachment is obsolete: true
Attached patch 1.9.0.x fix v3 (obsolete) — Splinter Review
Here's the 1.9.0.x version of the latest patch. I've got try-servers testing this on mozilla-central and 1.9.0.x right now.
Attachment #399849 - Attachment description: cvs trunk fix v1 → 1.9.0.x fix v1
Attached patch 1.9.0.x fix v3Splinter Review
sorry, wrong file -- here's the right one.
Attachment #400199 - Attachment is obsolete: true
Comment on attachment 400159 [details] [diff] [review]
m-c fix v3

Requesting review on patch v3.

The TryServer run yesterday had some sporadic failures, but I resubmitted it today (as "try-9b80ee2324be"), and it passed unittests on all platforms. Per fantasai's suggestion, I've also confirmed that this patch doesn't affect our behavior on the pagination reftests when I re-enable float breaking in columns.  
The crashtests in the patch are testcase #2 & #5 from this bug (to catch both assertions mentioned here).

See comment 18 & comment 19 for description of the patch itself.
Attachment #400159 - Flags: review?(fantasai.bugs)
Attachment #400159 - Flags: review?(roc)
Attachment #388514 - Attachment description: testcase 2 → testcase 2 (computedHeightLeftOver assertion)
Comment on attachment 400159 [details] [diff] [review]
m-c fix v3

As I said, great detective work on this, dholbert. :)
Attachment #400159 - Flags: review?(fantasai.bugs) → review+
pushed m-c fix v3:
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 400200 [details] [diff] [review]
1.9.0.x fix v3

Requesting approval for 1.9.0.x fix.
Attachment #400200 - Flags: approval1.9.0.15?
Are we going to also fix this in 1.9.2 and 1.9.1? Seems odd to push for a fix on our oldest supported branch and not the more up to date ones.
Flags: wanted1.9.0.x+
Flags: blocking1.9.2?
Flags: blocking1.9.0.15+
dveditz and I just chatted about this -- the answer to comment 26 is:
 - This bug doesn't affect 1.9.1, because patch that broke this never landed there.
 - This bug *does* affect 1.9.2 (and is a regression), so we do need this on 1.9.2

m-c patch doesn't apply cleanly on 1.9.2 -- I'll post a 1.9.2 patch shortly.
Attached patch 1.9.2 fix v3Splinter Review
Here's the 1.9.2 version of this fix. It's basically the same as the 1.9.0.x version (which applies on 1.9.2 with fuzz).

Requesting approval to fix this on 1.9.2.
Attachment #400610 - Flags: approval1.9.2?
Flags: in-testsuite? → in-testsuite+
(BTW, I just verified that the 1.9.2 patch passes unittests on the TryServer.  Mac & Linux were green, and Windows had just 1 sporadic mochitest timeout -- browser/components/sessionstore/test/browser/browser_248970_a.js -- which couldn't be caused by this patch)
Attachment #400200 - Flags: approval1.9.0.15? → approval1.9.0.15+
Comment on attachment 400200 [details] [diff] [review]
1.9.0.x fix v3

Approved for, a=dveditz for release-drivers
Checked in to CVS Trunk for
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.966; previous revision: 3.965
Checking in layout/generic/nsBlockReflowContext.cpp;
/cvsroot/mozilla/layout/generic/nsBlockReflowContext.cpp,v  <--  nsBlockReflowContext.cpp
new revision: 1.158; previous revision: 1.157
Checking in layout/generic/nsBlockReflowState.cpp;
/cvsroot/mozilla/layout/generic/nsBlockReflowState.cpp,v  <--  nsBlockReflowState.cpp
new revision: 3.548; previous revision: 3.547
Checking in layout/generic/nsBlockReflowState.h;
/cvsroot/mozilla/layout/generic/nsBlockReflowState.h,v  <--  nsBlockReflowState.h
new revision: 3.483; previous revision: 3.482
RCS file: /cvsroot/mozilla/layout/generic/crashtests/503961-1.xhtml,v
Checking in layout/generic/crashtests/503961-1.xhtml;
/cvsroot/mozilla/layout/generic/crashtests/503961-1.xhtml,v  <--  503961-1.xhtml
initial revision: 1.1
RCS file: /cvsroot/mozilla/layout/generic/crashtests/503961-2.html,v
Checking in layout/generic/crashtests/503961-2.html;
/cvsroot/mozilla/layout/generic/crashtests/503961-2.html,v  <--  503961-2.html
initial revision: 1.1
Checking in layout/generic/crashtests/crashtests.list;
/cvsroot/mozilla/layout/generic/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.128; previous revision: 1.127
Keywords: fixed1.9.0.15
(In reply to comment #27)
>  - This bug *does* affect 1.9.2 (and is a regression), so we do need this on
> 1.9.2

roc, can you grant approval1.9.2 on the patch for that branch? (attachment 400610 [details] [diff] [review])
Attachment #400610 - Flags: approval1.9.2? → approval1.9.2+
Thanks for the approval -- attachment 400610 [details] [diff] [review] pushed to 1.9.2:
Setting status1.9.2 --> beta1-fixed
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
This doesn't look fixed for 1.9.0. I built my own debug build yesterday (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/2009092315 GranParadiso/3.0.15pre).

Running testcase 1 on OS X, I get: 

###!!! ASSERTION: overflow containers out of order: '!(mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont))', file /Users/albill/mozilla/layout/generic/nsContainerFrame.cpp, line 1445

Running testcase 2, I get: 

###!!! ASSERTION: overflow container must not have computedHeightLeftOver: '!( IS_TRUE_OVERFLOW_CONTAINER(this) && computedHeightLeftOver )', file /Users/albill/mozilla/layout/generic/nsBlockFrame.cpp, line 1333

Running testcase 4, I get:

###!!! ASSERTION: overflow container must not have computedHeightLeftOver: '!( IS_TRUE_OVERFLOW_CONTAINER(this) && computedHeightLeftOver )', file /Users/albill/mozilla/layout/generic/nsBlockFrame.cpp, line 1333

Running testcase 5, I get: 

###!!! ASSERTION: overflow containers out of order: '!(mOverflowContList && mOverflowContList->ContainsFrame(aOverflowCont))', file /Users/albill/mozilla/layout/generic/nsContainerFrame.cpp, line 1445
Hrm, strange.  It's definitely fixed in my mozilla-central build...  I'm grabbing fresh checkouts of 1.9.0 & 1.9.2 and investigating both of those branches.
I've verified that this is fixed for me in a fresh 1.9.2 checkout:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2b1pre) Gecko/20090924 Namoroka/3.6b1pre
Built from

This is good news, because the 1.9.2 and 1.9.0 patches are almost identical.

I'll report back on 1.9.0 when that checkout & build are done... (its CVS checkout is going really slowly for some reason)
Ok, I can confirm that this is still broken on CVS trunk (1.9.0) with testcases 1,2,4,5.  I've verified (with "patch -R -F30 < patchfile") that 1.9.0 isn't missing any of the code from the 1.9.0 patch, or any of the code from 1.9.2 patch.  So this isn't just a case of missing a file in the CVS commit...

Looking into this further.
Based on comment 36 & comment 37, I tested a few patches that have landed on 1.9.2 but not on 1.9.0, and I think I've found the right one.  It looks like bug 399412 ( ) is what we need to fully fix this bug.  (It landed on mozilla-central before the 1.9.2 branch date.)

After applying that bug's patch on 1.9.0 (with fuzz=4, for crashtests.list bitrot), this bug's assertions go away on all testcases.
Depends on: 399412
abillings: I've now landed bug 399412 on CVS trunk, so the fix for this bug should now be complete on that branch.  Can you re-verify?  (Thanks again for catching this!)
This is now fixed. I built a 1.9.0 build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/2009092708 GranParadiso/3.0.15pre) and checked the same testcases. No assertions.
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.