366493 - MathML crash on 1.8 branches [@ nsIFrame::GetStateBits]

Status: RESOLVED WONTFIX

(Core :: MathML, defect)

Description

[Daniel Veditz [:dveditz]]

The original unreduced testcase from bug 306902 (attachment 194712 [details]) still crashes both FF1509 and FF2001 around step 500 with the following stack:

The original unreduced testcase crashes both FF1.5.0.10pre and FF2.0.0.2pre at around step 500, with the following stack

> 	nsIFrame::GetStateBits() Line 817	C++
	IncrementalReflow::AddCommand() Line 938	C++
 	PresShell::ProcessReflowCommands() Line 6898	C++
 	PresShell::WillPaint() Line 6565	C++
 	nsViewManager::DispatchEvent() Line 2035	C++
 	HandleEvent() Line 171	C++
 	...etc...

The trunk is fine, even prior to the reflow-branch landing when the code in IncrementalReflow::AddCommand() looked the same.

The stack looks vaguely familiar to me from earlier StirDOM crashes, but the closest bug I could find (bug 310933) was fixed in 1.8.0.2 by bug 291902 -- this must be some other way of calling IncrementalReflow::AddCommand() on a null frame.

Would be easy to stop the crash by putting a null check where the current assert is but that might just move the problem somewhere worse (the trunk doesn't have the null check, but doesn't crash). We may want to just live with this crash, but it does get in the way of fuzz-testing the current releases.

I guess we could try to see when the trunk got fixed

Comment 1

[Mats Palmgren (inactive)]

Firefox 2006-01-04-05 Linux works
Firefox 2006-01-03-05 Linux hangs with 100% CPU after 15 seconds
So it looks like bug 309120 fixed it on trunk.
Note the same stack in bug 309120 comment 38.

Comment 2

[Mats Palmgren (inactive)]

The null deref is caused by a call to nsContainerFrame::ReflowDirtyChild
with aChild==nsnull.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsContainerFrame.cpp&rev=1.239.6.3&root=/cvsroot&mark=388,396#387

I've traced it back to nsMathMLContainerFrame::ReLayoutChildren()
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=863#805

The ReflowDirtyChild() call at the end seems to assume that 'frame' is a
nsMathMLContainerFrame and that it will end up here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=967-988#967
(where aChild isn't used)

The top of the stack when we do AppendReflowCommand(nsnull,...) is:
PresShell::AppendReflowCommand 
nsContainerFrame::ReflowDirtyChild
nsMathMLContainerFrame::ReLayoutChildren
nsMathMLmoFrame::ReflowDirtyChild
nsMathMLContainerFrame::ReLayoutChildren
nsMathMLContainerFrame::ChildListChanged
nsMathMLContainerFrame::RemoveFrame
nsFrameManager::RemoveFrame
nsCSSFrameConstructor::ContentRemoved

FWIW, I added a null-check and it seems stable on Linux although there
are plenty of assertions...

Comment 3

[Daniel Veditz [:dveditz]]

Mats: where did you add the null check?

Comment 4

[Mats Palmgren (inactive)]

(In reply to comment #3)
> Mats: where did you add the null check?

I did an early return in PresShell::AppendReflowCommand() if aTargetFrame 
was null.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot#3678

Comment 5

[Mats Palmgren (inactive)]

... just to be clear - I do not suggest that null-check as a workaround,
I think that's a bit to central to be ignoring nulls which could lead to
more serious errors later on.  I think a null access crash is preferred in
that case.  A wallpaper for this bug (if we can't find the real issue)
should be in nsMathMLContainerFrame::ReLayoutChildren() IMO.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

I think this should be a safe fix for branches.
Tested on 1.8 and 1.8.0 on Linux.

Comment 8

[rbs]

I am not able to access the attachment in the URL field. I am getting the message:
"Sorry, you are not authorized to access this attachment."

Comment 9

[Mats Palmgren (inactive)]

Bugzilla bug I guess. Let me take the bug and see if that helps... try again?

Comment 10

[rbs]

Didn't help. To which bug is attachment 194712 [details] on? I am not seeing it on bug 306902.

Comment 11

[Mats Palmgren (inactive)]

It's on bug 306902 alright, but it's marked private, so it's the same
problem again I think.  Are you not a security-group member?

Comment 12

[rbs]

No.

Comment 14

[rbs]

I am looking at this right now.

For ease of testing, I downloaded and save a local copy of the testcase. At present, StirDOM is doing its thing and it seems to take a while to get to the crash point. I wish there was a trick to jump straight at the crash so as to leave me with some time to go to sleep later :-)

Comment 15

[Daniel Veditz [:dveditz]]

Not blocking since it looks like a safe crash, but if this gets reviewed we can approve it.

> I wish there was a trick to jump straight at the crash so as to
> leave me with some time to go to sleep later :-)

Usually the testcases can be reduced to a very small case, but when they can't then there's no short cut because we haven't been able to tell which bits could be removed without making the crash go away. E.g. we fixed bug 306902, but the original testcase still causes this problem.

Comment 16

[Carsten Book [:Tomcat]]

the testcase from Bug 306902 https://bugzilla.mozilla.org/attachment.cgi?id=194712 crashs also Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/2007082303 BonEcho/2.0.0.7pre for with TB35217311W

Comment 17

[Mats Palmgren (inactive)]

This bug only affect 1.8 branch (and older branches maybe), per comment 1.
I ran the testcase for about 10 minutes on current trunk build on Linux
just in case, it did not hang or crash.

Since it's a safe null-deref crash it doesn't seem worth the risk
of fixing.  Please reopen if you disagree.

-> WONTFIX

Comment 18

[Samuel Sidler (old account; do not CC)]

Can we open this bug then?