284001 - Crash [@ nsMathMLContainerFrame::GetType] with evil MathML testcase, using mo:hover{display:block;}

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Martijn Wargers (dead)]

See testcase. Hovering over the text in there causes a crash in current trunk
Mozilla builds.

Talkback ID: 3987420

nsMathMLContainerFrame::GetType 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp,
line 1156]
nsCSSFrameConstructor::FindNextSibling 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 8206]
nsCSSFrameConstructor::ContentInserted 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 9039]
nsCSSFrameConstructor::RecreateFramesForContent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 11553]
nsCSSFrameConstructor::RestyleElement 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 10143]
nsCSSFrameConstructor::ProcessOneRestyle 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13501]
nsCSSFrameConstructor::ProcessPendingRestyles 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13543]
nsCSSFrameConstructor::RestyleEvent::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13595]
SHELL32.dll + 0x520c24 (0x778b0c24)

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Crashes also with Mozilla1.4 and 1.7, so no (recent) regression.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So.. what happens here is that this->mEmbellishData.coreFrame is pointing to
0xdddddddd memory.  Not sure why that's happening...

Comment 4

[rbs]

display:block is the evil bit.

With the markup of the testcase:

<math xmlns="http://www.w3.org/1998/Math/MathML">
<munder>
  <mo>Hovering over this should not cause a crash</mo>
  <mrow><mi></mi></mrow>
</munder>
</math>

The |this| container is <munder> and its coreFrame is <mo>. Things seem to go
out of sync when :hover changes <mo> to a block, and frames get destroyed/recreated.

What is interesting is that changing to inline doesn't cause any problem (maybe
because the coreFrame is not taken out of the child list since it is inline).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Isn't it display:inline by default?

Comment 6

[rbs]

That's right. But the evil testcase is doing:

mo:hover{display:block;}

Comment 7

[Boris Zbarsky [:bzbarsky]]

Sure.  But you said:

> What is interesting is that changing to inline doesn't cause any problem

The point is, there is no change involved there... it's already inline, right?

Comment 8

[rbs]

Yes, you are right, nothing happens, mo:hover{display:inline;} doesn't trigger
any re-resolve because there is no style change.

Nor is there any trouble with mo:{display:block;} if set initially in the CSS.
But then adding mo:hover{display:inline;} triggers a similar crash.

Comment 9

[rbs]

Attachment: patch

MathML has a little helper |nsMathMLContainerFrame::ChildListChanged| that is
called at the tail-end of its overloaded |AppendFrames|, |InsertFrames|, etc.
It is meant to cater for situations such as in this crash -- where a change of
children invalidates certain pointers. 

The old code was just doing a |ReLayoutChildren(this)|. The patch changes
changes the |this| to a more suitable ancestor.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177340 [details] [diff] [review]
patch

r+sr=bzbarsky.	Seems reasonable..

Comment 11

[rbs]

Checked in.