71275 - GtkMozEmbedChrome can hold stale mContentShell reference

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Kevin Buhr]

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.

Comment 1

[Kevin Buhr]

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.

Comment 2

[Kevin Buhr]

Attachment: patch to make GtkMozEmbedChrome::mContentShell an nsWeakPtr

Comment 3

[Boris Zbarsky [:bzbarsky]]

setting status to new

Comment 4

[Adam Lock]

Reassigning to Chris.

Comment 5

[Christopher Blizzard (:blizzard)]

This will go away when I turn on the new embedding widget.

Comment 6

[Jus]

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

Comment 7

[Christopher Blizzard (:blizzard)]

It's already turned on in the 0.8.1 branch.  I'm waiting until the tip opens to
turn it on there.

Comment 8

[Christopher Blizzard (:blizzard)]

This has been fixed with my rewrite.