Closed Bug 323737 Opened 14 years ago Closed 14 years ago

Hang involving <mtable> and <mroot>

Categories

(Core :: MathML, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: rbs)

References

Details

(5 keywords)

Attachments

(2 files, 1 obsolete file)

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
Attached file testcase
Severity: normal → critical
Blocks: 323746
WFM now. Seems that it was fixed by my earlier checkin (perhaps bug 323738).

Do you still the bug at your end?
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.
WFM in a debug build.  My tree has a few patches in it, though.
Now I can reproduce in both nightlies and my debug builds.  Still on Mac.
rbs, is this still WFY (when you test with nightlies)?  This still affects me.
OS: Mac OS X 10.2 → Mac OS X 10.4
Flags: blocking1.9a1?
Hangs a Windows trunk nightly too.
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
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
Keywords: assertion
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.
(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).
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.
> 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.)
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.
Attachment #227264 - Attachment is obsolete: true
Attachment #227416 - Flags: review?(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.)
Attachment #227416 - Flags: superreview+
Attachment #227416 - Flags: review?(rbs)
Attachment #227416 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
Backed out due to bug 345563. -> REOPENING
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #227416 - Attachment is obsolete: true
Attachment #227416 - Flags: superreview+
Attachment #227416 - Flags: review+
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.
Attachment #227264 - Attachment is obsolete: false
Attachment #227264 - Flags: superreview+
Attachment #227264 - Flags: review+
Checked in.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Attachment #227264 - Flags: approval1.8.1.1?
Flags: blocking1.9a1?
Comment on attachment 227264 [details] [diff] [review]
clean up error return in nsMathMLmrootFrame::Reflow()

approved for 1.8 branch, a=dveditz for drivers
Attachment #227264 - Flags: approval1.8.1.1? → approval1.8.1.1+
Keywords: fixed1.8.1.1
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.
Attachment #227264 - Flags: approval1.8.0.9?
Comment on attachment 227264 [details] [diff] [review]
clean up error return in nsMathMLmrootFrame::Reflow()

Approved for 1.8.0 branch, a=jay for drivers.
Attachment #227264 - Flags: approval1.8.0.9? → approval1.8.0.9+
Landed on the 1.8.0 branch.
Keywords: fixed1.8.0.9
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
Status: RESOLVED → VERIFIED
Crashtest checked in.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.