1463016 - Remove nsIDOMWindow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Kris Maglione [:kmag]]

This is an empty stub interface that's currently only used in any useful way for getInterface calls.

Getting rid of it is a bit tricky, since JS consumers currently pass it to getInterface, and can't pass nsPIDOMWindowInner/nsPIDOMWindowOuter. I'm not sure what the best solution here is, but I'm thinking it would be nice to pass a Window constructor and convert that to nsPIDOMWindowInner internally.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 1 - Remove unnecessary JS QueryInterface(nsIDOMWindow) calls.

Review commit: https://reviewboard.mozilla.org/r/245390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245390/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 2 - Allow importing Window constructor in system scopes.

Review commit: https://reviewboard.mozilla.org/r/245392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245392/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 3 - Remove (mostly unnecessary) instanceof nsIDOMWindow checks.

Most of these checks are there for the side-effect of calling QueryInterface
on the object, and making sure it's actually queried to nsIDOMWindow. That's
never necessary for WebIDL objects, so those can easily be removed.

A few of the others seemed over-cautious, so I removed those as well. For
instance, if someone dispatches a domwindowopened observer notification with a
non-Window object, it's better that we fail loudly when we try to use it as a
window, rather than return silently.

The rest actually seem to have some purpose, so I switched most of them to use
Window.isInstance.

Review commit: https://reviewboard.mozilla.org/r/245394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245394/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 4 - Update XPIDL interfaces to use nsPIDOMWindow(Inner|Outer) rather than nsIDOMWindow.

Most of these seemed to want to deal with inner windows, so I changed those to
use Window. A few were definitely dealing with outer windows, though, so I
changed those to use WindowProxy.

The device sensors API was a weird case. It only ever added listeners as inner
windows, but the nsGlobalWindowOuter destructor had a copy of the inner window
destructor's code to remove them. Since outer windows are guaranteed never to
be added now, I removed that code.

Review commit: https://reviewboard.mozilla.org/r/245396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245396/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it.

Review commit: https://reviewboard.mozilla.org/r/245398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245398/

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID.

There are still a few places where it's going to be hard to remove
getInterface(nsIDOMWindow) calls. Since native callers should not be using
XPCOM interfaces when dealing with DOM Windows and we ideally want to stop
implementing any stub interfaces on DOM objects, that makes things difficult.

This workaround allows us to pass DOM constructors, in particular Window
objects, in place of IIDs, and automatically convert them to native IIDs for
QueryInterface purposes.

I took the simple approach here, and hard coded the handling of the Window
prototype. That's the general approach a lot of the rest of XPCConvert takes,
and I don't immediately forsee us needing to add other interfaces in the
future, so it seems sufficient.

Review commit: https://reviewboard.mozilla.org/r/245400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245400/

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

Review commit: https://reviewboard.mozilla.org/r/245402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245402/

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 8 - Remove remaining C++ uses of nsIDOMWindow.

Review commit: https://reviewboard.mozilla.org/r/245404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245404/

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1463016: Part 9 - Remove nsIDOMWindow.

Review commit: https://reviewboard.mozilla.org/r/245406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245406/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8979146 [details]
Bug 1463016: Part 2 - Allow importing Window constructor in system scopes.

https://reviewboard.mozilla.org/r/245392/#review251548

::: commit-message-cc786:1
(Diff revision 1)
> +Bug 1463016: Part 2 - Allow importing Window constructor in system scopes. r?bz

s/constructor/interface object/, because it's not a constructor.

::: dom/base/nsGlobalWindowInner.cpp:3080
(Diff revision 1)
>  
>  /* static */ bool
>  nsGlobalWindowInner::IsPrivilegedChromeWindow(JSContext* aCx, JSObject* aObj)
>  {
>    // For now, have to deal with XPConnect objects here.
> -  return xpc::WindowOrNull(aObj)->IsChromeWindow() &&
> +  auto* win = xpc::WindowOrNull(aObj);

It's worth documenting when win might be null: when we are setting up a Window.prototype in a non-window global, right?

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8979146 [details]
Bug 1463016: Part 2 - Allow importing Window constructor in system scopes.

https://reviewboard.mozilla.org/r/245392/#review251548

> It's worth documenting when win might be null: when we are setting up a Window.prototype in a non-window global, right?

Yup. Will add a comment.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8979149 [details]
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it.

https://reviewboard.mozilla.org/r/245398/#review251566

It might make sense to just add a chromeonly .docShell on windows instead of needing to go through document...  Conceptually the docshell is attached to the window, not the document, anyway.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> Comment on attachment 8979149 [details]
> Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update
> callers to use it.
> 
> https://reviewboard.mozilla.org/r/245398/#review251566
> 
> It might make sense to just add a chromeonly .docShell on windows instead of
> needing to go through document...  Conceptually the docshell is attached to
> the window, not the document, anyway.

Agree, this felt weird when I was updating the existing callers. But that felt like a separate bug. Happy to do it in this patch if you prefer, though.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8979150 [details]
Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID.

https://reviewboard.mozilla.org/r/245400/#review251580

::: commit-message-2d6bb:1
(Diff revision 1)
> +Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID. r?bz

"to pass Window constructors", right?

The other option would be to leave the nsIDOMWindow interface and just make the relevant GetInterface implementations return whatever they want there...  I guess that would be an issue if people getInterface in C++, though.

I'm probably OK with this if we file a followup to remove things that do getInterface(nsIDOMWindow) and then to remove this code.

::: js/xpconnect/src/XPCJSID.cpp:805
(Diff revision 1)
> +    }
>      return nullptr;
>  }
>  
>  bool
>  xpc_JSObjectIsID(JSContext* cx, JSObject* obj)

Might be worth a comment about why we're not checking for Window here.

Comment 15

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> The other option would be to leave the nsIDOMWindow interface and just make
> the relevant GetInterface implementations return whatever they want there...
> I guess that would be an issue if people getInterface in C++, though.

Yeah, I thought about that, but it made me nervous. C++ callers are definitely a concern. A bunch of C++ users have crept back in since khuey removed most of them, and it's not clear which of those are dealing with inner or outer windows.

The only semi-reasonable option I could think of was to return a tear-off that returned the window object when QIed to nsWrapperCache. I think that would probably be sane enough, but it requires sprinkling a bunch of nsIDOMWindow references in all of the notificationCallback objects that return DOM windows, which I didn't like. It also adds to the confusion on the JS side, where we already have a bunch of cargo culted code that tries to QI things to nsIDOMWindow when not necessary. It would be nice if JS and XPIDL code never had to care whether Windows were nsIThings at all, and just reference them by their WebIDL types.

> I'm probably OK with this if we file a followup to remove things that do
> getInterface(nsIDOMWindow) and then to remove this code.

Agreed. I'll file a follow-up. I'm still thinking about options for the notificationCallbacks uses.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8979151 [details]
Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

https://reviewboard.mozilla.org/r/245402/#review251586

::: toolkit/mozapps/update/tests/unit_aus_update/uiAutoPref.js:16
(Diff revision 1)
>  };
>  
>  const WindowMediator = {
>    getMostRecentWindow(aWindowType) {
>      executeSoon(check_status);
> -    return { getInterface: ChromeUtils.generateQI([Ci.nsIDOMWindow]) };
> +    return { getInterface: ChromeUtils.generateQI([]) };

Hmm.  How does this work when someone does a getInterface(Window) on this thing?  Or does no one do that?  In which case, why do we need a getInterface at all?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Agree, this felt weird when I was updating the existing callers. But that felt
> like a separate bug. Happy to do it in this patch if you prefer, though.

Separate bug is fine.

> Agreed. I'll file a follow-up.

Sounds great.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8979151 [details]
Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

https://reviewboard.mozilla.org/r/245402/#review251586

> Hmm.  How does this work when someone does a getInterface(Window) on this thing?  Or does no one do that?  In which case, why do we need a getInterface at all?

It doesn't. I fixed the few callers that were calling getInterface(nsIDOMWindow) on things that were already windows. I'll double-check where exactly this gets used and see if the getInterface method can be removed entirely.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979145 [details]
Bug 1463016: Part 1 - Remove unnecessary JS QueryInterface(nsIDOMWindow) calls.

https://reviewboard.mozilla.org/r/245390/#review264280

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979147 [details]
Bug 1463016: Part 3 - Remove (mostly unnecessary) instanceof nsIDOMWindow checks.

https://reviewboard.mozilla.org/r/245394/#review264282

::: browser/base/content/test/general/browser_tabfocus.js:75
(Diff revision 1)
>    function eventListener(event) {
>      // Stop the shim code from seeing this event process.
>      event.stopImmediatePropagation();
>  
>      var id;
> -    if (event.target instanceof Ci.nsIDOMWindow)
> +    if (Window.isInstance(event.target))

IIRC we should be able to also do `event.target instanceof Window` because we define `[Symbol.hasInstance]` on the `Window` object.

::: browser/components/downloads/DownloadsCommon.jsm:59
(Diff revision 1)
>      prefix: "Downloads"
>    };
>    return new ConsoleAPI(consoleOptions);
>  });
>  
> +Cu.importGlobalProperties(["Window"]);

I presume support for 'Window' in importGlobalProperties was added in part 2 :-)

::: browser/components/feeds/WebContentConverter.js:368
(Diff revision 1)
> -    let haveWindow = (aBrowserOrWindow instanceof Ci.nsIDOMWindow);
> +    let haveWindow = !(aBrowserOrWindow instanceof Ci.nsIBrowser);
>      let uri;
>      if (haveWindow) {
>        uri = Utils.checkAndGetURI(aURIString, aBrowserOrWindow);
>      } else {
>        // aURIString must not be a relative URI.
>        uri = Utils.makeURI(aURIString, null);
>      }

Can we flip this if statement?

::: browser/components/feeds/WebContentConverter.js:457
(Diff revision 1)
>      let haveWindow = aWindowOrBrowser &&
> -                     (aWindowOrBrowser instanceof Ci.nsIDOMWindow);
> +                     !(aWindowOrBrowser instanceof Ci.nsIBrowser);
>      let uri;
>      if (haveWindow) {
>        uri = Utils.checkAndGetURI(aURIString, aWindowOrBrowser);
>      } else if (aWindowOrBrowser) {
>        // uri was vetted in the content process.
>        uri = Utils.makeURI(aURIString, null);
>      }

Same here

::: toolkit/modules/addons/WebRequestContent.js:141
(Diff revision 1)
>        node = node.contentWindow;
>      }
>  
>      if (node) {
>        let window;
> -      if (node instanceof Ci.nsIDOMWindow) {
> +      if (node.Window && node instanceof node.Window) {

'Window' isn't exposed in this context I suppose? It might be clearer to import the Window interface object.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979148 [details]
Bug 1463016: Part 4 - Update XPIDL interfaces to use nsPIDOMWindow(Inner|Outer) rather than nsIDOMWindow.

https://reviewboard.mozilla.org/r/245396/#review264286

rs=me, though I think you'll need to change some of this due to my request to use 'nsGlobalWindowInner' instead of 'nsPIDOMWindowInner' in xpidl to make it match WebIDL.

::: dom/commandhandler/nsPICommandUpdater.idl:28
(Diff revision 1)
>     
>    /*
>     * Notify the command manager that the status of a command
>     * changed. It may have changed from enabled to disabled,
>     * or vice versa, or become toggled etc.
>     */
>  	void		commandStatusChanged(in string aCommandName);
>  	

Can we fix this trailing whitespace?

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979149 [details]
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it.

https://reviewboard.mozilla.org/r/245398/#review264288

rs=me - I scanned through a few files & scrolled through to make sure nothing seemed super weird.

::: browser/components/feeds/WebContentConverter.js:497
(Diff revision 1)
> -                         .getInterface(Ci.nsIWebNavigation)
> -                         .QueryInterface(Ci.nsIDocShellTreeItem)
> -                         .rootTreeItem
> -                         .QueryInterface(Ci.nsIInterfaceRequestor)
> -                         .getInterface(Ci.nsIDOMWindow)
>                           .wrappedJSObject;

Why are you accessing 'wrappedJSObject' on the domWindow object here?

I'm not sure what the point of that is.

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979152 [details]
Bug 1463016: Part 8 - Remove remaining C++ uses of nsIDOMWindow.

https://reviewboard.mozilla.org/r/245404/#review264292

::: dom/base/nsGlobalWindowInner.cpp:366
(Diff revision 1)
>    }
>    void Forget() { mWindow = nullptr; }
>    NS_IMETHOD GetInterface(const nsIID& aIID, void** aResult) override
>    {
>      if (mWindow) {
> -      if (aIID.Equals(NS_GET_IID(nsIDOMWindow))) {
> +      if (aIID.Equals(NS_GET_IID(nsIDOMWindow)) ||

Shouldn't we be removing this usage here?

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979153 [details]
Bug 1463016: Part 9 - Remove nsIDOMWindow.

https://reviewboard.mozilla.org/r/245406/#review264294

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersExtension.html:85
(Diff revision 1)
> -  ok(SpecialPowers.do_QueryInterface(window, "nsIDOMWindow"));
> -  is(SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(window, "nsIDOMWindow"), "document.nodeName"), "#document");
> +  ok(SpecialPowers.do_QueryInterface(window, "nsISupports"));
> +  is(SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(window, "nsISupports"), "document.nodeName"), "#document");

This feels like a slightly unnecessary test now...

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979150 [details]
Bug 1463016: Part 6 - Allow JS callers to Window constructors as IID.

https://reviewboard.mozilla.org/r/245400/#review264290

::: commit-message-2d6bb:12
(Diff revision 1)
> +I took the simple approach here, and hard coded the handling of the Window
> +prototype. That's the general approach a lot of the rest of XPCConvert takes,
> +and I don't immediately forsee us needing to add other interfaces in the
> +future, so it seems sufficient.

Only handling one type is reasonable here, but it seems like putting this capability inside of XPConnect rather than in JS might be overly complex  :-/.

::: js/xpconnect/src/XPCJSID.cpp:796
(Diff revision 1)
> +    if (dom::IsDOMConstructor(obj)) {
> +        const js::Class* clasp = js::GetObjectClass(obj);
> +        auto iface = dom::DOMIfaceAndProtoJSClass::FromJSClass(clasp);
> +        switch (iface->mPrototypeID) {
> +        case dom::prototypes::id::Window:
> +            return &NS_GET_IID(nsPIDOMWindowOuter);

Why are we getting the outer window's IID when we get a 'Window' object's prototype?

It also might be an option to just define a 

    const INNER_WINDOW_IID = Components.ID("{775dabc9-8f43-4277-9adb-f1990d77cffb}")

in XPCOMUtils.jsm?

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8979151 [details]
Bug 1463016: Part 7 - Update remaining getInterface(nsIDOMWindow) callers to use Window.

https://reviewboard.mozilla.org/r/245402/#review264296

::: dom/base/nsGlobalWindowInner.cpp:369
(Diff revision 1)
> +      if (aIID.Equals(NS_GET_IID(nsPIDOMWindowOuter))) {
> +        return mWindow->GetOuterWindow()->QueryInterface(aIID, aResult);
> +      }
> +    }

I think this should be a nsPIDOMWindowInner query, and that we should use nsPIDOMWindowInner's IID in the previous XPCJSID patch.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> IIRC we should be able to also do `event.target instanceof Window` because we define `[Symbol.hasInstance]` on the `Window` object.

We want to stop defining that symbol, in the long run.

Comment 28

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> > IIRC we should be able to also do `event.target instanceof Window` because we define `[Symbol.hasInstance]` on the `Window` object.
> 
> We want to stop defining that symbol, in the long run.

Even if we didn't, I'd be worried about using instanceof checks here. Even with the `hasInstance` hook, I'd expect it to touch Window.prototype, which we really want to lazify (bug 1475700). Window.isInstance shouldn't have that problem.

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d69b4fb1ed43751cfcbc0b4f2fe3b6a49bc0494
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it. r=nika

Comment 30

[Narcis Beleuzu [:NarcisB]]

Backed out for geckoview failures.

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fcfb99baa0f0fb60a7c420a712c6ae7c72576871
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/aaee68beff800bcf076d9c49b5c1f6531b135579
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=190748974&repo=mozilla-inbound&lineNumber=2050

Comment 31

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60c1c783f5e100e1e43b09be2f11b4d1d0b82bdf
Bug 1463016: Part 5 - Add domWindow property to DocShellTreeItem and update callers to use it. r=nika

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/60c1c783f5e1

Comment 33

[Jorg K (CEST = GMT+2)]

Any plans to land the other parts? As far as I can see, only part 5 landed so far.

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment 33 looks still valid and we still have nsIDOMWindow in tree. Should we reopen this?