385866 - [FIXr]Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, <col span="2">

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase (in a Mac trunk debug build of Firefox) causes three assertions and a crash:

###!!! ASSERTION: identical: 'pseudoType1 != pseudoType2', 
file nsGenConList.cpp, line 124

###!!! ASSERTION: null check on startContent should be sufficient to null check nodeContent as well, since if nodeContent is for the root, startContent (which is before it) must be too: 'nodeContent || !startContent', 
file nsCounterManager.cpp, line 145

###!!! ASSERTION: The possible descendant is null!: 'aPossibleDescendant', 
file nsContentUtils.cpp, line 1144

Crash (null dereference) with stack trace:
  0  nsINode::GetNodeParent
  1  nsContentUtils::ContentIsDescendantOf
  2  nsCounterList::SetScope
  ...

Bug 383129 has a similar crash signature, but the patch there is a patch to xul tree code, so this isn't a dup.

Comment 1

[-fullmetaljacket-]

testcase also crashed winxp sp2

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070625 Minefield/3.0a6pre ID:2007062515

Comment 2

[Bernd]

<col id="col" span="2"></col> creates two col frames which share the style context and the content node the first colframe is content based and the second is anonymous col due to the span.

I guess that ASSERTION: identical: 'pseudoType1 != pseudoType2', 
file nsGenConList.cpp, line 124 is bogus in this case.

The next assert is probably the result of keeping a reference to the second anonymous col frame past the removal of the <col> node.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> creates two col frames which share the style context and the content node

We shouldn't be setting up quotes/counters for both of them.  That's the real bug here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 5

[Bernd]

"We shouldn't be setting up quotes/counters for both of them"

Why?

Comment 6

[Bernd]

Comment on attachment 273537 [details] [diff] [review]
Fix

we would enumerate frames rather than content

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273537 [details] [diff] [review]
Fix

We should probably do this on branches too.  This is a _very_ safe fix.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273537 [details] [diff] [review]
Fix

sr=dbaron.  Who knows what's right here -- it probably doesn't really matter.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273537 [details] [diff] [review]
Fix

Fairly straightforward fix for a crash due to a broken counters list.  Very safe.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273537 [details] [diff] [review]
Fix

a19=dbaron

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 273537 [details] [diff] [review]
Fix

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 14

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.7 using the testcase and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/2007090308 BonEcho/2.0.0.7pre + Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre ID:2007090304

no crash on testcase with this builds - adding verified keyword

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

No crash here with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre, using the testcase in comment 0.

Replacing fixed1.8.0.14 with verified1.8.0.14

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

(Second build ID should be Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre; copy/paste is at times non-functional in a VM, for some reason.)

Comment 17

[Jesse Ruderman]

Crashtest checked in.