Last Comment Bug 354766 - <mathml:mtd> with border-collapse <html:table> crashes [@ nsRuleNode::GetStyleData] [@ nsCachedStyleData::GetStyleData]
: <mathml:mtd> with border-collapse <html:table> crashes [@ nsRuleNode::GetStyl...
Status: RESOLVED FIXED
[sg:critical]
: crash, testcase, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: rbs
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 347580
  Show dependency treegraph
 
Reported: 2006-09-28 17:14 PDT by Jesse Ruderman
Modified: 2007-12-17 17:06 PST (History)
5 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (261 bytes, application/xhtml+xml)
2006-09-28 17:15 PDT, Jesse Ruderman
no flags Details
patch (837 bytes, patch)
2006-09-29 00:40 PDT, rbs
bernd_mozilla: review+
bzbarsky: superreview+
mconnor: approval1.8.0.9+
mconnor: approval1.8.1.1+
Details | Diff | Review

Description Jesse Ruderman 2006-09-28 17:14:45 PDT
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 2006-09-28 17:15:31 PDT
Created attachment 240557 [details]
testcase
Comment 2 Bernd 2006-09-28 22:01:37 PDT
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 2006-09-28 22:11:17 PDT
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 2006-09-28 22:38:38 PDT
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 2006-09-28 23:27:06 PDT
>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 2006-09-28 23:55:26 PDT
> 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 2006-09-29 00:17:14 PDT
I think that will work.
Comment 8 rbs 2006-09-29 00:40:36 PDT
Created attachment 240588 [details] [diff] [review]
patch
Comment 9 rbs 2006-09-29 01:39:02 PDT
Comment on attachment 240588 [details] [diff] [review]
patch

I should have mentioned that this patch effectively fixes the crash.
Comment 10 Bernd 2006-09-29 02:41:35 PDT
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 [:bz] 2006-09-29 06:40:52 PDT
Comment on attachment 240588 [details] [diff] [review]
patch

This could probably use a little comment explaining what we're doing....
Comment 12 rbs 2006-09-29 10:42:51 PDT
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 [:bz] 2006-10-08 15:06:22 PDT
rbs, is that patch acceptable for the branches?
Comment 14 rbs 2006-10-08 21:54:42 PDT
Yes, it is pretty safe for the branch.
Comment 15 Boris Zbarsky [:bz] 2006-10-08 22:01:51 PDT
OK.  If you feel ok landing it there, please request 1.8.0.9 approval?
Comment 16 rbs 2006-10-08 23:01:34 PDT
Comment on attachment 240588 [details] [diff] [review]
patch

Safe patch for a crasher.
Comment 17 Mike Connor [:mconnor] 2006-11-07 06:45:36 PST
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 2006-11-29 14:00:16 PST
fixed1.8.1.1, fixed1.8.0.9
Comment 19 Jay Patel [:jay] 2006-12-08 14:23:29 PST
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 2007-12-17 17:06:50 PST
Crashtest checked in.

Note You need to log in before you can comment on or make changes to this bug.