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

RESOLVED FIXED

Status

()

Core
MathML
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: rbs)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.9, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(2 attachments)

261 bytes, application/xhtml+xml
Details
837 bytes, patch
Bernd
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 240557 [details]
testcase
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical]

Comment 2

11 years ago
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

11 years ago
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.
(Assignee)

Comment 4

11 years ago
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

11 years ago
>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.



(Assignee)

Comment 6

11 years ago
> 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

11 years ago
I think that will work.
(Assignee)

Updated

11 years ago
Assignee: nobody → rbs
Component: Layout: Tables → MathML
QA Contact: layout.tables → ian
(Assignee)

Comment 8

11 years ago
Created attachment 240588 [details] [diff] [review]
patch
Attachment #240588 - Flags: superreview?(bzbarsky)
Attachment #240588 - Flags: review?(bernd_mozilla)
(Assignee)

Comment 9

11 years ago
Comment on attachment 240588 [details] [diff] [review]
patch

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

Comment 10

11 years ago
Comment on attachment 240588 [details] [diff] [review]
patch

looks good to me, at least if MathTableRowFrames can live with a non MathML child.
Attachment #240588 - Flags: review?(bernd_mozilla) → review+

Comment 11

11 years ago
Comment on attachment 240588 [details] [diff] [review]
patch

This could probably use a little comment explaining what we're doing....
Attachment #240588 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 12

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 13

11 years ago
rbs, is that patch acceptable for the branches?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
(Assignee)

Comment 14

11 years ago
Yes, it is pretty safe for the branch.

Comment 15

11 years ago
OK.  If you feel ok landing it there, please request 1.8.0.9 approval?
(Assignee)

Comment 16

11 years ago
Comment on attachment 240588 [details] [diff] [review]
patch

Safe patch for a crasher.
Attachment #240588 - Flags: approval1.8.1.1?
Attachment #240588 - Flags: approval1.8.0.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
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
Attachment #240588 - Flags: approval1.8.1.1?
Attachment #240588 - Flags: approval1.8.1.1+
Attachment #240588 - Flags: approval1.8.0.9?
Attachment #240588 - Flags: approval1.8.0.9+

Updated

11 years ago
Whiteboard: [sg:critical] → [sg:critical][checkin needed (1.8 branch)]
(Assignee)

Comment 18

11 years ago
fixed1.8.1.1, fixed1.8.0.9
Keywords: fixed1.8.0.9, fixed1.8.1.1

Comment 19

11 years ago
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.
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Whiteboard: [sg:critical][checkin needed (1.8 branch)] → [sg:critical]
Group: security
(Reporter)

Comment 20

10 years ago
Crashtest checked in.
Flags: in-testsuite+
Crash Signature: [@ nsRuleNode::GetStyleData] [@ nsCachedStyleData::GetStyleData]
You need to log in before you can comment on or make changes to this bug.