Closed Bug 397518 Opened 17 years ago Closed 17 years ago

"ASSERTION: The block in an {ib} split shouldn't be living inside an inline" with mathml

Categories

(Core :: MathML, defect)

Product:
Core
Component:
MathML
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

testcase
(deleted), application/xhtml+xml
Details
test
(deleted), application/xml
Details
fix v2
(deleted), patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Attached file testcase (deleted) — 
Loading the testcase triggers:

###!!! ASSERTION: The block in an {ib} split shouldn't be living inside an inline: '!IsInlineFrame(newParentFrame)', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8014

This assertion was added in bug 390425.
I'm starting to think that when MathML wraps non-MathML, the non-MathML should be in a block. This would avoid all kinds of problems, including the construction of the random nsLineLayout, and hat happens when there's a <br> or other forced break in the non-MathML.
Sounds good to me.
It seems to me that the <math> frame should always be a blockframe from the inside; it can either be an "inline-block" one or not depending on the display.  Or something.

In any case, MathML is violating frame construction invariants here (an always has)....
Assignee: nobody → rbs
Component: Layout → MathML
QA Contact: layout → ian
Flags: blocking1.9?
I'll take this, since I think this is the right way to fix bug 385265.
Assignee: rbs → roc
Blocks: 385265
> It seems to me that the <math> frame should always be a blockframe from the
> inside; it can either be an "inline-block" one or not depending on the display.
> Or something.

That's kinda hard. Anyway, lots of MathML elements can contain non-MathML frames.

Wrapping non-MathML children in blocks is kinda hard too, but it's not as bad, I hope.
Does a mathml inline frame actually participate in line layout?  If not, would just not claiming that type bit quiet the assert without really changing any rendering?

In other words, is this frame acting more like a inline-level replaced element than an actual inline?  We do allow block-like things inside inlines when we don't want the block rendering as a block (e.g. replaced elements), right?  Or is the issue here that we would get unconstrained widths going on?
> Does a mathml inline frame actually participate in line layout?

No

> If not, would just not claiming that type bit quiet the assert without really
> changing any rendering?

MathML frames don't claim to be eLineParticipant. The problem here is we have a span not inside a block, and then we try to do an IB split inside that span.

If we wrap the span in a block, that block will be treated a lot like an inline-block, and the IB split inside the span will work. The visual results could be weird because the block can get arbitrarily tall, but that doesn't matter.

Maybe I misunderstood your comment.
nsMathMLmathInlineFrame claims to be eLineParticipant.  That's what was triggering the assert in this case when I looked at it in gdb (when I made comment 3).  Its IsFrameOfType() impl is:

481   virtual PRBool IsFrameOfType(PRUint32 aFlags) const {
482     return nsInlineFrame::IsFrameOfType(aFlags & ~(nsIFrame::eMathML));
483   }
Flags: blocking1.9? → blocking1.9+
Oh, OK, I stand corrected, sorry.

But not claiming eLineParticipant won't really help. The IB split will still fail and leave us in a decidedly dodgy situation.
Well, we'll split up to the mathml inline.  So the mathml inline will have a block kid.

I agree that's dodgy, esp. since it calls nsInlineFrame::Reflow.  I just don't think it's necessarily fatal.  If it's nonfatal (i.e. just looks weird during layout) and isn't conforming MathML markup, we shouldn't worry too much.  So the only concern, I think, is that this block will get unconstrained width or some such.
I still need to do this block-wrapping thing to fix other bugs, like what happens when you have MathML containing a <br>.
Attached patch fix as described (obsolete) (deleted) — — Splinter Review 
This seems to work. I'll attach a (manual) testcase in a moment that tests DOM changes. MathML layout is broken because it needs to be updated to the reflow branch ... e.g. we don't size the contained blocks properly, but this is not a new issue. The frame tree structures looked right when I dumped them.

This fixes the assertion here and the "wrong line container" assertion as well.

I think the CALC_BOUNDING_METRICS flag for reflow is fundamentally the wrong approach --- threading bounding metrics accumulation through all of block and inline reflow (and potentially more) is too much code. Instead I'm adding a ComputeTightBounds API to nsIFrame. I think this makes a lot more sense. Rather than refactor the MathML code here, I'm keep CALC_BOUNDING_METRICS for use in reflow of MathML frames and switching to ComputeTightBounds when we cross from MathML to non-MathML. At some point we should refactor MathML itself to use it internally as well.
Attachment #283665 - Flags: superreview?(dbaron)
Attachment #283665 - Flags: review?(dbaron)
Attached file test (deleted) — 
simple dynamic testcase
Any chance you could find a different reviewer for this?  I have the habit of pushing anything involving frame construction to the bottom of my queue...
Attachment #283665 - Flags: superreview?(dbaron)
Attachment #283665 - Flags: superreview?(bzbarsky)
Attachment #283665 - Flags: review?(dbaron)
Attachment #283665 - Flags: review?(bzbarsky)
With this patch, I'm seeing this warning

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLmfracFrame.cpp&rev=1.64&mark=281,283#268

with layout/mathml/tests/simple.xml.

frameDen->GetNextSibling() is non-NULL.  Removing the check on this, and dragging a selection to make the <mi> symbols visible, displays the <mfrac> (without the bar but that may be another issue and) with the numerator on top of the denominator (which appears correctly positioned).

(In reply to comment #15)
> with the numerator on top of the denominator

What I mean is that the numerator and denominator are both in the denominator's position.



Comment on attachment 283665 [details] [diff] [review]
fix as described

>Index: layout/base/nsCSSFrameConstructor.cpp
>+nsCSSFrameConstructor::FlushAccumulatedBlock(nsFrameConstructorState& aState,
>+  nsStyleContext* parentContext = aParentFrame->GetStyleContext();

It doesn't matter that much for anonymous boxes, but something more like:

nsStyleContext* parentContext =
  nsFrame::CorrectStyleParentFrame(aParentFrame,
   nsCSSAnonBoxes::mozMathMLAnonymousBlock)->GetStyleContext();

is probably better here.

>+  // then, create a block frame that will wrap the table frame. Make it a

"table"?  What table?

>+  for (nsIFrame* f = aBlockItems->childList; f; f = f->GetNextSibling()) {
>+    f->SetParent(blockFrame);
>+  }

Don't we need to set NS_FRAME_HAS_CHILD_WITH_VIEW as needed on the block?  I think we should just use ReparentFrame() here; it covers that, as well as some other niggling details.

>@@ -8569,24 +8635,29 @@ nsCSSFrameConstructor::ContentAppended(n
>+  if (parentFrame->IsFrameOfType(nsIFrame::eMathML))
>+    return RecreateFramesForContent(parentFrame->GetContent());

I think we should explicitly walk up until we get to a frame whose parent is not MathML and then reframe that.

As written, say we have a 3-deep nesting of MathML frames:

  A
   B
    C

Someone appends a kid to C.  We'll go to recreate C and remove it.  That will go to recreate B.  We remove B, which recreates A.  We remove and then reinsert A, which constructs all the frames... Then we reinsert B, then we reinsert C.  That gives us content doubling, unless I've missed something.  Should be pretty easy to write a test for this, in fact.

Same for ContentInserted and ContentRemoved.

>@@ -8954,25 +9025,30 @@ nsCSSFrameConstructor::ContentInserted(n
>@@ -9415,31 +9491,39 @@ nsCSSFrameConstructor::ContentRemoved(ns
>+    nsIFrame* possibleMathMLAncestor = parentType == nsGkAtoms::blockFrame ? 
>+         parentFrame->GetParent() : parentFrame;

This could use a short comment explaining the gyrations.

>Index: layout/base/nsCSSFrameConstructor.h
>+  nsresult FlushAccumulatedBlock(nsFrameConstructorState& aState,

This could use a dram o' documentation.

>Index: layout/generic/nsFrame.cpp
>Index: layout/generic/nsIFrame.h
>+   * Compute a tight bounding rectangle for the frame. This is a rectangle

Say which coord system it's in?

>Index: layout/generic/nsTextFrameThebes.cpp
>+GetReferenceRenderingContext(nsTextFrame* aTextFrame, nsIRenderingContext* aRC)

>+  gfxContext* ctx = static_cast<gfxContext*>
>+          (tmp->GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT));

I assume |ctx| either keeps |tmp| alive or doesn't depend on any data from it?  Because we're about to let |tmp| die as we return |ctx|.

> nsTextFrame::PaintText(nsIRenderingContext* 

So we're adding a reference rendering context get for the EnsureTextRun() and another for the TEXT_HYPHEN_BREAK case.  Is it worth trying to avoid doing that twice?

I'm not sure I follow why the transform would be a problem, to be honest...  Is it not a problem if the textrun is in cache but was created with a non-reference context?  E.g. in nsTextFrame::Reflow?

>+nsTextFrame::ComputeTightBounds(gfxContext* aContext) const
>+  gfxTextRun::Metrics metrics =
>+        mTextRun->MeasureText(provider.GetStart().GetSkippedOffset(),
>+                              ComputeTransformedLength(provider), PR_TRUE,
>+                              aContext, &provider);

I'm going to trust you on this call... I tried sorting through this stuff, but it would take me too long to convince myself that every part here is what we want.

>+  return RoundOut(metrics.mBoundingBox) + nsPoint(0, mAscent);

Here, though, would it make more sense to do:

  return RoundOut(metrics.mBoundingBox + 
                  gfxPoint(0, metrics.mAscent));

Or do we know for sure that mAscent and metrics.mAscent are somehow nicely related?  If the latter, and if we really want mAscent, I'd document why that's what we want and add some asserts if possible.

It looks like after this patch MathML inline frames are still eLineParticipant.  Is that on purpose?

Minusing pending changes to the frame construction code.
Attachment #283665 - Flags: superreview?(bzbarsky)
Attachment #283665 - Flags: superreview-
Attachment #283665 - Flags: review?(bzbarsky)
Attachment #283665 - Flags: review-
(In reply to comment #17)
> It doesn't matter that much for anonymous boxes, but something more like:
> 
> nsStyleContext* parentContext =
>   nsFrame::CorrectStyleParentFrame(aParentFrame,
>    nsCSSAnonBoxes::mozMathMLAnonymousBlock)->GetStyleContext();
> 
> is probably better here.

Done

> >+  // then, create a block frame that will wrap the table frame. Make it a
> 
> "table"?  What table?

Sorry, bad copy-paste. Fixed.

> >+  for (nsIFrame* f = aBlockItems->childList; f; f = f->GetNextSibling()) {
> >+    f->SetParent(blockFrame);
> >+  }
> 
> Don't we need to set NS_FRAME_HAS_CHILD_WITH_VIEW as needed on the block?  I
> think we should just use ReparentFrame() here; it covers that, as well as some
> other niggling details.

Done

> >@@ -8569,24 +8635,29 @@ nsCSSFrameConstructor::ContentAppended(n
> >+  if (parentFrame->IsFrameOfType(nsIFrame::eMathML))
> >+    return RecreateFramesForContent(parentFrame->GetContent());
> 
> I think we should explicitly walk up until we get to a frame whose parent is
> not MathML and then reframe that.
> 
> As written, say we have a 3-deep nesting of MathML frames:
> 
>   A
>    B
>     C
> 
> Someone appends a kid to C.  We'll go to recreate C and remove it.  That will
> go to recreate B.  We remove B, which recreates A.  We remove and then
> reinsert A, which constructs all the frames... Then we reinsert B, then we
> reinsert C. That gives us content doubling, unless I've missed something.
> Should be pretty easy to write a test for this, in fact.

I think you're right, but the obvious testcase doesn't show it for me. I'll fix it up anyway.

> >@@ -8954,25 +9025,30 @@ nsCSSFrameConstructor::ContentInserted(n
> >@@ -9415,31 +9491,39 @@ nsCSSFrameConstructor::ContentRemoved(ns
> >+    nsIFrame* possibleMathMLAncestor = parentType == nsGkAtoms::blockFrame ? 
> >+         parentFrame->GetParent() : parentFrame;
> 
> This could use a short comment explaining the gyrations.

Sure

> >Index: layout/base/nsCSSFrameConstructor.h
> >+  nsresult FlushAccumulatedBlock(nsFrameConstructorState& aState,
> 
> This could use a dram o' documentation.

Done.

> >Index: layout/generic/nsFrame.cpp
> >Index: layout/generic/nsIFrame.h
> >+   * Compute a tight bounding rectangle for the frame. This is a rectangle
> 
> Say which coord system it's in?

Done.

> >Index: layout/generic/nsTextFrameThebes.cpp
> >+GetReferenceRenderingContext(nsTextFrame* aTextFrame, nsIRenderingContext* aRC)
> 
> >+  gfxContext* ctx = static_cast<gfxContext*>
> >+          (tmp->GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT));
> 
> I assume |ctx| either keeps |tmp| alive or doesn't depend on any data from it? 
> Because we're about to let |tmp| die as we return |ctx|.

nsThebesRenderingContext is just a wrapper around a gfxContext. We addref the gfxContext so it's OK to let the wrapper die.

> > nsTextFrame::PaintText(nsIRenderingContext* 
> 
> So we're adding a reference rendering context get for the EnsureTextRun() and
> another for the TEXT_HYPHEN_BREAK case.  Is it worth trying to avoid doing
> that twice?

It's necessary, we want a reference context to be used to create the hyphen text run. Added a comment.

> I'm not sure I follow why the transform would be a problem, to be honest...
> Is it not a problem if the textrun is in cache but was created with a
> non-reference context?  E.g. in nsTextFrame::Reflow?

The context used in nsTextFrame::Reflow is a reference context, by definition. The goal is to ensure that glyph advances are calculated assuming hinting is computed based on an identity transformation.

> >+  return RoundOut(metrics.mBoundingBox) + nsPoint(0, mAscent);
> 
> Here, though, would it make more sense to do:
> 
>   return RoundOut(metrics.mBoundingBox + 
>                   gfxPoint(0, metrics.mAscent));
> 
> Or do we know for sure that mAscent and metrics.mAscent are somehow nicely
> related?  If the latter, and if we really want mAscent, I'd document why
> that's what we want and add some asserts if possible.

They should be related, but mAscent is what we actually use to paint so it's the right one to use even if they were differnet. I'll add a comment.

> It looks like after this patch MathML inline frames are still
> eLineParticipant. Is that on purpose?

Yes. I don't need to change it here, so I thought I should leave it alone for now.
> It's necessary, we want a reference context to be used to create the hyphen

What I meant was... would it be possible to create the reference context earlier?  But I guess we want to fast-path the case of no hyphen and an already-existing textrun?
> > As written, say we have a 3-deep nesting of MathML frames:
> > 
> >   A
> >    B
> >     C
> > 
> > Someone appends a kid to C.  We'll go to recreate C and remove it.  That will
> > go to recreate B.  We remove B, which recreates A.  We remove and then
> > reinsert A, which constructs all the frames... Then we reinsert B, then we
> > reinsert C. That gives us content doubling, unless I've missed something.
> > Should be pretty easy to write a test for this, in fact.
> 
> I think you're right, but the obvious testcase doesn't show it for me. I'll fix
> it up anyway.

I think what happens is that when we reinsert C for the last time, that also triggers recreation of B, etc. So we end up in a good final state where we recreated everything, but only after we recreated the subtree rooted at A four times. So this is "correct" but leads to a nice exponential blowup if you increase the nesting depth, and indeed I can reproduce that in a testcase. So I'll fix this up just to avoid that blowup.
Ah, right.  That was my first instinct, but I somehow convinced myself that we'd get incorrect rendering instead...  That'll teach me to review patches at 1:30am.  :(
(In reply to comment #19)
> > It's necessary, we want a reference context to be used to create the hyphen
> 
> What I meant was... would it be possible to create the reference context
> earlier?  But I guess we want to fast-path the case of no hyphen and an
> already-existing textrun?

Yeah. Painting soft hyphens is not worth optimizing. In fact if we're going to optimize, the thing to do is to cache a reference context in nsDisplayListBuilder so that every text frame doesn't have to create its own when it needs to create a text run to paint.
Attached patch fix v2 (deleted) — — Splinter Review 
This fixes those issues. I ended up addressing the A B C recreation issue by having RecreateFramesForContent walk up to the top of the MathML stack.

I noticed one other issue which is that FlushAccumulatedBlock should just do nothing when passed no frames to wrap, instead of create an empty block, which was creating lots of unnecessary empty blocks...
Attachment #283665 - Attachment is obsolete: true
Attachment #284579 - Flags: superreview?(bzbarsky)
Attachment #284579 - Flags: review?(bzbarsky)
> cache a reference context in nsDisplayListBuilder

That sounds like a pretty good idea.  Let's get it filed?
Comment on attachment 284579 [details] [diff] [review]
fix v2

I just realized we tend to use refs, not pointers, for inout nsFrameItems in the frame constructor...  But either way is fine, really.

r+sr=bzbarsky
Attachment #284579 - Flags: superreview?(bzbarsky)
Attachment #284579 - Flags: superreview+
Attachment #284579 - Flags: review?(bzbarsky)
Attachment #284579 - Flags: review+
> That sounds like a pretty good idea.  Let's get it filed?

We should only do it if we find a testcase that it helps, since it adds a little code. Right?
Fair enough.
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 368573
Flags: in-testsuite?
Depends on: 399946
Depends on: 399940
Depends on: 400904
Depends on: 401178
Depends on: 402400
I checked in the testcase as a crashtest.  The patch in this bug was pretty large, though, so it might deserve some more tests.
Flags: in-testsuite? → in-testsuite+
Depends on: 416429
Comment on attachment 284579 [details] [diff] [review]
fix v2

>Index: layout/mathml/base/src/nsMathMLContainerFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.h,v
>retrieving revision 1.74
>diff -u -p -1 -2 -r1.74 nsMathMLContainerFrame.h
>--- layout/mathml/base/src/nsMathMLContainerFrame.h	14 Jun 2007 17:36:27 -0000	1.74
>+++ layout/mathml/base/src/nsMathMLContainerFrame.h	12 Oct 2007 04:04:34 -0000
>@@ -111,25 +111,26 @@ public:
>   NS_IMETHOD
>   ReResolveScriptStyle(PRInt32 aParentScriptLevel)
>   {
>     PropagateScriptStyleFor(this, aParentScriptLevel);
>     return NS_OK;
>   }
> 
>   // --------------------------------------------------------------------------
>   // Overloaded nsHTMLContainerFrame methods -- see documentation in nsIFrame.h
> 
>   virtual PRBool IsFrameOfType(PRUint32 aFlags) const
>   {
>-    return nsHTMLContainerFrame::IsFrameOfType(aFlags & ~(nsIFrame::eMathML));
>+    return !(aFlags & nsIFrame::eLineParticipant) &&
>+      nsHTMLContainerFrame::IsFrameOfType(aFlags & ~(nsIFrame::eMathML));
>   }

I've been trying to figure out why you added this code, and I can't.  nsMathMLContainerFrame doesn't have any base classes that are eLineParticipant.
Blocks: 996319
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: