2.03 KB, patch
|Details | Diff | Splinter Review|
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?
Assignee: adamlock → danm
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
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.
Severity: major → critical
Summary: unparented windows open incorrectly in (many) embedding apps → embedded window ownership models differ from Mozilla's
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:
Created attachment 35345 [details] [diff] [review] winEmbed adopts the Mozilla style of window ownership
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
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.
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.
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.