411582 - Crash [@ nsTableFrame::InsertFrames] with display:table-column-group and <svg:symbol>

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers a null deref [@ nsTableFrame::InsertFrames].

Comment 1

[Daniel Holbert [:dholbert]]

Crashes on Linux as well.

Comment 2

[Bernd]

is this a regression or a new tweak on fuzzers?

Comment 3

[Bernd]

in my pretty old debug build (2007121514)
it produces only a

JavaScript error: file:///C:/temp/bug411582.html, line 7: document.getElementByI
d("s") is null

So probably this got enabled recently by a svg fix.

Pseudo frame generation with in svg  probably some nsCSSFrameConstructor::ConstructDocElementFrame blues.

Comment 4

[Mats Palmgren (inactive)]

Attachment: frame dumps #1

'aFrameList' are all pseudo frames with the root as content,
which looks like a reasonably valid edge case to me.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/tables/nsTableFrame.cpp&rev=3.710&root=/cvsroot&mark=2284,2285,2288-2290,2292#2260

Comment 5

[Mats Palmgren (inactive)]

Attachment: WIP (diff -w)

Fall through if 'container' is null (we will append the frames).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Wait.  Wouldn't incoming pseudos wrapped around kids always be set to the table's content?  That is, shouldn't we actually be digging inside the pseudo to find the first different content, rather than walking the parent chain?

Comment 7

[Mats Palmgren (inactive)]

Isn't that what the code does?  but it never finds anything but the
root content.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, indeed.  But why, then?  Where're all the pseudo-frames coming from, if there is nothing inside them?  That looks wrong.

Comment 9

[Mats Palmgren (inactive)]

Attachment: WIP2

We're processing a restyle event for the <svg:symbol> which calls
RecreateFramesForContent() which calls ContentInserted():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1450&root=/cvsroot&mark=9136,9150#9134
where we try to construct a frame for it... while doing that we
build up a pseudo frames state -- I'm guessing this is unavoidable
since we can't tell beforehand if the innermost CreateFrame will
actually create a frame or not? -- then we come back here and
create the pseudo frames from the state, event though we never
created a frame for the child.

So maybe we should simply destroy the pseudo frames if there
were no real frame created? can we really do that in general?
If so, this will possibly also help fix bug 162063 by avoiding
creation of unnecessary pseudo frames, at least in some cases...

I'm still doing ProcessPseudoFrames() followed by Destroy() here -
we could add a method on nsPseudoFrames to optimize this...
(I don't think we can just skip calling ProcessPseudoFrames()
because that can leak frames IIRC)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, right.  Any codepath that uses aHaltProcessing will screw this up right now.  We really need to move to a better frame construction system, where we'll figure out exactly what will be constructed, _then_ construct pseudo-frames, and then construct it, instead of what we have now.

I don't think you can tell whether the pseudo-frames were actually needed without looking into them to see whether they wrap something.  That is, having an empty child list before pseudoframe processing is no guarantee of anything.

We should probably just add a null-check in the table code and file a bug to remove it once we fix frame construction.  Assign that bug to me?

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

Filed bug 411823

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 296452 [details] [diff] [review]
Patch rev. 1 (diff -w)

a=beltzner for 1.9

Comment 13

[Mats Palmgren (inactive)]

mozilla/layout/tables/nsTableFrame.cpp 	3.711
mozilla/layout/tables/crashtests/411582.xhtml 	1.1
mozilla/layout/tables/crashtests/crashtests.list 	1.32 

-> FIXED