323737 - Hang involving <mtable> and <mroot>

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060115 Firefox/1.6a1

In a nightly build, the testcase causes a hang.

In a debug build, the testcase causes two assertion failures (a thousand times each) followed by an abort.

###!!! ASSERTION: redo line on totally empty line: 'aState.IsImpactedByFloat()', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 3930

###!!! ASSERTION: unconstrained height on totally empty line: 'NS_UNCONSTRAINEDSIZE != aState.mAvailSpaceRect.height', file /Users/admin/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 3932

Block(mtable)(1)@0x3492acc: yikes! spinning on a line over 1000 times!
Abort

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[rbs]

WFM now. Seems that it was fixed by my earlier checkin (perhaps bug 323738).

Do you still the bug at your end?

Comment 3

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060122 Firefox/1.6a1

Still hangs for me in today's nightly.  Haven't retested a debug build.

Comment 4

[Jesse Ruderman]

WFM in a debug build.  My tree has a few patches in it, though.

Comment 5

[Jesse Ruderman]

Now I can reproduce in both nightlies and my debug builds.  Still on Mac.

Comment 6

[Jesse Ruderman]

rbs, is this still WFY (when you test with nightlies)?  This still affects me.

Comment 7

[Jesse Ruderman]

Hangs a Windows trunk nightly too.

Comment 8

[Alex Vincent [:WeirdAl]]

This testcase is not 100% reproducible.  But when I did reproduce it:

WARNING: invalid markup: file c:/mozsource/mozilla/layout/mathml/base/src/nsMathMLmrootFrame.cpp, line 212

###!!! ASSERTION: bad break type: '(NS_STYLE_CLEAR_NONE != breakType) || (NS_STYLE_CLEAR_NONE != aState.mFloatBreakType)', file c:/mozsource/mozilla/layout/generic/nsBlockFrame.cpp, line 4113

This is then followed by the two assertions Jesse reports in comment 0.  The line numbers have changed slightly.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060605 SeaMonkey/1.5a

Hardware, OS -> All

Comment 9

[Alex Vincent [:WeirdAl]]

So I'm tracing this through from the warning, and it looks to me like the warning leads to the assertions pretty rapidly.  Line 213 of nsMathMLmrootFrame.cpp returns NS_OK (which for an error condition seems odd).

The next frame out:
>	gklayout.dll!nsLineLayout::ReflowFrame(nsIFrame * aFrame=0x03aef164, unsigned int & aReflowStatus=61796708, nsHTMLReflowMetrics * aMetrics=0x00000000, int & aPushedFrame=0)  Line 997	C++

  rv = aFrame->Reflow(mPresContext, metrics, reflowState, aReflowStatus);
> if (NS_FAILED(rv)) {
    NS_WARNING( "Reflow of frame failed in nsLineLayout" );
    return rv;
  }

frameType evaluates to true, but none of the four conditions match frameType (nsLayoutAtoms::placeholderFrame, nsLayoutAtoms::textFrame, nsLayoutAtoms::letterFrame, nsLayoutAtoms::brFrame), so nothing happens there.

Two frames out, we run into the assertions.

I'm not sure I can help much more at this point; I don't know layout code at all.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> Line 213 of
> nsMathMLmrootFrame.cpp returns NS_OK (which for an error condition seems odd).

no, Reflow shouldn't return a failure nsresult on bad markup.   It should only return a failure nsresult for program errors (e.g., out of memory).

Comment 11

[steve.swanson]

Attachment: clean up error return in nsMathMLmrootFrame::Reflow()

On my Windows system, the hang happens about half the time.  Don't have an explanation for that.  But the patch seems to prevent the hang.

My diagnosis:  the invalid markup is triggering nsMathMLContainerFrame::ReflowError().  This calls GetBoundingMetrics() on the RenderingContext with string "invalid-markup".  Failure happens (bug 324857).  The error path didn't mark the frame as complete.

GetBoundingMetrics() is a bit of overkill here.  You could just use the default line height and measure the string normally.

Comment 12

[rbs]

> The error path didn't mark the frame as complete.

Good catch! (An incomplete frame may be reflowed again to give it a chance to split itself -- which means over and over in this case where the frame is never marked as complete...)

Care to just change the signature of ::ReflowError to include aReflowState and aStatus (like in ::Reflow)? This will have a domino effect in the other files, but will bullet-proof everybody once for all, not just <mroot>.

(I don't care very much about the speed of ::ReflowError because it is a rare situation.)

Comment 13

[steve.swanson]

Attachment: have nsMathMLContainerFrame::ReflowError() mark frame as complete

I agree this is a better way to deal with this problem.

The ripples went all the way out to nsIMathMLFrame::Place().  The change in signature probably means the IID should change.

Comment 14

[rbs]

Comment on attachment 227416 [details] [diff] [review]
have nsMathMLContainerFrame::ReflowError() mark frame as complete

r+sr=rbs

Patch looks alright to me. I am going to check it in ASAP. (Soon after sending my last comment, I went away at a conference + a holiday break.)

Comment 15

[rbs]

Checked in.

Comment 16

[steve.swanson]

It looks like this caused bug 345563.

In particular, the change to the signature of nsIMathMLFrame::Place() didn't happen to all derived classes.  I'll work on a supplemental patch first thing in the morning.

Comment 17

[rbs]

Backed out due to bug 345563. -> REOPENING

Comment 18

[rbs]

Comment on attachment 227264 [details] [diff] [review]
clean up error return in nsMathMLmrootFrame::Reflow()

r+sr=rbs on this first patch.  Discussion from bug 345563 has meant that this patch was actually a sufficient fix.

Comment 19

[rbs]

Checked in.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 227264 [details] [diff] [review]
clean up error return in nsMathMLmrootFrame::Reflow()

approved for 1.8 branch, a=dveditz for drivers

Comment 21

[rbs]

Comment on attachment 227264 [details] [diff] [review]
clean up error return in nsMathMLmrootFrame::Reflow()

This has landed on the 1.8 branch. Because it is a hang, it is also a good candidate for the 1.8.0 branch.

Comment 22

[Jay Patel [:jay]]

Comment on attachment 227264 [details] [diff] [review]
clean up error return in nsMathMLmrootFrame::Reflow()

Approved for 1.8.0 branch, a=jay for drivers.

Comment 23

[rbs]

Landed on the 1.8.0 branch.

Comment 24

[Carsten Book [:Tomcat]]

Verified fixed for 1.8.0.9 and 1.8.1.1. 
Tested with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.1pre) Gecko/20061201 BonEcho/2.0.0.1pre
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.9pre) Gecko/20061201 Firefox/1.5.0.9pre also on Windows Vista and Fedora FC6

Comment 25

[Jesse Ruderman]

Crashtest checked in.