Closed Bug 1288029 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Security, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files, 7 obsolete files)

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.
Flags: needinfo?(bram)
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?
Whiteboard: [userContextId][domsecurity-backlog]
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.
Attached image 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: nobody → amarchesini
Attachment #8783556 - Flags: review?(gijskruitbosch+bugs)
Attachment #8783557 - Flags: review?(gijskruitbosch+bugs)
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)
Attached patch part 3 - UI (obsolete) — Splinter Review
Attachment #8783558 - Flags: review?(gijskruitbosch+bugs)
Attached patch part 3 - UI (obsolete) — Splinter Review
Attachment #8783558 - Attachment is obsolete: true
Attachment #8783558 - Flags: review?(gijskruitbosch+bugs)
Attachment #8783560 - Flags: review?(gijskruitbosch+bugs)
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 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)
Attached patch in_use.patch (obsolete) — Splinter Review
Attachment #8783556 - Attachment is obsolete: true
Attachment #8783557 - Attachment is obsolete: true
Attachment #8783560 - Attachment is obsolete: true
Attachment #8783571 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8783571 [details] [diff] [review]
in_use.patch

I didn't see your latest review.
Attachment #8783571 - Flags: review?(gijskruitbosch+bugs)
Attached patch in_use.patch (obsolete) — Splinter Review
Attachment #8783571 - Attachment is obsolete: true
Attachment #8783580 - Flags: review?(gijskruitbosch+bugs)
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)
Attached patch in_use.patch (obsolete) — Splinter Review
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 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)
> 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)
Attached patch in_use.patchSplinter Review
Attachment #8783601 - Attachment is obsolete: true
Attachment #8783753 - Flags: review?(gijskruitbosch+bugs)
Attached image screenshot
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+
> 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
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
https://hg.mozilla.org/mozilla-central/rev/4f4a7f9d6e59
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1298671
Flags: needinfo?(amarchesini)
(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)
(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)
Blocks: 1307394
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.