354766 - <mathml:mtd> with border-collapse <html:table> crashes [@ nsRuleNode::GetStyleData] [@ nsCachedStyleData::GetStyleData]

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
  1. Load the testcase.

A nightly crashes [@ nsRuleNode::GetStyleData] dereferencing zero, but my debug build crashes  [@ nsCachedStyleData::GetStyleData] dereferencing a random address, so [sg:critical].

The debug build also complains:
###!!! ASSERTION: not implemented: '!IsBorderCollapse(aParentFrame)', file /Users/admin/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1848

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Bernd]

short german description of this bug: Mitgegangen, mitgehangen. 
I guess one needs to implement http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableCellFrame.cpp#1133

Comment 3

[Bernd]

Once a table is border collapse the cellmap changes and is is expected that the td correspond to nsBCTableCellFrame rather than to  nsTableCellFrame. So the assert is right to the point. However it does not help against really determined people.

Comment 4

[rbs]

bernd, what if it is the reverse, i.e., the table has border collapse, and the cell is nsTableCellFrame. Will that crash too?

In order words, is nsBCTableCellFrame stronger against the mismatch? The reason I am asking is because I could then change mtable in mathml.css to be border collapse, and change nsMathMLmtdFrame to just inherit from nsBCTableCellFrame across the board.

If this is a no-no too, then we can simply fix this bug with:
if (IsBorderCollapse(aParentFrame))
 return nsBCTableCellFrame()
[i.e., those determined people get the default non-MathML frames and don't crash]

Comment 5

[Bernd]

>the table has border collapse, and the cell is nsTableCellFrame
This should NEVER happen. I can't imagine that one (Jesse and Martijn are not included in this) can construct a case where this happens. ::IsBorderCollapse protects against it.

>I am asking is because I could then change mtable in mathml.css to be border
>collapse, 
You don't want make border collapse the default for mtables. You really don't want this. Trust me.

>those determined people get the default non-MathML frames and don't
crash

Probably a better option but I am not sure that the mathml code does not rely on mrow to have only mtd children.

Comment 6

[rbs]

> Probably a better option but I am not sure that the mathml code does not rely
> on [mrow] (you meant mtr) to have only mtd children.

There is only one spot with a static cast to nsTableCellFrame*:
http://lxr.mozilla.org/mozilla/source/layout/mathml/base/src/nsMathMLmtableFrame.cpp#206

Shoudn't be a problem, as nsBCTableCellFrame is a subclass of nsTableCellFrame, right?

If not, I can add a test for frametype == ::tableCell (rather than ::BCtableCell), and exit early from that function. (It is of little consequence to exit early from that function.)

Comment 7

[Bernd]

I think that will work.

Comment 8

[rbs]

Attachment: patch

Comment 9

[rbs]

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

I should have mentioned that this patch effectively fixes the crash.

Comment 10

[Bernd]

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

looks good to me, at least if MathTableRowFrames can live with a non MathML child.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

This could probably use a little comment explaining what we're doing....

Comment 12

[rbs]

Checked in with these comments added next to that code:

// <mtable> is border separate in mathml.css and the MathML code doesn't implement
// border collapse. For those users who style <mtable> with border collapse,
// give them the default non-MathML table frames that understand border collpase.
// This won't break us because MathML table frames are all subclasses of the default
// table code, and so we can freely mix <mtable> with <mtr> or <tr>, <mtd> or <td>.
// What will happen is just that non-MathML frames won't understand MathML attributes
// and will therefore miss the special handling that the MathML code does.

Comment 13

[Boris Zbarsky [:bzbarsky]]

rbs, is that patch acceptable for the branches?

Comment 14

[rbs]

Yes, it is pretty safe for the branch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK.  If you feel ok landing it there, please request 1.8.0.9 approval?

Comment 16

[rbs]

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

Safe patch for a crasher.

Comment 17

[Mike Connor [:mconnor]]

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

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

Comment 18

[rbs]

fixed1.8.1.1, fixed1.8.0.9

Comment 19

[Jay Patel [:jay]]

v.fixed on 1.8.0 and 1.8.1 branches with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9) Gecko/20061206 Firefox/1.5.0.9
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

No crash with testcase.

Comment 20

[Jesse Ruderman]

Crashtest checked in.