1506608 - Make the initial tab remote immediately instead of initially making it non-remote

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[:Gijs (he/him)]

We should do this because it avoids creating and then immediately re-creating a browser, and all its frame scripts etc., so it makes window opening faster.

There's a few issues with doing this right now. This trypush ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=306f577a29c017f5352497b82e6bd0054fc616df&selectedJob=201442995 ) is a bit old, but the fundamental issue seems to be that we have a bunch of tests that call `window.open` in the parent process, which ends us up with an opener in the parent. Prior to fixing this bug we then make the content frame remote after initially making it non-remote (which created the initial frame loader which gets the opener ref). After the patch, we make the content frame remote immediately, which gets the opener ref but is also remote, which trips this assertion: https://searchfox.org/mozilla-central/rev/06d5d5ae4396be85f26e8548323ee6c12e7bce4e/dom/base/nsFrameLoader.cpp#187-188

We can just fix most of these tests. I don't think this can currently be hit by users and/or websites - it's only our own random code that manually calls window.open in a chrome context without  . Alternatively, we could potentially null out the opener in this case instead of asserting, but given the tests seem to just be wrong, my gut sense is to fix the tests.

Comment 1

[:Gijs (he/him)]

So, this is perhaps not quite as easy as I thought. In particular, the assertion failures happen when something that runs in the parent process calls window.open(), with e10s enabled. The confusion is that the testcases seem to be using the chrome window to try to open a content window. I don't think we need to support that case, and all the cases I see on try are essentially tests that don't mean specifically to test that usecase, and so from that perspective, this is OK - we just update the tests. These cases are suspect anyway because in-content windows end up with a window.opener ref that points to a browser window instead of a content window, and that asymmetry strikes me as wrong (and has been seriously problematic when the opposite happened, cf bug 1096319).

However, trying to update the browser_dbg_multiple-windows.js test, I realized that this would also affect effectively any parent-process pages (like about:debugging, or about:performance, or about:preferences, ... ) loaded in the content area that try to call window.open (because I tried to just switch to ContentTask.spawn(... content.open(...)) and that didn't help, because the open page is a chrome: URL that is privileged). I'm not convinced (m)any of our shipped content does that today, but I expected we don't really want to break parent-process opener links in e10s. So I tried to figure out how to teach the browser.js code to detect this case.

Now, the problem is, for that to happen, we need to know in the JS running in the newly-opened browser window that we have an opener. But window.opener is null. We end up with an opener in this code: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/xul/XULFrameElement.cpp#89-96 , but that's not JS-accessible (ie prior to calling that code window.opener in the parent browser window is null).

The other confusing thing is that the case where we call window.open() from within the content area, I haven't been able to figure out a way to make things actually work properly. That is, to get a normal browser window with chrome - running something like `window.open("chrome://browser/content/policies/aboutPolicies.xhtml", "_blank", "all,chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar,height=500,width=500")` from the content console open on e.g. about:preferences doesn't work - the XHTML doc ends up being the toplevel window (bgrins, is that a browser.xhtml thing?), BrowserWindowTracker.windowCount doesn't include the newly opened window. So from that perspective, I'm not sure if we do care about that case, given that AFAICT it doesn't really work very well. But if we don't care about that case, is the TakeOpenerForInitialContentBrowser() code just dead in the parent process? Should we remove it?

The other confusing thing (well, confusing for me) is that the implementation of OpenBrowserWindow uses window.openDialog() in the chrome window, and that Just Works, also after the patch, including setting the window.opener ref to the previous window. I don't understand why window.open is different.

Nika, can you shed some light here as you seem to have refactored some of this code? Specifically:
1) do we care about the window.open() case from browser windows?
2) same question, but for content windows. If so, what would be the best way of exposing some of this info to JS so we can use it to determine if we need a remote or non-remote browser?
3) do you know if this is web-exposed or used in-browser today in some way?

If you want to test some of this stuff yourself, you can use this patch: https://hg.mozilla.org/try/rev/b531224d6c75834f335a83c9ddd7d13c94c8d777

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #1)
> So, this is perhaps not quite as easy as I thought. In particular, the
> assertion failures happen when something that runs in the parent process
> calls window.open(), with e10s enabled

For the avoidance of doubt, this isn't a problem with e10s disabled because the JS code in browser.js always creates parent-process browsers and the assertion from comment #0 doesn't trigger and everything is happy.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #1)
> The other confusing thing is that the case where we call window.open() from
> within the content area, I haven't been able to figure out a way to make
> things actually work properly. That is, to get a normal browser window with
> chrome - running something like
> `window.open("chrome://browser/content/policies/aboutPolicies.xhtml",
> "_blank",
> "all,chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar,
> height=500,width=500")` from the content console open on e.g.
> about:preferences doesn't work - the XHTML doc ends up being the toplevel
> window (bgrins, is that a browser.xhtml thing?),
> BrowserWindowTracker.windowCount doesn't include the newly opened window. So
> from that perspective, I'm not sure if we do care about that case, given
> that AFAICT it doesn't really work very well. But if we don't care about
> that case, is the TakeOpenerForInitialContentBrowser() code just dead in the
> parent process? Should we remove it?

Unless if you've built with `MOZ_BROWSER_XHTML=1` in your env, window.open behavior shouldn't be changed. And if you have, the difference would be at this line: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/xpfe/appshell/nsXULWindow.cpp#2140 assigning browser.xhtml instead of browser.xul (due to https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/browser/confvars.sh#37-41).

What I think is happening is that when you open a chrome URI via window.open we don't call nsXULWindow::CreateNewContentWindow but rather nsXULWindow::CreateNewChromeWindow (which never wraps the window inside of browser.xul). Likely here: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/xpfe/appshell/nsXULWindow.cpp#2095

Comment 4

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> What I think is happening is that when you open a chrome URI via window.open
> we don't call nsXULWindow::CreateNewContentWindow but rather
> nsXULWindow::CreateNewChromeWindow (which never wraps the window inside of
> browser.xul). Likely here:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/xpfe/appshell/nsXULWindow.cpp#2095

Ah, OK, so it seems that if I use:

window.open("chrome://browser/content/policies/aboutPolicies.xhtml", "_blank", "toolbar,location")

from content, the "right" behavior happens, though it asserts after the patch from https://hg.mozilla.org/try/rev/b531224d6c75834f335a83c9ddd7d13c94c8d777 . I guess that means we should continue to care about this case, though I don't know off-hand how best to expose the opener... I'll have a look whether I can just expose this on window with webidl, or xpcom via nsIXULWindow or something.

Comment 5

[:Gijs (he/him)]

Just going to move the ni to a review.

Comment 6

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42ac6e64ad13f57e9ef65cbd24d8fef58544b9a

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1506608 - default to remote for e10s windows, r?mconley!,nika!

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d42ac6e64ad13f57e9ef65cbd24d8fef58544b9a

Annnd this is green. \o/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a765b8ac85d
default to remote for e10s windows, r=mconley,nika

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4a765b8ac85d

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4a765b8ac85d

Comment 12

[Ionuț Goldan [:igoldan]]

Performance wins noticed! \0/

== Change summary for alert #17701 (as of Tue, 20 Nov 2018 12:00:41 GMT) ==

Improvements:

  3%  sessionrestore linux64 opt e10s stylo     393.08 -> 381.08
  3%  sessionrestore linux64 pgo e10s stylo     367.00 -> 356.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=17701