1085537 - Make it possible to get to remote browser from content window CPOW

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Given a content window CPOW in the parent process, it'd be extremely handy to be able to resolve that content window CPOW to the remote browser that the content window is ultimately contained within.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] Make it possible to get to remote browser from content window CPOW. r=?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Make it possible to get to remote browser from content window CPOW. r=?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Add tests

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8508781 [details] [diff] [review]
Make it possible to get to remote browser from content window CPOW. r=?

I'm not really jazzed about adding a whole new JSM[1] just to hold this WeakMap, but I couldn't think of a better place to put it. Suggestions welcome.

Alternatively, we could file a bug to fold a bunch of the Remote stuff into RemoteUtils - like RemoteWebProgress, RemoteFinder, RemoteWebNavigation.

[1]: Especially because of https://groups.google.com/forum/#!topic/mozilla.dev.platform/sAuhWQOAWic

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8508782 [details] [diff] [review]
Add tests

It's a shame that something like my loadPage function isn't available somewhere in the test framework, as I find myself writing it all over the place for various mochitest-browser tests. Is there a better place we could put it?

Comment 6

[Dave Townsend [:mossop]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> Comment on attachment 8508782 [details] [diff] [review]
> Add tests
> 
> It's a shame that something like my loadPage function isn't available
> somewhere in the test framework, as I find myself writing it all over the
> place for various mochitest-browser tests. Is there a better place we could
> put it?

We load a bunch of files into the test scope. I think it's time to add a new source file where we can put shared functions like these.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8508782 [details] [diff] [review]
Add tests

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

::: toolkit/modules/tests/browser/browser.ini
@@ +7,5 @@
>  [browser_Geometry.js]
>  [browser_InlineSpellChecker.js]
>  [browser_Troubleshoot.js]
> +[browser_RemoteUtils.js]
> +skip-if = !e10s # RemoteUtils is only ever used with remote browsers.

It seems problematic to be adding an API that only works in e10s mode. Can we make it work in both?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8508781 [details] [diff] [review]
Make it possible to get to remote browser from content window CPOW. r=?

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

I don't think this will work. The window changes every you navigate.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bill McCloskey (:billm) from comment #8)
> Comment on attachment 8508781 [details] [diff] [review]
> Make it possible to get to remote browser from content window CPOW. r=?
> 
> Review of attachment 8508781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this will work. The window changes every you navigate.

Hrm - I just tested it, and you're right. :/

What would you suggest I send up instead? The docshell tree owner?

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

How about the chrome global? I don't remember what the docshell tree owner is.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 12

[Mike Conley (:mconley) (:⚙️)]

This makes more sense - I've put it in BrowserUtils, and adjusted it to use the chrome global like our add-on shims.

billm - I notice that when I run the test with --e10s on a debug build, I leak docshells... I'm guessing this would be fixed by the work you said would be required on your end for this bug?

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8509079 [details] [diff] [review]
Patch v1

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

I'll give you a gold start if you update tabbrowser._getTabForContentWindow to use this!

::: toolkit/content/widgets/remote-browser.xml
@@ +258,3 @@
>            switch (aMessage.name) {
>              case "Browser:Init":
> +              let BrowserUtils =

let {BrowserUtils} = Cu.import("resource://gre/modules/BrowserUtils.jsm", {});

::: toolkit/modules/BrowserUtils.jsm
@@ +249,5 @@
> +                                    .QueryInterface(Ci.nsIDocShellTreeItem)
> +                                    .sameTypeRootTreeItem
> +                                    .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                    .getInterface(Ci.nsIContentFrameMessageManager);
> +    if (!this._globalToBrowserMap.has(chromeGlobal)) {

No need for this, WeakMap.get returns undefined if the key isn't in the map anyway

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm not sure why we'd be leaking docshells. I was actually expecting the opposite problem: that elements of the weakmap would unexpectedly disappear.

Mike, can you make the comment discouraging the use of this function a little stronger? Any use of this function, even not in a loop, can really mess up performance. If the content process is janked, then even one use of the function will jank chrome as well.

For the same reason, I would rather not change _getTabForContentWindow to use this function. Iterating over <browser> elements is likely to be a lot faster than using this function. A single IPC roundtrip costs us roughly 50us, and we're doing six of them with this patch (all the QIs and stuff). We could iterate over a lot of tabs in 300us.

I think this function should really be reserved for places where we don't know if this is a top-level window.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #14)
> I'm not sure why we'd be leaking docshells. I was actually expecting the
> opposite problem: that elements of the weakmap would unexpectedly disappear.
> 
> Mike, can you make the comment discouraging the use of this function a
> little stronger? Any use of this function, even not in a loop, can really
> mess up performance. If the content process is janked, then even one use of
> the function will jank chrome as well.
> 
> For the same reason, I would rather not change _getTabForContentWindow to
> use this function. Iterating over <browser> elements is likely to be a lot
> faster than using this function. A single IPC roundtrip costs us roughly
> 50us, and we're doing six of them with this patch (all the QIs and stuff).
> We could iterate over a lot of tabs in 300us.
> 
> I think this function should really be reserved for places where we don't
> know if this is a top-level window.

I that's the case then we probably shouldn't implement this function at all and simply call _getTabForContentWindow with window.top.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, in some cases we still won't know what XUL window the window came from. I guess we could iterate over all browser.xul windows or something though.

Sorry I didn't think of this earlier. I had forgotten we had getTabForContentWindow.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Ok, I think I've found a solution to my particular problem that both:

a) Won't put Firefox specific code into toolkit/
b) Will allow print preview to work normally for both e10s and non-e10s windows

(b) is what I filed this for to begin with, but with my solution, I don't think I'm going to need this anymore.

Comment 18

[Neil Deakin]

Will your solution handle the issue in bug 1083269?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Hrm - sorry Neil, I forgot about bug 1083269.

My solution goes like this:

Have printUtils.js check to see if the current browser is using remote tabs. If not, resolve the browser to be the chromeEventHandler. If so, check for a function defined in the window called getBrowserForContentWindow. If it's not defined, throw. Else, call it passing in the content window.

For PrintUtils at least, the XUL app that uses remote tabs is now responsible for defining getBrowserForContentWindow in its window scope, and having it return something useful.

I could adapt this solution to go through BrowserUtils instead to make it accessible to SimpleTest.js, but a quick look at that file makes it seem that we try to keep it cross-browser compatible - Cu.import'ing BrowserUtils doesn't look like a thing we'll be able to do. Is my impression correct?

Comment 20

[Neil Deakin]

Have a solution for that bug.