To make splitting the inner and outer windows easier I went through all external interfaces from nsGlobalWindow, and changed them to use nsGlobalWindow{Inner,Outer} instead of directly using nsGlobalWindow. These are the patches to do this step. The next big step for nsGlobalWindow is to change more internal code to use {Inner,Outer} where possible. This might take the form of moving functions individually to the more specific classes, or it might take the form of a copy-and-paste split, followed by paring down each type to only the functions it needs. I'm planning to do this first, before doing any merging with nsDocShell, as getting the outer window into a distinct type will make it considerably easier to work with.

MozReview-Commit-ID: 5fLoC23MuNE

This is a large patch which tries to switch many of the external consumers of nsGlobalWindow to instead use the new Inner or Outer variants. MozReview-Commit-ID: 99648Lm46T5

These were originally exposed directly as static methods on nsGlobalWindow, but as they are clearly associated with either the inner or outer window, it makes more sense for them to be called as such. MozReview-Commit-ID: LFq8EfnhDlo

MozReview-Commit-ID: LH3lKkFqTgG

MozReview-Commit-ID: 3be0C8OXSKo

MozReview-Commit-ID: KbCpDFoWyTe

MozReview-Commit-ID: Av1OJCBCZ6o

MozReview-Commit-ID: Bi6zvHxWPdw

Not flagging anyone for review until I've confirmed that the stuff isn't horribly broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36a4d3651d87cb184eaf5726a93781c563fdcff3

(In reply to Nika Layzell [:mystor] from comment #9) > Not flagging anyone for review until I've confirmed that the stuff isn't > horribly broken: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=36a4d3651d87cb184eaf5726a93781c563fdcff3 OK - so there are some ASAN leaks which seem to be showing up which I can't reproduce locally - I imagine they're due to me making a mistake WRT wrappers when moving types around. /me is going to request reviews in case the problem is obvious to the reviewer. I think the next major steps after these patches are to: a) Remove references to nsGlobalWindow::Cast from within nsGlobalWindow & delete the functions. b) Duplicate the entire nsGlobalWindow class into two separate nsGlobalWindow{Inner,Outer} objects, and then clean up code until that compiles & runs. c) Remove now-dead code from the specific classes until the two types are cleanly separated. (Step b will of course almost certainly have complications). ============== Context =============== This is a new approach I'm taking on the window splitting. It turns out (**surprise**) that changing the lifetime of nsDocShell (which is necessary to cleanly move all of nsGlobalWindowOuter into it) is going to be very tricky and expose a _lot_ of underlying assumptions in our codebase. I've run into many of these with just trying to move a few small functions into nsDocShell, and trying to synchronize the lifetimes. I think it will be cleaner to first split nsGlobalWindow into two separate classes, which can be done much more quickly & cleanly, and then re-evaluate the task of merging nsGlobalWindowOuter and nsDocShell into the same class. This bug is the first step in that direction. I introduce 2 new subclasses of nsGlobalWindow: nsGlobalWindowInner and nsGlobalWindowOuter. These classes have the exact same layout as nsGlobalWindow, and have very few of their own methods or data. I then changed the construction code and as many of the external consumers of the raw nsGlobalWindow class to use these {Inner,Outer} classes instead of the raw nsGlobalWindow as possible. This opens the way for nsGlobalWindow to be split without having to worry about also changing users of the class from outside.

Comment on attachment 8925706 [details] [diff] [review] Part 1: Add the nsGlobalWindowInner/Outer classes, r=smaug (see comment 10 for context) This patch introduces the new subclasses to nsGlobalWindow, but doesn't change external code to use them yet.

Comment on attachment 8925707 [details] [diff] [review] Part 2: Switch many consumers to nsGlobalWindow{Inner,Outer}, r=smaug (see comment 10 for context) This patch switches the straightforward external consumers of nsGlobalWindow to using the {Inner,Outer} variants. It doesn't change webidl exposed methods or some trickier components such as the memory usage reporter, which are changed in separate patches. If this patch is too large or you don't feel comfortable reviewing some of the changes in components other than DOM, let me know.

Comment on attachment 8925708 [details] [diff] [review] Part 3: Move Get{Inner,Outer}WindowWithId onto the specific subclasses, r=smaug (see comment 10 for context) This patch just switches Get{Inner,Outer}WindowWithId to be static members on their respective subclasses rather than on nsGlobalWindow, as it is one of the main reasons why classes outside of nsGlobalWindow still have references to the class.

Comment on attachment 8925709 [details] [diff] [review] Part 4: Split WindowByIdTable into two separate tables for inner and outer windows, r=bz (see comment 10 for context) With the split of nsGlobalWindow into two separate classes, there is one major datastructure which contains both inner and outer windows: The WindowByIdTable. This patch does the majority of the work to rework this table into 2 separate tables, one for inner and one for outer windows. This patch by itself doesn't build, as it also needs the changes from Part 5 for nsWindowMemoryReporter, which were significant enough that I didn't feel comfortable making them in the same patch.

Comment on attachment 8925710 [details] [diff] [review] Part 5: Rework nsWindowMemoryReporter to only examine inner windows, r=mccr8 (see comment 10 for context) This is the second part of the patch from Part 4. When splitting the WindowByIdTable into 2 different tables, this component was the only consumer which actually tried to handle both inner and outer windows. I talked with you on IRC and you suggested I could probably get around this problem by changing the reporter to only investigate inner windows - this patch is an attempt to do this.

Comment on attachment 8925711 [details] [diff] [review] Part 6: Change WebIDL bindings to refer to nsGlobalWindowInner rather than nsGlobalWindow, r=bz (see comment 10 for context) The last major reason why code referred to nsGlobalWindow instead of nsGlobalWindow{Inner,Outer} was that we still used nsGlobalWindow as the type for the `Window` webidl interface. This patch is an attempt to change this behaviour such that we instead pass in nsGlobalWindowInner when trying to unwrap or wrap objects with the `Window` webidl interface. This required changes to some WebIDL binding code & configuration, as well as a few method signature changes.

Comment on attachment 8925712 [details] [diff] [review] Part 7: Change some consumers within nsGlobalWindow to use the more specific Cast() calls, r=smaug (see comment 10 for context) Now that all of the external consumers of ::Cast have been changed, I wanted to make nsGlobalWindow::Cast private to discourage new code from using it. To do this I had to change some consumers from within nsGlobalWindow.cpp to not use nsGlobalWindow::Cast anymore - this is the patch to do that.

Comment on attachment 8925713 [details] [diff] [review] Part 8: Make nsGlobalWindow::Cast private to discourage usage from external code, r=bz (see comment 10 for context) This is just the patch which makes nsGlobalWindow::Cast private, so that other people don't use it anymore :-).

> OK - so there are some ASAN leaks which seem to be showing up which I can't > reproduce locally - I imagine they're due to me making a mistake WRT > wrappers when moving types around. /me is going to request reviews in case > the problem is obvious to the reviewer. Huh - those actually look like they might be bug 1331159 / bug 1331152, so they might not be my fault? *shrug*

Turns out that actually just removing nsGlobalWindow::Cast alltogether was only a couple more lines of code, so I just did it. MozReview-Commit-ID: DCyyvQ0xKOj

(In reply to Nika Layzell [:mystor] from comment #19) > Huh - those actually look like they might be bug 1331159 / bug 1331152, so > they might not be my fault? *shrug* Hmm, I was going to say it looked different, but the leaks are happening in test_headless_screenshot.html which is in fact the source of those other intermittent leaks, so maybe you'll be okay? That test was disabled recently, so maybe it will be okay. (The basic problem is that the test spawns a new Firefox in a subprocess, and uses some kind of fast exit path that seems to leak.)

(In reply to Andrew McCreight [:mccr8] from comment #21) > Hmm, I was going to say it looked different, but the leaks are happening in > test_headless_screenshot.html which is in fact the source of those other > intermittent leaks, so maybe you'll be okay? That test was disabled > recently, so maybe it will be okay. (The basic problem is that the test > spawns a new Firefox in a subprocess, and uses some kind of fast exit path > that seems to leak.) I also thought the leaks were different - but when I saw that it was happening in _literally the same test_, I figured it was probably at least related. My change might have exacerbated the bug somehow?

How do nsGlobalWindowInner and Outer relate to nsPIDOMWindowInner and Outer? Are we replacing these interfaces or something else?

yeah, I'm a bit lost too here now.

(In reply to Ben Kelly [:bkelly] from comment #23) > How do nsGlobalWindowInner and Outer relate to nsPIDOMWindowInner and Outer? > Are we replacing these interfaces or something else? Basically, currently we have nsGlobalWindow which is the implementation, and nsPIDOMWindowInner/Outer are two interfaces which are implemented by the inner and outer versions of the window respectively. Due to the common implementation class, they are not "real" base classes, but rather nsGlobalWindow inherits from nsPIDOMWindow<nsISupports> which happens to look the same as nsPIDOMWindowInner/Outer, and we just cast the pointers around. This hasn't changed with nsGlobalWindow{Inner,Outer}. These two new classes are subclasses of nsGlobalWindow right now and have no different code than nsGlobalWindow. They currently only exist to try to add inner/outer window information to external consumers of the raw nsGlobalWindow class. In a follow-up bug I will delete nsGlobalWindow and replace it with two separate implementations of nsGlobalWindow{Inner,Outer}. Unlike with nsGlobalWindow today, these implementations will actually directly implement the nsPIDOMWindow{Inner,Outer} interfaces. I'll also then be able to decouple the inner and outer window implementations completely, and we'll be able to use different interfaces for them. TL;DR: No, this doesn't replace nsPIDOMWindow{Inner,Outer}, but rather it's an extension of that divide into the implementation to make a future implementation split possible.

Comment on attachment 8925709 [details] [diff] [review] Part 4: Split WindowByIdTable into two separate tables for inner and outer windows, r=bz >+ // NOTE: We remove ourselves from this table in >+ // nsGlobalWindow::~nsGlobalWindow. We can't do it here because we have to This comment should be in ~nsGlobalWindowOuter/Inner, not in the constructors. >+++ b/dom/media/MediaManager.cpp > nsGlobalWindow* window = iter.Data(); Why not nsGlobalWindowInner here, and skip the AsInner() call? r=me

Comment on attachment 8925711 [details] [diff] [review] Part 6: Change WebIDL bindings to refer to nsGlobalWindowInner rather than nsGlobalWindow, r=bz So my first question is... can we call nsGlobalWindowInner something like "mozilla::dom::Window" and declare it in mozilla/dom/Window.h? And call nsGlobalWindowOuter something like mozilla::dom::WindowProxy; not sure about the header it should live in. It sure would be more readable than continuing to cart along the Inner/Outer bit. Followup ok for this part, if we decide we do want to have the naming like that. > nsGlobalWindow::FirePopupBlockedEvent(nsIDocument* aDoc, >+ // XXX: This is a different object, but webidl requires an inner window here OK. So we should strongly consider changing PopupBlockedEvent and PopupBlockedEventInit to have WindowProxy, not Window. In JS it's all the same thing anyway (and gah, toolkit/content/browser-content.js gets the requestingDocument via ev.requestingWindow.document)... At least this way no one will be under any illusions as to what's being handed around. >+++ b/dom/base/nsJSEnvironment.cpp >+PrintWinURI(nsGlobalWindowInner *win) I suppose, but the code we used to have worked on either sort of window... I can see wanting to debug an outer window like this too. Would be nice to preserve that ability one way or another. >+PrintWinCodebase(nsGlobalWindowInner *win) Similar here. r=me

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #26) > This comment should be in ~nsGlobalWindowOuter/Inner, not in the > constructors. Ok - I don't currently have an explicit destructor for nsGlobalWindow{Inner,Outer} which is why I didn't put it there, but I can add one. > Why not nsGlobalWindowInner here, and skip the AsInner() call? Apparently I missed a place I needed to change while grepping through & rewriting uses of nsGlobalWindow. Thanks! The call to AsInner() is still necessary, as nsGlobalWindowInner doesn't inherit from nsPIDOMWindowInner yet (I cant change that until I finish the split), (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27) > So my first question is... can we call nsGlobalWindowInner something like > "mozilla::dom::Window" and declare it in mozilla/dom/Window.h? And call > nsGlobalWindowOuter something like mozilla::dom::WindowProxy; not sure about > the header it should live in. It sure would be more readable than > continuing to cart along the Inner/Outer bit. I want to do this, but I wanted to get the split rolling without doing a full namespace rewrite. I'll do the namespace move in a follow-up once the split is complete I think. > OK. So we should strongly consider changing PopupBlockedEvent and > PopupBlockedEventInit to have WindowProxy, not Window. In JS it's all the > same thing anyway (and gah, toolkit/content/browser-content.js gets the > requestingDocument via ev.requestingWindow.document)... At least this way > no one will be under any illusions as to what's being handed around. Should this be a follow-up or should I do it in this patch? I'm OK with either. > I suppose, but the code we used to have worked on either sort of window... > I can see wanting to debug an outer window like this too. Would be nice to > preserve that ability one way or another. I could add {Inner,Outer} variants of these functions if that would work better? nsGlobalWindow as a common base is ideally going away. ni? for the questions above ^

> I'll do the namespace move in a follow-up once the split is complete I think. Sounds good. > Should this be a follow-up or should I do it in this patch? Follow-up. > I could add {Inner,Outer} variants of these functions if that would work better? Yeah, I think so. We could probably have a templated thing that works on either type that those functions call to not have too much copy-paste (but keep the individual functions, because calling templated things in a debugger is a bit of a PITA).

Comment on attachment 8925710 [details] [diff] [review] Part 5: Rework nsWindowMemoryReporter to only examine inner windows, r=mccr8 Review of attachment 8925710 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand this window stuff very well, but hopefully this makes sense. Please check in on the ghost window telemetry to make sure nothing too weird happens. I'd expect it to get cut in half, maybe, but not drop to zero.

Pushed by nika@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e71c83eb16f6 Part 1: Add the nsGlobalWindowInner/Outer classes, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6e961b87dc Part 2: Switch many consumers to nsGlobalWindow{Inner,Outer}, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/5b48b5a65a46 Part 3: Move Get{Inner,Outer}WindowWithId onto the specific subclasses, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6ed81bfc67 Part 4: Split WindowByIdTable into two separate tables for inner and outer windows, r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/32a9df9f2b07 Part 5: Rework nsWindowMemoryReporter to only examine inner windows, r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4b951a1f54 Part 6: Change WebIDL bindings to refer to nsGlobalWindowInner rather than nsGlobalWindow, r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/eadac792a9e3 Part 7: Remove nsGlobalWindow::Cast in favor of nsGlobalWindow{Inner,Outer}::Cast, r=smaug

https://hg.mozilla.org/mozilla-central/rev/e71c83eb16f6 https://hg.mozilla.org/mozilla-central/rev/ff6e961b87dc https://hg.mozilla.org/mozilla-central/rev/5b48b5a65a46 https://hg.mozilla.org/mozilla-central/rev/ca6ed81bfc67 https://hg.mozilla.org/mozilla-central/rev/32a9df9f2b07 https://hg.mozilla.org/mozilla-central/rev/cd4b951a1f54 https://hg.mozilla.org/mozilla-central/rev/eadac792a9e3