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

VERIFIED FIXED

Status

()

Core
Layout
P4
critical
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

({crash, regression, testcase})

Trunk
x86
Windows XP
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:Rs], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
Created attachment 252762 [details]
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

11 years ago
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?
Summary: Crash [@ nsHTMLContainerFrame::CreateViewForFrame] with nested applet in xul window → Crash [@ nsHTMLContainerFrame::CreateViewForFrame] with nested <applet>s
Same thing happens if I load the testcase as XHTML.
Summary: Crash [@ nsHTMLContainerFrame::CreateViewForFrame] with nested <applet>s → Crash [@ nsHTMLContainerFrame::CreateViewForFrame] with nested applet as root in XML window
Created attachment 252884 [details] [diff] [review]
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.
Attachment #252884 - Flags: superreview?(jst)
Attachment #252884 - Flags: review?(jonas)
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?
> 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.  ;)
Hmm.. why do scrollbars have the JS gunk at all? Is this due to the mac stuff?
I think so... though the handlers might need it too; I can't recall.
I have ideas for getting rid of the mac script stuff. Maybe we could do that and see if it solves this?
We could try, sure.

Comment 10

11 years ago
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 :).
Flags: blocking1.9?
Comment on attachment 252884 [details] [diff] [review]
Possible patch

Clearing sr flag until Jonas figures out whether we need this or not.
Attachment #252884 - Flags: superreview?(jst)
Flags: blocking1.9? → blocking1.9+
Depends on: 384612
Comment on attachment 252884 [details] [diff] [review]
Possible patch

I think I'll take Jonas' earlier comment as r-.
Attachment #252884 - Flags: review?(jonas) → review-
Whiteboard: [dbaron-1.9:Rs]
Priority: -- → P4
Patch in bug 384612 should have fixed this.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED

Updated

7 years ago
Assignee: nobody → bzbarsky
Crash Signature: [@ nsHTMLContainerFrame::CreateViewForFrame]
You need to log in before you can comment on or make changes to this bug.