862918 - nsGlobalModalWindow should only be created for actual modal content windows

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0b7992cf8677

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Only pass aIsModalContentWindow if we're actually a content docshell. v1

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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Only create nsGlobalModalWindow for the primary content shell. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a utility method on Cu to get the class name of an object. v1

instanceof is behaving sketchily on modal window, and I want this test to be robust.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests for showModalDialog. v2

These are pretty basic for now, but I'm going to be adding to them in the other bugs
where I munge this stuff.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 7

[Bobby Holley (:bholley)]

(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 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739064 [details] [diff] [review]
Part 1 - Only pass aIsModalContentWindow if we're actually a content docshell. v1

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739065 [details] [diff] [review]
Part 2 - Only create nsGlobalModalWindow for the primary content shell. v1

r=me

Comment 10

[Boris Zbarsky [: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

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...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739067 [details] [diff] [review]
Part 4 - Tests for showModalDialog. v2

r=me

Comment 12

[Bobby Holley (:bholley)]

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.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18bd27173b5d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1728f06727
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1093cca56b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80773d7faa38

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Follow-up to disable the test on mobile.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4083aca79d32

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18bd27173b5d
https://hg.mozilla.org/mozilla-central/rev/4b1728f06727
https://hg.mozilla.org/mozilla-central/rev/8d1093cca56b
https://hg.mozilla.org/mozilla-central/rev/80773d7faa38
https://hg.mozilla.org/mozilla-central/rev/4083aca79d32