Open Bug 1549204 Opened 7 months ago Updated 29 days ago

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

Categories

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

defect

Tracking

()

Tracking Status
relnote-firefox --- 66+
firefox-esr60 --- affected
firefox66 --- affected
firefox67 --- affected
firefox68 --- affected

People

(Reporter: Gijs, Unassigned, NeedInfo)

References

(Blocks 3 open bugs)

Details

(Whiteboard: cert2019,[domsecurity-backlog1])

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.

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

Restrict Comments: true

Adding to release notes as a known issue for 66.0.4.

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.

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.

Duplicate of this bug: 1549238

(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?

Whiteboard: cert2019

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.

Priority: -- → P3
Whiteboard: cert2019 → cert2019,[domsecurity-backlog1]
Duplicate of this bug: 1549013
Flags: needinfo?(tanvi)

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

Flags: needinfo?(jkt)

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?

Flags: needinfo?(jkt) → needinfo?(pdolanjski)
Duplicate of this bug: 1588638

(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.

Flags: needinfo?(tanvi) → needinfo?(arthur)

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.

You need to log in before you can comment on or make changes to this bug.