862078 - Use an about:config preference to control multiprocess browsing

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

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

The purpose of this bug is to convert all the uses of MOZ_E10S_COMPAT from a preprocessor conditional to a regular preference. This way, people can test out multiprocess browsing using normal Firefox builds.

Comment 1

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

Attachment: convert MOZ_E10S_COMPAT to gMultiProcessBrowser

This patch adds a new global variable, gMultiProcessBrowser. It gets initialized at startup. This means you'll have to restart the browser after changing the preference. However, that's still much better than having to recompile.

In the two places where MOZ_E10S_COMPAT is used in all.js, I just used the normal defaults for non-e10s builds. People will have to change those prefs along with browser.tabs.remote if need be (although in fact, I think the situation with graphics and e10s is very much in flux, so this may not be an issue anymore).

Comment 2

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

Attachment: add more gMultiProcessBrowser checks

This patch adds checks for gMultiProcessBrowser in more places that are broken in e10s builds. I considered filing bugs for each hunk here, but it doesn't really seem worth it. It's easy to find these by grepping for gMultiProcessBrowser.

Comment 3

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

Attachment: a few extra checks

This patch is similar to the last one. It disables session restore for multiprocess browsing, and it also returns null for the securityUI.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 737706 [details] [diff] [review]
add more gMultiProcessBrowser checks

I don't like how this adds to the blame history for many critical code pieces.

In some cases this can be mitigated very easily, for instance:

>     // Replace initial page URIs with an empty string
>     // only if there's no opener (bug 370555).
>-    if (gInitialPages.indexOf(uri.spec) != -1)
>-      value = content.opener ? uri.spec : "";
>-    else
>+    if (gInitialPages.indexOf(uri.spec) != -1) {
>+      if (!gMultiProcessBrowser)
>+        value = content.opener ? uri.spec : "";
>+      else
>+        value = "";

value = !gMultiProcessBrowser && content.opener ? uri.spec : "";

>             _shouldShowProgress: function (aRequest) {
>               if (this.mBlank)
>                 return false;
> 
>               // Don't show progress indicators in tabs for about: URIs
>               // pointing to local resources.
>-              try {
>-                let channel = aRequest.QueryInterface(Ci.nsIChannel);
>-                if (channel.originalURI.schemeIs("about") &&
>-                    (channel.URI.schemeIs("jar") || channel.URI.schemeIs("file")))
>-                  return false;
>-              } catch (e) {}
>+              if (!gMultiProcessBrowser) {
>+                try {
>+                  let channel = aRequest.QueryInterface(Ci.nsIChannel);
>+                  if (channel.originalURI.schemeIs("about") &&
>+                      (channel.URI.schemeIs("jar") || channel.URI.schemeIs("file")))
>+                    return false;
>+                } catch (e) {}
>+              }
> 
>               return true;
>             },

if (gMultiProcessBrowser)
  return true;

Comment 5

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

Attachment: add more gMultiProcessBrowser checks, v2

I took Dão's advice and tried to minimize the patch churn. Also, at Felipe's suggestion, I filed bugs on some of the bigger pieces that need to be fixed and commented about them.

Comment 6

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

Attachment: a few extra checks, v2

Felipe wanted me to rename _disabled to something that suggested it was disabled because of electrolysis.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 737702 [details] [diff] [review]
convert MOZ_E10S_COMPAT to gMultiProcessBrowser

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

::: browser/base/content/browser.js
@@ +3995,5 @@
> +      // _hostChanged, otherwise onSecurityChange will short circuit.
> +      var securityUI = gBrowser.securityUI;
> +      this._hostChanged = true;
> +      this.onSecurityChange(null, null, securityUI.state);
> +    }

I guess you could also do a

if (gMultiProcessBrowser)
  return;

here

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 739364 [details] [diff] [review]
add more gMultiProcessBrowser checks, v2

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

::: browser/base/content/browser.js
@@ +4167,5 @@
>  
>          this.status = "";
>          this.setDefaultStatus(msg);
>  
> +        if (!gMultiProcessBrowser) {

probably best to leave this item disabled for multiprocess, so you can add the !gMultiProcessBrowser condition in the if below instead of wrapping this block of code.
Same for the next block below

@@ +4406,5 @@
>        if (gURLBar)
>          gURLBar.removeAttribute("level");
>      }
>  
> +    if (!gMultiProcessBrowser) {

and I suppose a `if (gMultiProcessBrowser) return` will also avoid some churn here

Comment 9

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa0960e8b22
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8bfc9dbdef7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/019a9c42ce2e

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9aa0960e8b22
https://hg.mozilla.org/mozilla-central/rev/b8bfc9dbdef7
https://hg.mozilla.org/mozilla-central/rev/019a9c42ce2e