897066 - In e10s builds, some pages should be loaded in the chrome process

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

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

This is true for about:addons, about:preferences, chrome URLs, and various other URLs.

We need to remove the current browser element, change its remote attribute, and add it back to the document. We can use session restore to save and restore the history for the tab.

Comment 1

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

Attachment: some-tabs-nonremote

This patch gets us most of the way to where we want to be. It loads a tab remotely or non-remotely based on the URL. When we change whether a browser element is remote, the history is lost. I think we can fix this once session restore is in a better state in e10s.

I considered using an nsIProgressListener in the child to catch navigations to non-remote URLs. I had trouble getting this to work though. This patch covers most of the cases we care about.

I also added a spiffy underline to tabs that are loaded remotely. When we turn on e10s for real, we can remove that. But I think it's a nice way now for users to see that e10s is actually there.

I'm not too happy with the method names, so I'd welcome suggestions.

Felipe, I'm giving the review to Gavin since you seem kind of overloaded. Feel free to chime in though.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 784635 [details] [diff] [review]
some-tabs-nonremote

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <!-- Returns true if we want to load the content for this URI in a remote process. -->
>+      <method name="shouldBrowserBeRemote">

>+            if (!Services.prefs.getBoolPref("browser.tabs.remote"))
>+              return false;

You can reference gMultiProcessBrowser here.

>+            if (!aURI)
>+              return true;

This might deserve a comment... 

>+            if (aURI instanceof Ci.nsIURI)
>+              aURI = aURI.spec;

Can we just enforce that this function get passed a string and avoid this magical behavior? Seems like the callers know what types they have and can pass .spec as needed. Then you can also call the param "url" (we're not consistent, but we try to use "url" for strings and "uri" for nsIURIs).

>+            if (aURI.startsWith("about:") &&
>+                aURI.toLowerCase() != "about:home" &&
>+                aURI.toLowerCase() != "about:blank")
>+            {
>+              return false;

Hmm, this is maybe not ideal (there are various other unprivileged about: pages). I suppose ideally you would get the relevant nsIAboutModule (by creating a URI, getting its .path, appending that to NS_ABOUT_MODULE_CONTRACTID_PREFIX and calling getService), and then call its getURIFlags and check the result for URI_SAFE_FOR_UNTRUSTED_CONTENT.

Alternatively, maybe you could check !Services.scriptSecurityManager.isSystemPrincipal(Services.io.newChannelFromURI(makeURI(url)).owner) (handling any exceptions thrown), but not sure if that will catch all the right cases (worth testing).

>+      <method name="updateBrowserRemoteness">

>+            // Unhook our progress listener.

>+            filter.removeProgressListener(tabListener);

I don't think you need to disconnect the filter from the tabListener here. It's just a dumb wrapper, disconnecting the two from the webProgress should be sufficient.

>+            // Change the "remote" attribute.
>+            let parent = aBrowser.parentNode;
>+            parent.removeChild(aBrowser);
>+            aBrowser.setAttribute("remote", aRemote ? "true" : "false");
>+            parent.appendChild(aBrowser);

Hmm, does this work? I thought we ran into some issues with it not working for the first-tab case.

>+            // Restore the progress listener.
>+            this.mTabListeners[index] = tabListener =
>+              this.mTabProgressListener(tab, aBrowser, false);

Can you just re-use the existing tabListener rather than re-creating one? I guess if you can't, you probably have to keep filter/listener disconnect mentioned above, since the filter may not be cycle collected.

>+            if (aRemote)
>+              tab.setAttribute("remote", "true");
>+            else
>+              tab.removeAttribute("remote");

It would be nice to break these changes out into a separate patch.

>       <!-- throws exception for unknown schemes -->
>       <method name="loadURI">

>+            this.updateBrowserRemoteness(this.mCurrentBrowser, this.shouldBrowserBeRemote(aURI));
>             return this.mCurrentBrowser.loadURI(aURI, aReferrerURI, aCharset);

Seems like the browser will be in the wrong state if the loadURI call throws (as mentioned in the comment) or otherwise fails, or if the page ends up redirecting itself. What's the plan for addressing that?

>     <content context="tabContextMenu" closetabtext="&closeTab.label;">
>       <xul:stack class="tab-stack" flex="1">
>-        <xul:hbox xbl:inherits="pinned,selected,titlechanged"
>+        <xul:hbox xbl:inherits="pinned,selected,titlechanged,remote"
>                   class="tab-background">
>-          <xul:hbox xbl:inherits="pinned,selected,titlechanged"
>+          <xul:hbox xbl:inherits="pinned,selected,titlechanged,remote"
>                     class="tab-background-start"/>
>-          <xul:hbox xbl:inherits="pinned,selected,titlechanged"
>+          <xul:hbox xbl:inherits="pinned,selected,titlechanged,remote"
>                     class="tab-background-middle"/>
>-          <xul:hbox xbl:inherits="pinned,selected,titlechanged"
>+          <xul:hbox xbl:inherits="pinned,selected,titlechanged,remote"
>                     class="tab-background-end"/>

These changes aren't necessary, unless you have the need to specifically style one of these elements based on the "remote" attribute (which you aren't doing in the CSS changes).

Comment 3

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

Attachment: patch v2

Here's a new patch.

Comment 4

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

Attachment: remote-underline

I split the underlining stuff into a separate patch.

Comment 5

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

> Hmm, does this work? I thought we ran into some issues with it not working for the first-tab
> case.

It does work, and in fact this is the technique we use now to make the first tab remote. I think that the problems we had before were startup issues where we created the first tab dynamically, and addons didn't like that.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 785243 [details] [diff] [review]
remote-underline

I guess ideally this patch would be in a separate bug, too.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate patch

Per discussion on IRC, here's a patch that makes it a bit clearer that this is not the desired setup longer term (I don't think the front-end should be responsible for adjusting browser types like this).

Comment 8

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

Comment on attachment 785271 [details] [diff] [review]
alternate patch

Seems good to me.

Comment 9

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aee889a42555
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/45cd37a5b834

Comment 10

[Gary [:streetwolf52]]

Is this a WIP?  I ask because I turned on the pref and ran into all sorts of problems such as right-click not working, video artifacts, Home page displays the busy pointer for 5-10 seconds when starting Fx.  Also breaks some add-ons.

Windows 8 Pro x64.

Comment 11

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

Yes, we have a long way to go before this will be done. I would discourage people from setting this pref.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aee889a42555
https://hg.mozilla.org/mozilla-central/rev/45cd37a5b834

Comment 13

[Steven MacLeod [:smacleod]]

Comment on attachment 785243 [details] [diff] [review]
remote-underline

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

Noticed something while merging my patches with the latest m-c

::: browser/base/content/tabbrowser.xml
@@ +1390,5 @@
>  
>              t.setAttribute("crop", "end");
>              t.setAttribute("onerror", "this.removeAttribute('image');");
> +            if (remote)
> +              t.setAttribute("remote", "true");

This seems to be unreachable code here. It looks like remote will always be undefined at this point, (|let remote = ...| happens later in the function).

It appears that this if block made it into m-c both here, and further down after remote is declared.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Oops, sorry - my "alternate patch" refactored some of that code without taking into account the underline patch that Bill intended to land with it.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Fixed:
https://hg.mozilla.org/integration/fx-team/rev/132579922893

Thanks for catching that, Steven!

Comment 16

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

Thanks guys.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/132579922893