Closed Bug 960783 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

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.
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".
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.
OS: Linux → All
Hardware: x86_64 → All
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!
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.
Attached patch chrome-flagsSplinter Review
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8369633 - Flags: review?(ehsan)
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.
Attachment #8369636 - Flags: review?(matt.woodrow)
Attached patch remote-window (obsolete) — Splinter Review
These are the frontend changes to create and use the new prefs and to make the new menu items.
Attachment #8369638 - Flags: review?(felipc)
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...
(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 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.
Attachment #8369633 - Flags: review?(ehsan) → review+
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.
Attachment #8369636 - Flags: review?(matt.woodrow) → review+
Attached patch remote-window v2Splinter Review
Got rid of the localization.
Attachment #8369638 - Attachment is obsolete: true
Attachment #8369638 - Flags: review?(felipc)
Attachment #8369706 - Flags: review?(felipc)
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.
Attachment #8369706 - Flags: review?(felipc) → review+
Attached patch crash-reporterSplinter Review
This patch sets DOMIPCEnabled in the crash report if the user has ever opened an out-of-process window since startup.
Attachment #8369719 - Flags: review?(benjamin)
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.
Oops, forgot to set the flag. See comment above.
Flags: needinfo?(trev.saunders)
(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.
Flags: needinfo?(trev.saunders)
Attached patch accessibilitySplinter Review
Sounds good. Thanks.
Attachment #8369755 - Flags: review?(trev.saunders)
Attachment #8369755 - Flags: review?(trev.saunders) → review+
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.
Attachment #8369765 - Flags: review?(felipc)
Attached patch test-changesSplinter Review
This patch just renames browser.tabs.remote to browser.tabs.remote.autostart in tests that need remote tabs.
Attachment #8369767 - Flags: review?(ted)
Attachment #8369765 - Flags: review?(felipc) → review+
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.)
Attachment #8369767 - Flags: review?(ted) → review+
Attached patch autostart-prefSplinter Review
You're right Ted. I guess I need this. Having this pref on all branches shouldn't do any harm.
Attachment #8371972 - Flags: review?(ted)
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.
Attachment #8371972 - Flags: review?(ted) → review?(aki)
Attachment #8371972 - Flags: review?(aki) → review+
I pushed the mozharness change early since it's totally harmless.
https://hg.mozilla.org/build/mozharness/rev/76b348205b29
in production
Attachment #8369719 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/ff1b69bfa7f2
https://hg.mozilla.org/mozilla-central/rev/08b4b4b4aef3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(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.
(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
(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?
Depends on: 972700
(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).
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.
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...
(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.
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 <wmccloskey@mozilla.com>
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.
Irving's bug is actually unrelated to bug 962528.  For it I've opened bug 974616.
Verified with latest build of Nightly
Status: RESOLVED → VERIFIED
Depends on: 996160
Depends on: 1009030
No longer depends on: 1003313
You need to log in before you can comment on or make changes to this bug.