We should not expose Containers in privateBrowsing

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
It seems super confusing having containers in private Browsing. We should disable the entry in the menus and disable the hamburger icon.
(Assignee)

Updated

2 years ago
Flags: needinfo?(bram)

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Whiteboard: [userContextId]
Whiteboard: [userContextId] → [userContextId][domsecurity-active]

Comment 1

2 years ago
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.
Flags: needinfo?(bram)
(Assignee)

Comment 2

2 years ago
Created attachment 8750239 [details] [diff] [review]
privateBrowsing.patch
Attachment #8750239 - Flags: review?(gijskruitbosch+bugs)

Comment 3

2 years ago
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.
Attachment #8750239 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8753382 [details] [diff] [review]
privateBrowsing.patch

This patch applies your comments. You can test it on top of m-i, but not m-c yet.
Attachment #8750239 - Attachment is obsolete: true
Attachment #8753382 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Depends on: 1233886

Comment 5

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

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

Thanks! Sorry for the delay.
Attachment #8753382 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/901e68a29cae
https://hg.mozilla.org/mozilla-central/rev/901e68a29cae
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.