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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

The testcase hits an assertion added in bug 705877:

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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Get rid of code that reresolved style on overflow containers multiple times and in the process violated some invariants other code depended on.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

This patch passes try as well.

Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/a17a380948d2

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/a17a380948d2

Comment 10

[Alex Keybl [:akeybl]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/89187d407e7e

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sorry, I meant to set the fixed flag in comment 11...

Comment 13

[Vlad [QA]]

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.