GtkMozEmbedChrome can hold stale mContentShell reference

RESOLVED FIXED

Status

()

Core
Document Navigation
--
major
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Kevin Buhr, Assigned: blizzard)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
I noticed this problem in SkipStone (see http://www.muhri.net/skipstone/), a
small browser that embeds a gtk_moz_embed window.  When built against my
2001/03/04 pull of SeaMonkey, it crashes on startup.  It appears to me to be a
Mozilla bug rather than a SkipStone bug.

The entire problem occurs during execution of GtkMozEmbedPrivate::LoadChrome.

In nsFrameFrame.cpp's nsHTMLFrameInnerFrame::CreateDocShell, a newly created
kWebShellCID instance is added to the container tree owner:

        parentTreeOwner->ContentShellAdded(docShellAsItem, ...)

where the parentTreeOwner is actually a GtkMozEmbedChrome, so it stores a plain
pointer to this "docShellAsItem" interface in its mContentShell pointer.

Later on, the method nsXULDocument::InsertStyleSheetAt calls
PresShell::StyleSheetAdded -> PresShell::ReconstructFrames ->
StyleSetImpl::ReconstructDocElementHierarchy, which rebuilds the entire chrome 
tree, and this includes freeing the nsHTMLFrameInnerFrame that holds that last
XPCOM reference (in mSubShell) to the webshell, destroying it and leaving the
mContentShell pointer dangling.

The crash surfaces a little later, usually in
GtkMozEmbedPrivate::OnChromeStateChange, when

        treeOwner->GetPrimaryContentShell(getter_AddRefs(contentItem));

causes the reference count of the bogus treeOwner->mContentShell to be increased.

I assume the same problem exists in the Photon embedding code, since
PhMozEmbedChrome::ContentShellAdded does the same thing.

I'm not exactly sure who's responsibility this is.  I don't *think* it's a
problem with the particular SkipStone app doing the embedding.  And I'm not sure
whether it's GtkMozEmbedChrome's responsibility to hold a weak reference to the
mContentShell, whether content shells should be checking if they are the primary
content of their tree owner and removing themselves if so, or what.

This problem does *not* appear under an up-to-date Mozilla 0.8 build, only under
SeaMonkey, but I haven't been able to figure out what change is responsible.

Let me know if any other information is needed.
(Reporter)

Comment 1

17 years ago
Here's a patch that implements a weak reference for "mContentShell".  Again, I'm
not sure if this is the right solution, but SkipStone runs fine with this patch.
 Mozilla just gives a warning:

...
WEBSHELL- = 2
Warning: Failed to find primary content shell!  I will try again later.
WEBSHELL+ = 3
...

when it tears down and rebuilds the tree.

If this patch *is* the correct solution, a similar patch must be applied to the
Photon embedding stuff, too.
(Reporter)

Comment 2

17 years ago
Created attachment 27108 [details] [diff] [review]
patch to make GtkMozEmbedChrome::mContentShell an nsWeakPtr
setting status to new
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

17 years ago
Reassigning to Chris.
Assignee: adamlock → blizzard
(Assignee)

Comment 5

17 years ago
This will go away when I turn on the new embedding widget.
Status: NEW → ASSIGNED

Comment 6

17 years ago
I get the exact same fault with a new pull/build of mozilla against a new build
of galeon. 
Running on RedHat 7.0
gcc 2.96-78
20 March 2001.

When can the new embed component be expected?
In the mean time Ill try the patch
(Assignee)

Comment 7

17 years ago
It's already turned on in the 0.8.1 branch.  I'm waiting until the tip opens to
turn it on there.
(Assignee)

Comment 8

17 years ago
This has been fixed with my rewrite.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.