Last Comment Bug 736389 - "Assertion failure: mElements.Contains(cur)" with -moz-column, :first-child
: "Assertion failure: mElements.Contains(cur)" with -moz-column, :first-child
Status: RESOLVED FIXED
[qa+]
: assertion, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86_64 Mac OS X
: P1 critical (vote)
: mozilla14
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: stirdom 618949 705877
  Show dependency treegraph
 
Reported: 2012-03-15 22:56 PDT by Jesse Ruderman
Modified: 2012-05-21 05:33 PDT (History)
11 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
testcase (863 bytes, application/xhtml+xml)
2012-03-15 22:56 PDT, Jesse Ruderman
no flags Details
stack trace (6.75 KB, text/plain)
2012-03-15 22:56 PDT, Jesse Ruderman
no flags Details
Get rid of code that reresolved style on overflow containers multiple times and in the process violated some invariants other code depended on. (5.97 KB, patch)
2012-03-15 23:37 PDT, Boris Zbarsky [:bz] (still a bit busy)
mats: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-03-15 22:56:16 PDT
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
Comment 1 Jesse Ruderman 2012-03-15 22:56:42 PDT
Created attachment 606480 [details]
stack trace
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-03-15 23:14:55 PDT
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 [:bz] (still a bit busy) 2012-03-15 23:22:35 PDT
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 [:bz] (still a bit busy) 2012-03-15 23:26:25 PDT
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 [:bz] (still a bit busy) 2012-03-15 23:37:49 PDT
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.
Comment 6 Mats Palmgren (:mats) 2012-03-16 09:46:05 PDT
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 [:bz] (still a bit busy) 2012-03-16 10:14:15 PDT
This patch passes try as well.

Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/a17a380948d2
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-03-16 10:20:14 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-17 17:06:22 PDT
https://hg.mozilla.org/mozilla-central/rev/a17a380948d2
Comment 10 Alex Keybl [:akeybl] 2012-03-22 13:54:35 PDT
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 [:bz] (still a bit busy) 2012-03-23 11:32:17 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/89187d407e7e
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-04-06 17:47:15 PDT
Sorry, I meant to set the fixed flag in comment 11...
Comment 13 Vlad [QA] 2012-05-21 05:33:16 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.