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

RESOLVED FIXED

Status

()

Core
Layout
P4
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: fantasai)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 288184 [details]
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.
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?
Flags: blocking1.9?
It's not allowed to be stuck there indefinitely, no.
(Assignee)

Comment 3

11 years ago
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.
Assignee: nobody → fantasai.bugs

Comment 4

11 years ago
Not sure what the priority on this should be...
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(Reporter)

Comment 5

11 years ago
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
(Assignee)

Comment 6

11 years ago
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?
> 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.
(Assignee)

Comment 8

11 years ago
> 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.
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?
(Assignee)

Comment 10

11 years ago
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.
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.
I think the priority here is wrong.  Historically, getting stuff stuck on the overflow list is a good way to generate crashes.
But also historically we have tried to tolerate having stuff stuck on the overflow list without crashing.

Comment 14

11 years ago
(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?
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.
(Assignee)

Comment 16

11 years ago
I don't think it matters much what the priority is: I think I have a fix for this.
(Assignee)

Comment 17

11 years ago
Created attachment 293810 [details] [diff] [review]
patch

Needs more testing. My main question here is, should GetTailContinuation() be added to nsIFrame instead of nsLayoutUtils?
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.
(Assignee)

Comment 19

11 years ago
> 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..)
> 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).
(Assignee)

Comment 21

10 years ago
Created attachment 296016 [details]
testcase A - content appended with {ib} and overflow containers
(Assignee)

Comment 22

10 years ago
Created attachment 296017 [details]
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.
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.
(Assignee)

Comment 24

10 years ago
Created attachment 296057 [details]
testcase C - content appended before :after
(Assignee)

Comment 25

10 years ago
Created attachment 296058 [details]
testcase D - content appended before :after (different codepath)
(Assignee)

Comment 26

10 years ago
Created attachment 296059 [details]
testcase E - content inserted after :before
(Assignee)

Comment 27

10 years ago
Created attachment 296060 [details]
testcase F - content appended after :before
(Assignee)

Comment 28

10 years ago
Created attachment 296061 [details] [diff] [review]
patch 2

This patch gives correct renderings for all of the above. I'm having trouble writing testcases to trigger those two printfs, though.
Attachment #293810 - Attachment is obsolete: true
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.
(Assignee)

Comment 30

10 years ago
Created attachment 296470 [details]
testcase E - content inserted after :before
Attachment #296059 - Attachment is obsolete: true
(Assignee)

Comment 31

10 years ago
Created attachment 296554 [details] [diff] [review]
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.
Attachment #296061 - Attachment is obsolete: true
Attachment #296554 - Flags: superreview?(roc)
Attachment #296554 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
+  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?
(Assignee)

Comment 33

10 years ago
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.
Yeah I think you should, thanks :-)
(Assignee)

Comment 35

10 years ago
Created attachment 297577 [details] [diff] [review]
patch v3
Attachment #296554 - Attachment is obsolete: true
Attachment #297577 - Flags: superreview?(roc)
Attachment #297577 - Flags: review?(bzbarsky)
Attachment #296554 - Flags: superreview?(roc)
Attachment #296554 - Flags: review?(bzbarsky)
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 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?
Oh, and I would expect to see some tests here (presumably one test per change, right?).
(Assignee)

Comment 39

10 years ago
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...
(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
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.
(Assignee)

Comment 42

10 years ago
Created attachment 299384 [details] [diff] [review]
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.
Attachment #297577 - Attachment is obsolete: true
Attachment #299384 - Flags: superreview?(roc)
Attachment #299384 - Flags: review?(bzbarsky)
Attachment #297577 - Flags: superreview?(roc)
Attachment #297577 - Flags: review?(bzbarsky)
(Assignee)

Comment 43

10 years ago
Created attachment 299385 [details]
reftests.zip

Includes tests A-E, plus some more.
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.  ;)
Attachment #299384 - Flags: review?(bzbarsky) → review+
Attachment #299384 - Flags: superreview?(roc) → superreview+
(Reporter)

Comment 45

10 years ago
The patch was checked in 2008-01-27 00:13, right?  Should this be marked as FIXED?
(Assignee)

Comment 46

10 years ago
Uh, right. It was. I think I'd forgotten to hit submit here; I remember writing the comment saying that this is fixed. ^^;
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 47

10 years ago
Looks like you checked in your reftests too.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.