What happens if some container tab is opened and the user disables Containers from about:preferences?

RESOLVED FIXED in Firefox 51

Status

()

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

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

(Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

a year ago
Maybe we should show a dialog asking to close all the opened container tabs?
We have the same problem in about:containers and the 'remove' button.

Bram, we need your help :) And UI as well.
(Assignee)

Updated

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

Updated

a year ago
Summary: What happens if some container tab is opened and the user disabled Containers from about:preferences? → What happens if some container tab is opened and the user disables Containers from about:preferences?

Updated

a year ago
Whiteboard: [userContextId][domsecurity-backlog]
(Assignee)

Comment 1

a year ago
What I think we should do is:

1. you cannot disable containers in about:preferences if some container tab is opened. If the user tries to disable containers, we can show a alert with a message.

2. for about:containers, for 'remove', we can use the same alert message.
Created attachment 8781656 [details]
prompt.png

Jared suggested using something similar to the prompt that users receive (image attached) when they attempt to close a private window while a download is still in progress. When a user attempts to disable the container feature while there's container tabs currently opened, we would prompt them something similar to the following:

* If you disable the container feature, all currently opened container tabs will be closed
* Cancel action (don't disable containers)
(Assignee)

Comment 3

a year ago
Created attachment 8783556 [details] [diff] [review]
part 1 - ContextualIdentityService.countContainerTabs
Assignee: nobody → amarchesini
Attachment #8783556 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 4

a year ago
Created attachment 8783557 [details] [diff] [review]
part 2 - ContextualIdentityService.closeAllContainerTabs()
Attachment #8783557 - Flags: review?(gijskruitbosch+bugs)

Comment 5

a year ago
Comment on attachment 8783556 [details] [diff] [review]
part 1 - ContextualIdentityService.countContainerTabs

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

This patch doesn't do anything.
Attachment #8783556 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 6

a year ago
Created attachment 8783558 [details] [diff] [review]
part 3 - UI
Attachment #8783558 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 7

a year ago
Created attachment 8783560 [details] [diff] [review]
part 3 - UI
Attachment #8783558 - Attachment is obsolete: true
Attachment #8783558 - Flags: review?(gijskruitbosch+bugs)
Attachment #8783560 - Flags: review?(gijskruitbosch+bugs)

Comment 8

a year ago
Comment on attachment 8783560 [details] [diff] [review]
part 3 - UI

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

::: browser/components/preferences/in-content/privacy.js
@@ +102,5 @@
> +                      (Ci.nsIPrompt.BUTTON_TITLE_IS_STRING * Ci.nsIPrompt.BUTTON_POS_1);
> +
> +    let prompter = Services.ww.getNewPrompter(window);
> +    let rv = prompter.confirmEx(title, message, buttonFlags, okButton,
> +                                cancelButton, null, null, {});

Is there a difference with Services.prompt.confirmEx and just passing |window| as the first arg? That seems like it'd be simpler...

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +178,5 @@
> +
> +disableContainersAlertTitle=Close all the Container Tabs?
> +disableContainersMsgMultiple=If you disable Containers now, %S container tabs will be closed. Are you sure you want to disable Containers?
> +disableContainersMsg=If you disable Containers now, 1 container tab will be closed. Are you sure you want to disable Containers?
> +disableContainersOkButtonMultiple=Close %S Container Tabs

Instead of the single/multiple item, you should use PluralForm.jsm and pass the number of tabs. See e.g. http://searchfox.org/mozilla-central/source/browser/components/preferences/cookies.js#594-596 for an example. This goes for both the buttons and the alert message.
Attachment #8783560 - Flags: review?(gijskruitbosch+bugs) → feedback+

Comment 9

a year ago
Comment on attachment 8783557 [details] [diff] [review]
part 2 - ContextualIdentityService.closeAllContainerTabs()

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

Can we have a test of this, especially with multiple container tabs, so we don't accidentally "optimize" the loop here and then make it break because we're removing items from the loop while iterating?

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +319,5 @@
> +
> +      let tabbrowser = win.gBrowser;
> +      for (let i = tabbrowser.browsers.length - 1; i >= 0; --i) {
> +	let currentBrowser = tabbrowser.getBrowserAtIndex(i);
> +	if (currentBrowser.hasAttribute('usercontextid')) {

Nit: Double quotes please.

Also, if these are browsers, can we just check the principal thing rather than the attribute? In fact, it seems like this code would be simpler if we just iterated through tabs instead:

for (let i = tabbrowser.tabs.length - 1; i >= 0; --i) {
  let tab = tabbrowser.tabs[i];
  let {userContextId} = tab.linkedBrowser.contentPrincipal.originAttributes;
  if (userContextId > 0) {
    tabbrowser.removeTab(tab);
  }
}

it also seems like you could just have 1 implementation here, like:

forAllContainerTabs(callback) {
 // stuff
}

and then make countAllContainerTabs and closeAllContainerTabs both call this with their own callback to either increment a counter or close the tabs in question.
Attachment #8783557 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 10

a year ago
Created attachment 8783571 [details] [diff] [review]
in_use.patch
Attachment #8783556 - Attachment is obsolete: true
Attachment #8783557 - Attachment is obsolete: true
Attachment #8783560 - Attachment is obsolete: true
Attachment #8783571 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 11

a year ago
Comment on attachment 8783571 [details] [diff] [review]
in_use.patch

I didn't see your latest review.
Attachment #8783571 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 12

a year ago
Created attachment 8783580 [details] [diff] [review]
in_use.patch
Attachment #8783571 - Attachment is obsolete: true
Attachment #8783580 - Flags: review?(gijskruitbosch+bugs)

Comment 13

a year ago
Comment on attachment 8783580 [details] [diff] [review]
in_use.patch

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

A unit-y browser-chrome test for the "remove all container tabs" thing would still be really useful.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +306,5 @@
> +	continue;
> +      }
> +
> +      let tabbrowser = win.gBrowser;
> +      for (let i = 0; i < tabbrowser.browsers.length; ++i) {

This is now a forward iterating loop, which means that in the deletion case, this will skip every tab right after a tab we remove. It also doesn't iterate over tabs, but over browsers - was there a reason for this? :-)
Attachment #8783580 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 14

a year ago
Created attachment 8783601 [details] [diff] [review]
in_use.patch

with test.
Attachment #8783580 - Attachment is obsolete: true
Attachment #8783601 - Flags: review?(gijskruitbosch+bugs)
What does this look like?  Can you add a screenshot?
Flags: needinfo?(amarchesini)

Comment 16

a year ago
Comment on attachment 8783601 [details] [diff] [review]
in_use.patch

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

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +301,5 @@
> +    let windowList = Services.wm.getEnumerator("navigator:browser");
> +    while (windowList.hasMoreElements()) {
> +      let win = windowList.getNext();
> +
> +      if (win.closed || !win.gBrowser) {

It's actually kind of odd that this lives on a toolkit JSM, now that I think about it. Do we not have a useful place in the frontend where this can live? In privacy.js itself if necessary?

@@ +306,5 @@
> +	continue;
> +      }
> +
> +      let tabbrowser = win.gBrowser;
> +      for (let i = tabbrowser.browsers.length - 1; i >= 0; --i) {

Still iterating over browsers rather than tabs, and using attributes rather than the content principal. Why? :-)

::: toolkit/components/contextualidentity/moz.build
@@ +7,5 @@
>  EXTRA_JS_MODULES += [
>      'ContextualIdentityService.jsm',
>  ]
>  
> +BROWSER_CHROME_MANIFESTS += ['tests/browser/browser.ini']

There's already a browser manifest and tests in browser/components/contextualidentity

::: toolkit/components/contextualidentity/tests/browser/browser_count_and_remove.js
@@ +7,5 @@
> +
> +function openTabInUserContext(userContextId) {
> +  let tab = gBrowser.addTab("about:blank", {userContextId});
> +  gBrowser.selectedTab = tab;
> +  tab.ownerGlobal.focus();

Why are you calling focus()? This shouldn't be necessary.

@@ +15,5 @@
> +  // make sure userContext is enabled.
> +  yield new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true]
> +    ]}, resolve);

You don't need half of this:

yield SpecialPowers.pushPrefEnv({"set": [[...]]});

works (pushPrefEnv returns a promise).
Attachment #8783601 - Flags: review?(gijskruitbosch+bugs)
Maybe the message prompt can be something along this line:

Close All Container Tabs?

If you disable Container Tabs now, {number} container tabs will be closed.

[Keep enabled] [Close {number} container tabs]
Flags: needinfo?(bram)
(Assignee)

Comment 18

a year ago
> It's actually kind of odd that this lives on a toolkit JSM, now that I think
> about it. Do we not have a useful place in the frontend where this can live?
> In privacy.js itself if necessary?

The reason why I think this is a good place is because in the future with about:containers (or similar page) we need the same feature when a container is deleted. ContextualIdentityService is already used for styling tabs, so it's kind of good component to do this task as well.

I cannot use contentPrincipal.originAttributes, because if the page contains an about:something, it uses system principal where originAttributes.userContextId is always 0.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 19

a year ago
Created attachment 8783753 [details] [diff] [review]
in_use.patch
Attachment #8783601 - Attachment is obsolete: true
Attachment #8783753 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 20

a year ago
Created attachment 8783838 [details]
screenshot

Comment 21

a year ago
Comment on attachment 8783753 [details] [diff] [review]
in_use.patch

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

So should the ContextualIdentityService.jsm really live in browser, if it is hardcoded to deal specifically with tabbrowser/Firefox UI?
Attachment #8783753 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 22

a year ago
> So should the ContextualIdentityService.jsm really live in browser, if it is
> hardcoded to deal specifically with tabbrowser/Firefox UI?

We moved to toolkit in bug 1285889 for use it in modules such as ForgetAboutSite.jsm

Comment 23

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4a7f9d6e59
Close container tabs when Containers are disabled from about:preferences, r=gijs

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f4a7f9d6e59
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Both plural strings require a comment before
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms
Flags: needinfo?(amarchesini)
(Assignee)

Updated

a year ago
Blocks: 1298671
Flags: needinfo?(amarchesini)

Comment 26

a year ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> https://hg.mozilla.org/mozilla-central/rev/4f4a7f9d6e59
> 
>> +disableContainersMsg=If you disable Container Tabs now, #S container tab will be closed. Are you sure you want to disable Container Tabs?;If you disable Containers Tabs now, #S container tabs will be closed. Are you sure you want to disable Containers Tabs?

Shouldn’t the plural form use "Container Tabs" instead of "ContainerS Tabs" (2x)? (Might be overlooked when adding "Tabs")
As far as I can tell "Container" is used as an adjective in this case (even if it's a noun), so "Container tabs" is correct (like "Private tabs")
Were specs followed for these strings?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 29

a year ago
(In reply to Francesco Lodolo [:flod] from comment #28)
> Were specs followed for these strings?

As far as I know, we don't have specs for these strings.
Flags: needinfo?(amarchesini) → needinfo?(tanvi)
Bram, do we need to run these by Michelle?  The strings are only present in Nightly, as the Containers project is going through a redesign with Test Pilot.

Either way, we shouldn't make Containers plural here... it should always be Container Tabs, not Containers Tabs.  baku, can you correct that?
Flags: needinfo?(tanvi)
Flags: needinfo?(bram)
Flags: needinfo?(amarchesini)
Hi Tanvi, saying “Container Tabs” for the plural form is the right thing to do. We’ll consult Michelle when we’re about to land in Test Pilot and beyond.
Flags: needinfo?(bram)
(Assignee)

Updated

a year ago
Blocks: 1307394
(Assignee)

Comment 32

a year ago
Bug 1307394 is about fixing the plural form of 'Container Tabs'.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.