330903 - [FIX]Crash with evil testcase, using html, xforms elements in xul document

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes current trunk Mozilla build on load.
You must have the xforms extension installed to get the crash.

Not sure if this is in fact a layout bug, maybe it's an xforms bug (then please reassign), but the crash is happening in layout code at least.

Seems a regression, doesn't crash in 2006-02-22 build, crashes in 2006-02-23 build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-02-22+08&maxdate=2006-02-23+10&cvsroot=%2Fcvsroot
Regression from bug 307421?

I can't get it to crash in my debug build, there I only get an assertion:
###!!! ASSERTION: should have a parent frame: 'docParentFrame', file c:/mozilla/
mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8283

Also cc-ing Allan, I heard he was interested in fixing fuzzy bugs ;)

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

The testcase crashes immediately after loading for me.

Comment 2

[Martijn Wargers (dead)]

Talkback ID: TB16516955Y
nsLayoutUtils::GetFloatFromPlaceholder  

Comment 3

[Allan Beaufour]

(In reply to comment #0)
> I can't get it to crash in my debug build, there I only get an assertion:
> ###!!! ASSERTION: should have a parent frame: 'docParentFrame', file
> c:/mozilla/
> mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8283

Crashes just fine for me on my debug trunk build :(
 
> Also cc-ing Allan, I heard he was interested in fixing fuzzy bugs ;)

heh. Unfortunately on a more long term basis, I'm quite bogged down by extension/xforms land right now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is XTF involved in the testcase in question?

Comment 5

[Allan Beaufour]

(In reply to comment #4)
> Is XTF involved in the testcase in question?

It's hard to avoid XTF when you use XForms :)

The testcase creates a "just_some_random_name" element in the XForms namespace so it hits our (XTF) element factory ... I wonder what it gets back. A null pointer?

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> (In reply to comment #4)
> > Is XTF involved in the testcase in question?
> 
> It's hard to avoid XTF when you use XForms :)
> 
> The testcase creates a "just_some_random_name" element in the XForms namespace
> so it hits our (XTF) element factory ... I wonder what it gets back. A null
> pointer?

Yes, it sure does. Something inside XTF should handle that and insert a "dummy element". We can do that in our element factory, but it should be the core handling that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

We probably create an element _somehow_.  I'd be interested in seeing what the frame dump looks like before the styles are added...

That said, though, is the fact that the root is "isindex" actually relevant in this bug?  And the fact that the document is XUL and not XML?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Wait.  Where do we get a null pointer?  What does the content model look like for this testcase?

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> Wait.  Where do we get a null pointer?

nsLayoutUtils::GetFloatFromPlaceholder() calls PlaceholderFrame::GetRealFrameForPlaceholder(aFrame), which returns null.

But the real problem is probably earlier:
"ASSERTION: should have a parent frame: 'docParentFrame', file nsCSSFrameConstructor.cpp, line 8289"
inside nsCSSFrameConstructor::ReconstructDocElementHierarchy()
where "nsIFrame* docParentFrame = docElementFrame->GetParent()" apparently return null.

> What does the content model look like for this testcase?

Is there an easy way to figure that out? :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed.

roc, do you think we should get this in on branches?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, if no regressions surface. I'm pretty sure the problems this hack was supposed to work around were gone before 1.8.

Comment 13

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

This seems a little scary to me -- brings back memories of crashes related to popupsets.  And I don't recall any improvements to management of popupsets or whatever other weird stuff happens in that area.  Although if the only difference is destroying the scrollframe, maybe that's not an issue.  In any case, I didn't dig in to the CVS history for why the change was made originally, although I do vaguely remember it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

For what it's worth, roc and I did dig into the CVS history, and the frame tree issues the original patch addresses are gone now...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on branch 1.8 branch.