403369 - "ASSERTION: prev sibling not in line list" with -moz-column, {ib}

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 4839

I think the word "test" appears in the wrong place, too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I this case aPrevFrame is on the overflow containers list.

Odd that it's stuck there while we're not in reflow.  Or is that allowed?

Comment 2

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

It's not allowed to be stuck there indefinitely, no.

Comment 3

[fantasai]

I should probably be working on this. I'll dig into my mozilla bugs this week; had to spend a few weeks away for spec work and other stuff.

Comment 4

[Mike Schroepfer]

Not sure what the priority on this should be...

Comment 5

[Jesse Ruderman]

Now it triggers two assertions:

###!!! ASSERTION: aPrevFrame must be the last continuation in its chain!: '!aPrevFrame || (!aPrevFrame->GetNextContinuation() || IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame->GetNextContinuation())) && !IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame)', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 676

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 4839

Comment 6

[fantasai]

So what's happening is that the aPrevFrame passed to nsFrameManager->InsertFrames is an overflow container. This isn't supposed to happen: if you're inserting a new frame as another frame's nextSibling, you want to attach it to the last *non-overflow-container* frame in its flow chain, not simply to the last frame in its flow chain. The troublesome caller here is nsCSSFrameConstructor::AppendFrames, down near 
  // Just insert after parentFrame

 I fixed this for several other types of frame insertions with a loop like

  while (frame->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) {
    frame = frame->GetPrevInFlow();
    NS_ASSERTION(frame, "first-in-flow can't be overflow container");
  }

but seem to have missed the part that deals with special block-in-inline stuff. Unfortunately I'm having trouble understanding this function. bz, can you take a look?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Unfortunately I'm having trouble understanding this function.

Which "this function" function?  nsCSSFrameConstructor::AppendFrames?

Looking through the frame constructor (as of rev 1.1434) I see the following places where you might have missed things (or that might have been checked in after your checkin):

* Line 8764 (in ContentAppended)
* Line 9211 (in ContentAppended)
* Line 11252 (in MaybeRecreateContainerForIBSplitterFrame)

Outside of the frame constructor, there is the caller in GetLastChildFrame in nsLayoutUtils.cpp.

That covers all but the one caller of GetLastContinuation() that you in fact changed (the one in nsCSSFrameConstructor::FindPreviousSibling).  That seems to be the only "frame insertion" that looks at NS_FRAME_IS_OVERFLOW_CONTAINER.

Is the overflow container stuff documented anywhere?  If yes, where?  If not, http://wiki.mozilla.org/Gecko:Continuation_Model is a capital place to document it, imo.

Comment 8

[fantasai]

> Which "this function" function?  nsCSSFrameConstructor::AppendFrames?

yes.

> or that might have been checked in after your checkin

Seems like it. I do remember searching for "GetLast" in nsCSSFrameConstructor.cpp.

> Is the overflow container stuff documented anywhere?

In nsContainerFrame.h mainly, with a few related notes in nsIFrame.h. I added a pointer in the wiki.

Comment 9

[Boris Zbarsky [:bzbarsky]]

All AppendFrames does is make sure that:

1)  If we're appending and there is ::after pseudo, insert before it.
2)  If we're appending an inline at the very end of a parent inline, and the
    previous sibling is a block, then we do the {ib} stuff.

Thanks for updating the docs!

Perhaps we need a method to get the "last non-overflow continuation" or something like that, and use it in these various places as needed?

Comment 10

[fantasai]

re 2) Yeah, I don't get the {ib} stuff. :P I'm not sure what the frame tree is expected to look like in that case. Is it documented somewhere?

> Perhaps we need a method to get the "last non-overflow continuation" or
> something like that, and use it in these various places as needed?

That's exactly what I've been thinking.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm not sure it's documented...  we need to do that, yeah.  The basic idea is that if you have:

  <span><div></div></span>

the frame tree will look like:

  InlineFrame(span)
  BlockFrame(span)[anonymous block]
    BlockFrame(div)

If I now append a <font> to the <span>, I should get the following:

  InlineFrame(span)
  BlockFrame(span)[anonymous block sibling]
    BlockFrame(div)
  InlineFrame(span)[special sibling]
    InlineFrame(font)

The problem is that the parent in AppendFrames is the BlockFrame(span), since that's the "last" frame for the span.  So the code constructs that second InlineFrame(span) and sticks the newly-appended inline into it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think the priority here is wrong.  Historically, getting stuff stuck on the overflow list is a good way to generate crashes.

Comment 13

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

But also historically we have tried to tolerate having stuff stuck on the overflow list without crashing.

Comment 14

[Mike Schroepfer]

(In reply to comment #12)
> I think the priority here is wrong.  Historically, getting stuff stuck on the
> overflow list is a good way to generate crashes.
> 

What do you think it should be?

Comment 15

[Boris Zbarsky [:bzbarsky]]

P2 or P3, depending on whether it's something that can definitely be used to crash or something that's just likely to cause crashes.

Comment 16

[fantasai]

I don't think it matters much what the priority is: I think I have a fix for this.

Comment 17

[fantasai]

Attachment: patch

Needs more testing. My main question here is, should GetTailContinuation() be added to nsIFrame instead of nsLayoutUtils?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm not sure why we're using state bits in some places and null kids in other places to detect overflow containers...

Can you think of appropriate uses for GetLastContinuation()?  If that's hard, perhaps the nsIFrame api should in fact be GetTailContinuation().  Or perhaps both should be on nsIFrame.  I don't see a very good reason to put this on nsLayoutUtils.

Comment 19

[fantasai]

> I'm not sure why we're using state bits in some places and null kids in other
> places to detect overflow containers...

It depends on whether we're looking for the tail end of a continuation or the parent of the last child. If we're looking for the parent of the last child, we need to start at the real last continuation and look backwards until we find the last child: overflow containers can be parents of real children. (I'd expect the same to apply for mostly empty parent frames with a fixed height, too, actually..)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> It depends on whether we're looking for the tail end of a continuation or the
> parent of the last child.

OK.  That might deserve some comments (and perhaps the "parent of the last child" thing should indeed be a utility method).

Comment 21

[fantasai]

Attachment: testcase A - content appended with {ib} and overflow containers

Comment 22

[fantasai]

Attachment: testcase B - content appended inside element with continuations

Forget {ib} splits and overflow containers, we're not dealing properly with content appended in frames with continuations at all.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Hmm.  I guess we assume that continuations are there because some child content causes them to exist, which is not necessarily the case with block-level continuations (but is for inlines).  So yeah, we need to fix that.

Comment 24

[fantasai]

Attachment: testcase C - content appended before :after

Comment 25

[fantasai]

Attachment: testcase D - content appended before :after (different codepath)

Comment 26

[fantasai]

Attachment: testcase E - content inserted after :before

Comment 27

[fantasai]

Attachment: testcase F - content appended after :before

Comment 28

[fantasai]

Attachment: patch 2

This patch gives correct renderings for all of the above. I'm having trouble writing testcases to trigger those two printfs, though.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Your "content inserted after :before" testcase is wrong.  You're inserting after the whitespace node that comes after the :before, not immediately after the :before.  Taking out that whitespace should trigger the "fire 2" printf.

Same thing for the "content appended after :before" testcase.

For "fire 3", you want something like:

  <span>
    <div id="1">
    </div>
    <div id="2">
    </div>
  </span>

And then add an inline child to the <span> such that:

1)  It comes before the <div id="1"> or
2)  It comes between the <div>s

Or you want:

  <span>
    <div id="1">
    </div>
    <span id="2">
    </span>
  </span>

and add an inline child anywhere after the <div>.

The tests in http://lxr.mozilla.org/seamonkey/source/layout/reftests/ib-split/ try to cover all the possible {ib} cases (not including :before/:after, however); you can just try loading them all to see which ones hit your printf.

Comment 30

[fantasai]

Attachment: testcase E - content inserted after :before

Comment 31

[fantasai]

Attachment: patch v2

Couldn't get your suggestions to trigger "fire 3", but layout/reftests/ib-split/remove-from-split-inline-1.html did trigger it.

Comment 32

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

+  while (!firstChildFrame && lastContinuation) {
+    lastContinuation = lastContinuation->GetPrevContinuation();
+    firstChildFrame = lastContinuation->GetFirstChild(nsnull);
+  }

This seems wrong, if lastContinuation becomes null then we'll just crash. Why can't we use GetTailContinuation here anyway? In any case you've got duplicate code here and in ContentAppended, why not share it via nsLayoutUtils say?

Comment 33

[fantasai]

lastContinuation isn't used later in the function. We can't use GetTailContinuation because we're looking for the parent of the last child, which could very well be an overflow container. (GetLastContinuation isn't right, either, because the last continuation may not have any children.)

I guess I could create an nsLayoutUtils::GetLastParentContinuation() function.

Comment 34

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

Yeah I think you should, thanks :-)

Comment 35

[fantasai]

Attachment: patch v3

Comment 36

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

Actually I don't like the name GetLastParentContinuation. How about GetLastContinuationWithChild?

Also, how about making GetTailContinuation a nonvirtual function in nsIFrame? Saves vtable space, no real downside.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 297577 [details] [diff] [review]
patch v3

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -11163,25 +11161,25 @@ nsCSSFrameConstructor::MaybeRecreateCont
>        // Not a kid of the third part of the IB split
>        GetSpecialSibling(parent) || !IsInlineOutside(parent) ||
>        // Or not the only child
>-       aFrame->GetLastContinuation()->GetNextSibling() ||
>+       aFrame->GetTailContinuation()->GetNextSibling() ||

Just to make sure this is right, what does the sibling chain look like here?  That is, is the first continuation for the "next" content node after aFrame's content node the next sibling of aFrame's last continuation, or tail continuation?

Roc's comment 36 makes a lot of sense to me.

I assume accessible/src/html/nsHyperTextAccessible.cpp and layout/tables/nsTableColGroupFrame.cpp don't need changing here?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Oh, and I would expect to see some tests here (presumably one test per change, right?).

Comment 39

[fantasai]

GetLastContinuationWithChild works for me. How do I make a non-virtual function on nsIFrame? Declare it in nsIFrame and define it at the end of nsIFrame.h..?

accessible/src/html/nsHyperTextAccessible.cpp seems to be dealing with inline frames, so I'd assume not. layout/tables/nsTableColGroupFrame.cpp likewise won't be dealing with overflow containers.

Testcases A-F together with layout/reftests/ib-split/remove-from-split-inline-1.html seem to hit all the changes. I don't think they hit all the changes with overflow containers present, though. I guess I can create more...

Comment 40

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

(In reply to comment #39)
> GetLastContinuationWithChild works for me. How do I make a non-virtual function
> on nsIFrame? Declare it in nsIFrame and define it at the end of nsIFrame.h..?

define it in nsFrame.cpp

Comment 41

[Boris Zbarsky [:bzbarsky]]

What I want for each change is a testcase that fails without that change and succeeds with it.  That way we make sure this doesn't regress.

If the testcases in this bug cover that, then great.  Certainly remove-from-split-inline-1.html doesn't fit the bill, since it passes right now.

Comment 42

[fantasai]

Attachment: patch v4

nsCSSFrameConstructor::ContentAppended
+  // Get continuation that parents the last child
+  parentFrame = nsLayoutUtils::GetLastContinuationWithChild(parentFrame);

Tested by:
  content-inserted-000.xhtml
  content-inserted-001.xhtml

Additional test:
  content-inserted-005.xhtml

nsLayoutUtils::GetLastChildFrame
+  // Get the last continuation frame that's a parent
+  nsIFrame* lastParentContinuation = nsLayoutUtils::GetLastContinuationWithChild(aFrame);

Tested by:
  content-inserted-002.xhtml
  content-inserted-006.xhtml

Additional test:
  content-inserted-003.xhtml

nsCSSFrameConstructor::ContentInserted
+      // Insert the new frames after the last continuation of the :before
+      prevSibling = firstChild->GetTailContinuation();

Tested by:
  content-inserted-007.xhtml

Additional test:
  content-inserted-004.xhtml

nsCSSFrameConstructor::FindFrameForContent
+    // The frame may have a continuation. If so, we want the last
+    // non-overflow-container continuation as our previous sibling.
+    sibling = sibling->GetTailContinuation();

Tested by:
  content-inserted-008.xhtml (assertion; no layout error)
  content-inserted-009.xhtml 

We agreed the MaybeRecreateContainer change isn't reftestable.

Comment 43

[fantasai]

Attachment: reftests.zip

Includes tests A-E, plus some more.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 299384 [details] [diff] [review]
patch v4

r=bzbarsky assuming the "tested by" tests fail without this patch and pass with.

Note: I haven't read the tests; I trust you on those.  ;)

Comment 45

[Jesse Ruderman]

The patch was checked in 2008-01-27 00:13, right?  Should this be marked as FIXED?

Comment 46

[fantasai]

Uh, right. It was. I think I'd forgotten to hit submit here; I remember writing the comment saying that this is fixed. ^^;

Comment 47

[Jesse Ruderman]

Looks like you checked in your reftests too.