Containers: Opening same-origin link in new tab from active container should put new tab in same container

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: freddyb, Assigned: baku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: [domsecurity-active][usercontextId][uplift49+])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
Steps to repeat:

1) Open Bugzilla in a work container
2) open related bug in a new tab
3) realize the new tab is not logged in. can not comment.

I would expect Firefox to defaulted to same-container when opening new tabs that visit the same origin.

I know this is tricky to get right. So I want to focus on the same-origin case first.
(For other origins, especially those already opened in another container, the user might also want to use the existing session. But they also might really, really not. Let's do this ina a follow-up bug.)
(Reporter)

Comment 1

a year ago
Baku asked me to NI him, forgot to set this.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

a year ago
I think we should open the same container in case the origin is the same. Tanvi, feedback?
I remember we had a similar feature, could it be that is a regression?
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini) → needinfo?(tanvi)
(Assignee)

Comment 3

a year ago
Created attachment 8758310 [details] [diff] [review]
right_click.patch
Flags: needinfo?(tanvi)
Attachment #8758310 - Flags: review?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
Comment on attachment 8758310 [details] [diff] [review]
right_click.patch

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

::: browser/base/content/browser.js
@@ +5512,5 @@
> +
> +  if (doc.nodePrincipal.originAttributes.userContextId) {
> +    // If the 2 URIs have the same origin, we use the same userContextId,
> +    // otherwise we disable the referrer.
> +    if (makeURI(href).prePath == referrerURI.prePath) {

makeURI can return null, in which case this will throw, which needs fixing here and in ContentClick.jsm.

This also won't work well for pages that open about:blank or open a data or javascript URI. It's also fragile for e.g. subdomains (so if I go from foo.com to tickets.foo.com, do I really want to break out of the usercontextid?).

Why are we using origins in this way?

::: browser/modules/ContentClick.jsm
@@ +42,5 @@
>        }
>        return;
>      }
>  
> +    let uri = Services.io.newURI(json.href, null, null);

This can throw, so please deal with that.

@@ +87,5 @@
>  
> +    if (json.originAttributes.userContextId) {
> +      // If the 2 URIs have the same origin, we use the same userContextId,
> +      // otherwise we disable the referrer.
> +      if (uri.prePath == browser.documentURI.prePath) {

This will then need to deal with the null uri case - but as noted in the non-e10s code, not sure why we're doing this this way.
Attachment #8758310 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 5

a year ago
Created attachment 8758651 [details] [diff] [review]
right_click.patch

Bram, we need your help! Here the problem:

Tab A (Container 1) has a link. The user does middle click on a link. Firefox opens a tab in background with the selected URL. Which UserContextId should we use?

There are many options:

1. same Container always. With referrer.

2. default container always... (this is the current setup). No referrer.

3. if the host/origin is the same we use the same container (with referrer), otherwise we use the default one (no referrer).

My patch implements 3, but it's easy to change.
If you like 3, what about if tab1 loads 'www.foobar.com' and the link is 'ticket.foobar.com' ?
Attachment #8758310 - Attachment is obsolete: true
Flags: needinfo?(bram)

Comment 6

a year ago
P1 because of referrer leakage.
Priority: -- → P1
Whiteboard: [domsecurity-active] → [domsecurity-active][usercontextId]
(Assignee)

Comment 7

a year ago
The referrer leakage is a bigger and separate problem. I filed bug 1277765 for that.

About this bug: Bram, note that from the context menu, "Open link in a New Tab" opens a tab using the default container.
So, if we want to be consistent with the context menu, we should mark this bug as 'wontfix'.
Status: ASSIGNED → NEW
Depends on: 1277765
(Reporter)

Comment 8

a year ago
One might also consider "fixing" this by allowing a shortcut "Open link in a New <type> Tab" where <type> is the current container, including color and icon. One could still find the other container types behind the nested selection of all containers that already exists.
Frederik Braun’s suggestion here aligns with what I’ve suggested over email in a related problem:

When I’m in container ‘foo’:

* cmd+click opens the link in container ‘foo’
* Right click produces these options (in this order)
  * Open Link in a New ‘foo’ Tab
  * Open Link in a New Tab
  * Open Link in a New Container Tab
    * containername2
    * containername3
    * (‘foo’ is not listed, because I’m currently on ‘foo’ container’)

This means that the menu changes depending on the containers you’re currently in. What do you think?
Flags: needinfo?(bram)
(Assignee)

Comment 10

a year ago
Created attachment 8760742 [details] [diff] [review]
link.patch

In order to apply this patch, you must apply the dependence as well: bug 1277765
Attachment #8758651 - Attachment is obsolete: true
Attachment #8760742 - Flags: review?(gijskruitbosch+bugs)

Updated

a year ago
Status: NEW → ASSIGNED
Comment on attachment 8760742 [details] [diff] [review]
link.patch

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

Can I make an Nth suggestion? If I have a link in a particular container tab, and open it in a new tab / window / whatever, I would still expect this to be using the same container. If I want to open it somewhere else (different container / no container), that should be the non-default / additional menu option. Much like e.g. in a private window, all the modifier-click and context menu click options will keep the link private. Taking myself out of that state requires the jumping through hoops, not the other way around. I think that behaviour should govern containers as well.

Either way, there's a number of issues below, so no r+ on account of those issues.

::: browser/base/content/browser-context.inc
@@ +53,5 @@
>        <menuitem id="context-openlinkincurrent"
>                  label="&openLinkCmdInCurrent.label;"
>                  accesskey="&openLinkCmdInCurrent.accesskey;"
>                  oncommand="gContextMenu.openLinkInCurrent();"/>
> +# label and usercontextid are dinamically set.

Nit: dynamically

@@ +68,5 @@
>              label="&openLinkCmdInContainerTab.label;"
>              accesskey="&openLinkCmdInContainerTab.accesskey;"
>              hidden="true">
>          <menupopup oncommand="gContextMenu.openLinkInTab(event);"
> +                   onpopupshowing="return gContextMenu.createUserContextMenu(event);" />

I'm not an amazing fan of the abstraction of what this does into an identically named function on gContextMenu that does something different from the version in utilityOverlay.js

Don't we want the same change for the other menus like this one anyway, if this is the UI we're pursuing?

::: browser/base/content/utilityOverlay.js
@@ +408,5 @@
>  
>  // Populate a menu with user-context menu items. This method should be called
>  // by onpopupshowing passing the event as first argument. addCommandAttribute
>  // param is used to set the 'command' attribute in the new menuitem elements.
> +function createUserContextMenu(event, addCommandAttribute = true, excludeUserContextId = 0) {

This conflicts with the work in bug 1272416.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +447,5 @@
>  <!ENTITY searchSetAsDefault.accesskey "D">
>  
>  <!ENTITY openLinkCmdInTab.label       "Open Link in New Tab">
>  <!ENTITY openLinkCmdInTab.accesskey   "T">
> +<!ENTITY openLinkCmdInSameContainerTab.accesskey "O">

This conflicts with the access key for 'open link', a few lines below.
Attachment #8760742 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8760742 [details] [diff] [review]
> link.patch
> 
> Review of attachment 8760742 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can I make an Nth suggestion? If I have a link in a particular container
> tab, and open it in a new tab / window / whatever, I would still expect this
> to be using the same container. If I want to open it somewhere else
> (different container / no container), that should be the non-default /
> additional menu option. Much like e.g. in a private window, all the
> modifier-click and context menu click options will keep the link private.
> Taking myself out of that state requires the jumping through hoops, not the
> other way around. I think that behaviour should govern containers as well.

--> ni bram.
Flags: needinfo?(bram)
(Assignee)

Comment 13

a year ago
> > Can I make an Nth suggestion? 

This is what I implemented. Am I wrong? By default, right click, or context-menu (first item) is: Open link in the 'FooBar' container. So we keep the same container, always.
(Assignee)

Comment 14

a year ago
> >  <!ENTITY openLinkCmdInTab.label       "Open Link in New Tab">
> >  <!ENTITY openLinkCmdInTab.accesskey   "T">
> > +<!ENTITY openLinkCmdInSameContainerTab.accesskey "O">
> 
> This conflicts with the access key for 'open link', a few lines below.

I invented it :) I should ask bram for this. Bram what is the best accessKey for this new 'Open link in a new 'Foobar' Tab' entry in the context menu?
(Assignee)

Comment 15

a year ago
> Don't we want the same change for the other menus like this one anyway, if
> this is the UI we're pursuing?

This is what we want only for the context menu. The other menus are kept as they are so far.
(In reply to :Gijs Kruitbosch from comment #11)
> Much like e.g. in a private window, all the
> modifier-click and context menu click options will keep the link private.
> […] I think that behaviour should govern containers as well.

This sounds logical and reasonable. +1.

My original thinking was to make same-origin links open in the same container, while links that point to different domains should open elsewhere. But this gets complex. For example, what if the domain being opened has been opened before in the other container?

Following the Private Browsing behaviour makes it somewhat easier to think about. All links open in the same container by default, and if I want it to open in a different container (including the default container), I need to right-click and select which container I’d like to open it with.

For what it’s worth, I think that there should be a way to make container limited to a domain or a certain set of domains (although this is a hard problem to solve).

For example, on my home session, I want to be signed into both google.com and youtube.com. On my work session, I only want to be signed into google.com. One day, my colleague sends me a YouTube link on my work email. The ideal situation is for Firefox to always open the right site in the right session, with little to no tweaking on my part.
(In reply to Andrea Marchesini (:baku) from comment #14)
> > >  <!ENTITY openLinkCmdInTab.label       "Open Link in New Tab">
> > >  <!ENTITY openLinkCmdInTab.accesskey   "T">
> > > +<!ENTITY openLinkCmdInSameContainerTab.accesskey "O">
> > 
> > This conflicts with the access key for 'open link', a few lines below.
> 
> I invented it :) I should ask bram for this. Bram what is the best accessKey
> for this new 'Open link in a new 'Foobar' Tab' entry in the context menu?

Is it possible for the access key to change based on my current container? If I’m currently inside ‘foo’ container, then the access key for “Open link in a new ‘foo’ Tab” is T. If I’m inside the default container, then the access key for “Open link in a new Tab” is T.
Flags: needinfo?(bram)
(In reply to Andrea Marchesini (:baku) from comment #13)
> > > Can I make an Nth suggestion? 
> 
> This is what I implemented. Am I wrong? By default, right click, or
> context-menu (first item) is: Open link in the 'FooBar' container. So we
> keep the same container, always.

I meant that the item labeled "open link in new tab" should do what your new item "open link in the foobar container" does, and we shouldn't add new menuitems.

Additionally, your patch hasn't changed the behaviour for ctrl/alt/meta-click and middleclick.
(Assignee)

Comment 19

a year ago
> Additionally, your patch hasn't changed the behaviour for
> ctrl/alt/meta-click and middleclick.

This is done in bug 1277765
(Assignee)

Comment 20

a year ago
Created attachment 8761930 [details] [diff] [review]
link.patch

Discussing this issue with Bram, we decided to go for this:

If you are not in a container:
 Open link in a new Tab
 Open link in a container tab -> submenu [ list of containers ]

If you are in a container:
 Open link in a new '%S' Tab
 Open link in a container tab -> submenu [ no-container | list of containers ]

This patch implements this feature.
Attachment #8760742 - Attachment is obsolete: true
Attachment #8761930 - Flags: review?(gijskruitbosch+bugs)
I can confirm this implementation. This achieves the best compromise of keeping the context menu clean and making ‘open in same container’ a default behaviour, while still giving ‘no container’ a place to exist.

Andrea, the wording for the submenu item should be “No Container” to make it consistent with the rest of the items. Capitalised start of the letter, and no dash.

What do you think?
(Assignee)

Comment 22

a year ago
userContextNone.label = No Container
Already done :)
Comment on attachment 8761930 [details] [diff] [review]
link.patch

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

::: browser/base/content/nsContextMenu.js
@@ +162,4 @@
>      var shouldShow = this.onSaveableLink || isMailtoInternal || this.onPlainTextLink;
>      var isWindowPrivate = PrivateBrowsingUtils.isWindowPrivate(window);
>      var showContainers = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +

Nit: Don't add a newline please.

::: browser/base/content/utilityOverlay.js
@@ +422,5 @@
>  
>    let bundle = document.getElementById("bundle_browser");
>    let docfrag = document.createDocumentFragment();
>  
> +  // If we are exlucing a userContextId, we want to add a 'no-container' item.

Nit: excluding

@@ +429,5 @@
> +    menuitem.setAttribute("usercontextid", "0");
> +    menuitem.setAttribute("label", bundle.getString("userContextNone.label"));
> +    menuitem.setAttribute("accesskey", bundle.getString("userContextNone.accesskey"));
> +
> +    // We don't set oncommand/command attribute because if we have to exlclude

nit: "an oncommand/command attribute"
nit: "exclude"

@@ +430,5 @@
> +    menuitem.setAttribute("label", bundle.getString("userContextNone.label"));
> +    menuitem.setAttribute("accesskey", bundle.getString("userContextNone.accesskey"));
> +
> +    // We don't set oncommand/command attribute because if we have to exlclude
> +    // a userContextId we are generating the contextMenu for the rightclick and

nit: "generating a context menu", leave out "for the rightclick"
(right click is implied when talking about a context menu)
Attachment #8761930 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 24

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ea7d3d347f
'Open link in a new <container_name> Tab' in the context menu, r=gijs
Backed out for failing browser-chrome test browser_referrer_open_link_in_container_tab2.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0a3fa38dc2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=20ea7d3d347f575315495d90e5bf571ed734c6ea
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=29968656&repo=mozilla-inbound

06:09:41     INFO -  81 INFO browser_referrer_open_link_in_container_tab: policy=[undefined] rel=[undefined] http:// -> http://
06:09:41     INFO -  82 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a menupopup. -
06:09:41     INFO -  83 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a first container entry. -
06:09:41     INFO -  84 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a first container entry. -
06:09:41     INFO -  85 INFO TEST-PASS | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have a usercontextid value. -
06:09:41     INFO -  86 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | We have the right usercontextid value. - Got 1, expected 0
06:09:41     INFO -  Stack trace:
06:09:41     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
06:09:41     INFO -  chrome://mochitests/content/browser/browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js:onPopupShown:29
Flags: needinfo?(amarchesini)
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #25)
> Backed out for failing browser-chrome test
> browser_referrer_open_link_in_container_tab2.js:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0a3fa38dc2
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=20ea7d3d347f575315495d90e5bf571ed734c6ea
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=29968656&repo=mozilla-
> inbound
> 
> 06:09:41     INFO -  81 INFO browser_referrer_open_link_in_container_tab:
> policy=[undefined] rel=[undefined] http:// -> http://
> 06:09:41     INFO -  82 INFO TEST-PASS |
> browser/base/content/test/referrer/
> browser_referrer_open_link_in_container_tab2.js | We have a menupopup. -
> 06:09:41     INFO -  83 INFO TEST-PASS |
> browser/base/content/test/referrer/
> browser_referrer_open_link_in_container_tab2.js | We have a first container
> entry. -
> 06:09:41     INFO -  84 INFO TEST-PASS |
> browser/base/content/test/referrer/
> browser_referrer_open_link_in_container_tab2.js | We have a first container
> entry. -
> 06:09:41     INFO -  85 INFO TEST-PASS |
> browser/base/content/test/referrer/
> browser_referrer_open_link_in_container_tab2.js | We have a usercontextid
> value. -
> 06:09:41     INFO -  86 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/referrer/
> browser_referrer_open_link_in_container_tab2.js | We have the right
> usercontextid value. - Got 1, expected 0
> 06:09:41     INFO -  Stack trace:
> 06:09:41     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
> 06:09:41     INFO - 
> chrome://mochitests/content/browser/browser/base/content/test/referrer/
> browser_referrer_open_link_in_container_tab2.js:onPopupShown:29

And also, I just realized, I would expect this to start failing the context menu tests when we flip the switch on usercontextid stuff. And/or, ideally, we should add a test to that set of tests to verify that this works correctly.
userContextOpenLink.label = Open Link in New %S Tab

If you plan to replace %S with one of the possible containers (e.g. "Open Link in New Shopping Tab"), this is all but localizable, with issues varying from capitalization (minor) to different grammar case.

Comment 28

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6aa8a42eb1
'Open link in a new <container_name> Tab' in the context menu, r=gijs
(Assignee)

Updated

a year ago
Flags: needinfo?(amarchesini)
(Assignee)

Comment 29

a year ago
> If you plan to replace %S with one of the possible containers (e.g. "Open
> Link in New Shopping Tab"), this is all but localizable, with issues varying
> from capitalization (minor) to different grammar case.

We also want to support custom containers. At this point the name will not be localized.

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a6aa8a42eb1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Sounds like something we want to uplift for test pilot in 49.
Whiteboard: [domsecurity-active][usercontextId] → [domsecurity-active][usercontextId][uplift49+]
(In reply to Paul Theriault [:pauljt] from comment #31)
> Sounds like something we want to uplift for test pilot in 49.

The patch landed in this bug has new strings, not really happy to break string freeze so late in the aurora cycle. Also, if I understand it correctly, this feature is not enabled by default in 49?
baku, can we uplift this patch without a string change?  Can the string change part be taken out for the uplift?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 34

a year ago
The string is "Open Link in New %S Tab" and it's quite important for this bug. I cannot uplift this patch without it.
Francesco, what do you suggest here?
Flags: needinfo?(amarchesini) → needinfo?(francesco.lodolo)
My suggestion would be to let it ride the trains with 50, especially considering the feature is preffed off outside of Nightly, and I haven't heard of any plans to change that decision.

The only reason to uplift this "as is" in mozilla-aurora is if we plan to enable the feature in 49.
Flags: needinfo?(francesco.lodolo)
The reason for uplift is that we want to use it for Test Pilot in Firefox 49.  In Test Pilot, we can use an addon that does UI changes, but it cannot make platform changes.  Since this bug has platform changes, we want to uplift to 49.  Is there a way that we can skip localizing this string for 49 and just localize for 50 while still uplifting the code?  Thanks!
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #36)
> Is there a way that we can skip localizing this string for 49
> and just localize for 50 while still uplifting the code?

Create an ad-hoc version of the patch with strings hard-coded (not exposed in .properties file). Having one string in English doesn't seem an issue for international Test Pilot users, given that all experiments have been running only in English.
Andrea, can I ask for you to make a patch as per comment 37 for uplift?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 39

a year ago
Created attachment 8769565 [details] [diff] [review]
patch for m-a

Approval Request Comment
[Feature/regressing bug #]: Containers
[User impact if declined]: an important feature will be missing for test-pilot.
[Describe test coverage new/current, TreeHerder]: it's green on m-i
[Risks and why]: none, this feature is not exposed to content
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8769565 - Flags: approval-mozilla-aurora?
Comment on attachment 8769565 [details] [diff] [review]
patch for m-a

Part of Test Pilot work, please uplift.
Attachment #8769565 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox49: --- → affected
patch does not apply cleanly to aurora:

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 2
adding 1276880 to series file
renamed 1276880 -> link.patch
applying link.patch
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh link.patch
Flags: needinfo?(amarchesini)
(Assignee)

Comment 42

a year ago
Created attachment 8771779 [details] [diff] [review]
m-a
Attachment #8769565 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8771779 - Attachment is patch: true

Comment 43

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6dcf10108bd
status-firefox49: affected → fixed
You need to log in before you can comment on or make changes to this bug.