286122 - Freeze/crash [@ IncrementalReflow::AddCommand] with evil mathml testcase, using display:block;

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
Since bug 284001 was fixed, I thought, well let's try crashing Mozilla another
time ;)

This looks like a regression:
Doesn't freeze/crash with:
Firefox trunk build of 2005-02-07 07:03am
Freezes/crashes with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/2005020806
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=2005-02-07+07%3A00%3A00&maxdate=2005-02-08+07%3A00%3A00&cvsroot=%2Fcvsroot
Maybe because of the fix for bug 267085?

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Talkback ID: TB4341961K

IncrementalReflow::AddCommand 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 931]
PresShell::ProcessReflowCommands 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6407]
ReflowEvent::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6261]
PL_HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/xpcom/threads/plevent.c,
line 699]
SHELL32.dll + 0x520c24 (0x778b0c24)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm...  So problematic code seems to be in
nsMathMLContainerFrame::ReLayoutChildren.  This calls ReflowDirtyChild() on an
ancestor frame, but passes null as the child to reflow.  That ends up crashing
after some asserts because we end up with a reflow command targeted at null.

The fix for bug 267085 probably exposed this, but it didn't work right before
that either (except for not crashing).

Comment 4

[rbs]

The |null| in ReflowDirtyChild is common (see nsInlineFrame). If my memory
serves me right, it means reflow every child.

Eh Martijn, you are exploring an unchartered territory... you are testing the
code to its limits in a very challenging an interesting way... You are
uncovering scenarii that have probably not surfaced so far because they are not
encountered in real life (MathML-speaking). Even the JavaScripted MathML editor
hadn't exposed them (http://www.newmexico.mackichan.com/MathML/mathmled.htm).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> The |null| in ReflowDirtyChild is common (see nsInlineFrame)

nsInlineFrame overrides ReflowDirtyChild and always passes itself to the call it
makes on its parent.

On the other hand, the MathML code is calling ReflowDirtyChild() on an
nsContainerFrame (effectively).  This used to just call into
nsFrame::CreateAndPostReflowCommand (with a null aTargetFrame in this case),
which simply threw and NS_ERROR_NULL_POINTER.

Now I can fix up presshell to do that again instead of crashing, and hope that
all this stuff goes away soon (David's reflow rewrite gets rid of it all, or so
the plan goes)... Or we could fix MathML to do the right thing here, since I
don't see any documentation anywhere that says passing null to ReflowDirtyChild
should reflow all kids, and I don't see that it did that before my patch.

Comment 6

[rbs]

> nsInlineFrame overrides ReflowDirtyChild and always passes itself to the call it
> makes on its parent.

So does nsMathMLContainerFrame::ReflowDirtyChild() with actually replicates the
code. As MathML frames are conceptually inline frames (although stacked in
strange fashion), they tend to emulate common inline tasks (except in some rare
cases, e.g. <math display="block"> or some anonymous block containers).

I am speaking from memory. I have to check what is going in the testcase to make
a sound judgement.

Comment 7

[Boris Zbarsky [:bzbarsky]]

You're right that nsMathMLContainerFrame does the same thing as nsInlineFrame.

In the testcase in question, we have the following frame model (just the part
that's relevant here) when the ReflowDirtyChild call is made:

nsTableRowFrame
  nsTableCellFrame
    nsBlockFrame
      nsMathMLmoFrame

The codepath that we follow is that we're inside
nsMathMLmoFrame::ReflowDirtyChild.  We end up calling ReLayoutChildren() and
passing it the the nsBlockFrame (since the blockframe probably has no useful
embellish data in sight).  That puts us in
nsMathMLContainerFrame::ReLayoutChildren.  This does a walk up the frame tree
starting from its argument (the nsBlockFrame) till it hits a mathml frame (which
it won't, given that frame tree), or the <math> tag (which happens to be the
nsTableRowFrame).

So we're calling ReflowDirtyChild() on the nsTableRowFrame and passing it null.
 And that lands us in nsContainerFrame::ReflowDirtyChild() and crashes.

Comment 8

[rbs]

Attachment: patch

Comment 9

[rbs]

Attachment: patch

oops, wrong file, take this one instead.

Comment 10

[rbs]

Checked in.

Comment 11

[Martijn Wargers (dead)]

Verified, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051028 Firefox/1.6a1