960783 - Create a "New window in separate process" menu option in Nightly

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

This idea started when we were talking about how to turn electrolysis on in nightly. Doing it in the near future would really anger a lot of people because basic stuff like printing still doesn't work. However, we do want to make it available to more people. A good middle ground would be to add a new menu option "New window in separate process" (name could be improved). It would be sort of like private browsing mode.

Currently electrolysis is enabled/disabled on startup based on the browser.tabs.remote pref. Perhaps we could make that a tristate value, with the third value meaning "only in windows where specifically requested". That would be the default in Nightly, whereas in other releases it would be completely disabled. To make the new option work, we would need (1) some UI work to add the menu item and track its value, (2) to refactor all the code that currently checks the pref.

It's pretty easy to search for all the code that checks the pref.
  https://mxr.mozilla.org/mozilla-central/search?string=BrowserTabsRemote
The only troublesome stuff that I see is in graphics, and Vlad is optimistic that we'll have OMTC enabled everywhere pretty soon. Besides that, OMTC is a per-window option, so we still have options if that work isn't finished in time.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Bikeshed-ish: I don't think you mean "New window in separate process" -- you really mean "New window with out-of-process tabs", right?   But given that it's just a nightly/developer tool, it could just be "New E10s window" or "New OOP-tabs window".

Comment 2

[Chris Peterson [:cpeterson]]

The "New e10s Window" command should be #ifdef RELEASE_BUILD so it is only available on Nightly and Aurora. We don't want Release or even Beta users to see this feature.

Comment 3

[(no longer active)]

Hmm, in case the number of callers depending on the pref is not too many at this point (which seems to be a correct assumption based on the link in comment 0) then perhaps we can do this "properly" by storing a flag on the docshell and propagate that to the child docshells and make our code look for that flag instead of the pref.  Once we have that we can just have the pref determine the default flag status.  This setup is what we use for per-window PB and seems to have served our needs fairly well.  Let me know if you need more info on how to make that work!

Comment 4

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

Here's how I ended up doing the prefs for this:

If browser.tabs.remote = false, then remote (i.e., out of process) tabs are completely inaccessible. This is how we'll keep this feature disabled in non-nightly builds.

If browser.tabs.remote = true and browser.tabs.remote.autostart = false, then browser windows will normally be in-process. However, the user will see a new menu option, "New OOP Window" that will open a window with remote tabs. This configuration will become the default for nightlies.

If browser.tabs.remote = true and browser.tabs.remote.autostart = true, then browser windows will normally have remote tabs. However, the user will see a new menu option, "New In-process Window" that will open a window with in-process tabs. This configuration will be used by people who want to test e10s more extensively.

Comment 5

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

Attachment: chrome-flags

This patch adds a flag on the window and on the nsILoadContext to denote whether remote tabs should be used by the window. I mostly just copied what we do for private browsing windows.

One change I made is that the isRemoteWindow value is computed inside CalculateChromeFlags rather than later on in OpenWindowInternal. That's because the nsWidgetInitData needs to know whether the window will be remote, and it gets created and used before isPrivateBrowsingWindow is defined (see the next patch). Also, I feel like it makes more sense to do it in CalculateChromeFlags, and it allows us to use one bit on the chrome flags instead of two.

Comment 6

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

Attachment: compositor-always-on

This change requires us to be able to use OMTC for some windows and not for others. This patch moves around some of the configuration flags to make this work.

If browser.tabs.remote = true (which will be the case for all nightly users when this lands), then we'll always create a compositor thread. However, we'll only use compositing for a given window if the OMTC pref is set or if a special flag is set in the nsWidgetInitData. That flag will only be set for windows where we want to use remote tabs.

If browser.tabs.remote = false, then everything should work as before.

Comment 7

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

Attachment: remote-window

These are the frontend changes to create and use the new prefs and to make the new menu items.

Comment 8

[(no longer active)]

Comment on attachment 8369638 [details] [diff] [review]
remote-window

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +279,5 @@
>  <!ENTITY newPrivateWindow.accesskey "W">
> +<!ENTITY newRemoteWindow.label     "New OOP Window">
> +<!ENTITY newRemoteWindow.accesskey "P">
> +<!ENTITY newNonRemoteWindow.label     "New In-process Window">
> +<!ENTITY newNonRemoteWindow.accesskey "I">

Are we planning to ship these menu items ever?  If not, it might make sense to hardcode their names in English in the code and not bother all of our localizers with these four new strings...

Comment 9

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> Are we planning to ship these menu items ever?  If not, it might make sense
> to hardcode their names in English in the code and not bother all of our
> localizers with these four new strings...

Good point. We don't have plans to ship them. I'll update the patch.

Comment 10

[(no longer active)]

Comment on attachment 8369633 [details] [diff] [review]
chrome-flags

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

r=me with the comments below addressed.  Note that I don't feel very strongly about the naming, so if you disagree that's fine.

::: docshell/base/nsDocShell.cpp
@@ +2250,5 @@
> +
> +NS_IMETHODIMP
> +nsDocShell::SetRemoteTabs(bool aUseRemoteTabs)
> +{
> +    mUseRemoteTabs = aUseRemoteTabs;

So this skips the logic that we have for the PB case which sets the flags on all of the child docshells.  If that's fine with you, it's fine by me as well.

::: docshell/base/nsILoadContext.idl
@@ +68,5 @@
>  
> +  /**
> +   * Attribute that determines if remote (out-of-process) tabs should be used.
> +   */
> +  readonly attribute boolean useRemoteTabs;

Please rev the uuid.

::: dom/ipc/TabChild.cpp
@@ +696,5 @@
>    MOZ_ASSERT(loadContext);
>    loadContext->SetPrivateBrowsing(
>        mChromeFlags & nsIWebBrowserChrome::CHROME_PRIVATE_WINDOW);
> +  loadContext->SetRemoteTabs(
> +      mChromeFlags & nsIWebBrowserChrome::CHROME_REMOTE_WINDOW);

I think you should use a better name.  We have the notion of remoting controlled by the -remote/-no-remote command line arguments, which may be confused with this.  Can we use CHROME_OOP_WINDOW or something?

::: embedding/browser/webBrowser/nsIWebBrowserChrome.idl
@@ +80,5 @@
>      // Whether this was opened by nsGlobalWindow::ShowModalDialog.
>      const unsigned long CHROME_MODAL_CONTENT_WINDOW   = 0x00080000;
>  
> +    // Whether this window should use remote (out-of-process) tabs.
> +    const unsigned long CHROME_REMOTE_WINDOW          = 0x00100000;

I think you should use a better name.  We have the notion of remoting controlled by the -remote/-no-remote command line arguments, which may be confused with this.  Can we use CHROME_OOP_WINDOW or something?

::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
@@ +1441,5 @@
> +    bool remote;
> +    if (Preferences::GetBool("browser.tabs.remote.autostart")) {
> +      remote = !WinHasOption(aFeatures, "non-remote", 0, &presenceFlag);
> +    } else {
> +      remote = WinHasOption(aFeatures, "remote", 0, &presenceFlag);

Same note about the naming.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8369636 [details] [diff] [review]
compositor-always-on

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

::: gfx/thebes/gfxPlatform.cpp
@@ +416,5 @@
>      mozilla::gl::GLContext::StaticInit();
>  #endif
>  
> +    bool useOffMainThreadCompositing = GetPlatform()->ShouldUseOffMainThreadCompositing();
> +    useOffMainThreadCompositing |= OffMainThreadCompositionRequired();

Maybe we should rename this variable to be startupCompositorThread so it's clearer that this just prep work, not actually enabling OMTC.

Comment 12

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

Attachment: remote-window v2

Got rid of the localization.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8369706 [details] [diff] [review]
remote-window v2

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

::: browser/base/content/browser-menubar.inc
@@ +28,5 @@
>                            command="Tools:PrivateBrowsing"
>                            key="key_privatebrowsing"/>
> +                <menuitem id="menu_newRemoteWindow"
> +                          label="New OOP Window"
> +                          accesskey="P"

"P" is the accesskey for Print.  Since we're not translating these strings, the "I" accesskey might also be a conflict in others languages. So let's leave the accesskeys out.

@@ +33,5 @@
> +                          command="Tools:RemoteWindow"/>
> +                <menuitem id="menu_newNonRemoteWindow"
> +                          label="New In-process Window"
> +                          accesskey="I"
> +                          command="Tools:NonRemoteWindow"/>

add hidden="true" attributes for these two items and unhide the right one based on the prefs.
(Helps perf to not have to size/layout an item unnecessarily and then hide it)

::: browser/base/content/browser.js
@@ +924,5 @@
>      // Misc. inits.
>      CombinedStopReload.init();
>      CombinedBackForward.init();
>      gPrivateBrowsingUI.init();
> +    gRemoteTabsUI.init();

this can be done in delayedStartup instead of here

@@ +3264,5 @@
>    }
>  
> +  if (options && options.remote) {
> +    extraFeatures += ",remote";
> +  } else if (options && !options.remote) {

check that remote=false was really defined instead of being falsy.

Comment 14

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

Attachment: crash-reporter

This patch sets DOMIPCEnabled in the crash report if the user has ever opened an out-of-process window since startup.

Comment 15

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

Trevor, can you please explain what this code is for?
  http://mxr.mozilla.org/mozilla-central/source/accessible/src/windows/msaa/nsWinUtils.cpp#62
We're trying to make e10s be a per-window setting, and I'm not sure how to deal with this check.

Comment 16

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

Oops, forgot to set the flag. See comment above.

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Bill McCloskey (:billm) from comment #15)
> Trevor, can you please explain what this code is for?
>  
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/windows/msaa/
> nsWinUtils.cpp#62
> We're trying to make e10s be a per-window setting, and I'm not sure how to
> deal with this check.

that's code that's trying to make content processes accessible by creating win32 windows for them.  I'm not incredibly familiar with that setup, and don't care about it much, but its probably worth keeping for now.  I believe you can replace that check with a check that we're in a content process and be ok.

Comment 18

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

Attachment: accessibility

Sounds good. Thanks.

Comment 19

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

Attachment: context-menu-loading

I think I must have missed this when I converted browser.tabs.remote to use Services.appinfo.browserTabsRemote. In any case, this is a better way to do the test.

Comment 20

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

Attachment: test-changes

This patch just renames browser.tabs.remote to browser.tabs.remote.autostart in tests that need remote tabs.

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 8369767 [details] [diff] [review]
test-changes

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

I think this will silently break the "reftest-ipc" and "crashtest-ipc" test suites:
https://hg.mozilla.org/build/mozharness/file/34a6f3cd7858/configs/unittests/linux_unittest.py#l100

Changing that might be tricky, because that config is used across all branches. Would it be harmful to set both the old and the new pref? If not then you ought to be able to do that. (Note that the same thing is also in {mac,win}_unittest.py, but those tests are currently disabled.)

Comment 22

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

Attachment: autostart-pref

You're right Ted. I guess I need this. Having this pref on all branches shouldn't do any harm.

Comment 23

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

Comment on attachment 8371972 [details] [diff] [review]
autostart-pref

This patch sets an extra pref that we'll need in the future in order to get remote tabs. It should be fine to have it set on older branches.

Comment 24

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

I pushed the mozharness change early since it's totally harmless.
https://hg.mozilla.org/build/mozharness/rev/76b348205b29

Comment 25

[John Hopkins (:jhopkins)]

in production

Comment 26

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1b69bfa7f2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/08b4b4b4aef3

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff1b69bfa7f2
https://hg.mozilla.org/mozilla-central/rev/08b4b4b4aef3

Comment 28

[Vincent Chen [:vichen]]

Hi, Bill, for comment#20, shall we also modify http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/remotereftest.py#347 to keep b2g reftest in OOP?

Comment 29

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

(In reply to Vincent Chen [:vichen] from comment #28)
> Hi, Bill, for comment#20, shall we also modify
> http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/
> remotereftest.py#347 to keep b2g reftest in OOP?

As far as I can tell, that preference governs whether we run an HTTP server on the same machine as we run tests. I don't think it should be affected by this bug.

Comment 30

[Vincent Chen [:vichen]]

(In reply to Bill McCloskey (:billm) from comment #29)
> (In reply to Vincent Chen [:vichen] from comment #28)
> > Hi, Bill, for comment#20, shall we also modify
> > http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/
> > remotereftest.py#347 to keep b2g reftest in OOP?
> 
> As far as I can tell, that preference governs whether we run an HTTP server
> on the same machine as we run tests. I don't think it should be affected by
> this bug.

Hi, Bill,
Sorry for refer to wrong file. It's http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftestb2g.py#417

Comment 31

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

(In reply to Vincent Chen [:vichen] from comment #30)
> Hi, Bill,
> Sorry for refer to wrong file. It's
> http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/
> runreftestb2g.py#417

I'm not sure I understand. That code sets browser.tabs.remote to false. This patch should only affect things where browser.tabs.remote was true. Did b2g reftests ever run out-of-process?

Comment 32

[Vincent Chen [:vichen]]

(In reply to Bill McCloskey (:billm) from comment #31)
> (In reply to Vincent Chen [:vichen] from comment #30)
> > Hi, Bill,
> > Sorry for refer to wrong file. It's
> > http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/
> > runreftestb2g.py#417
> 
> I'm not sure I understand. That code sets browser.tabs.remote to false. This
> patch should only affect things where browser.tabs.remote was true. Did b2g
> reftests ever run out-of-process?

To enable b2g reftest in OOP, we have to set browser.tabs.remote to true  (https://bugzilla.mozilla.org/show_bug.cgi?id=922680#c4).

Comment 33

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

If you want to change b2g reftests so that they run out of process, then you should set browser.tabs.remote and browser.tabs.remote.autostart to true.

Comment 34

[Onno Ekker [:nONoNonO UTC+1]]

I suppose this string gets to be localized instead of being hard-coded? And it's probably worthwhile to add a tooltip to this menu entry, since OOP or e10s or even electorlysis doesn't mean that much to most users...

Comment 35

[(no longer active)]

(In reply to comment #34)
> I suppose this string gets to be localized instead of being hard-coded? And
> it's probably worthwhile to add a tooltip to this menu entry, since OOP or e10s
> or even electorlysis doesn't mean that much to most users...

We chose to not localize this string since this is a temporary option that will only appear in Nightly.

Comment 36

[:Irving Reid (No longer working on Firefox)]

I've seen some very odd glitching in my default profile, that I bisected down to one of the patches from this bug:

changeset:   168253:08b4b4b4aef3
user:        Bill McCloskey <[e-mail]>
date:        Tue Feb 11 09:01:08 2014 -0800
summary:     Bug 960783 - Support "new out-of-process window" menu item in nightly (r=felipe,bsmedberg,trevor,ted)

see https://bugzilla.mozilla.org/show_bug.cgi?id=962528#c10 and forward.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

Irving's bug is actually unrelated to bug 962528.  For it I've opened bug 974616.

Comment 38

[Tracy Walker [:tracy]]

Verified with latest build of Nightly