373472 - "ASSERTION: DidReflow() was never called" with <msub> and <mo>

Status: RESOLVED WORKSFORME

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

The testcase triggers:

###!!! ASSERTION: DidReflow() was never called: '!(childFrame->GetStateBits() & NS_FRAME_IN_REFLOW)', file /Users/jruderman/trunk/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp, line 536

This assertion was added in bug 366012.

Comment 1

[Mats Palmgren (inactive)]

The underlying cause for the assertion occurred also before bug 366012
as indicated by this warning:
WARNING: it is wrong to fire stretch more than once on a frame: file nsMathMLmoFrame.cpp, line 585

The problem seems to be that the "parent will call Stretch()" is tested
differently by the parent and child:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.163&root=/cvsroot&mark=1078-1080#1031
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.163&root=/cvsroot&mark=495-508#489

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This makes the assertion and warning silent.

Comment 3

[rbs]

Comment on attachment 258375 [details] [diff] [review]
Patch rev. 1

r+sr=rbs. Nice catch.

It is better to call it IsDirectChildStretcher, rather than just IsChildStretcher -- because there is the case of the stretching of siblings. For example
<mrow>
  <munder> <-- isEmbellish=true here
    <mo>...</mo> <--core
    <mo>...</mo> <--sibling
  </munder>
  ...
</mrow>

In this case, <munder> will get its Stretch() from <mrow>, and pass it to the core, and _then_, once it knows the new size of the core, it will stretch the sibling of the core (so <munder> is really also a child-stretcher, albeit in a deferred way. It does so only after getting the stretch from its parent <mrow>. Whereas <mrow> doesn't wait. It calls the stretch, and so is a direct child stretcher.

=====================
Note for the curious: the check |presentationData.baseFrame == this| goes back to the time when the parent would have only stretched the embellished child and so the total if-check was meant to guarantee that the |parentWillStretch_ME_|. But these days, siblings of an embellished child are deferred, and the parent stretches them immediately after stretching the embellished child as I hinted above. The deferred stretching was added in bug 117652. To quickly see what these gimmicks amount to, see the difference in these screenshots:
Without the deferred stretching: attachment 63204 [details]
With the deferred stretching: attachment 63213 [details]

Comment 4

[rbs]

Speaking of the sibling, be sure that the patch plays nice with something like:

<math xmlns="http://www.w3.org/1998/Math/MathML">
  <munder>
    <mo minsize="30px">&RightArrow;</mo>  <!- core -->
    <mo>&RightArrow;</mo>  <-- sibling -->
  </munder>
</math>

The <munder> and both <mo>'s will have the embellished flags set. Check to ensure the sibling doesn't get its stretch prematurely -- it should only get its stretch after the stretch of the core. I am having second thoughts that that might not be the case with the patch.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 258375 [details] [diff] [review]
Patch rev. 1

I've changed my mind regarding this patch.  After spending a couple of
days trying to understand the Reflow/Place/Stretch control flow I think
the current approach to error handling will be extremely hard to get
right.  The root of the problem is that ReflowError clears the flags:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.164&root=/cvsroot&mark=91-93#85

It's very hard to determine who was responsible to call Stretch()
after that.  Before my last patch in bug 366012 the error strategy
was "set NS_MATHML_ERROR bit, then bail out". Bug 366012 added
forced calls to DidReflow() when that happened (mostly correct, but
I now think that are edge cases we missed, as this bug tells).

My proposal for a new strategy is that we keep those flags intact
and keep FinalizeReflow/Place/Stretch going even if the
NS_MATHML_ERROR bit is set.  Then we make the Place() implementors
responsible for calling FinishReflowChild() when aPlaceOrigin = true
EVEN if there are errors (eg. ReflowError() was called).
That should be simpler to deal with since we can count on having
the same Place/Stretch control flow even when error bit is set and thus
we should reach Place() with aPlaceOrigin = true eventually, where we
make the FinishReflowChild() calls.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

Patch as outlined in last comment, some highlights:
 1. don't clear the flags in ReflowError()
 2. don't bail early in Stretch() if the error bit is set and make sure
    the code does not assume certain frames are present etc (they don't
    AFAICT).
 3. Add FinishReflowChildren() calls in Place()
 4. change NS_WARNING for multiple Stretch calls on a frame per reflow
    to be an NS_ERROR instead - this should not happen
 5. (slightly unrelated) fix ReflowError() to fallback to GetWidth() if
    GetBoundingMetrics() fails (it's currently unimplemented in thebes).
    This allows us to see those friendly red "markup-error" boxes again ;-)


FWIW, I think the difference in the "parent will call Stretch()" tests
(see links in comment 1) has something to do with the logic inherent
in nsMathMLContainerFrame::Stretch():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.163&root=/cvsroot&mark=306,321,322#300
I'm leaving these tests as is for now, hopefully they are correct.
If not, we should get assertions about missing FinishReflowChild() calls
and/or multiple Stretch() calls.

Comment 8

[rbs]

What about trying simply to get the DidReflow() in the subtree. The motivation is that we know that when there is an error, the corresponding MathML subtree becomes meaningless, and so we are not trying to get the layout right; we know that the top-most erroneous frame will show the friendly red "invalid-markup" error message.

So what I would have done myself would have been to preserve the "set NS_MATHML_ERROR bit, then bail out". But mindful of bug 366012... , walk the subtree and call the DidReflow() on descendants that need it. (Speed is not an issue here since this is an error condition which is expected to be rare.) In other words, what I would suggest you to try is:

1/ undo the patch for bug 366012.
2/ then before bailing out in ReflowError(), walk the entire subtree and call DidReflow() for those descendants that have their in-reflow bit set.

[This way, there won't be any miss and again remembering that we have that "invalid-markup" error message on the top-most container and we are not worrying about the correctness of the positionning of its hidden kids.]

Comment 9

[Mats Palmgren (inactive)]

(In reply to comment #8)
> What about trying simply to get the DidReflow() in the subtree.

Sure, that would work too I guess. 

> 1/ undo the patch for bug 366012.

I think we need to keep it mostly as is actually, but we need to walk
the child subtrees instead of just the children in DidReflowChildren.

> 2/ then before bailing out in ReflowError(), walk the entire subtree...

We need to take care of errors returned by Reflow() and Place() in
addition to ReflowError().  I think the patch from bug 366012 correctly
picked those places (but misses the fact that lower descendants could
be missing DidReflow() instead of just the immediate children due to a
deferred Stretch() at more than one level).

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Call DidReflowChildren recursively for MathML frames.
Also includes the GetBoundingMetrics() fallback and a couple of
assertions from Patch rev. 2.

I'm still slightly in favor of rev. 2 though.  I think it would be more
robust and easier to maintain in the long run.

Comment 11

[rbs]

I guess I am having this feeling that it is a bit overkill for the effect we want -- which is to just call DidReflow on hidden/unused kids. I appreciate your time and effort, though.

Do you mind trying another version without the patch for bug 366012. The underlying idea is that because we _recurse_ down the subtree, we just have to do it once (or a small number of times--if unavoidable) from a top-most container, not from all spots. This could therefore give a smaller and more robust code overall.

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #11)
> Do you mind trying another version without the patch for bug 366012. The
> underlying idea is that because we _recurse_ down the subtree, we just have
> to do it once (or a small number of times--if unavoidable) from a top-most
> container, not from all spots.

That would require that we start propagating the NS_MATHML_ERROR bit
upward so that the "top-most" container can detect something is
wrong and make the call.  We would also need to propagate nsresult
through Stretch which we currently don't do.
I'm not convinced it would be an improvement over the current code.

Comment 13

[rbs]

Could be simpler than that. What I would suggest you to try is this:

  NS_IMETHOD
  nsMathMLContainerFrame::DidReflow(nsPresContext*           aPresContext,
                                   const nsHTMLReflowState*  aReflowState,
                                   nsDidReflowStatus         aStatus)

  {
+   for all child in child-list
+     if (child is in-reflow)
+       DidReflowSubtree(child);
+
    mPresentationData.flags &= ~NS_MATHML_STRETCH_DONE;
    return nsHTMLContainerFrame::DidReflow(aPresContext, aReflowState, aStatus);
  }

with DidReflowSubtree doing the recursion and then child->DidReflow() on return.
===========

This caters for all the cases of bad/good mathml child inside a mathml/non-mathml container. What will happen is that:

if inside mathml container:
  good child: will get its DidReflow() through the normal code path
  bad child: only get its DidReflow() if its (mathml) parent container is good,
             providing the opportunitiny to pass the DidReflow down its subtree.

if inside non-mathml container, since it always does the DidReflow()
  good child: will return early
  bad child: will get this opportunitiny to pass the DidReflow down its subtree.
---------

Because all mathml are eventually inside a non-mathml, a top DidReflow() is bound to kick in. Seems to me that this means that we won't need to concern ourselves with tracking things around and can get away without your earlier patch. Let me know if I am overlooking something here.

Comment 14

[rbs]

Attachment: desirable patch

This patch implements what I described earlier. Works for me (on this bug, bug 366012 and bug 375562).

Comment 15

[Boris Zbarsky [:bzbarsky]]

So.... if Reflow() throws, is the caller actually guaranteed to call DidReflow()?  That's what this patch is assuming, right?

Comment 16

[rbs]

Yep. It seems so for main layout, whereas MathML bails out -- and now with this added twist.

Now, what this patch is guarantying, per the algorithm in comment 13, is that the DidReflow() kicks in, not necessarily from the direct XML parent caller, but eventually, as the Reflow() recursion unwinds, a good MathML parent would send a wave of missing DidReflowChildren() if/when needed before we get out of the MathML subtree.

Comment 17

[rbs]

+  if (NS_MATHML_HAS_ERROR(mPresentationData.flags) || NS_FAILED(rv))
+  	return rv | NS_ERROR_UNEXPECTED;

I am having some second thoughts about this. 

There two cases:

1) an error that has already been dealt with: 
NS_MATHML_HAS_ERROR(mPresentationData.flags) 

Seems this should then cause |return NS_OK| (the flag says that the red "invalid markup" is the feedback the user gets). The |return NS_OK| is what used to be there.

2) an error that hasn't yet been dealt with:
NS_FAILED(rv))

Perhaps should cause the |return rv| (so that the reflow throws).


I will test further and comment back later.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260977 [details] [diff] [review]
desirable patch

For what it's worth, dbaron and I both feel that an error rv from Reflow() means that the caller should bail immediately without calling DidReflow (and probably we should wipe out the frame tree at that point).

If you need to communicate something else, you should probably use a special success code...

Comment 19

[Jesse Ruderman]

I still see the bug with this testcase.

Comment 20

[Jesse Ruderman]

WFM.  Is the patch still desirable?

Comment 21

[Jesse Ruderman]

Attachment: testcase 2

This testcase still triggers the assertion for me.

Comment 22

[Jesse Ruderman]

WFM with testcase 2 as well now.

Comment 23

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/b64a7e4a35e2