Closed Bug 862918 Opened 11 years ago Closed 11 years ago

nsGlobalModalWindow should only be created for actual modal content windows


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: bholley, Assigned: bholley)




(4 files)

Right now, we compute whether to use nsGlobalModalWindow based on some very fuzzy heuristics, which cause us to create them for various things that aren't actually modal content windows. This is a problem, because nsGlobalModalWindow outers skip various cleanup steps under the assumption that it will be taken care of by the calling ShowModalDialog. So while the outer for the actual modal dialog ends up getting cleaned up, all of the other ones just assert/leak.

This is part of my epic quest to make modal dialogs sane. Patches coming.
Currently, the parent browser.xul docshell ends up getting created as an
nsGlobalModalWindow, because we pass nsIWebBrowserChrome::CHROME_OPENAS_CHROME
from nsWindowWatcher when creating the initial container window. The other
plausible thing to do here would be to add CHROME_OPENAS_CHROME to the call to
CreateTopLevelWindow in nsXULWindow::CreateNewContentWindow, since it's very
clear there that we're just creating the containing browser.xul docshell and not
creating the actual docshell of interest to the caller. But that seems more likely
to have unintended consequences, so we fix it here.
Attachment #739064 - Flags: review?(bzbarsky)
instanceof is behaving sketchily on modal window, and I want this test to be robust.
Attachment #739066 - Flags: review?(bzbarsky)
These are pretty basic for now, but I'm going to be adding to them in the other bugs
where I munge this stuff.
Attachment #739067 - Flags: review?(bzbarsky)
Comment on attachment 739066 [details] [diff] [review]
Part 3 - Add a utility method on Cu to get the class name of an object. v1

Review of attachment 739066 [details] [diff] [review]:

::: js/xpconnect/idl/xpccomponents.idl
@@ +412,5 @@
> +     * specifically trying to detect whether the object is a proxy, this is
> +     * probably what you want.
> +     */
> +    [implicit_jscontext]
> +    string getClassName(in jsval aObj, in bool aUnwrap);

Why not ACString?
(In reply to :Ms2ger from comment #6)
> Why not ACString?

because using a char ** makes it really easy to just strdup clasp->name. It's all the same to xpconnect, anyway.
Comment on attachment 739064 [details] [diff] [review]
Part 1 - Only pass aIsModalContentWindow if we're actually a content docshell. v1

Attachment #739064 - Flags: review?(bzbarsky) → review+
Comment on attachment 739065 [details] [diff] [review]
Part 2 - Only create nsGlobalModalWindow for the primary content shell. v1

Attachment #739065 - Flags: review?(bzbarsky) → review+
Comment on attachment 739066 [details] [diff] [review]
Part 3 - Add a utility method on Cu to get the class name of an object. v1

r=me though if we really cared we'd make the unwrap arg optional, ask for an optional_argc, and default it to true if not passed...
Attachment #739066 - Flags: review?(bzbarsky) → review+
Comment on attachment 739067 [details] [diff] [review]
Part 4 - Tests for showModalDialog. v2

Attachment #739067 - Flags: review?(bzbarsky) → review+
There was one orange of the try push, which was the result of me adding test coverage that triggered an assertion without blaming+gcing. I've added SpecialPowers.expectAssertions() as well as a gc() at the end to make sure that the assertion gets blamed to the test. I'll be removing the assertion entirely in bug 860941.

Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.