Closed Bug 416907 Opened 12 years ago Closed 11 years ago

Crash [@ nsHTMLFramesetFrame::Reflow] with frameset in mroot

Categories

(Core :: Layout, defect, P4, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: martijn.martijn, Assigned: zwol)

Details

(4 keywords)

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file testcase
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...
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: tracking1.9+
Still crashes, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080905031348 Minefield/3.1b1pre
Flags: blocking1.9.1?
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.
Flags: blocking1.9.1? → wanted1.9.1+
Assignee: nobody → zweinberg
Reproducible on Linux with an FF3.0.3 build.
OS: Windows XP → All
Hardware: PC → All
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.
Attachment #345416 - Flags: superreview?(roc)
Attachment #345416 - Flags: review?(mozbugz)
Attachment #345416 - Flags: superreview?(roc)
Attachment #345416 - Flags: superreview+
Attachment #345416 - Flags: review?(mozbugz)
Attachment #345416 - Flags: review+
Keywords: checkin-needed
Comment on attachment 345416 [details] [diff] [review]
fix
[Checkin: Comment 8]

http://hg.mozilla.org/mozilla-central/rev/9a5f7f7d0141
Attachment #345416 - Attachment description: fix → fix [Checkin: Comment 8]
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Rediffed against 1.9.0.x CVS, no changes.
Attachment #345507 - Flags: approval1.9.0.5?
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.
Status: RESOLVED → VERIFIED
(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.
Attachment #345507 - Flags: approval1.9.0.5? → approval1.9.0.5-
OK, here is a patch that makes the minimum necessary change.
Attachment #345507 - Attachment is obsolete: true
Attachment #346603 - Flags: approval1.9.0.5?
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.
Attachment #346603 - Flags: review?(roc)
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
Attachment #346603 - Flags: approval1.9.0.5? → approval1.9.0.5+
Keywords: checkin-needed
Whiteboard: [c-n: 1.9.0 branch]
Fix checked into the 1.9.0 branch
Whiteboard: [c-n: 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).
Crash Signature: [@ nsHTMLFramesetFrame::Reflow]
You need to log in before you can comment on or make changes to this bug.