Filing as security-sensitive because of comments in older (fixed) bugs involving the same crash signature.

Here's a smaller and more readable test case. This looks like another manifestation of the double-reflow problem that caused bug 416907, but I'm not sure how we managed to do it this time. Still investigating.

Oof, this is a nasty one. The problem structure is <msup><mo><frameset/></mo></msup>; to trigger a crash you have to have display:block on the outer <msup>, but the bug doesn't have anything to do with that. We hit nsHTMLFramesetFrame::Reflow for the first time with this stack trace... <msup>.nsMathMLContainerFrame::ReflowChild() <msup>.nsContainerFrame::ReflowChild() <mo>.nsMathMLmoFrame::Reflow() <mo>.nsMathMLTokenFrame::Reflow() <mo>.nsMathMLContainerFrame::ReflowChild() <mo>.nsContainerFrame::ReflowChild() <frameset>.nsHTMLFramesetFrame::Reflow() It returns successfully, and so do the immediate two callers. <mo>.nsMathMLTokenFrame::Reflow() then calls nsMathMLContainerFrame::FinalizeReflow(), which in turn calls nsMathMLTokenFrame::Place(), but not nsMathMLTokenFrame::Stretch(), because it needs to give the <msup> a chance to do that. We keep returning, all the way to <msup>.nsMathMLContainerFrame::ReflowChild(). Nothing has called DidReflow() for the <frameset>. At this point we are hosed, because nsMathMLmsupFrame::Place() will detect invalid markup and return, and the <mo> will get finalized, but the <frameset> won't.

After a great deal of staring at gdb I came to the conclusion that the simplest, safest fix for this bug was to have DidReflowChildren() recurse over *all* descendant frames of the problem frame. The first included test case crashes FF3.0.3 and does not crash trunk after this patch. The second test case does not crash Firefox 3.0.3, even though it uses some of the same code paths, but then, the actual crash here is easy to perturb out of existence. So I'm including it anyway. This patch should apply unmodified to 1.9.0.x, so I'm flagging it for approval to that branch; please let me know if it doesn't actually apply.

+ DidReflowChildren(grandchild, nsnull); This should be frame->DidReflowChildren I guess. Or else make DidReflowChildren static. Also, the DidReflowChildren should be inside the NS_FRAME_IN_REFLOW guard, right?

(In reply to comment #4) > + DidReflowChildren(grandchild, nsnull); > > This should be frame->DidReflowChildren I guess. Or else make > DidReflowChildren static. I made it static. 'frame' doesn't necessarily point to a nsMathMLContainerFrame object. > Also, the DidReflowChildren should be inside the NS_FRAME_IN_REFLOW guard, > right? That would miss a frame that has its NS_FRAME_IN_REFLOW bit set but is a descendant of a frame that doesn't have that bit set. I'm not prepared to swear that that never happens. (I imagine it's not *supposed* to happen, but this function's job is to clean up broken frame tree invariants, so it seems best to be as defensive as possible.)

If the crash is exploitable, it might be best to have the tests in a separate patch that only gets checked in once the bug is revealed.

I have no idea whether it's exploitable. The crash is a read dereference of a NULL pointer, and as far as I can tell, both pointer and offset are going to be zero when we get there with the bug triggered.

(In reply to comment #5) > (In reply to comment #4) > > Also, the DidReflowChildren should be inside the NS_FRAME_IN_REFLOW guard, > > right? > > That would miss a frame that has its NS_FRAME_IN_REFLOW bit set but is a > descendant of a frame that doesn't have that bit set. I'm not prepared to > swear that that never happens. (I imagine it's not *supposed* to happen, but > this function's job is to clean up broken frame tree invariants, so it seems > best to be as defensive as possible.) I think we should ensure that never happens. We should add debug code in DidReflow that checks all children (in all child lists) and verifies that their NS_FRAME_IN_REFLOW has already been cleared, so if this invariant is violated we'll get clear assertions near the problem. If we find violations of that we can fix them separately. That helps here because it means DidReflowChildren can't cause a real increase in the amount of work we do. The MathML code that defers DidReflow should not get us into situations where we've marked a frame as done with reflow but not its children.

So something like this? But I don't think we want to do it now, because the assertion fires nearly twenty thousand times in the reftests, and 1300 times in the mochitests; most of these cases seem to have nothing to do with MathML. Moving the recursion in DidReflowChildren into the NS_FRAME_IN_REFLOW conditional does not break the test cases for this bug again, though. So we could maybe do that part now and the assertions after 3.1?

Checking for FRAME_IS_DIRTY or NS_FRAME_HAS_DIRTY_CHILDREN is not correct. There are reasons why we might exit reflow with dirty children --- in a few cases we rely on iterative reflow. If there are still assertion failures for children which are still IN_REFLOW or FIRST_REFLOW, then yeah, let's just leave the extra assertions out for now and have DidReflowChildren recursion guarded by NS_FRAME_IN_REFLOW.

Yeah, we still get the same number of assertion failures for IN_REFLOW|FIRST_REFLOW only. I'll attach another iteration of the patch shortly, and file a bug for adding the assertions post-1.9.1.

Hopefully final revision as discussed above. I'll file a separate bug for the DidReflow() sanity checks.

On the 1.9.0 branch both testcases crash on a null dereference at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameSetFrame.cpp&rev=3.210&mark=1156#1154 with no sign of using freed objects. Not an exploitable crash, but the investigation seems to have revealed some ugliness we should fix in case there are other paths in that would yield more exploitable results. I see the assertion "Someone didn't override Reflow or ComputeAutoSize: 'Not Reached'" from nsLeafFrame.cpp line 131 (twice) before the crash with both testcases.

(In reply to comment #13) > On the 1.9.0 branch both testcases crash on a null dereference at > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameSetFrame.cpp&rev=3.210&mark=1156#1154 That's the same place it crashes on trunk. > with no sign of using freed objects. Not an exploitable crash, but the > investigation seems to have revealed some ugliness we should fix in case there > are other paths in that would yield more exploitable results. I don't know if that means you're saying this should get fixed on 1.9.0 branch or not. Did you try the patch? > I see the assertion "Someone didn't override Reflow or ComputeAutoSize: 'Not > Reached'" from nsLeafFrame.cpp line 131 (twice) before the crash with both > testcases. I believe that to be harmless, but the patch in bug 416907 fixes it. It'll appear any time <frameset/> is used inside MathML, whether or not in a context that provokes a crash.

Comment on attachment 347877 [details] [diff] [review] (rev 3) only recurse when calling DidReflow Moving to post beta 2 queue; should this be nominated for blocking final release?

It's a crash, so I'm nominating it for blocking final.

Comment on attachment 347877 [details] [diff] [review] (rev 3) only recurse when calling DidReflow a191=beltzner

Pushed http://hg.mozilla.org/mozilla-central/rev/1695db512c46 for m-c and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2407bc6d2773 for 1.9.1

Comment on attachment 347877 [details] [diff] [review] (rev 3) only recurse when calling DidReflow Approved for 1.9.0.6, a=dveditz for release-drivers.

Fix checked into the 1.9.0 branch

Verified for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre. It no longer crashes but reports "invalid markup". Good enough for me.

Verified fixed for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090112 Shiretoko/3.1b3pre.

sorry for the inconvenience. i meant to detect any bugs before the branch merge that were still RESO fixed, but had a verified1.9.1 keyword next to them.

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre