368183 - Crash [@ nsHTMLContainerFrame::CreateViewForFrame] with nested applet as root in XML window

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, this crashes on load for me when loading, with current trunk builds.
This regressed between 2007-01-12 and 2007-01-13:
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=2007-01-12+04&maxdate=2007-01-13+07&cvsroot=%2Fcvsroot

I think this is a regression from bug 366207.

Comment 1

[Jesse Ruderman]

WFM on Mac (trunk nightly 2007-01-22) with Java enabled.

Is the XUL mime type necessary, or does it crash with an XHTML mime type too?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Same thing happens if I load the testcase as XHTML.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

So here's why we crash, as far as I can tell:

1)  We go to create frames for the outer <object>.  This constructs a block
    frame for it (not an nsObjectFrame), because it's the root element.
2)  Since overflow is not set hidden, we construct the root scrollframe.
3)  We create the anonymous <scrollbar> nodes.
4)  Creating frames for the <scrollbar>s attaches their XBL bindings.
5)  Binding attachment calls WrapNative on the <scrollbar>s.
6)  Being XUL, the wrapper parent is whatever GetParent() returns, which
    in this case is the <object>.
7)  Wrapping the <object> triggers EnsureInstantiation (from classinfo).
8)  EnsureInstantiation tries to reconstruct the frame for the <object>; we
    reenter frame construction and start doing stuff, but the frame tree is
    in a bogus state right then, and we crash.

This patch changes step 6.  I don't know why XUL parents to the GetParent() but I assume it's to force binding instantiation on ancestors if a child is wrapped.  But in this case, for anonymous content, I don't really think we want to force such binding instantiation...

I suppose I could also just compare the GetBindingParent() of the kid with that of the parent.  That would have the same result and might be clearer.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There are other reasons for parenting to the parent in XUL I think. It'll mean that things like event-handler-attributes see properties of the entire parent chain when they execute.

The two scary steps in the above description sounds to me like 5 and 7. Especially, could we delay the EnsureInstantiation call until later?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> things like event-handler-attributes see properties of the entire parent
> chain

But do we want that for anonymous kids?

I agree that step 7 is scary.  The issue is that we assume that being wrapped means script is asking for us, and if it is we have to assume it might want to call functions on the plugin... so we have to instantiate so we can install the plugin prototype stuff, etc.

The problem is that we could be all ready to instantiate but not have done it yet, because normally instantiation happens off an event.

Now the other possible fix is to do step 4 later.  Or rather, split up binding attachment into two separate parts -- creation of anonymous content (which we do want to happen during frame construction) and attachment of the JS gunk (which is what's biting us here, and which we could try doing at a "safe" time, e.g. right before calling the constructor).

A third option is to only parent XUL to the parent node if the parent node is also XUL.  ;)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. why do scrollbars have the JS gunk at all? Is this due to the mac stuff?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I think so... though the handlers might need it too; I can't recall.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I have ideas for getting rid of the mac script stuff. Maybe we could do that and see if it solves this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

We could try, sure.

Comment 10

[timeless]

i think the main purpose was supporting scrollbars at either end. note of course that beos still allows for this, and theoretically some other system themes on linux might :).

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 252884 [details] [diff] [review]
Possible patch

Clearing sr flag until Jonas figures out whether we need this or not.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 252884 [details] [diff] [review]
Possible patch

I think I'll take Jonas' earlier comment as r-.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Patch in bug 384612 should have fixed this.

Comment 14

[Carsten Book [:Tomcat]]

verified fixed using the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2008010104 Minefield/3.0b3pre ID:2008010104. No crash on testcase -> Verified fixed