See testcase which crashes in current trunk build on load. This seems to have regressed between 2006-12-15 and 2006-12-17: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-15+04&maxdate=2006-12-17+06&cvsroot=%2Fcvsroot http://crash-stats.mozilla.com/report/index/14dedbe8-d8fa-11dc-9966-001a4bd43ed6 0 nsHTMLFramesetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) mozilla/layout/generic/nsFrameSetFrame.cpp:1155 1 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) mozilla/layout/generic/nsContainerFrame.cpp:760 2 nsMathMLContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp:884 3 nsMathMLmrootFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) mozilla/layout/mathml/base/src/nsMathMLmrootFrame.cpp:187 4 nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, int&) mozilla/layout/generic/nsLineLayout.cpp:856 5 nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) mozilla/layout/generic/nsBlockFrame.cpp:3586 6 nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) mozilla/layout/generic/nsBlockFrame.cpp:3408 7 nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, int*) mozilla/layout/generic/nsBlockFrame.cpp:3257 8 nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, int*) mozilla/layout/generic/nsBlockFrame.cpp:2314 9 nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) mozilla/layout/generic/nsBlockFrame.cpp:1897 10 nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) mozilla/layout/generic/nsBlockFrame.cpp:936 11 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) mozilla/layout/generic/nsContainerFrame.cpp:760 12 CanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) mozilla/layout/generic/nsHTMLFrame.cpp:584 13 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) mozilla/layout/generic/nsContainerFrame.cpp:760 etc...

Still crashes, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080905031348 Minefield/3.1b1pre

Looks like a null pointer dereference, probably because Init wasn't called. (And while we're here, maybe nsFrameSetFrame::Init should propagate failure from the base class init call?)

Actually, no, I think the problem is that the MathML code's ReflowError codepath causes us to reflow twice with NS_FRAME_FIRST_REFLOW set, probably because it's not calling DidReflow.

Reproducible on Linux with an FF3.0.3 build.

There are two bail-out paths in nsMathMLmrootFrame::Reflow(). One of them calls DidReflowChildren(), the other doesn't. Adding a call to DidReflowChildren() fixes the crash. I'm going to audit all of the MathML reflow methods for similar bugs, and I'm going to try to figure out where these assertions are coming from: ###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file layout/generic/nsLeafFrame.cpp, line 131 ###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file layout/generic/nsLeafFrame.cpp, line 131 Also, this diagnostic seems pointless to me: WARNING: invalid markup: file /home/zack/src/mozilla/moz-central/layout/mathml/base/src/nsMathMLmrootFrame.cpp, line 249 because (a) nobody but us browser devs looks at warnings on standard error, and (b) there is a much more effective way of diagnosing invalid MathML already, i.e. the replacement of the busted element with a text frame that says "invalid-markup" in white on red. Does anyone mind if I kill it and any similar diagnostics I find during the above audit?

(In reply to comment #5) > Also, this diagnostic seems pointless to me: > > WARNING: invalid markup: file > /home/zack/src/mozilla/moz-central/layout/mathml/base/src/nsMathMLmrootFrame.cpp, > line 249 > > because (a) nobody but us browser devs looks at warnings on standard error, and > (b) there is a much more effective way of diagnosing invalid MathML already, > i.e. the replacement of the busted element with a text frame that says > "invalid-markup" in white on red. Does anyone mind if I kill it and any > similar diagnostics I find during the above audit? That reasoning and killing makes sense to me.

Here's the fix. I couldn't find any other cases where this is a problem, but perhaps someone who knows mathml should try something similar with other elements. Also, I'm not sure it is correct to make the various frameset leaf frames' GetIntrinsicHeight methods return zero, but it did shut up the assertions.

Comment on attachment 345416 [details] [diff] [review] fix [Checkin: Comment 8] http://hg.mozilla.org/mozilla-central/rev/9a5f7f7d0141

Rediffed against 1.9.0.x CVS, no changes.

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081031 Minefield/3.1b2pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081031 Minefield/3.1b2pre. I verified using the testcase in the bug.

(In reply to comment #5) > Does anyone mind if I kill it and any similar diagnostics I find during the > above audit? On stable branches we do mind. Please create a minimal branch patch that just fixes the bug.

Comment on attachment 345507 [details] [diff] [review] backported fix for 1.9.0.x branch need a minimal patch without code cleanups.

OK, here is a patch that makes the minimum necessary change.

Comment on attachment 346603 [details] [diff] [review] minimal 1.9.0.x fix Do we need a second review on the removal of the static cast changes in nsMathMLmsubsupFrame.cpp and nsMathMLmsupFrame.cpp? Those functions are declared returning nsresult so returning a cast to a pointer can't be right. If it's just a difference of NS_WARNING() lines I'm happy to carry over the original review, but now there's a substantive change from the reviewed version and I don't know if that's innocuous or not.

Actually, the casts were no-ops in the original code. - return static_cast<nsMathMLContainerFrame*>(aFrame)->ReflowError(...); + return aFrame->ReflowError(...); In both cases, the thing that's being cast is aFrame; aFrame is a nsMathMLContainerFrame subclass, and ReflowError is defined only as a method of nsMathMLContainerFrame. Therefore, the casts have no effect, so in the trunk patch, I took them out to make the code more readable. But in the minimized backport, that change is not necessary to the fix.

> aFrame is a nsMathMLContainerFrame subclass Looking at the code again, I can make an even stronger statement: in all three places where I took out a static_cast<nsMathMLContainerFrame*>(aFrame), the declared type of aFrame was exactly nsMathMLContainerFrame*. So the casts would be no-ops even if there were another ReflowError definition floating around.

Ah, I missed the parens around aFrame (and don't know the precendence of C++ casts) -- I thought you were casting the return value. Thanks for the explanation

Comment on attachment 346603 [details] [diff] [review] minimal 1.9.0.x fix Approved for 1.9.0.5, a=dveditz for release-drivers

Fix checked into the 1.9.0 branch

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008120105 GranParadiso/3.0.5pre. Testcase crash 1.9.0.4 but simply gives "invalid markup" in 1.9.0.5 (as it does in Trunk).