656130 - "ASSERTION: unexpected child list" and more

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: unexpected child list: 'Error', file /builds/slave/cen-osx64-dbg/build/layout/generic/nsBlockFrame.cpp, line 4701

and a bunch of other assertions of varying scariness. Some variants crash.

First seen with http://hg.mozilla.org/mozilla-central/rev/e0f6db50231f, and seen several times since, so probably a regression from the 24 hours before that:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1461de626881&tochange=e0f6db50231f

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Timothy Nikkel (:tnikkel)]

The position: relative div never gets marked as a absolute containing block, so the child absolute item gets inserted directly to its frame instead of to its absolute containing block.

This might be due to Ehsan's work on absolute stuff.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Definitely.

Comment 4

[(no longer active)]

This is what's happening here.  The XUL stack frame has FCDATA_SKIP_ABSPOS_PUSH set on its fcdata, which causes it not to be registered as an absolute container.  nsCSSFrameConstructor::GetAbsoluteContainingBlock however does not check to make sure that the positioned frames are in fact an absolute container, which causes the absolutely positioned div to be injected in the unexpected spot in the frame tree.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 531805 [details] [diff] [review]
Patch (v1)

Review of attachment 531805 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 7

[Boris Zbarsky [:bzbarsky]]

For what it's worth, isn't the long-term plan to eliminate FCDATA_SKIP_ABSPOS_PUSH completely?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we can do that for SVG.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Fair.

Comment 10

[(no longer active)]

Attachment: Patch (v2)

So the previous version of the patch failed a bunch of tests for column set body elements containing abs-pos kids, and one thing led to another, and this patch is now much larger, but it passes all of the tests locally now.  This patch also makes column set frames, button frames and fieldset frames abs-pos containers as well.  I added a testcase for frameset because no other tests in the tree covered it before.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 532073 [details] [diff] [review]
Patch (v2)

Review of attachment 532073 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +3782,5 @@
>      nsFrameConstructorSaveState absoluteSaveState;
>  
>      if (bits & FCDATA_FORCE_NULL_ABSPOS_CONTAINER) {
>        aState.PushAbsoluteContainingBlock(nsnull, absoluteSaveState);
> +    } else if (!(bits & FCDATA_SKIP_ABSPOS_PUSH) && display->IsAbsolutelyPositioned()) {

Why are you changing this?

Comment 12

[Boris Zbarsky [:bzbarsky]]

So I think the GetAbsoluteContainingBlock that results from this patch is not quite right.  The changes to fieldset and button help, but it'll do weird things with non-first-continuation positioned inlines (which are NOT going to be IsAbsoluteContainer()), right?

If we don't have any more consumers left that are pushing weird absolute containing blocks then I think the right implementation of GetAbsoluteContainingBlock is something like:

{
  // Starting with aFrame, look for a frame that is positioned and is an
  // absolute containing block.  We only need to check positioned to avoid slow
  // walks to first continuations.
  for (nsIFrame* frame = aFrame; frame; frame = frame->GetParent()) {
    if (frame->IsFrameOfType(nsIFrame::eMathML)) {
      // If it's mathml, bail out -- no absolute positioning out from inside
      // mathml frames.  Note that we don't make this part of the loop
      // condition because of the stuff at the end of this method...
      return nsnull;
    }

    const nsStyleDisplay* disp = frame->GetStyleDisplay();
    if (!disp->IsPositioned()) {
      continue;
    }
    nsIFrame* firstContinuation = frame->GetFirstContinuation();
    if (!firstContinuation->IsAbsoluteContainer()) {
      continue;
    }
    // For tables, return the outer frame table
    if (firstContinuation->GetType() == nsGkAtoms::tableFrame) {
      return firstContinuation->GetParent();
    }
    if (firstContinuation->GetType() == nsGkAtoms::tableOuterFrame) {
      return firstContinuation;
    }
    return firstContinuation;
  }
  // If we didn't find it, then use the document element containing block
  return mHasRootAbsPosContainingBlock ? mDocElementContainingBlock : nsnull;
}

This is assuming that only things that are IsPositioned() are pushed as absolute containers... but so is the attached patch (or more precisely, the attached patch assumes that if an absolute container is pushed then either it or some close ancestor of it that is _also_ pushed is positioned; I believe in practice this is eqivalent since we only push one thing for any given frame we're constructing).  If that assumption is incorrect, then both approaches need fixing.

I just looked, and we push absolute containing blocks in these cases:

1)  Button frame (fixed in attached patch).
2)  select (listbox) frame (not fixed in attached patch).
3)  Fieldset frame (fixed in attached patch).
4)  ConstructFrameFromItemInternal.  Already correct for the non-scrollframe case, but for
    the scrollframe case we push the scrolled frame right now.  That should be fixable.
5)  ConstructBlock (fixed in attached patch).
6)  Positioned inline frames.  Already correct.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and for button and fieldset we should add more tests based on the testcases in the bugs that bug 10209 blocks.  We can do that in those bugs, though.

Comment 14

[(no longer active)]

(In reply to comment #11)
> Comment on attachment 532073 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=532073
> Patch (v2)
> 
> Review of attachment 532073 [details] [diff] [review]:
>  --> (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=656130&attachment=532073)
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +3782,5 @@
> >      nsFrameConstructorSaveState absoluteSaveState;
> >  
> >      if (bits & FCDATA_FORCE_NULL_ABSPOS_CONTAINER) {
> >        aState.PushAbsoluteContainingBlock(nsnull, absoluteSaveState);
> > +    } else if (!(bits & FCDATA_SKIP_ABSPOS_PUSH) && display->IsAbsolutelyPositioned()) {
> 
> Why are you changing this?

Oh, sorry, I was experimenting and forgot to take this out, will fix it in the next version of the patch.

Comment 15

[(no longer active)]

(In reply to comment #12)
> So I think the GetAbsoluteContainingBlock that results from this patch is not
> quite right.  The changes to fieldset and button help, but it'll do weird
> things with non-first-continuation positioned inlines (which are NOT going to
> be IsAbsoluteContainer()), right?

Those inlines should not act as abs-pos containers, right?  In other words, I think only the first continuation of every frame type should act an an abs-pos container.

> If we don't have any more consumers left that are pushing weird absolute
> containing blocks then I think the right implementation of
> GetAbsoluteContainingBlock is something like:
> 
> {
>   // Starting with aFrame, look for a frame that is positioned and is an
>   // absolute containing block.  We only need to check positioned to avoid slow
>   // walks to first continuations.
>   for (nsIFrame* frame = aFrame; frame; frame = frame->GetParent()) {
>     if (frame->IsFrameOfType(nsIFrame::eMathML)) {
>       // If it's mathml, bail out -- no absolute positioning out from inside
>       // mathml frames.  Note that we don't make this part of the loop
>       // condition because of the stuff at the end of this method...
>       return nsnull;
>     }
> 
>     const nsStyleDisplay* disp = frame->GetStyleDisplay();
>     if (!disp->IsPositioned()) {
>       continue;
>     }
>     nsIFrame* firstContinuation = frame->GetFirstContinuation();
>     if (!firstContinuation->IsAbsoluteContainer()) {
>       continue;
>     }

This seems to implement my guess above...

>     // For tables, return the outer frame table
>     if (firstContinuation->GetType() == nsGkAtoms::tableFrame) {
>       return firstContinuation->GetParent();
>     }
>     if (firstContinuation->GetType() == nsGkAtoms::tableOuterFrame) {
>       return firstContinuation;
>     }
>     return firstContinuation;
>   }
>   // If we didn't find it, then use the document element containing block
>   return mHasRootAbsPosContainingBlock ? mDocElementContainingBlock : nsnull;

Can this happen in reality?  We should always hit the viewport frame before this, right?

> }
> 
> This is assuming that only things that are IsPositioned() are pushed as
> absolute containers... but so is the attached patch (or more precisely, the
> attached patch assumes that if an absolute container is pushed then either it
> or some close ancestor of it that is _also_ pushed is positioned; I believe in
> practice this is eqivalent since we only push one thing for any given frame
> we're constructing).  If that assumption is incorrect, then both approaches
> need fixing.

I think that this assumption should be correct.

> I just looked, and we push absolute containing blocks in these cases:
> 
> 1)  Button frame (fixed in attached patch).
> 2)  select (listbox) frame (not fixed in attached patch).
> 3)  Fieldset frame (fixed in attached patch).
> 4)  ConstructFrameFromItemInternal.  Already correct for the non-scrollframe
> case, but for
>     the scrollframe case we push the scrolled frame right now.  That should be
> fixable.
> 5)  ConstructBlock (fixed in attached patch).
> 6)  Positioned inline frames.  Already correct.

I'll play with this a bit more.  I'm not quite sure about item 4 above, but I'll look into the code and will try to figure it out.

Comment 16

[(no longer active)]

(In reply to comment #13)
> Oh, and for button and fieldset we should add more tests based on the testcases
> in the bugs that bug 10209 blocks.  We can do that in those bugs, though.

Agreed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Those inlines should not act as abs-pos containers, right?

Yes, but the first continuation _should_.  With the attached patch, if you dynamically insert an abs pos kid of a non-first-continuation inline it'll end up not as a child of the inline at all.

> Can this happen in reality?

Which "this"?

> I'm not quite sure about item 4 above

Fwiw, I'm pretty darned sure about it.  I'd be happy for you to double-check, of course!  Let me know if there's anything I can do to help; we need to get this done before aurora branch, right?

Comment 18

[(no longer active)]

(In reply to comment #17)
> > Those inlines should not act as abs-pos containers, right?
> 
> Yes, but the first continuation _should_.  With the attached patch, if you
> dynamically insert an abs pos kid of a non-first-continuation inline it'll end
> up not as a child of the inline at all.

Can you please suggest a test case for that?

> > Can this happen in reality?
> 
> Which "this"?

The loop running until the end of the parent chain without having found an absolute container.

> > I'm not quite sure about item 4 above
> 
> Fwiw, I'm pretty darned sure about it.  I'd be happy for you to double-check,
> of course!  Let me know if there's anything I can do to help; we need to get
> this done before aurora branch, right?

Oh, I meant I'm not sure about the exact problem, but I think I can figure it out by looking at the code!  I wasn't questioning your statement.  :-)

Ideally all of this is done before the Aurora branch.  If not, I'll back out the stuff landed in 10209.  But right now this is my 1st priority, so I doubt if that would be necessary.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Can you please suggest a test case for that?

  <span style="position:relative">Some<br>
     <span id="insertion-parent">Text/span></span>

Force layout, then insert an abs-pos kid as a child of insertion-parent.

> The loop running until the end of the parent chain without having found an
> absolute container.

Hmm.  Without the IsPositioned() check, no, because we do push mDocElementContainingBlock as an absolute containing block.  So that last return can just return null, yes.

> I meant I'm not sure about the exact problem

Oh.  In this situation:

  <div style="overflow: scroll; position: relative">Text</div>

we currently push the scrolledframe as the absolute containing block as far as I know.  We should be pushing the scrollframe instead.

Comment 20

[(no longer active)]

Attachment: Patch (v3)

I think this patch should address all of the 6 items in comment 12 (I still haven't closely studied comment 19).  I'll continue working on this tomorrow.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> So that last return can just return null, yes.

Except my version still has an IsPositioned() check.  So we do still need the more complicated return statement.

Comment 22

[(no longer active)]

Attachment: Patch (v4)

I think I got everything right this time...

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 532453 [details] [diff] [review]
Patch (v4)

Review of attachment 532453 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 24

[(no longer active)]

Attachment: Patch (v5)

Fixed a bunch of stuff that bz caught over IRC.  This version of the patch also adds tests for scrollframe containing block calculations performed in this patch.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532749 [details] [diff] [review]
Patch (v5)

>+    if (frame->GetType() == nsGkAtoms::scrollFrame) {

This needs to also check for nsGkAtoms::listControlFrame to handle the <select> case, and I think test for comboboxes (and for those you need to get the list and _then_ get its scrolled frame).  We should add tests for dynamic insertion of abs-pos kids of a <select> of both kinds.

The other option to consider is disallowing abs pos inside a <select> altogether; we'd still need to check for it here if we do that like we check for MathML.

I hate form controls.  :(

>+    if (candidate->GetType() == nsGkAtoms::tableOuterFrame) {
>+      return candidate;
>+    }
>+    return candidate;

I'd prefer to replace that if block with a comment about how just returning |candidate| is the right thing for outer tables, I think.  ;)

Also, looking at the code now, I'd prefer s/candidate/absPosCBCandidate/.

>\ No newline at end of file

Please add one.

r- for now because of the <select> issues.  :(

Comment 26

[(no longer active)]

Attachment: <select> with abs-pos kids

(In reply to comment #25)
> Comment on attachment 532749 [details] [diff] [review] [review]
> Patch (v5)
> 
> >+    if (frame->GetType() == nsGkAtoms::scrollFrame) {
> 
> This needs to also check for nsGkAtoms::listControlFrame to handle the
> <select> case, and I think test for comboboxes (and for those you need to
> get the list and _then_ get its scrolled frame).  We should add tests for
> dynamic insertion of abs-pos kids of a <select> of both kinds.

Hmm, OK, now I'm puzzled.  Should we support abs-pos kids inside list control frames at all?  Like in this test case?  No other browser seems to honor this (see the testcase).  If anything, I think we need to ignore them explicitly (which should mean removing the PushAbsoluteContainingBlock call from InitSelectFrame too).

> The other option to consider is disallowing abs pos inside a <select>
> altogether; we'd still need to check for it here if we do that like we check
> for MathML.

See above.

> >+    if (candidate->GetType() == nsGkAtoms::tableOuterFrame) {
> >+      return candidate;
> >+    }
> >+    return candidate;
> 
> I'd prefer to replace that if block with a comment about how just returning
> |candidate| is the right thing for outer tables, I think.  ;)

Heh, embarrassing!  Will fix in the next version.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Should we support abs-pos kids inside list control frames at all?

From a web compat standapoint, doesn't matter for now.  Other browsers don't really do CSS layout on kids of <select> at all, fwiw.

I would be fine with whichever approach here leads to simplest code.  The options seem to be to either allow them to be positioned (and then I think we should use some sane containing block when the <select> is positioned, not just use the nearest positioned ancestor of the <select>) or not allow them to be positioned (a la MathML).

Comment 28

[(no longer active)]

(In reply to comment #27)
> I would be fine with whichever approach here leads to simplest code.  The
> options seem to be to either allow them to be positioned (and then I think
> we should use some sane containing block when the <select> is positioned,
> not just use the nearest positioned ancestor of the <select>) or not allow
> them to be positioned (a la MathML).

I'm leaning on the side of not making it possible for children of <select> elements to be positioned at all.  <select> elements themselves should be allowed to be positioned, as any other block element is (/will be).

Comment 29

[Boris Zbarsky [:bzbarsky]]

That sounds fine to me.

Comment 30

[(no longer active)]

Attachment: Patch (v6)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535151 [details] [diff] [review]
Patch (v6)

Per our irc conversation, just nix the listControlFrame type check and comment since select descendants can never be positioned already (which I missed earlier), and r=me.

Comment 32

[(no longer active)]

Attachment: Patch (v7)

Took out the listControlFrame check from GetAbsoluteContainingBlock, as it's not really necessary.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535162 [details] [diff] [review]
Patch (v7)

r=me

Comment 34

[Jesse Ruderman]

Patch no longer applies cleanly.

Comment 35

[Martijn Wargers (dead)]

Attachment: testcase

https://crash-stats.mozilla.com/report/index/e2a67ead-320a-43cf-ba1f-5e1302110623
0 	xul.dll 	nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval 	layout/base/nsCSSFrameConstructor.cpp:8930
1 	xul.dll 	nsCSSFrameConstructor::RecreateFramesForContent 	layout/base/nsCSSFrameConstructor.cpp:9070
2 	xul.dll 	nsCSSFrameConstructor::ProcessRestyledFrames 	
3 	xul.dll 	mozilla::css::RestyleTracker::ProcessRestyles 	layout/base/RestyleTracker.cpp:240
4 	xul.dll 	nsCSSFrameConstructor::ProcessPendingRestyles 	layout/base/nsCSSFrameConstructor.cpp:11613
5 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4810
6 	xul.dll 	PresShell::WillPaint 	layout/base/nsPresShell.cpp:7608
7 	xul.dll 	nsViewManager::CallWillPaintOnObservers 	view/src/nsViewManager.cpp:1604
8 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:902
9 		@0x80

Comment 36

[(no longer active)]

Attachment: Crashtest

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 542589 [details] [diff] [review]
Crashtest

Review of attachment 542589 [details] [diff] [review]:
-----------------------------------------------------------------

Why is .orig in your patch? Those files should be excluded always.

::: layout/generic/crashtests/656130-2.html
@@ +1,1 @@
> +<html>

<!DOCTYPE HTML> --- tests should be in standards mode whenever possible

Comment 38

[(no longer active)]

(In reply to comment #37)
> Comment on attachment 542589 [details] [diff] [review] [review]
> Crashtest
> 
> Review of attachment 542589 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Why is .orig in your patch? Those files should be excluded always.

My mistake, will remove it before landing.

> ::: layout/generic/crashtests/656130-2.html
> @@ +1,1 @@
> > +<html>
> 
> <!DOCTYPE HTML> --- tests should be in standards mode whenever possible

Will fix this before landing too.

Comment 39

[Johnathan Nightingale [:johnath]]

We fixed this in 6 with a backout, but AIUI we didn't back out on trunk, so this is a problem on aurora 7 again. Are we going to fix this with backout, or patch on aurora?

Comment 40

[Boris Zbarsky [:bzbarsky]]

I believe we did the backout on trunk as well, recently.  Ehsan will know for sure next week when he gets back.

Comment 41

[Jesse Ruderman]

It was backed out on trunk (before 7 merged to aurora). See bug 10209 comment 119.

Comment 42

[Johnathan Nightingale [:johnath]]

Ah, okay. Then does it need tracking-firefox7?

Comment 43

[(no longer active)]

(In reply to comment #42)
> Ah, okay. Then does it need tracking-firefox7?

Not sure, but I'll mark it as fixed on 7 anyways.

Comment 44

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/731bbf32a6fd
https://hg.mozilla.org/mozilla-central/rev/49b3a14730de