1272754 - contextid not being propagated when switching a tab into non-e10s

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Switching the current tab to a non-e10s window doesn't propagate the contextid but does preserve the website that the user was previously browsing. We should be preserving both the container and the website in this situation.

STR:

* open and login into bugzilla using the work container
* once logged in, right click on the tab and select "Open in a new non-e10s window"
* the new non-e10s window will load bugzilla using the default container (contextid=0) rather than opening it in a work container

Comment 1

[Jim Mathies [:jimm]]

non-shipping feature, tracks but doesn't block e10s.

Comment 2

[Kamil Jozwiak [:kjozwiak]]

This is still occurring with the latest version of Nightly. I quickly went through the STR from comment #0 to see if this might have been fixed as a byproduct of all the work going into the container feature.

Build Used:

* fx50.0a1 buildId: 20160712030234, changeset: aac8ff1024c5

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Attachment: Switching a tab into non-e10s should preserve userContextId.

I thought this patch should fix this bug. My printf at line 974 says the new docshell has correct userContextId, but when I tried the STR, the newly created tab is still in default container.

I don't know what's wrong. Baku, would you please take a look?

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8771383 [details] [diff] [review]
Switching a tab into non-e10s should preserve userContextId.

Tim just traced the code with me. We found that my method was totally wrong, and we believe we have the right fix this time. I'll upload a patch later.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Attachment: Switching a tab into non-e10s should preserve userContextId.

Baku, would you please review this patch?

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8772476 [details] [diff] [review]
Switching a tab into non-e10s should preserve userContextId.

I cannot review this code.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8772476 [details] [diff] [review]
Switching a tab into non-e10s should preserve userContextId.

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

::: browser/base/content/tabbrowser.xml
@@ +2997,5 @@
>                throw "This method is intended only for e10s testing!";
>              }
>              let url = aTab.linkedBrowser.currentURI.spec;
> +            let userContextId = aTab.hasAttribute("usercontextid") ? aTab.getAttribute("usercontextid") : 0;
> +            return window.openDialog("chrome://browser/content/", "_blank", "chrome,all,dialog=no,non-remote", url,

This seems wrong. We are asking for the opening of a new window, for a chrome URL. This chrome URL should not receive the userContextId because it will run using systemPrincipal.

To me the bug is when we swap the docShell.

Comment 8

[Jonathan Hao (inactive) [:jhao]]

This chrome URL won't actually receive the userContextid. The userContextId is passed in as an extra argument [1], and will not be used until the chrome URL is fully loaded. My patch retrieves the extra argument at _delayedStartup [2], when the actually url is about to be loaded. System principal will not be used at that time.

http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8169
http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1154

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Baku, does Comment 8 change your mind?

Comment 10

[Andrea Marchesini [:baku]]

Do we swap the docShell between a e10s and an non-e10s docShell?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

To be absolutely clear, the "Open in non-e10s Window" context menu item for tabs will not ship to release users. That item, along with the "- e10s" prefix in the tab titles, and the "Open non-e10s Window" toolbar button, are restricted to the Nightly and Aurora channels.

So with that information in mind, is fixing this really necessary?

Comment 12

[Jonathan Hao (inactive) [:jhao]]

(In reply to Andrea Marchesini (:baku) from comment #10)
> Do we swap the docShell between a e10s and an non-e10s docShell?

No, we don't because "Open in non-e10s Window" keeps the originally e10s window and opens a whole new non-e10s one.

The user action triggers tabbrowser.xml:openNonRemoteWindow() and it creates a window by:

window.openDialog("chrome://browser/content/", "_blank", "chrome,all,dialog=no,non-remote", url, userContextId);

When the window is painted, the MozAfterPaint event will trigger browser.js:_delayedStartup(). It will eventually go to this else block: http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1153. The code that swaps the docshell is in this block: http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#1124 and won't be executed in this code path.

Comment 13

[Jonathan Hao (inactive) [:jhao]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #11)
> To be absolutely clear, the "Open in non-e10s Window" context menu item for
> tabs will not ship to release users. That item, along with the "- e10s"
> prefix in the tab titles, and the "Open non-e10s Window" toolbar button, are
> restricted to the Nightly and Aurora channels.
> 
> So with that information in mind, is fixing this really necessary?

Before container is in TestPilot, users who want this feature can only use it in Nightly and Aurora channels.  They can still be affected.  So, if there isn't too much risk, I think we should fix this.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8772476 [details] [diff] [review]
Switching a tab into non-e10s should preserve userContextId.

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

I don't think this is going to work with e10s, since gBrowser.webNavigation is not going to be accessible.

So far, Test Pilot studies have had user populations in the tens of thousands. I wonder, for those users, if it makes more sense to hide the "Open in non-e10s" menu options if they're within the testing pool.

::: browser/base/content/browser.js
@@ +1847,5 @@
>    // This function throws for certain malformed URIs, so use exception handling
>    // so that we don't disrupt startup
>    try {
> +    if (aUserContextId) {
> +      gBrowser.webNavigation.setOriginAttributesBeforeLoading({ userContextId: aUserContextId });

This is going to break with e10s, where we have no access to the nsIWebNavigation in the parent process.

Comment 15

[Jonathan Hao (inactive) [:jhao]]

Attachment: Switching a tab into non-e10s should preserve userContextId.

Thanks, Mike. I pass the userContextId further down in this patch and I think it should be compatible with e10s this time.

Comment 16

[Jonathan Hao (inactive) [:jhao]]

The code path is:

tabbrowser.xml::loadTabs() ->
tabbrowser.xml::loadURI() ->
browser.xml::loadURI() ->
tabbrowser.xml::loadURIWithFlags() -> (remote and non-remote both has an implmentation)
browser.js::_loadURIWithFlags()

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Attachment: Switching a tab into non-e10s should preserve userContextId.

I removed an unused argument.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Attachment: Switching a tab into non-e10s should preserve userContextId.

I forgot to refresh before the last upload. Sorry for all the fuss.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8775501 [details] [diff] [review]
Switching a tab into non-e10s should preserve userContextId.

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

Before I review this, what do you think of my idea in comment 14 of hiding the menuitems for Test Pilot participants? The reason I ask, is because I'm seeing this bug track 49, and I assume you want to uplift, and that worries me a bit because while the patch is small, it's altering some pretty critical paths for loading content.

So let me know what you think of hiding the menuitems instead.

Comment 20

[Jonathan Hao (inactive) [:jhao]]

If this patch concerns you this much, we can hide the menu items instead.

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Attachment: Disable "Open in new non-e10s window" from non-default containers.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8775922 [details] [diff] [review]
Disable "Open in new non-e10s window" from non-default containers.

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

Note that there is still the e10s-button for opening non-e10s windows that's in the application menu for Nightly / Aurora builds: http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/browser/components/customizableui/CustomizableWidgets.jsm#1262

You might want to have that button be disabled at the same time that the URL bar is updated to reflect which container is selected.

::: browser/base/content/browser.js
@@ +7609,4 @@
>          menuItem.hidden = !gMultiProcessBrowser;
> +        if (menuItem.id == "context_openNonRemoteWindow") {
> +          menuItem.disabled = this.contextTab.hasAttribute("usercontextid") &&
> +                              parseInt(this.contextTab.getAttribute("usercontextid"));

Nit: if the attribute doesn't exist, getAttribute will return null, which parseInt will convert to NaN, which is false-y.

You should be able to simplify this to:

menuItem.disabled = !!parseInt(this.contextTab.getAttribute("usercontextid"));

Comment 23

[Jonathan Hao (inactive) [:jhao]]

Are you talking about the "New non-e10s Window under the File menu and also in the hamburger menu?  That command will always open a window with a tab in the default container, and is not related to this bug.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jonathan Hao [:jhao] from comment #23)
> Are you talking about the "New non-e10s Window under the File menu and also
> in the hamburger menu?  That command will always open a window with a tab in
> the default container, and is not related to this bug.

That's true - I apologize, you're absolutely right on that.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8775922 [details] [diff] [review]
Disable "Open in new non-e10s window" from non-default containers.

You were absolutely right that we only need this to apply to the tab context menu item, and not the panel or toolbar buttons. r=me with the nit I brought up in comment 22 fixed.

Comment 26

[Jonathan Hao (inactive) [:jhao]]

Attachment: Disable "Open in new non-e10s window" from non-default containers.

Thank you, Mike.

Comment 27

[Ethan Tseng [:ethan]]

Hi Tanvi and Paul,

This bug affects aurora users when they are executing steps described in comment 0.
Shall we uplift this bug to aurora (FF 50)?

Comment 28

[Tanvi Vyas[:tanvi]]

Is the "Open Link in new non-e10s window" available in Aurora?  Is Aurora e10s by default?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tanvi Vyas [:tanvi] from comment #28)
> Is the "Open Link in new non-e10s window" available in Aurora?  Is Aurora
> e10s by default?

Yes to both questions.

Comment 30

[Tanvi Vyas[:tanvi]]

Containers is not enabled by default in Aurora 50.  When we go to Test Pilot in 49 and do enable Containers there, I don't think e10s will be enabled for test pilot users.  So if we uplift this, we'd only be uplifting for Aurora users who enable containers.  I don't expect that percentage to be very high so I'm okay with just keeping this in 51.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4192b6a9958
Disable "Open in new non-e10s window" from non-default containers. r=mconley

Comment 32

[Ethan Tseng [:ethan]]

(In reply to Tanvi Vyas [:tanvi] from comment #30)
> Containers is not enabled by default in Aurora 50.  When we go to Test Pilot
> in 49 and do enable Containers there, I don't think e10s will be enabled for
> test pilot users.  So if we uplift this, we'd only be uplifting for Aurora
> users who enable containers.  I don't expect that percentage to be very high
> so I'm okay with just keeping this in 51.

Sounds reasonable.

Comment 33

[Carsten Book [:Tomcat]]

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