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

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Daniel Holbert [:dholbert]]

Confirmed on a linux m-c build from today.  Platform --> All

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (computedHeightLeftOver assertion)

Here's a further-reduced testcase.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

reduced a tad more, and switched to basic HTML (rather than xhtml).

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Daniel Holbert [:dholbert]]

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)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (linux-only; computedHeightLeftOver assertion)

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.

Comment 10

[Jesse Ruderman]

Testcase 4 doesn't trigger any assertions for me on Mac.  Testcase 2 still does.

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 5 ("containers out of order" assertion)

Here's a reduced testcase that reproduces the original "overflow containers out of order" assertion on my Linux box.

Comment 13

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: m-c fix v1

This patch passes reftests locally, and it's currently going through try-server for extra bulletproofing.

fantasai, do you think this fix makes sense?

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: 1.9.0.x fix v1

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.

Comment 17

[Daniel Holbert [:dholbert]]

(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.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252616915.1252624295.23293.gz#err1

In that testcase, the third page ends up missing the left half of its green line.  Looking into it.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: m-c fix v2

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.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: m-c fix v3

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

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: 1.9.0.x fix v3

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.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: 1.9.0.x fix v3

sorry, wrong file -- here's the right one.

Comment 22

[Daniel Holbert [:dholbert]]

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.

Comment 23

[fantasai]

Comment on attachment 400159 [details] [diff] [review]
m-c fix v3

As I said, great detective work on this, dholbert. :)

Comment 24

[Daniel Holbert [:dholbert]]

pushed m-c fix v3: http://hg.mozilla.org/mozilla-central/rev/8ec892323499

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 400200 [details] [diff] [review]
1.9.0.x fix v3

Requesting approval for 1.9.0.x fix.

Comment 26

[Daniel Veditz [:dveditz]]

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.

Comment 27

[Daniel Holbert [:dholbert]]

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.

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: 1.9.2 fix v3

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.

Comment 29

[Daniel Holbert [:dholbert]]

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

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 400200 [details] [diff] [review]
1.9.0.x fix v3

Approved for 1.9.0.15, a=dveditz for release-drivers

Comment 31

[Daniel Holbert [:dholbert]]

Checked in to CVS Trunk for 1.9.0.15:
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.966; previous revision: 3.965
done
Checking in layout/generic/nsBlockReflowContext.cpp;
/cvsroot/mozilla/layout/generic/nsBlockReflowContext.cpp,v  <--  nsBlockReflowContext.cpp
new revision: 1.158; previous revision: 1.157
done
Checking in layout/generic/nsBlockReflowState.cpp;
/cvsroot/mozilla/layout/generic/nsBlockReflowState.cpp,v  <--  nsBlockReflowState.cpp
new revision: 3.548; previous revision: 3.547
done
Checking in layout/generic/nsBlockReflowState.h;
/cvsroot/mozilla/layout/generic/nsBlockReflowState.h,v  <--  nsBlockReflowState.h
new revision: 3.483; previous revision: 3.482
done
RCS file: /cvsroot/mozilla/layout/generic/crashtests/503961-1.xhtml,v
done
Checking in layout/generic/crashtests/503961-1.xhtml;
/cvsroot/mozilla/layout/generic/crashtests/503961-1.xhtml,v  <--  503961-1.xhtml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/generic/crashtests/503961-2.html,v
done
Checking in layout/generic/crashtests/503961-2.html;
/cvsroot/mozilla/layout/generic/crashtests/503961-2.html,v  <--  503961-2.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.128; previous revision: 1.127
done

Comment 32

[Daniel Holbert [:dholbert]]

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

Comment 33

[Daniel Holbert [:dholbert]]

Thanks for the approval -- attachment 400610 [details] [diff] [review] pushed to 1.9.2:
   http://hg.mozilla.org/releases/mozilla-1.9.2/rev/88ccf54a351d
Setting status1.9.2 --> beta1-fixed

Comment 34

[Al Billings [:abillings - ex-MoCo]]

This doesn't look fixed for 1.9.0. I built my own 1.9.0.15pre debug build yesterday (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.0.15pre) 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

Comment 35

[Daniel Holbert [:dholbert]]

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.

Comment 36

[Daniel Holbert [:dholbert]]

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 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5ca1bfef41bb

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)

Comment 37

[Daniel Holbert [:dholbert]]

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.

Comment 38

[Daniel Holbert [:dholbert]]

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 ( http://hg.mozilla.org/mozilla-central/rev/056faf1793c7 ) 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.

Comment 39

[Daniel Holbert [:dholbert]]

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

Comment 40

[Al Billings [:abillings - ex-MoCo]]

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:1.9.0.15pre) Gecko/2009092708 GranParadiso/3.0.15pre) and checked the same testcases. No assertions.