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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9.3a1
assertion, regression, testcase, verified1.9.0.15
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.0.15 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 unaffected)

Details

Attachments

(7 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 388326 [details]
testcase

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

Comment 1

9 years ago
Confirmed on a linux m-c build from today.  Platform --> All
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

9 years ago
Created attachment 388514 [details]
testcase 2 (computedHeightLeftOver assertion)

Here's a further-reduced testcase.
(Assignee)

Comment 5

9 years ago
Created attachment 388521 [details]
testcase 3

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

Updated

9 years ago
Attachment #388521 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Comment 7

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

Updated

9 years ago
Keywords: regression
(Assignee)

Comment 9

9 years ago
Created attachment 399578 [details]
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.
(Reporter)

Comment 10

9 years ago
Testcase 4 doesn't trigger any assertions for me on Mac.  Testcase 2 still does.
(Assignee)

Comment 11

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

Comment 12

9 years ago
Created attachment 399784 [details]
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.
(Assignee)

Comment 13

9 years ago
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
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Comment 15

9 years ago
Created attachment 399846 [details] [diff] [review]
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?
Attachment #399846 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #399846 - Flags: review? → review?(fantasai.bugs)
(Assignee)

Comment 16

9 years ago
Created attachment 399849 [details] [diff] [review]
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.
(Assignee)

Comment 17

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

Updated

9 years ago
Attachment #399846 - Flags: review?(fantasai.bugs)
(Assignee)

Comment 18

9 years ago
Created attachment 400145 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Attachment #399846 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #399849 - Attachment is obsolete: true
(Assignee)

Comment 19

9 years ago
Created attachment 400159 [details] [diff] [review]
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.)
Attachment #400145 - Attachment is obsolete: true
(Assignee)

Comment 20

9 years ago
Created attachment 400199 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Attachment #399849 - Attachment description: cvs trunk fix v1 → 1.9.0.x fix v1
(Assignee)

Comment 21

9 years ago
Created attachment 400200 [details] [diff] [review]
1.9.0.x fix v3

sorry, wrong file -- here's the right one.
Attachment #400199 - Attachment is obsolete: true
(Assignee)

Comment 22

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

Updated

9 years ago
Attachment #400159 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Attachment #388514 - Attachment description: testcase 2 → testcase 2 (computedHeightLeftOver assertion)

Comment 23

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

Comment 24

9 years ago
pushed m-c fix v3: http://hg.mozilla.org/mozilla-central/rev/8ec892323499
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

9 years ago
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.
status1.9.1: --- → wontfix
Flags: wanted1.9.0.x+
Flags: blocking1.9.2?
Flags: blocking1.9.0.15+
(Assignee)

Comment 27

9 years ago
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.
status1.9.1: wontfix → ---
(Assignee)

Comment 28

9 years ago
Created attachment 400610 [details] [diff] [review]
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.
Attachment #400610 - Flags: approval1.9.2?
(Assignee)

Updated

9 years ago
Flags: in-testsuite? → in-testsuite+
(Assignee)

Comment 29

9 years ago
(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 1.9.0.15, a=dveditz for release-drivers
(Assignee)

Comment 31

8 years ago
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
Keywords: fixed1.9.0.15
(Assignee)

Comment 32

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

Comment 33

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

Comment 35

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

Comment 36

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

Comment 37

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

Comment 38

8 years ago
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.
Depends on: 399412
(Assignee)

Comment 39

8 years ago
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:1.9.0.15pre) Gecko/2009092708 GranParadiso/3.0.15pre) and checked the same testcases. No assertions.
Keywords: fixed1.9.0.15 → verified1.9.0.15
status1.9.1: --- → unaffected
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.