403569 - "ASSERTION: Parent not consistent with exepectations" with -moz-column, float, :before

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: comparing iterators over different lists: 'mListLink == aOther.mListLink', file /Users/jruderman/trunk/mozilla/layout/generic/nsLineBox.h, line 694

###!!! ASSERTION: Parent not consistent with exepectations: 'aOldParent == aFrame->GetParent()', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 592

I've seen the first assertion in other bugs, so this bug report is mostly about the second assertion.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I've seen the first assertion in other bugs,

Can you point me to one?  That code is obviously wrong and easy to fix.

As for this bug:

(gdb) frame
#1  0xb4ca4e0c in nsBlockFrame::DrainOverflowLines (this=0x9201e8c, aState=@0xbfffb640)
    at ../../../mozilla/layout/generic/nsBlockFrame.cpp:4350
4350            ReparentFrame(frame, prevBlock, this);
(gdb) p prevBlock
$20 = (nsBlockFrame *) 0x91e4484
(gdb) p this
$21 = (nsBlockFrame *) 0x9201e8c
(gdb) p frame->GetParent()
$22 = (nsBlockFrame *) 0x920a9f0
(gdb) p frame->GetParent()->GetNextInFlow()
$25 = (nsBlockFrame *) 0x91e4484
(gdb) p frame->GetParent()->GetNextInFlow()->GetNextInFlow()
$26 = (nsBlockFrame *) 0x9201e8c

So the frame's parent is the prev-in-flow of prevBlock, whereas the code in ReparentFrame asserts that it's equal to prevBlock....


roc, how serious is this?

Comment 2

[Jesse Ruderman]

The first assertion also appears in bug 393330, for example.

Comment 3

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

That's bad enough to fix.

Comment 4

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

Ugh

Comment 5

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

The first assertion is gone on trunk.

Comment 6

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

Attachment: Reduced testcase

This testcase is a bit simpler. Relevant piece of the frame tree after we've reflowed the initial content and having just recreated the frame for the span due to the style change:

             line 0x4101190c: count=1 state=block,clean,prevmarginclean,impacted,not wrapped,before:nobr,after:nobr[0x4018] {0,0,24720,12000} <
                Block(optgroup)(3)@0x41011564 next-in-flow=0x41221020 {0,0,24720,12000} [state=00100000] sc=0x410114b8(i=0,b=1)<
                  line 0x410118bc: count=1 state=block,clean,prevmarginclean,impacted,not wrapped,before:nobr,after:nobr[0x4018] {0,0,24720,12000} <
                    Block(optgroup)(3)@0x41011724 next-in-flow=0x41220fa0 {0,0,24720,12000} [state=00100040] sc=0x41011620(i=0,b=0) pst=:before<
                    >
                  >
                >
              >
              Float-list<
                ImageFrame(img)(1)@0x410113cc {0,0,57492,18000} [state=00200110] [content=0x3d131ef0] [src=http://www.mozilla.com/img/home/main-feature3.jpg]
              >
            >
            Block(body)(3)@0x412210a0 prev-in-flow=0x41010f80 {25680,0,24720,0} [state=0010100c] [overflow=0,0,24720,1200] sc=0x41010f30(i=1,b=1) pst=:-moz-column-content<
              line 0x41221078: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4049] {0,0,24720,1200} <
                Block(optgroup)(3)@0x41221020 next=0x410117f4 prev-in-flow=0x41011564 {0,0,24720,1200} [state=00101004] sc=0x410114b8(i=0,b=2)<
                  line 0x41220ff8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4048] {0,0,24720,1200} <
                    Block(optgroup)(3)@0x41220fa0 next=0x41613580 prev-in-flow=0x41011724 {0,0,24720,1200} [state=00100044] sc=0x41011620(i=1,b=0) pst=:before<
                      line 0x410117cc: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,0,1624,1200} <
                        Text(-1)@0x41011278[0,3,T]  {0,0,1624,1200} [state=10600040] [content=0x3d16dd10] sc=0x4101177c pst=:-moz-non-element<
                          "foo"
                        >
                      >
                    >
                  >
                  line 0x410118e4: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4009] {0,0,0,0} <
                    Block(span)(0)@0x41613580 {0,0,0,0} [state=00100402] sc=0x41011834(i=0,b=0)<
                    >
                  >

Check this out:

(gdb) p ((nsIFrame*)0x41613580)->mParent
$21 = (nsIFrame *) 0x41011564

That should obviously be 0x41221020, but it's the first-in-flow optgroup instead. Something went wrong during frame construction and gave it the wrong parent, probably connected with the ::before rule for the optgroup label. This leads to the assertion later.

Comment 7

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

Attachment: fix

So the problem is simply that the code that moves new frame children to after ::before frames for the parent, updates parentFrame but neglects to actually change the mParents of the child frames. So the gist of the patch is to call MoveChildrenTo to achieve that.

For a while I was going to use ReparentFrame to do that, but then I realized it's not suitable (because it only works when we're bumping frames down into a wrapper). But along the way I did notice that it's always called in a loop to reparent an entire sibling chain, so I refactored it to do the loop itself. It's trivial, but if you just want to review the MoveChildrenTo bits and ignore the rest, that's fine.

Comment 8

[Jesse Ruderman]

Attachment: testcase without optgroup

Indeed, the relevant rule from forms.css is the first "optgroup:before" rule.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 288826 [details] [diff] [review]
fix

So not using ReparentFrame means we don't reparent style contexts, right?

How does that interact with first-line?  For example, what happens if a block has ::before and ::first-line and the ::before wraps to the second line, then you insert some stuff at the beginning of the block?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Put another way, why exactly was ReparentFrame unsuitable?  It looks like the two methods do identical things except ReparentFrame reparents the style context...

Comment 11

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

MoveChildrenTo propagates NS_FRAME_HAS_CHILD_WITH_VIEW arbitrarily far up the frame tree, while ReparentFrameList propagates it only one level.

Also, MoveChildrenTo does the float reparenting thing, while ReparentFrameList doesn't, but of course I'm not enabling that in my call to MoveChildrenTo. I probably should be, right?

Maybe I should fuse MoveChildrenTo and ReparentFrameList? It looks like reparenting style contexts should be fine for the MoveChildrenTo callers (IB splitting) and NS_FRAME_HAS_CHILD_WITH_VIEW propagation should be harmless for ReparentFrameList callers. Those callers can disable float reparenting in MoveChildrenTo.

Alternatively I could just add style context reparenting to MoveChildrenTo without unifying the functions. What do you think?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> MoveChildrenTo propagates NS_FRAME_HAS_CHILD_WITH_VIEW arbitrarily far up the
> frame tree

Yeah, I think I did that just in case...

> Also, MoveChildrenTo does the float reparenting thing, while ReparentFrameList
> doesn't, but of course I'm not enabling that in my call to MoveChildrenTo. I
> probably should be, right?

That's a really interesting question.  The float reparenting thing needs to happen if the new parent has a different closest block ancestor from the old parent.  In the primary use of MoveChildrenTo ({ib} splits) this is the case: we're moving kids from the inline into the anonymous block or vice versa.  The only way I can see it happening with the code you're adding is if the ancestor of the :before is a block and that block has a next-in-flow and the :before flows into that next-in-flow.  So either printing or columns.  There are no mutations in printing, so we only need to worry about columns.  What is the right float behavior in columns in that case?  Should the floats be on the float list of the first-in-flow block, or wherever the placeholder is?

> Maybe I should fuse MoveChildrenTo and ReparentFrameList? It looks like
> reparenting style contexts should be fine for the MoveChildrenTo callers (IB
> splitting)

Fine, but a little wasteful (it's a no-op, but takes work to discover that).  Though I suspect we'll detect that pretty quickly in the style system and not do too much work...  Unifying them does seem pretty tempting, I will admit.

All that said, I'm looking at this ::before code and I'm a little confused, to be honest.  Why exactly are we doing this _after_ we've constructed the new child frames?  Why not just change the prevSibling/nextSibling getting code to look for ::before if !prevSibling?  We always end up doing it anyway, so there should be no perf impact, right?  And this would always give us the right parent frame before we start frame construction.

Comment 13

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

(In reply to comment #12)
> > Also, MoveChildrenTo does the float reparenting thing, while ReparentFrameList
> > doesn't, but of course I'm not enabling that in my call to MoveChildrenTo. I
> > probably should be, right?
> 
> That's a really interesting question.  The float reparenting thing needs to
> happen if the new parent has a different closest block ancestor from the old
> parent.  In the primary use of MoveChildrenTo ({ib} splits) this is the case:
> we're moving kids from the inline into the anonymous block or vice versa.  The
> only way I can see it happening with the code you're adding is if the ancestor
> of the :before is a block and that block has a next-in-flow and the :before
> flows into that next-in-flow.  So either printing or columns.  There are no
> mutations in printing, so we only need to worry about columns.

That's exactly this testcase.

> What is the
> right float behavior in columns in that case?  Should the floats be on the
> float list of the first-in-flow block, or wherever the placeholder is?

They should be where the placeholder is. So I need to fix that.

> > Maybe I should fuse MoveChildrenTo and ReparentFrameList? It looks like
> > reparenting style contexts should be fine for the MoveChildrenTo callers (IB
> > splitting)
> 
> Fine, but a little wasteful (it's a no-op, but takes work to discover that). 
> Though I suspect we'll detect that pretty quickly in the style system and not
> do too much work...  Unifying them does seem pretty tempting, I will admit.
> 
> All that said, I'm looking at this ::before code and I'm a little confused, to
> be honest.  Why exactly are we doing this _after_ we've constructed the new
> child frames?  Why not just change the prevSibling/nextSibling getting code to
> look for ::before if !prevSibling?  We always end up doing it anyway, so there
> should be no perf impact, right?  And this would always give us the right
> parent frame before we start frame construction.

I was wondering that myself. Let me do some CVS archaeology...

Comment 14

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

So it turns out that the adjustment of prevSibling in the presence of ::before content was originally placed just before mFrameManager->InsertFrames, which was fine back then because at that time the code didn't handle parent changes at all. Later the parent change code was added in bug 343540, reviewed by some idiot who didn't think of the consequences.

Time to fix that.

Comment 15

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

Attachment: like so

Works great. Fixes the bug here, doesn't regress the testcases in bug 347853 or bug 343540.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 288929 [details] [diff] [review]
like so

Yeah, I like this much better!

Comment 17

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

checked in.

Comment 18

[Jesse Ruderman]

Crashtests checked in.