76522 - embedded window ownership models differ from Mozilla's

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Dan M]

Different platforms' WindowCreator::CreateChromeWindow have different ownership 
models on the returned nsIWebBrowserChrome object. In Mozilla, the 
nsIWebBrowserChrome object is an nsContentTreeOwner; an object owned by the 
nsXULWindow. The window is a strong owner, so it returns from creation with a 
refcount of 2 (one from the factory; one from the strong owner).
  winEmbed (as do all the embedding apps, I'm pretty sure) implements the 
nsIWebBrowserChrome as a top-level window; no one owns it. It returns from 
creation with a refcount of 1 from the factory. In WindowWatcher the two paths 
merge, and the newly created window is either released prematurely or leaked, 
depending on which ownership model WindowWatcher assumes.
  What am I supposed to say now that isn't all cursing?

Comment 1

[Dan M]

.

Comment 2

[Brendan Eich [:brendan]]

I know some good curse-words.

We really do not want to recapitulate the XFE/MacFE/WinFE/etc. mess of the
classic codebase.  Yet that's what all these embedding apps with their varying
rules and usages seem to be doing.

/be

Comment 3

[Peter Trudelle]

IMO, we need to document and support exactly *one* way to use this and other 
APIs.  Otherwise, one-off differences will kill us.  Adding crash keyword, 
->critical based on triage with Daniel yesterday.

Comment 4

[Dan M]

  Yes, since the Right Thing is clearly open to interpretation, this is on my 
list of things to get into the docs. Oddly, it only seems to be a problem on 
winEmbed. mfcEmbed already treats the newly created chrome object's refcount 
correctly, and the gtk and PP equivalents don't open a window at all under the 
circumstances where this problem first showed up (go to http://www.yahoo.com.jp 
(once bug 69121 was fixed)). So they may still have problems, but if so, it's not 
easily discoverable.

  The heart of the problem is different assumptions about refcounting in Mozilla 
and winEmbed. A typical usage pattern in Mozilla is

nsCOMPtr<nsIWebBrowserChrome> newWin;
someService->MakeNewWindow(..., getter_AddRefs(newWin))

(approximately. As noted in the original comment, that interface is actually a 
contained object in Mozilla; not a top-level window.) This pattern is rightfully 
confusing, because the reference returned by MakeNewWindow is removed when newWin 
goes out of scope. The window had better be self-referential, or it will be 
quickly destroyed. (And note this is what WindowWatcher does, as does most 
Mozilla code, which was causing the window's premature destruction and a crash.)

A typical usage pattern in winEmbed has been

nsIWebBrowserChrome *newWin;
someService->MakeNewWindow(..., &newWin)

making the implicit assumption that the reference returned by the service is the 
owning reference, will be removed by some cleanup method as the window is being 
closed, and should therefore be allowed to float loose after creation.

Personally I think the winEmbed interpretation makes more sense; it's certainly 
easy to see how someone could ignorantly make that assumption. However, it's less 
robust. The Mozilla interpretation works if the object happens to be a contained 
member with a lifetime of its own, as well as if the object is uncontained.

The moral is, winEmbed must adopt the Mozilla style of ownership, and this bug is 
fairly easily fixed like this:

Comment 5

[Dan M]

Attachment: winEmbed adopts the Mozilla style of window ownership

Comment 6

[Conrad Carlen (not reading bugmail)]

Since in winEmbed the chrome is the top-level window object, the extra addref
would be the only way out of this. FWIW, in PPEmbed, the chrome is owned by the
window object so it uses the Mozilla ownership model. r=ccarlen

Comment 7

[Adam Lock]

r=adamlock

I think there is some confusion surrounding ownership of windows, but as long as 
someone, somewhere hangs onto a reference I don't see that either mechanism is 
wrong. In this instance the code is all in the winEmbed client so how the 
reference is discarded doesn't matter that much. I don't mind the change the 
smart pointers because it's perhaps a bit tidier, but it may confuse people to 
see the pointer go out of scope and the window live-on because someone like 
window watcher is still hanging onto a ref.

Comment 8

[Dan M]

Actually I've come to like this ownership model better. It puts the window's 
lifetime in the control of the window, rather than throwing it on the mercy of 
its caller. That is, carefully constructed ownership code (which my patch isn't) 
can have explicit methods like "Take/Release-OwnershipOfYourself" and use them in 
appropriate places to make it obvious what you're doing. I think that's more 
documentable and more easily understood, or at least has the potential to be.

Note that I mistyped the test URL above. It should be http://www.yahoo.co.jp/. 
Currently nsFontPackageHandler::NeedFontPackage opens a parentless window 
(exercising WindowCreator and this bug). And as far as I can tell, this happens 
only on Windows, explaining why this problem is visible only on Windows.

Well alrighty then. I'm still not sure whether this is a problem in gtkEmbed 
(Conrad tells me the PPEmbed ownership model is already the "correct" one) but at 
least we know why the crash is happening only on Windows. And it's fixed on 
Windows. Closing.