Closed Bug 1316740 Opened 4 years ago Closed 4 years ago

Deleting all your container tabs doesn't remove the top colored border


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




Tracking Status
firefox53 --- fixed


(Reporter: pdehaan, Assigned: baku)


(Blocks 1 open bug)


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


(1 file, 1 obsolete file)

1. Go to about:preferences#containers and make sure you have a few different tab containers (with different icons/colors).
2. Open a bunch of tabs in different tab containers.
3. Delete all your tab containers.

### Actual results:
All your previously open tabs that were part of tab containers (and have the lovely colored top border, still have a top border, even though the tab container has been deleted).

### Expected results:
After a tab container is deleted, we redraw all tabs in that container and remove the colored border.
If you try to delete a container that has a currently opened tab, we are supposed to show an alert to the user that asks them to close the open container tabs.  Is that not the case yet?
(In reply to Tanvi Vyas [:tanvi] from comment #1)
> If you try to delete a container that has a currently opened tab, we are
> supposed to show an alert to the user that asks them to close the open
> container tabs.  Is that not the case yet?

It looks like this part isn't working as expected. You won't receive any prompts when removing containers that are currently opened. This applies to both the default and custom containers.

When you remove a container that's still opened via about:preferences#containers, the following occurs:

* the awesomebar indicators are removed
* the container is removed from "File -> New Container Tab"
* the container is removed from the Hamburger Menu
* the container is removed from the right click context menu
* the container is removed from the "down arrow -> New Container Tab"
* the tab indicator at the top of each tab is not being removed
* loading the websites in the tab once you've removed the container will still use the userContextId

The default behaviour that was supposed to be implemented in bug#1267916 was to prompt the user whenever they're removing a container that's still being used/opened.
Priority: -- → P1
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-backlog][OA]
Duplicate of this bug: 1316747
Attached patch delete_container.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8811872 - Flags: review?(jkt)
Attachment #8811872 - Flags: review?(jkt) → review?(gijskruitbosch+bugs)
Comment on attachment 8811872 [details] [diff] [review]

Review of attachment 8811872 [details] [diff] [review]:

Close, but I might as well look at this again after the changes.

::: browser/components/contextualidentity/test/browser/browser_count_and_remove.js
@@ +27,4 @@
>    openTabInUserContext(1);
> +  is(ContextualIdentityService.countAllContainerTabs(), 2, "2 container tabs created");
> +  is(ContextualIdentityService.countContainerTabs(1), 2, "2 container tabs created");

Please update this message and the ones below to include "with id 1/2" as appropriate.

::: browser/components/preferences/in-content/containers.js
@@ +42,5 @@
> +    let userContextId = parseInt(button.getAttribute("value"), 10);
> +
> +    let count = ContextualIdentityService.countContainerTabs(userContextId);
> +    if (count == 0) {
> +      ContextualIdentityService.remove(userContextId);

To avoid duplication and the early return, just put the confirmEx usage in an if (count > 0) instead? If the rv of the confirmEx != 0, bail out before removing stuff.

::: browser/locales/en-US/chrome/browser/preferences/
@@ +196,5 @@
> +
> +# LOCALIZATION NOTE (removeContainerMsg): Semi-colon list of plural forms.
> +# See:
> +# #S is the number of container tabs
> +removeContainerMsg=If you remove this Container now, #S container tab will be closed. Are you sure you want to remove this Container?;If you remove this Container now, #S container tabs will be closed. Are you sure you want to remove this Container?

Not sure about capitalizing "Container" in this sentence. We don't do that with "website", "page", "tab" or other concepts. Do we really want to always capitalize this (not just in all-caps-titles) ?

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +298,5 @@
>      this._forEachContainerTab(function() { ++count; });
>      return count;
>    },
> +  closeContainerTabs(userContextId) {

For both this and the counting method, it'd be more idiomatic JS to have an optional userContextId argument, and just check if that was not provided (ie falsy, as a userContextId of 0 is meaningless anyway) or if it matches the usercontextid of the tab, before counting/removing the tab. Doing that would also make this a smaller patch. :-)
Attachment #8811872 - Flags: review?(gijskruitbosch+bugs)
Attachment #8811872 - Attachment is obsolete: true
Attachment #8813672 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8813672 [details] [diff] [review]

Review of attachment 8813672 [details] [diff] [review]:

Last change below, with that r=me

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +287,4 @@
>      let count = 0;
> +    this._forEachContainerTab(function(tab) {
> +      if (!userContextId ||
> +          parseInt(tab.getAttribute("usercontextid"), 10) == userContextId) {

And now that I'm looking at this patch, you could push the extra param down into forEachContainerTab (the two methods we're changing here are the only callers, I think), and keep the callback functions the same. :-)
Attachment #8813672 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by
Add warning messages when containers are deleted, r=Gijs
Pushed by
Fix an apostrophes in, r=me
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1320376
No longer depends on: 1320376
This landed in 53 and Custom Containers landed in 52.  This means that you can't properly delete a Container in 52.  So if they are doing something "private" and expect for it to be completely deleted, there will still be remnants (existing tabs, cookies/data set by existing tabs in the contextid).  This should be okay, since the UI for Custom Containers will change once we get to release, and we recommend users us Nightly if they want to experiment with Containers.  Just wanted to note it here.
You need to log in before you can comment on or make changes to this bug.