1549204 - Consider a better approach to clearing data if container-using extensions get disabled/uninstalled

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

To avoid having data that is effectively inaccessible to users and may linger "forever", we clear site data and container configuration:

  async observe(aSubject, aTopic) {
    if (aTopic === "nsPref:changed") {
      const contextualIdentitiesEnabled = Services.prefs.getBoolPref(CONTEXTUAL_IDENTITY_ENABLED_PREF);
      if (!contextualIdentitiesEnabled) {
        await this.closeContainerTabs();
        this.notifyAllContainersCleared();
        this.resetDefault();
      }
    }
  },

https://searchfox.org/mozilla-central/rev/b2015fdd464f598d645342614593d4ebda922d95/toolkit/components/contextualidentity/ContextualIdentityService.jsm#142

Unfortunately, that isn't always expected, like in the recent addon-armageddon.

Ideally, we should come up with a way to avoid this, whether that is user-prompting, or waiting until after a restart and then prompting, or something else.

Comment 1

[:Gijs (he/him)]

Perhaps we can distinguish the appDisabled case and not flip the containers pref then.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding to release notes as a known issue for 66.0.4.

Comment 3

[Johann Hofmann [:johannh]]

Yeah, that is... unfortunate. I feel like clearing all container data when flipping this pref is not a good idea in general. Couldn't the WebExtension code recognize a situation where "all container-related add-ons have been removed" and ask for data removal then?

I'm also not sure whether this whole thing is necessary at all. Most™️ (but not all, see bug 1541885) of this data will be cleared with any regular Clear History/Forget about this site action anyway.

Comment 4

[Jonathan Kingston [:jkt] he/him]

The bigger concern initially was having an addon that created many containers that need to be thrown away. Simplifying all of this code would make me happy. I think the prompt suggestion is the simplest solution with minimal discussion required that could be implemented quickly.

Going forward there was a suggestion to making containers default enabled but not have any default containers (the long press menu wouldn't be visible until the user had a single container). This would make the about:preferences code simpler and most users still never see the containers feature without an addon.

I would prefer if addon created containers were prompted for removal when the addon is removed rather than all container data.

Comment 6

[Tanvi Vyas[:tanvi]]

(In reply to Jonathan Kingston [:jkt] from comment #4)

Going forward there was a suggestion to making containers default enabled but not have any default containers (the long press menu wouldn't be visible until the user had a single container). This would make the about:preferences code simpler and most users still never see the containers feature without an addon.

We could have a separate pref to include/exclude the pre-defined containers. What are the pros and cons of leaving containers enabled in all Firefox branches without any pre-defined containers?

Comment 7

[Jonathan Kingston [:jkt] he/him]

We could have a separate pref to include/exclude the pre-defined containers. What are the pros and cons of leaving containers enabled in all Firefox branches without any pre-defined containers?

Pros:

• Familiarity - Users could discover this feature naturally rather than it appear after a container addon is enabled
• Reduced complexity in this clearing code, rather than the set of prefs to observe on extension uninstall. We already clean up data on a container removal so we wouldn't need anything extension related
• Removal of an extension related pref
• Any other conditional container checks would be removed reducing the number of prefs to be observed

Cons:

• UI would be exposed to everyone, however it's pretty buried

We could have a separate pref to include/exclude the pre-defined containers.

I wouldn't add this complexity, I suggest we just roll out containers to all users that don't have it enabled after removing the code that initialises the container set. We could also remove all of the resetDefault code too.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Can you ni? the appropriate product person to make a determination on releasing to all users? That way we can then address bug 1554040.

Comment 10

[Jonathan Kingston [:jkt] he/him]

Hey :pdol I think this would be your call.

TL;DR If we can enable some of this UI based on number of containers this issue should go away. Essentially all UI that is gated on the containers.enabled pref would be switched to look at the number of containers. For release build users, we wouldn't supply any containers by default so the users would have to discover it through about:preferences.

Does that sound like a resonable change that we can make?

Comment 12

[Tanvi Vyas[:tanvi]]

(In reply to Jonathan Kingston [:jkt] from comment #10)

Hey :pdol I think this would be your call.

TL;DR If we can enable some of this UI based on number of containers this issue should go away. Essentially all UI that is gated on the containers.enabled pref would be switched to look at the number of containers.
So containers would be on for all users, but the predefined containers wouldn't exist. Hence, it should no effect to non-containers users. But for Containers users, they won't lose their container data if something like armagaddon happens again. Arthur, does this sound okay to you?

For release build users, we wouldn't supply any containers by default so the users would have to discover it through about:preferences.

Discover what through about:preferences? The pref to turn containers on? I think that UI is not exposed in release. We could expose it, but that is also a product call.

Does that sound like a resonable change that we can make?

Note we should have plans in place for users who uninstall all containers addons and haven't set the pref to enable containers themselves - we should delete the container data in that case. So that it isn't laying around under the covers. If the pref to enable containers is always on for everyone, then we will need to do something extra to delete this data. Previously, we should turned the pref off and the data was deleted.

Comment 13

[Shane Caraveo (:mixedpuppy)]

From reading the backlog, and after having gone through a lot of extension settings code/fixes, I'd consider, as others have said, that we only flip the pref when all container extensions have been uninstalled. We currently flip the pref if one is disabled, but that may happen (in the future) just by entering safe mode. Actually, one patch I need to address safe mode/extension issues will flip this pref.

If any container extension is installed, we enable
If any container extension is enabled (we could lock the pref at this point)
If all container extensions are disabled, it is unlocked. If the user turns it off at that point, data is removed.
If all container extensions are uninstalled, we reset the pref to what it was initially (ie would remain user set if that were the case)

Even with the above, it seems a little unreasonable to remove data when this is flipped by the user in preferences and not notify them first.

So, if the above extension changes are acceptable, lets make a new bug for that part, but leave this one for dealing with the data loss in the underlying system.

Comment 15

[Luke Crouch [:groovecoder]]

This came up again in the context of "Refresh Firefox" removing someone's Containers data completely, and we have 50+ upvotes on the bug in the Multi-Account Containers repo.

Since :pdol arthur are not on Firefox anymore, ... :mconca - can we get some help here?

Comment 16

[Mike Conca [:mconca]]

As a containers user myself, I've felt the pain here. I'd like to see us eventually address containers as an entire feature surface--improving the UX, exposing their value in an easy to approach manner and, yes, fix rough edges like this bug. That is not currently on any roadmap I'm aware of. I'm supportive, but in the end it will come down to priorities and resources.

Comment 18

[Francis :mckenfra]

Attachment: Bug 1549204 - Prevent containers being reset if extensions are disabled. r=robwu

Currently, when all extensions using contextualIdentities are disabled, preference
'privacy.userContext.enabled' gets set to false (on Stable, not Nightly).

ContextualIdentityService observes this preference, and deletes all the containers.

This patch adds a new preference 'privacy.userContext.keepData'. This new preference
persists until all extensions using contextualIdentities are uninstalled.

ContextualIdentityService observes this preference, and only deletes all containers
when both 'privacy.userContext.enabled' and 'privacy.userContext.keepData' are false.

Note: extensions are not fully uninstalled until Firefox is restarted. Therefore
after uninstalling all extensions using contextualIdentities, the containers
are not actually deleted until Firefox is restarted.

Comment 19

[Francis :mckenfra]

Attachment: Bug 1549204 - Prevent containers being reset if extensions are disabled. r=robwu

Currently, when all extensions using contextualIdentities are disabled, preference
'privacy.userContext.enabled' gets set to false (on Stable, not Nightly).

ContextualIdentityService observes this preference, and deletes all the containers.

This patch adds a new preference 'privacy.userContext.installed'. This new preference
persists until all extensions using contextualIdentities are uninstalled.

ContextualIdentityService observes this preference, and only deletes all containers
when both 'privacy.userContext.enabled' and 'privacy.userContext.installed' are false.

Comment 20

[Francis :mckenfra]

Attachment: Bug 1549204 - Add unit test. r=robwu

Depends on D158567

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 5 duplicates and 14 votes.
:3ecdbelo, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 23

[Luke Crouch [:groovecoder]]

We're doing out monthly Container triage meeting now, and this has come up again as a really bad bug. Any chance to bump this up in priority and severity? For Containers users, it destroys ALL of their container & assignment data.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1549204 extension disable/uninstall should never disable containers

Comment 26

[Shane Caraveo (:mixedpuppy)]

The attached patch addresses the issue of extensions causing data loss, however it does not address the underlying container module which should be fixed on it's own.

Comment 27

[B.J. Herbison]

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)

The attached patch addresses the issue of extensions causing data loss, however it does not address the underlying container module which should be fixed on it's own.

Extensions causing data loss might be fixed, but data loss still exists. Nightly decided to restart with no containers. I was able to restore it by disabling/enabling extensions and restarting, but my containers and the cookies/login contexts in the containers were lost.

Comment 28

[:Gijs (he/him)]

Is there something preventing the add-on patch here from landing? And/or, do we have (other/additional) concrete next steps here?

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24a8bfb2dd2d
extension disable/uninstall should never disable containers r=zombie

Comment 30

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/24a8bfb2dd2d