1446940 - Replace window...QueryInterface(Ci.nsIDocShell) usage with window.document.docShell

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[Johann Hofmann [:johannh]]

nsIDocument has a docShell attribute that can be used directly instead of doing QueryInterface on the window, which should make our code slightly faster and much easier on the eyes.

QI usage is here:

https://searchfox.org/mozilla-central/search?q=docshell+%3D.*query&case=false&regexp=true&path=.js

https://searchfox.org/mozilla-central/search?q=.QueryInterface%28Ci.nsIDocShell%29&path=

I'm a bit hesitant to try and do this in a find-and-replace rewrite, we might just spin off individual good first bugs.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bug 1463291 is going to make it possible to do window.docShell directly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop getting docshells from windows via getInterface in toolkit

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop getting dochells from windows via getInterface in browser

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop getting dochells from windows via getInterface in devtools

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop getting docshells from windows via getInterface in mobile

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Stop getting docshells from windows via getInterface in dom/editor/etc code

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8996584 [details] [diff] [review]
part 3.  Stop getting dochells from windows via getInterface in devtools

Review of attachment 8996584 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/source.js
@@ +401,5 @@
>        const win = this.threadActor._parent.window;
>        let principal, cacheKey;
>        // On xpcshell, we don't have a window but a Sandbox
>        if (!isWorker && win instanceof Ci.nsIDOMWindow) {
> +        const docShell = win.docShell;

Is this equivalent? It used to only get `Ci.nsIWebNavigation` and not Qi to `Ci.nsIDocShell`.

I see later on we return `this.docShell.QueryInterface(Ci.nsIWebNavigation);` as a `webNavigation` variable, so I wonder if we should be doing so here as well.

::: devtools/shared/DevToolsUtils.js
@@ +545,5 @@
>    }
>  
>    if (aOptions.window) {
>      // Respect private browsing.
> +    channel.loadGroup = aOptions.window.docShell

Ditto - should this Qi to nsIWebNavigation?

::: devtools/shared/gcli/commands/screenshot.js
@@ +372,5 @@
>          contentPolicyType: Ci.nsIContentPolicy.TYPE_INTERNAL_IMAGE
>        });
>        const input = channel.open2();
>  
> +      const loadContext = context.environment.chromeWindow.docShell

Ditto

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Is this equivalent?

Not in general, no.

> It used to only get `Ci.nsIWebNavigation` and not Qi to `Ci.nsIDocShell`.

Yep, and that was actually buggy.  The property it's getting (currentDocumentChannel) is defined on nsIDocShell.  So this code was relying on someone else having QIed to nsIDocShell in script before it gets to run!

> Ditto - should this Qi to nsIWebNavigation?

If we want to preserve all the side-effects of the old behavior, yes.  But the specific code right here in DevToolsUtils.js is not using any nsIWebNavigation bits on the object; it's just QIing to nsIDocumentLoader and getting a property defined on that interface.

> Ditto

Again, this code (in screenshot.js) is not using any nsIWebNavigation bits; just QIing to nsILoadContext.

It's possible there's some code somewhere else that relies on this code having done a past QI to nsIWebNavigation.  All I can say is that try is green...

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8996586 [details] [diff] [review]
part 5.  Stop getting docshells from windows via getInterface in dom/editor/etc code

Review of attachment 8996586 [details] [diff] [review]:
-----------------------------------------------------------------

Much better. Thanks.

::: dom/browser-element/BrowserElementCopyPaste.js
@@ +112,1 @@
>        if(targetDocShell.isMozBrowser) {

Hm. I guess this only worked by luck before, because we had an existing wrapper for the docShell that was already QIed to nsIDocShell?

::: dom/serviceworkers/test/test_devtools_bypass_serviceworker.html
@@ +14,5 @@
>  <script type="text/javascript">
>  "use strict";
>  
>  async function testBypassSW () {
> +  let Ci = SpecialPowers.Ci;

Is this actually needed?

::: dom/tests/mochitest/ajax/offline/offlineTests.js
@@ +310,2 @@
>                                     .QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
>                                     .getInterface(SpecialPowers.Ci.nsILoadContext);

Pretty sure this can just be docShell.QueryInterface(nsILoadContext)?

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1404,5 @@
>      return this._sendSyncMessage("SPPrefService", msg)[0];
>    },
>  
>    _getDocShell(window) {
> +    return window.docShell;

This function seems kind of silly now... but OK.

::: widget/nsITransferable.idl
@@ +114,1 @@
>     *                                  .QueryInterface(Ci.nsILoadContext);

The QueryInterface is actually completely unnecessary for JS callers, but *shrug*.

::: widget/tests/test_bug760802.xul
@@ +27,3 @@
>      .treeOwner
>      .QueryInterface(Ci.nsIInterfaceRequestor)
>      .nsIBaseWindow;

Huh. Apparently this works, and returns a tear-off that only implements nsIBaseWindow.

But the QI to nsIInterfaceRequestor is unnecessary.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I guess this only worked by luck before

Yep.  A lot of that going around....

> Is this actually needed?

Yes, it's used in some of the functions defined inside the "async function testBypassSW".  You can just see a use at the edge of the context as "Ci.nsIChannel.LOAD_BYPASS_SERVICE_WORKER".  It just seemed clearer to me to put the declaration above those functions, not below them.

> Pretty sure this can just be docShell.QueryInterface(nsILoadContext)?

True, though I'm a bit torn about whether we want to admit that or not...  I guess so many other places are doing that, we might as well.  Will fix.

> The QueryInterface is actually completely unnecessary for JS callers, but *shrug*.

Depends on whether they plan to get any properties off the loadcontext, right?  I guess if they are just passing to this API the QI is not needed.

> Huh. Apparently this works, and returns a tear-off that only implements nsIBaseWindow.

Huh, indeed.  Learned something new today.  ;)

I'll take out the extra QI here.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8996582 [details] [diff] [review]
part 1.  Stop getting docshells from windows via getInterface in toolkit

Review of attachment 8996582 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +44,2 @@
>                              .sameTypeRootTreeItem
>                              .QueryInterface(Ci.nsIDocShell);

In 2018, is the difference between the nsIDocShellTreeItem and nsIDocShell itself actually useful anymore? Can we fix up the interface inheritance in a way where this type of QI isn't required? Seems to me that any time that we access any of the *treeItem getters (at least from JS), we eventually want the docshell anyway. If we've got C++ consumers that want to navigate the tree without ever QI'ing to nsIDocShell itself (which would surprise me, but hey) then perhaps they can use non-idl methods or something.

::: toolkit/components/viewsource/content/viewSource-content.js
@@ +187,3 @@
>  
>        try {
> +        let otherDocShell = contentWindow.docShell;

Nit: can we move this out of the try... catch? Pretty sure this can't throw, but maybe I'm missing something.

::: toolkit/components/windowwatcher/test/browser_new_content_window_chromeflags.js
@@ +176,2 @@
>                        .QueryInterface(Ci.nsIWebBrowserChrome)
>                        .chromeFlags;

I find it bizarre that this file alone seems to have 3 different ways to get this information...

::: xpfe/appshell/test/test_hiddenPrivateWindow.xul
@@ +45,5 @@
> +          .hiddenPrivateWindow
> +          .docShell
> +          .QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIDOMWindow)),
> +   "hiddenPrivateWindow should be private");

I'm very confused. Is this just fixing a test that was passing a docshell when it should have been passing a window?

Comment 12

[:Gijs (he/him)]

Comment on attachment 8996583 [details] [diff] [review]
part 2.  Stop getting dochells from windows via getInterface in browser

Review of attachment 8996583 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1241,3 @@
>            .QueryInterface(Ci.nsIInterfaceRequestor)
>            .getInterface(Ci.nsIXULWindow)
>            .XULBrowserWindow = window.XULBrowserWindow;

What... what does this even do? :-\

::: browser/base/content/tabbrowser.js
@@ +657,5 @@
>      // When not using remote browsers, we can take a fast path by getting
>      // directly from the content window to the browser without looping
>      // over all browsers.
>      if (!gMultiProcessBrowser) {
> +      let browser = aWindow.docShell.chromeEventHandler;

Filed bug 1480041 on just checking for CPOWs here instead of for gMultiProcessBrowser...

::: browser/base/content/test/performance/head.js
@@ +61,1 @@
>    docShell.addWeakReflowObserver(observer);

Nit: just combine these lines and don't have the temp var

::: browser/components/customizableui/CustomizableUI.jsm
@@ +1714,5 @@
>            // Err, we're done.
>            break;
>          }
>          // Cue some voodoo
> +        target = target.defaultView.docShell.chromeEventHandler;

Nit: can you update the comment above this to be something more like:
// find containing browser or iframe element in the parent doc

::: browser/components/downloads/DownloadsTaskbar.jsm
@@ +126,2 @@
>                            .QueryInterface(Ci.nsIInterfaceRequestor)
>                            .getInterface(Ci.nsIXULWindow).docShell;

isn't this just aWindow.docShell.rootTreeItem.QI(nsIDocShell) ?

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #12)
> Comment on attachment 8996583 [details] [diff] [review]
> part 2.  Stop getting dochells from windows via getInterface in browser

Oh, late nit - please fix the (Freudian, I'm sure!) typo in the commit message - "dochell" is apt, but will make this hard to find when grep'ing through commit logs and so on... :-)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> In 2018, is the difference between the nsIDocShellTreeItem and nsIDocShell itself actually useful anymore?

Well, there are nsIDocShellTreeItems that are not nsIDocShell.  Specifically nsWebBrowser.  So nsIDocShell inherits from nsIDocShellTreeItem, but going the other way you need to QI.

We can look into changing that, of course...  It's not entirely clear to me whether nsWebBrowser ever really participates in the docshell treeitem tree.  For example, I'm pretty sure that .sameTypeRootTreeItem never returns an nsWebBrowser.

> Nit: can we move this out of the try... catch?

Just the .docShell get?  Sure, done.  I think .currentDescriptor can still throw.

> I find it bizarre that this file alone seems to have 3 different ways to get this information...

Yeah, this is a little silly.  nsIXULWindow and nsIWebBrowserChrome both have a .chromeFlags.  nsContentTreeOwner implements nsIWebBrowserChrome and forwards the access to mXULWindow.  nsChromeTreeOwner does NOT implement nsIWebBrowserChrome.  

So if you have a chrome docshell, you can't get the chromeflags off the treeowner; you have to find the xulwindow.

And if you have a content docshell, then if it's in-process you have an nsContentTreeOwner and can get the flags  directly from it (though getting the nsIXULWindow from it would work too).  If it's out-of-process, then I am pretty sure the treeowner is nsDocShellTreeOwner which also does not implement nsIWebBrowserChrome.

That all said, there's a way right now that will always work for all the cases: get the treeowner, getInterface nsIWebBrowserChrome from it, and get the flags.  Though in the tabchild case this is a bit of a mess because while TabChild itself implements nsIWebBrowserChrome and nsIWebBrowserChrome2, the nsIWebBrowserChrome3 implementation is delegated to its mWebBrowserChrome.  Why that and not the other interfaces, dunno...

This is all such a mess.  :(  We should really clean it up.

> Is this just fixing a test that was passing a docshell when it should have been passing a window?

Yes, exactly.  The callee expects a window; this test of the API was the only thing that passed docshells.  I considered changing the callee to expect a window-or-docshell but this seemed better.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'll add something to the commit message about the test fix.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> What... what does this even do? :-\

It calls nsXULWindow::SetXULBrowserWindow.  Various C++ code then looks for the nsIXULBrowserWindow from there.  And nsXULWindow uses it for GetTabCount().

> Filed bug 1480041 on just checking for CPOWs here instead of for gMultiProcessBrowser...

Thanks!

> Nit: just combine these lines and don't have the temp var

Done.

> Nit: can you update the comment above this to be something more like:

Done.

> isn't this just aWindow.docShell.rootTreeItem.QI(nsIDocShell) ?

No.  In the in-process content-docshell case, rootTreeItem is the root content docshell while this code is getting the root chrome docshell.

In the chrome-docshell case, I agree that they would be equivalent right now, given we only have one chrome docshell tree per window.

I don't know which sorts of windows come through here.

> please fix the (Freudian, I'm sure!) typo in the commit message

Hah!  Fixed.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8996582 [details] [diff] [review]
part 1.  Stop getting docshells from windows via getInterface in toolkit

Review of attachment 8996582 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpfe/appshell/test/test_hiddenPrivateWindow.xul
@@ +44,5 @@
> +  Services.appShell
> +          .hiddenPrivateWindow
> +          .docShell
> +          .QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIDOMWindow)),

(these should both be .docShell.domWindow now...)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe5a0e5be75
part 1.  Stop getting docshells from windows via getInterface in toolkit.  r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/956c6eeca873
part 2.  Stop getting docshells from windows via getInterface in browser.  r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f6f26864b7
part 3.  Stop getting docshells from windows via getInterface in devtools.  r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a747a896fc
part 4.  Stop getting docshells from windows via getInterface in mobile.  r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1489be7f57
part 5.  Stop getting docshells from windows via getInterface in dom/editor/etc code.  r=kmag

Comment 19

[Boris Zbarsky [:bzbarsky]]

> (these should both be .docShell.domWindow now...)

Oops, just saw this after I pushed.  Will do a followup.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9c03e7b60e
followup to fix review nit.  r=bzbarsky

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d563f77c20
followup to fix test breakage.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb10f4b8765
_another_ followup to fix test breakage from review comments. r=bzbarsky

Comment 23

[Kris Maglione [:kmag]]

For one reason or another, this (or bug 1479492) seems to have shaved 4-5K off of the base content JS numbers.

Comment 24

[Boris Zbarsky [:bzbarsky]]

This bug is more likely to have done it.

We _might_ be creating fewer XPCWrappedNativeTearOffs, maybe, if we're QIing stuff to fewer interfaces?  Maybe.  Hard to tell...

Comment 25

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #24)
> We _might_ be creating fewer XPCWrappedNativeTearOffs, maybe, if we're QIing
> stuff to fewer interfaces?  Maybe.  Hard to tell...

That'd be my guess, yeah. There's probably a measurable decrease in script sizes, too, though, since the new code is a lot simpler. And we're probably creating fewer scriptable IID wrappers in some compartments now, too.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Nit: just combine these lines and don't have the temp var

OK.   I am undoing this change because we use the var for the removeWeakReflowObserver call too, and we can't just reget it from "win" at that point because the test might have closed it by then.  And some tests do just that, leading to orange.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96504aefa81f
hopefully last-ever followup to fix test orange.  r=bzbarsky

Comment 28

[Jorg K (CEST = GMT+2)]

We should port some of this, right? We have some five call sites. Three look easy but the one in editingOverlay.js looks more complicated:
https://dxr.mozilla.org/comm-central/search?q=QueryInterface(Ci.nsIDocShell)&redirect=false

I'll file a bug and fix the first three.

Comment 29

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+2) from comment #28)
> We should port some of this, right? We have some five call sites. Three look
> easy but the one in editingOverlay.js looks more complicated:
> https://dxr.mozilla.org/comm-central/search?q=QueryInterface(Ci.
> nsIDocShell)&redirect=false

I'd expect some of the getInterface calls to eventually stop working, so it's probably better for you to port it sooner rather than later, yes.

Comment 30

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #26)
> > Nit: just combine these lines and don't have the temp var
> 
> OK.   I am undoing this change because we use the var for the
> removeWeakReflowObserver call too, and we can't just reget it from "win" at
> that point because the test might have closed it by then.  And some tests do
> just that, leading to orange.

Oh, ugh, sorry. I thought I checked and the variable was unused - I clearly missed the removeWeakReflowObserver call. :-(

Comment 31

[Boris Zbarsky [:bzbarsky]]

> We should port some of this, right?

Yes.  It's not super-urgent yet, but eventually (weeks?  months?) I would like to make some of these getInterface bits not work anymore, and make QI on window not work either.

You don't need to worry about QueryInterface(nsIDocShell) per se.  It's the getInterface calls to watch out for.

Comment 32

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/fbe5a0e5be75
https://hg.mozilla.org/mozilla-central/rev/956c6eeca873
https://hg.mozilla.org/mozilla-central/rev/d0f6f26864b7
https://hg.mozilla.org/mozilla-central/rev/f5a747a896fc
https://hg.mozilla.org/mozilla-central/rev/3b1489be7f57
https://hg.mozilla.org/mozilla-central/rev/ad9c03e7b60e
https://hg.mozilla.org/mozilla-central/rev/e0d563f77c20
https://hg.mozilla.org/mozilla-central/rev/9fb10f4b8765
https://hg.mozilla.org/mozilla-central/rev/96504aefa81f