"ASSERTION: Frame is not in the block" with xul and mathml

RESOLVED FIXED

Status

()

defect
P3
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: jruderman, Assigned: roc)

Tracking

(Blocks 2 bugs, {assertion, testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:Rs])

Attachments

(3 attachments, 2 obsolete attachments)

Posted file testcase
###!!! ASSERTION: Frame is not in the block!!!: 'startLine != block->end_lines()', file /Users/jruderman/trunk/mozilla/layout/generic/nsTextFrameThebes.cpp, line 871

###!!! ASSERTION: running past end: 'mCurrent != mListLink', file /Users/jruderman/trunk/mozilla/layout/base/../generic/nsLineBox.h, line 620

Note that bug 395450 also triggers the second assertion.
Assignee: rbs → roc
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Posted patch fix (obsolete) — Splinter Review
Here, somehow MathML is setting a constrained height on the block when reflowing it, but it doesn't create a block continuation so we end reflow with content in the block's overflow list. This is a bad bug but it should not lead to problems because we are supposed to tolerate content hanging around in an overflow list.

The actual problem occurs because BuildTextRuns uses FindLineFor to find where to start building text runs for the requested text frame. But the line isn't found because the text frame is in the block's overflow list.

The solution is to use nsBlockInFlowLineIterator instead of FindLineFor, to ensure we search the overflow list of the block as well. (It will also search next-in-flow blocks, although we shouldn't ever need that here.)

So why not replace all uses of FindLineFor with nsBlockInFlowLineIterator? It turns out this is a good idea. The two callers of FindLineFor in nsBlockFrame really should be searching lines in the overflow list and in next-in-flow blocks. And this way we can share the logic for finding descendants, including out-of-flow descendants.
Attachment #285963 - Flags: superreview?(dbaron)
Attachment #285963 - Flags: review?(dbaron)
This patch also fixes bug 395450.

I will also have a look at why MathML is setting a constrained height. That will need to be fixed separately.
Blocks: 395450
Whiteboard: [needs review]
(In reply to comment #2)
> I will also have a look at why MathML is setting a constrained height. That
> will need to be fixed separately.

Hopefully a smaller patch too.

I probably won't get to this review until after beta, or at least until I'm done with my remaining beta-blocking bug.
Posted patch MathML fixSplinter Review
MathML should never pass anything but NS_UNCONSTRAINEDSIZE as the available height when reflowing its child; it doesn't support pagination.
Attachment #285965 - Flags: superreview?(bzbarsky)
Attachment #285965 - Flags: review?(bzbarsky)
Attachment #285965 - Flags: superreview?(bzbarsky)
Attachment #285965 - Flags: superreview+
Attachment #285965 - Flags: review?(bzbarsky)
Attachment #285965 - Flags: review+
Whiteboard: [needs review] → [needs review][needs landing]
Whiteboard: [needs review][needs landing] → [needs review][needs landing][dbaron-1.9:Rs]
MathML fix checked-in.

Checking in layout/mathml/base/src/nsMathMLContainerFrame.cpp;
/cvsroot/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp,v  <--  nsMathMLContainerFrame.cpp
new revision: 1.170; previous revision: 1.169
done
Whiteboard: [needs review][needs landing][dbaron-1.9:Rs] → [needs review][dbaron-1.9:Rs]
So FindLineFor only found lines for children of the block (in-flow or floating).  AdvanceToLineContaining calls nsLayoutUtils::FindChildContainingDescendant, which makes me think you're intending it to find any descendant.  However, AdvanceToLineContaining also searches next-in-flows, and FindChildContainingDescendant won't find the right child for something that's a descendant of a next-in-flow.  We don't want that inconsistency.  I suspect the best way out of it is to make AdvanceToLineContaining again find only children of the block.

The comment for AdvanceToLineContaining should be updated to reflect this, and also to reflect that the out-of-flows it finds are floats and not absolutely positioned elements.
And maybe it would be better to make AdvanceToLineContaining part of the constructor so that you don't need to add mAtLine and the call pattern restriction for AdvanceToLineContaining (only right after the new constructor).
Attachment #285963 - Flags: superreview?(dbaron)
Attachment #285963 - Flags: superreview-
Attachment #285963 - Flags: review?(dbaron)
Attachment #285963 - Flags: review-
Whiteboard: [needs review][dbaron-1.9:Rs] → [dbaron-1.9:Rs]
Posted patch fix v2 (obsolete) — Splinter Review
I want to avoid building descendant searching into multiple call sites, so I've left that functionality in, but made it handle block next-in-flows at the same time. I also made it a constructor and added an IsValid API so users can check if the frame was found.
Attachment #291993 - Flags: superreview?(dbaron)
Attachment #291993 - Flags: review?(dbaron)
Whiteboard: [dbaron-1.9:Rs] → [dbaron-1.9:Rs][needs review]
Blocks: 412093
Comment on attachment 291993 [details] [diff] [review]
fix v2

>-    // If the target frame is floated, and this line contains the
>-    // float's placeholder, then we've found our line.
>-    if (line->HasFloats()) {
>-      for (nsFloatCache *fc = line->GetFirstFloat();
>-           fc != nsnull;
>-           fc = fc->Next()) {
>-        if (aFrame == fc->mPlaceholder->GetOutOfFlowFrame())
>-          return line;
>-      }
>-    }

Don't you need to put this chunk in the new constructor?  Or was nobody ever using this code to find floats?  (This seems like the only *major* problem here; the rest of the comments are relatively minor.)

>+nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(nsBlockFrame* aFrame,
>+                                                     nsIFrame* aFindFrame)

+  nsIFrame* immediateChild;

Could you call this "child" instead?  I can understand using the term "immediate descendant" to mean "child", except that we already have a word for "child".  "immediate child" is redundant.

Maybe assert at the very end that IsValid(), since you should have hit the early return otherwise?

>+  /**
>+   * Set up the iterator so that Next() will return the first line in the block
>+   * (or its next in flows). You can't call Prev() after this.
>+   */
>+  nsBlockInFlowLineIterator(nsBlockFrame* aFrame);

Do you even need this?  It looks unused now (except the one case that I explain how to eliminate below) -- and I think removing it would let you make mAtLine be debug-only.  Or maybe even remove it entirely, since you could make those checks IsValid(), I'd think.

>+  nsBlockInFlowLineIterator backIterator(block);
>   if (aForFrameLine) {
>+    backIterator = nsBlockInFlowLineIterator(block, *aForFrameLine, PR_FALSE);
>   } else {
>+    backIterator = nsBlockInFlowLineIterator(block, aForFrame);
>+    NS_ASSERTION(backIterator.IsValid(), "aForFrame not found in block, someone lied to us");
>+    NS_ASSERTION(backIterator.GetContainer() == block, "Someone lied to us about the block");
>   }

You can accomplish this without the empty constructor by binding a temporary to a reference (See C++ 12.2 [class.temporary], clause 5):

nsBlockInFlowLineIterator &backIterator =
  aForFrameLine ? nsBlockInFlowLineIterator(block, *aForFrameLine, PR_FALSE)
                : nsBlockInFlowLineIterator(block, aForFrame);
and then make the assertions (with "aForFrameLine ||" if appropriate).


r+sr=dbaron with those things fixed
Attachment #291993 - Flags: superreview?(dbaron)
Attachment #291993 - Flags: superreview-
Attachment #291993 - Flags: review?(dbaron)
Attachment #291993 - Flags: review-
(In reply to comment #10)
> >+  nsBlockInFlowLineIterator backIterator(block);
> >   if (aForFrameLine) {
> >+    backIterator = nsBlockInFlowLineIterator(block, *aForFrameLine, PR_FALSE);
> >   } else {
> >+    backIterator = nsBlockInFlowLineIterator(block, aForFrame);
> >+    NS_ASSERTION(backIterator.IsValid(), "aForFrame not found in block, someone lied to us");
> >+    NS_ASSERTION(backIterator.GetContainer() == block, "Someone lied to us about the block");
> >   }
> 
> You can accomplish this without the empty constructor by binding a temporary
> to a reference (See C++ 12.2 [class.temporary], clause 5):
> 
> nsBlockInFlowLineIterator &backIterator =
>   aForFrameLine ? nsBlockInFlowLineIterator(block, *aForFrameLine, PR_FALSE)
>                 : nsBlockInFlowLineIterator(block, aForFrame);
> and then make the assertions (with "aForFrameLine ||" if appropriate).

This apparently only works with const references.
(In reply to comment #10)
> (From update of attachment 291993 [details] [diff] [review])
> >-    // If the target frame is floated, and this line contains the
> >-    // float's placeholder, then we've found our line.
> >-    if (line->HasFloats()) {
> >-      for (nsFloatCache *fc = line->GetFirstFloat();
> >-           fc != nsnull;
> >-           fc = fc->Next()) {
> >-        if (aFrame == fc->mPlaceholder->GetOutOfFlowFrame())
> >-          return line;
> >-      }
> >-    }
> 
> Don't you need to put this chunk in the new constructor?  Or was nobody ever
> using this code to find floats?  (This seems like the only *major* problem
> here; the rest of the comments are relatively minor.)

This is handled by the code in the constructor that converts out-of-flow frames to their placeholders while we look for the immediate block child that contains the target frame.

> >+nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(nsBlockFrame* aFrame,
> >+                                                     nsIFrame* aFindFrame)
> 
> +  nsIFrame* immediateChild;
> 
> Could you call this "child" instead?  I can understand using the term
> "immediate descendant" to mean "child", except that we already have a word for
> "child".  "immediate child" is redundant.

OK
Posted patch fix v3Splinter Review
This updates to trunk and takes comments into account. We recently landed a fix that added a nsBlockInFlowLineIterator(nsBlockFrame*, PRBool*) constructor which finds the first line in a block chain; this avoids the need to have nsBlockInFlowLineIterator in a special state in which only IsValid can be called. So I changed the signature for the frame-finding constructor to match.
Attachment #285963 - Attachment is obsolete: true
Attachment #291993 - Attachment is obsolete: true
Attachment #305428 - Flags: superreview?(dbaron)
Attachment #305428 - Flags: review?(dbaron)
(In reply to comment #11)
> This apparently only works with const references.

Oh, I think it's actually that temporaries like that are implicitly const.
Comment on attachment 305428 [details] [diff] [review]
fix v3

>+      line_iterator fline = iter.GetLine();
>       MarkLineDirty(fline);

This could just be "MarkLineDirty(iter.GetLine());".

r+sr=dbaron
Attachment #305428 - Flags: superreview?(dbaron)
Attachment #305428 - Flags: superreview+
Attachment #305428 - Flags: review?(dbaron)
Attachment #305428 - Flags: review+
Checked in. I don't have a good way to test this right now, but the patch in bug 412093 relies on this.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:Rs][needs review] → [dbaron-1.9:Rs]
You need to log in before you can comment on or make changes to this bug.