1270471 - We should not expose Containers in privateBrowsing

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

It seems super confusing having containers in private Browsing. We should disable the entry in the menus and disable the hamburger icon.

Comment 1

[Bram Pitoyo [:bram]]

Yes. On private windows, all containers access points should be disabled.

It might be smart to follow the OS convention on hiding “New Container Tab” inside the File-Edit-View menu: when Containers is unavailable, disable the menu item but don’t hide it. For example, when your clipboard is empty, the “Paste” menu item is greyed out but doesn’t disappear. Containers should follow this rule, too.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: privateBrowsing.patch

Comment 3

[:Gijs (he/him)]

Comment on attachment 8750239 [details] [diff] [review]
privateBrowsing.patch

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

::: browser/base/content/browser.js
@@ +4026,5 @@
> +  menu.hidden = !Services.prefs.getBoolPref("privacy.userContext.enabled");
> +  if (PrivateBrowsingUtils.isWindowPrivate(window)) {
> +    menu.setAttribute("disabled", "true");
> +  } else {
> +    menu.removeAttribute("disabled");

A window can't change private state, and the disabled attribute is not there by default, so I think we're OK to not have the else clause here.

::: browser/base/content/tabbrowser.xml
@@ +6582,5 @@
>            return createUserContextMenu(event);
>          }
>  
>          let containersEnabled = Services.prefs.getBoolPref("privacy.userContext.enabled");
>          document.getElementById("alltabs-popup-separator1").hidden = !containersEnabled;

This patch doesn't apply on m-c, and I don't understand what this separator is - searching DXR or checking browser.xul on hg.m.o gets me no hits, and there's no direct deps for this bug - shouldn't that also be hidden if containers are not enabled in this window?

@@ +6588,5 @@
> +
> +        containersTab.hidden = !containersEnabled;
> +        containersTab.setAttribute("disabled",
> +                                   PrivateBrowsingUtils.isWindowPrivate(window)
> +                                     ? "true" : "false");

"alltabs_containersTab" too has no hits. Here, too, I think we can just:

if (containersEnabled && PrivateBrowsingUtils.isWindowPrivate(window)) {
  containersTab.setAttribute("disabled", "true");
}

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1118,5 @@
> +
> +      if (PrivateBrowsingUtils.isWindowPrivate(win)) {
> +        aNode.setAttribute("disabled", "true");
> +      } else {
> +        aNode.removeAttribute("disabled");

Again, not sure we need the 'else' clause.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: privateBrowsing.patch

This patch applies your comments. You can test it on top of m-i, but not m-c yet.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8753382 [details] [diff] [review]
privateBrowsing.patch

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

Thanks! Sorry for the delay.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/901e68a29cae

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/901e68a29cae