355993 - [FIX]Crash [@ nsIFrame::GetPositionIgnoringScrolling] with MathML table, position: fixed

Status: VERIFIED FIXED

(Core :: MathML, defect, P3)

Description

[Jesse Ruderman]

Loading the testcase triggers three assertions and crash:

###!!! ASSERTION: dangling frame without a content node: 'content', file /Users/admin/trunk/mozilla/layout/mathml/base/src/nsMathMLFrame.cpp, line 236

###!!! ASSERTION: containing block height must be constrained: 'containingBlockHeight != NS_AUTOHEIGHT', file /Users/admin/trunk/mozilla/layout/generic/nsHTMLReflowState.cpp, line 1038

###!!! ASSERTION: Should hit cbrs->frame before we run off the frame tree!: 'aContainingBlock', file /Users/admin/trunk/mozilla/layout/generic/nsHTMLReflowState.cpp, line 1011


Thread 0 Crashed:
0   libgklayout.dylib         	0x073a64d4 nsIFrame::GetPositionIgnoringScrolling() + 40 (nsLayoutAtoms.h:649)
1   libgklayout.dylib         	0x06b577f4 nsHTMLReflowState::CalculateHypotheticalBox(nsPresContext*, nsIFrame*, nsIFrame*, nsMargin&, nsHTMLReflowState const*, nsHypotheticalBox&) + 1608 (nsHTMLReflowState.cpp:1012)
2   libgklayout.dylib         	0x06b57ae4 nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsHTMLReflowState const*, int, int) + 448 (nsHTMLReflowState.cpp:1067)
3   libgklayout.dylib         	0x06b5a46c nsHTMLReflowState::InitConstraints(nsPresContext*, int, int, nsMargin*, nsMargin*) + 2628 (nsHTMLReflowState.cpp:1965)
...

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061008 Minefield/3.0a1

I see also a crash (new profile): TB24316337Q 
Latest build without this crash is 1.9a1_2006100813. 

Comment 3

[rbs]

Looks like an upshot from the removal of "position: static" in mathml.css. If so, it might still be a bit early to check bz's patch for bug 322625 in the branches.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, sure.  If you noticed, I requested blocking for the branch milestones _after_ the current ones in bug 322625...  That gives me bake time.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and _please_ mark deps to regressions.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the <mtd> is ending up fixed-positioned.  Probably because it ends up not having a table display type, so table stuff dumps it into the "foreign" section.  Then we get into nsMathMLFrame::GetPresentationDataFrom, which asserts that its parent (the viewport) has a content (which is false).  Then it uses this content, so crashes.

The right answer would seem to not position the mtd, but I'm not sure how we can go about doing this, esp. in the face of XBL.  I'll think about it later, I guess; class now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, so the _real_ fundamental problem is this:

(gdb) frame 4
#4  0xb68d974f in nsMathMLContainerFrame::RebuildAutomaticDataForChildren (
    aParentFrame=0x8bbb5a4)
    at ../../../../../mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp:787
787         RebuildAutomaticDataForChildren(childFrame);
(gdb) p childFrame
$15 = (class nsIFrame *) 0x8bbb71c
(gdb) p childFrame->GetFirstChild(0)->GetParent()
$16 = (nsIFrame *) 0x8dc68f0

So the first child of |childFrame| doesn't have |childFrame| as the parent (in fact, it has a viewport frame).  That's pretty bogus.

(gdb) whats childFrame
0xb6ad5108 <_ZTV12nsBlockFrame+8>:      0xb6329568 <_ZN12nsBlockFrame14QueryInterfaceERK4nsIDPPv>
(gdb) satom childFrame->GetContent()->Tag()
$14 = {mString = 0xb6a527e9 "mtable", mAtom = 0xb6b39cc0}
(gdb) whats childFrame->GetFirstChild(0)
0xb6b183e8 <_ZTV24nsMathMLmtableOuterFrame+8>:  0xb68e8b98 <_ZN24nsMathMLmtableOuterFrame14QueryInterfaceERK4nsIDPPv>
(gdb) satom childFrame->GetFirstChild(0)->GetContent()->Tag()
$17 = {mString = 0xb6a527e9 "mtable", mAtom = 0xb6b39cc0}

childFrame is the anonymous block for the <mtable>.  The problem, of course, is that ConstructTableFrame allows tables to be positioned and if they are the returned outer table is NOT what you want to be sticking in your childItems.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixes the crash for me

Comment 9

[Boris Zbarsky [:bzbarsky]]

The other option is to change ConstructTableFrame to take a boolean that could be used to suppress positioning/floating of the outer table...  But I prefer this fix, somehow.

Comment 10

[rbs]

Is that a big change to achieve the other option, even using IsFrameOfType() rather than a parameter? (This also returns true for the mathml's outertable or its parent.) If we can enforce that there is no floating positioning inside the <math>, then we might be better off down the track. We won't have to worry about them being there sometimes (and allow provision for that in other things).

Comment 11

[Boris Zbarsky [:bzbarsky]]

It's not a big change to do the other thing, no.  IsFrameOfType won't cut it, since there's no frame to look at the type of other than the parent and the parent is NOT IsFrameOfType(eMathML) in this case -- it's the block that mathml sticks inside the mrow.

I just don't really like having to change random non-MathML code that MathML is reusing about MathML-specific quirks...  I can post a patch to do that, though, for comparison.

Comment 12

[rbs]

Yes, let's have a look at the alternative. I had this follow-up comment, but it collided with yours.

[... to be precise, in this case, the parent is a generic block wrapper (so IsFrameOfType() won't flag it as MathML). But the positioning problem will re-surface if <mtable> is later switched to an inline-table and the block wrapper is removed. So, still safe to stop it while here, if simple enough.]

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: So this also fixes the problem, as expected

I still prefer the first solution, but if you think there's random tree-walking stuff in MathML that could walk from the table to the viewport and then get confused, maybe we should do this....

Comment 14

[Boris Zbarsky [:bzbarsky]]

I should note that if <mtable> becomes inline-table (and the code in ConstructMathMLFrame enforces that) then we won't hit this code at all -- positioning or floating an "inline-table" makes it a "table".  See CSS 2.1, section 9.7, first row in the table there.

Comment 15

[rbs]

Comment on attachment 241733 [details] [diff] [review]
So this also fixes the problem, as expected

Let's go for this, as there is no latent apprehension that the positioning might awake and bite us.

(Since it is not conceivable to have math without matrices, commutative diagrams and the likes, these quirks to emulate inline-table are worth these maintenance cycles until inline-table is implemented.)

"positioning or floating an "inline-table" makes it a "table"" 

Hope this doesn't mean that it would be a block-table needing to re-emulate the inline-table...

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Hope this doesn't mean that it would be a block-table needing to re-emulate the
> inline-table...

It would be a block table per the CSS display type.  What MathML decides to do with that is your call.  I really don't care so long as:

1)  It doesn't crash
and
2)  It doesn't uglify the rest of our code too much.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 241733 [details] [diff] [review]
So this also fixes the problem, as expected

This kinda needs OK from bernd.  And we should file a bug on backing this out once inline-table happens.

Comment 18

[rbs]

Another option is to also have aTableCreator.AllowOutOfFlow(), so that the parameter doesn't have to be carried around.

>2)  It doesn't uglify the rest of our code too much.

No cause for alarm. That's not what I hear from people (some commercial) who have their own proprietary implementation (that I can't see). The feedback I get is that ours isn't that bad (to put it midly and be modest about it). I guess you must now have quite a good idea as to the real issues to _resolve_ per MathML.

Comment 19

[Bernd]

is this uglyyyyy :-(. Oh man I better offer a couple of sixpacks and get MathML not reusing table frames. 

>Another option is to also have aTableCreator.AllowOutOfFlow
This thing will die soon, see bug 243159. I will definitely r- anything that adds something to that structure. It has to go away. It is simply wrong.

I prefer the first patch as with the second patch every table has to pay the price that MathML tables have a problem.

Comment 20

[rbs]

Let's remain constructive, guys. We are trying to solve problems here.

Yet another option to avoid dragging the parameter around is (in peuso-code) aContent->NamespaceID() == MathML, at the spots where needed.

Comment 21

[Bernd]

no, problem. I will review the second patch if you can explain why the first one is bad. The second patch just adds code a to very frequent code path and I am pretty reluctant to see this code checked in. But if there is no other way then we have to go that way. The code is correct, but I don't like it. 

The comment about TableCreator is absolutely serious. I am trying hard to get rid of TableProcessChild. Its one of the corner stones of the table pseudo changes that are ahead. So please please no additions to that structure. 

Comment 22

[Boris Zbarsky [:bzbarsky]]

So I've done a bit more thinking about this.  For branches, I think I agree with rbs that the second patch is safer -- it makes sure that MathML can't suprise itself while walking around the tree.  I have a slight simplification of the patch that I'll post in a second, but I think we should go with it.

For trunk, I think the right approach is probably to take that patch for now and then once reflow branch lands and we can implement inline-table to work on doing that and backing this code out.

Seem reasonable?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Lightly tweaked

Comment 24

[rbs]

Comment on attachment 241787 [details] [diff] [review]
Lightly tweaked

sr=rbs

Comment 25

[rbs]

bernd said: "I will review the second patch if you can explain why the first
one is bad."

bz said: "it makes sure that MathML can't suprise itself while walking around the tree"

This sums up the motivation. With the first patch, <mtable> is taken out of flow -- outside the <math>, whereas MathML is very driven by its surrounding context. Just to give an example, if one sees <mo lspace="thinmathspace"> (anywhere inside, possible inside an <mtd>), then resolving this involves looking up the ancestry for a potential scoping <mstyle thinmathspace="..."> that tells what thinmathspace is. (Note that there could be <mstyle thinmathspace="..."> ... <mstyle attr-is-not-here> ...<mo lspace="thinmathspace">...</mo> ... </mstyle> ... </mstyle>.) So, we don't have much choice other than looking all the way up and stopping at the <math>. When <math> isn't there, we search all the way up in desperation, everytime, anywhere, before falling back to a default thinmathspace=1/18em. (Usually we also need to look around and style things in CSS unfriendly ways.) Trying to account/hunt for placeholders and do other trickeries wouldn't be pretty, and furthermore, it wouldn't even buy us much as people don't expect a fraction a/b with its numerator floating over there, away from the fraction bar, or a determinant |A| with its bars empty because the matrix A is out there, fixed-positioned on the corner of the screen, etc. That's why keeping MathML frames local in the <math>, if we can, is good in general.

Comment 26

[Bernd]

Comment on attachment 241787 [details] [diff] [review]
Lightly tweaked

Please add comments why we have this code path at all and what is necessary to remove the code.

A good place would be just before:
 -  nsIFrame* geometricParent = aState.GetGeometricParent(disp, parentFrame);
+  nsIFrame* geometricParent =
+    aAllowOutOfFlow ? aState.GetGeometricParent(disp, parentFrame) :
+                      parentFrame;
 and at the caller inside  ConstructMathMLFrame.



The change at 
 rv = aState.AddChild
also removes the ugliness that triggered my initial not too serious comment.

with the comments r+

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in with more comments.  Filed bug 356164 on removing this hack.  Requesting same flags as for bug 322625.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 241787 [details] [diff] [review]
Lightly tweaked

For branch, I'm almost happier with the version of the patch without the comments.

Comment 29

[Bernd]

Indeed the comments make no sense branch

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 241787 [details] [diff] [review]
Lightly tweaked

a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch version

Comment 32

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.1, 1.8.0.9

Comment 33

[Boris Zbarsky [:bzbarsky]]

I meant 1.8.1.1.

Comment 34

[Martijn Wargers (dead)]

Verified fixed on trunk by using the testcase with a trunk build before and and after the check-in of the patch.
Verified fixed on 1.8.1 and 1.8.0.x branch by looking at the bonsai logs of the MOZILLA_1_8_BRANCH branch and MOZILLA_1_8_0_BRANCH branch.

Comment 35

[Jesse Ruderman]

Crashtest checked in.