"Assertion failure: mElements.Contains(cur)" with -moz-column, :first-child

RESOLVED FIXED in Firefox 13

Status

()

Core
CSS Parsing and Computation
P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla14
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13+ verified)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 606479 [details]
testcase

The testcase hits an assertion added in bug 705877:

Assertion failure: mElements.Contains(cur), at layout/style/nsCSSRuleProcessor.cpp:3363
(Reporter)

Comment 1

5 years ago
Created attachment 606480 [details]
stack trace
Hmm.  So in this case the <li> that's a kid of the outer abs-pos div is what's missing from the parent stack.

And the reason it's missing is that we're reresolving style on the inner <div> directly, not through its placeholder.  And we're doing _that_ because we're dealing with the second continuation for that <div>, which is not only NS_FRAME_OUT_OF_FLOW but also NS_FRAME_IS_OVERFLOW_CONTAINER.  So we end up reresolving style on it directly from the child list of the outer <div>.

Why do we need to reresolve out-of-flow overflow containers directly here?  Wouldn't they just be handled during the walk along the continuations of the original out-of-flow when we walk through its placeholder?
The relevant code was added in bug 154892, in case that matters....

And it looks like it wasn't until the fix for bug 576649 that we started correctly reresolving continuations of NS_FRAME_OUT_OF_FLOW frames.  But I believe that given that fix the various places where the checks for NS_FRAME_IS_OVERFLOW_CONTAINER were added in UpdateViewsForTree and ReparentStyleContext and ReResolveStyleContext are wrong.  And the code in VerifyStyleTree should also walk continuations, and remove the check for overflow containers.
Ah, looks like the instance in UpdateViewsForTree was already removed in a followup patch in bug 576649.  And bug 618949 was filed to track removing the other instances...
Blocks: 618949
Created attachment 606485 [details] [diff] [review]
Get rid of code that reresolved style on overflow containers multiple times and in the process violated some invariants other code depended on.
Attachment #606485 - Flags: review?(matspal)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Comment on attachment 606485 [details] [diff] [review]
Get rid of code that reresolved style on overflow containers multiple times and in the process violated some invariants other code depended on.

This looks correct to me.  r=mats
Attachment #606485 - Flags: review?(matspal) → review+
This patch passes try as well.

Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/a17a380948d2
Whiteboard: [need review]
Target Milestone: --- → mozilla14
Comment on attachment 606485 [details] [diff] [review]
Get rid of code that reresolved style on overflow containers multiple times and in the process violated some invariants other code depended on.

[Approval Request Comment]
Regression caused by (bug #): bug 705877 
User impact if declined: Some styles for kids of continuations of absolutely 
   positioned elements inside columns or in printing might be wrong.  Probably
   not for printing, since we don't do dynamic reresolution there.  In addition,
   debug builds will have fatal assertions in this edge case.  One possible
   option is to make the assertion on aurora non-fatal if we think that would be
   useful.
Testing completed (on m-c, etc.): Passes tests in this bug, but I would
   probably let it bake for a few days before deciding either way.
Risk to taking this patch (and alternatives if risky): There is some risk in
   that we would not be reresolving style on a codepath where we used to do
   so.  The good news is that the behavior only changes in an edge case: an
   absolutely positioned element split across columns or pages.  And in the
   paged case we don't do any dynamic reresolution, as I said, so this should
   only affect columns.  In any case, there is existing code to correctly
   reresolve style on the elements in question, so the codepath being removed
   is superfluous as far as I and Mats can tell.  The risk is that somehow a
   case will come up where it's not superfluous, I guess.

   Alternatives include backing out bug 705877, disabling the optimization from
   bug 705877 entirely, just leaving this bug on aurora, or leaving it but
   making the assert non-fatal.
String changes made by this patch: None.
Attachment #606485 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/a17a380948d2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 606485 [details] [diff] [review]
Get rid of code that reresolved style on overflow containers multiple times and in the process violated some invariants other code depended on.

[Triage Comment]
My read of the risk evaluation is that this approach is sufficiently low risk relative to our other options. We don't think that an extra 5 weeks on m-c will shake out regressions better than the next 11 weeks on aurora/beta, so approving for Aurora 13.
Attachment #606485 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/89187d407e7e
tracking-firefox13: --- → ?

Updated

5 years ago
tracking-firefox13: ? → +
Sorry, I meant to set the fixed flag in comment 11...
status-firefox13: --- → fixed
Whiteboard: [qa+]

Comment 13

5 years ago
I've verified this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120517 Firefox/13.0 beta debug build

No assertion failure appear in the console when running the test case provided.
Setting the the flag to verified.
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.