Closed Bug 1407209 Opened 2 years ago Closed 2 years ago

Updating container extensions resets containers if there is only a single container add-on

Categories

(Firefox :: Security, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 blocking verified
firefox58 --- verified

People

(Reporter: jkt, Assigned: jkt)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [usercontextId])

Attachments

(1 file, 1 obsolete file)

In ContextualIdentityService.jsm we observe the container pref being disabled to clear up the last add-on or containers in general when they are disabled:


+++ b/toolkit/components/contextualidentity/ContextualIdentityService.jsm
   // observe() is only used to listen to container enabling pref
   async observe() {
     const contextualIdentitiesEnabled = Services.prefs.getBoolPref(CONTEXTUAL_IDENTITY_ENABLED_PREF);
     if (!contextualIdentitiesEnabled) {
       await this.closeContainerTabs();
       this.notifyAllContainersCleared();

And in the extension code we manage setting prefs through the setCallback:

ExtensionPreferencesManager.addSetting(CONTAINERS_ENABLED_SETTING_NAME, {
   prefNames: Object.keys(CONTAINER_PREF_INSTALL_DEFAULTS),

   setCallback(value) {
     if (value !== true) {
       return Object.assign(CONTAINER_PREF_INSTALL_DEFAULTS, {
         "privacy.userContext.extension": value,

The problem I am having is that CONTAINERS_ENABLED_SETTING_NAME or "privacy.extensions" is set to false on uninstall of the add-on then true again.

When this is the last extension enabled this has the side effect of clearing containers on upgrade.

So onStartup we set the setting with:
 ExtensionPreferencesManager.setSetting(extension, CONTAINERS_ENABLED_SETTING_NAME, extension.id);

However there is no other code controlling shutdown so wherever PrefManager sets this we will likely need to pass a shutdown reason the setCallback.
STR:
- Compile a container extension with ContextualIdentities permission. Ensure there are no different add-ons
- Install on the profile with about:debugging
- Open a container tab, make some custom add-ons
- Upgrade the addon to a newer version

Expected outcomes

Nothing changes

Actual

Containers are reset and tabs are closed
My previous message:

"make some custom add-ons"
Should have said:
"or make some custom containers"
Priority: -- → P1
Whiteboard: [usercontextId]
baku, this bug is no good.  Anyone with our addon (and no other container addons) in 57+ will get all their container data deleted on update.  Can you help jkt with this?
Flags: needinfo?(amarchesini)
I mentioned this in IRC, but wanted to ping you here too.

It looks like on update of addons we are flipping the prefs.
Flags: needinfo?(aswan)
This doesn't always seem to reproduce either, I added more debugging and the error is not reproducing right now.
I think in Nightly/Central you will need the following prefs to start with also.
privacy.userContext.enabled: false
privacy.userContext.extension: <blank>
privacy.userContext.longPressBehavior: 0
privacy.userContext.ui.enabled: false
privacy.usercontext.about_newtab_segregation.enabled: false
To help assist debugging we have the XPI's available here to download and install through about:addons:

https://github.com/mozilla/multi-account-containers/releases

Again using this setup again I can't reproduce any more. I can get into a state of having uninstalled the add-on and not having an extension listed in about:preferences with temporary extension however I think that is a symptom of the limitation of how we uninstall temp extensions and not the bug we are worried about with updates.
Annoyingly in removing 4.0.3 XPI via about:addons I get the following error also breaking the preference manager code which results in the about:preferences panel listing an addon that isn't installed any more too.

1507733800983	addons.xpi	WARN	Error loading bootstrap.js for @testpilot-containers: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Jonathan/AppData/Roaming/Mozilla/Firefox/Profiles/zeq7to1s.moo-pref-3/extensions/@testpilot-containers.xpi!/bootstrap.js :: <TOP_LEVEL> :: line 34"  data: no] Stack trace: resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///C:/Users/Jonathan/AppData/Roaming/Mozilla/Firefox/Profiles/zeq7to1s.moo-pref-3/extensions/@testpilot-containers.xpi!/bootstrap.js:34 < loadBootstrapScope()@resource://gre/modules/addons/XPIProvider.jsm:4364 < callBootstrapMethod()@resource://gre/modules/addons/XPIProvider.jsm:4431 < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:4799 < uninstall()@resource://gre/modules/addons/XPIProvider.jsm:5718 < doPendingUninstalls()@extensions.js:1848 < hide()@extensions.js:2977 < shutdown()@extensions.js:751 < shutdown()@extensions.js:219

Again I think this is a separate issue but perhaps we should clean up the PreferenceManager code to check for addons that failed to remove cleanly.
STR:
1. set xpinstall.signatures.required to false
2. Go to about:addons and install containers_drawer-1.0.2.xpi
3. see in about:config that "prefs have changed to make containers enabled in about:preferences you will see that the containers drawer is preventing "container tabs" from being disabled
4. Open a banking container tab and visit google.com
5. Go to about:addons in another tab and install containers_drawer-1.0.3.xpi


Expected outcome:
Container tab still to be open from 4.
userContext preferences will still be enabled

Actual:
Container tab is closed from 4.
userContext preferences will still be enabled


-------

Debugging I was using the simplest add-on I have that use containers https://github.com/jonathanKingston/container-drawer and building the addon with a custom applications,gecko,id like:

  "applications": {
    "gecko": {
      "id": "test@shoes.com",
      "strict_min_version": "57.0a1"
    }
  },
Then changing the version to build 1.0.2 and 1.0.3 xpi's.

I was also using the debugging patches in central:

https://pastebin.com/0YBYmdYx

The above changes just monitor the state of the ExtensionPreferencesManager and disabling the difference in preferences that Nightly has.

On installing the add-on:

  setting pref{"prefNames":["privacy.userContext.enabled","privacy.userContext.longPressBehavior","privacy.userContext.ui.enabled","privacy.usercontext.about_newtab_segregation.enabled","privacy.userContext.extension"]} {"id":"test@shoes.com","key":"privacy.containers","value":"test@shoes.com"}  ExtensionPreferencesManager.jsm:94


On upgrading the add-on:

  setting pref{"prefNames":["privacy.userContext.enabled","privacy.userContext.longPressBehavior","privacy.userContext.ui.enabled","privacy.usercontext.about_newtab_segregation.enabled","privacy.userContext.extension"]} {"key":"privacy.containers","initialValue":{"privacy.userContext.enabled":false,"privacy.userContext.longPressBehavior":0,"privacy.userContext.ui.enabled":false,"privacy.usercontext.about_newtab_segregation.enabled":false,"privacy.userContext.extension":""}}  ExtensionPreferencesManager.jsm:94
  setting pref{"prefNames":["privacy.userContext.enabled","privacy.userContext.longPressBehavior","privacy.userContext.ui.enabled","privacy.usercontext.about_newtab_segregation.enabled","privacy.userContext.extension"]} {"key":"privacy.containers","value":"test@shoes.com","id":"test@shoes.com"}  ExtensionPreferencesManager.jsm:94
  setting pref{"prefNames":["privacy.userContext.enabled","privacy.userContext.longPressBehavior","privacy.userContext.ui.enabled","privacy.usercontext.about_newtab_segregation.enabled","privacy.userContext.extension"]} {"id":"boob@shoes.com","key":"privacy.containers","value":"test@shoes.com"}  ExtensionPreferencesManager.jsm:94


On disabling the add-on:

  setting pref{"prefNames":["privacy.userContext.enabled","privacy.userContext.longPressBehavior","privacy.userContext.ui.enabled","privacy.usercontext.about_newtab_segregation.enabled","privacy.userContext.extension"]} {"key":"privacy.containers","initialValue":{"privacy.userContext.enabled":false,"privacy.userContext.longPressBehavior":0,"privacy.userContext.ui.enabled":false,"privacy.usercontext.about_newtab_segregation.enabled":false,"privacy.userContext.extension":""}}  ExtensionPreferencesManager.jsm:94
Following the STR from comment 9 but using a different extension (firefox_multi-account_containers versions 4.0.3 and 4.0.4) I can't reproduce what you reported.  I'll try again with your debugging patch applied in a moment.
Also can't reproduce with the patch from comment 9 applied.
Flags: needinfo?(aswan)
That is a little odd, I wasn't able to always reproduce every time. I was creating a new profile too.
I lose all my custom containers (and the defaults I deleted are resurrected) every time firefox updates. I have several different addons and one (Cookie AutoDelete) uses contextual identities, though I have experienced this problem since I started using the test-pilot addon.
I'm on FF 57.0b8 (64-bit) beta on Ubuntu Linux, btw.
I think the Bug 1368545 may be the cause of this.

The code in setPrefs:
item.initialValue || setting.setCallback(item.value);

Seems to be causing this, on update it looks like the initialValue is always being reset to, which obviously is a mistake.
So after speaking to bsilverberg this is actually expected behaviour of the ExtensionPreferenceManager which is why we are hitting this. It seems that the observer doesn't fire in Nightly nor does it always fire in dev/central on upgrade as it happens fast enough.

However this might mean a temporary patch is needed, I have an ugly patch however I'm not sure if it would get approved. This needs an uplift to 57 though.
Attached patch uglypatch.patch (obsolete) — Splinter Review
Patch to ignore the update for containers, which is causing the issue.
FYI, I submitted a patch that I think this does this much more neatly than the ugly patch. The alternate solution would be to delete containers on shutdown instead which comes with it's own issues too.
Flags: needinfo?(aswan)
I'm not sure exactly what the needinfo is.  I've never been that happy about how updates are handled in the PreferencesManager.  Flipping prefs during an upgrade just as a guard against the exceptional case of a failed update isn't great.  I think a better long-term fix would be for the PreferencesManager to leave the prefs in place when the uninstall for an upgrade (or downgrade) and attach an AddonListener.  That way it could find about failed updates but if it sees the new version install and activate successfully then it can just leave everything alone.

That doesn't solve the short-term 57 problem though :(
Flags: needinfo?(aswan)
Comment on attachment 8918944 [details]
Bug 1407209 - Add observer for preference changes whilst extensions are being updated.

https://reviewboard.mozilla.org/r/189840/#review194974

This looks like it will work, with my issues addressed, and I agree it's better than the "ugly" approach. You'll still need to pass it by a WebExtensions peer for review.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:45
(Diff revision 1)
>  Management.on("shutdown", (type, extension) => {
>    switch (extension.shutdownReason) {
>      case "ADDON_DISABLE":
>      case "ADDON_DOWNGRADE":
>      case "ADDON_UPGRADE":
> +      Services.obs.notifyObservers(null, "web-extension-preferences-updating");

I think you only want to do this for `ADDON_DOWNGRADE` and `ADDON_UPGRADE` but not for `ADDON_DISABLE`. That will break the general disable case.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:58
(Diff revision 1)
>  });
>  
>  Management.on("startup", (type, extension) => {
>    if (["ADDON_ENABLE", "ADDON_UPGRADE", "ADDON_DOWNGRADE"].includes(extension.startupReason)) {
>      this.ExtensionPreferencesManager.enableAll(extension);
> +    Services.obs.notifyObservers(null, "web-extension-preferences-updated");

I suppose it's safe to call this for every case, but, as above, I think you only need it for `ADDON_UPGRADE` and `ADDON_DOWNGRADE`.
Attachment #8918944 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8918944 [details]
Bug 1407209 - Add observer for preference changes whilst extensions are being updated.

https://reviewboard.mozilla.org/r/189840/#review195106

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:131
(Diff revision 1)
> -  async observe() {
> +    switch (aTopic) {
> +      case "web-extension-preferences-updating":
> +        this._webExtensionUpdating = true;
> +        break;
> +      case "web-extension-preferences-updated":
> +        this._webExtensionUpdating = false;

check the pref here as well. Just remove the 'break;' and add a comment saying that w ewant to check the pref when the extension is updated.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:58
(Diff revision 1)
>  });
>  
>  Management.on("startup", (type, extension) => {
>    if (["ADDON_ENABLE", "ADDON_UPGRADE", "ADDON_DOWNGRADE"].includes(extension.startupReason)) {
>      this.ExtensionPreferencesManager.enableAll(extension);
> +    Services.obs.notifyObservers(null, "web-extension-preferences-updated");

Better to have web-extension-preferences-updating and -updated dispatched for similar cases.
Attachment #8918944 - Flags: review?(amarchesini) → review+
Nicole, this is the bug we are looking to uplift to 57.  If we don't update, essentially anyone with a Containers addon will lose all the data stored in Containers when they update the addon.  And this might also happen when they update Firefox.  jkt is trying to confirm or deny the later.
saying "essentially" because there are some caveats here, but most users won't fit into one of the caveat buckets, so there isn't much of a point in enumerating them.
I believe this is a must-fix/blocker for 57 release. Tracking it as such.
Comment on attachment 8918944 [details]
Bug 1407209 - Add observer for preference changes whilst extensions are being updated.

https://reviewboard.mozilla.org/r/189840/#review195106

> Better to have web-extension-preferences-updating and -updated dispatched for similar cases.

Maybe it actually makes more sense to have replacing and replaced? Given that downgrades are the same and that is what we are talking about here more.
Will update tomorrow for tests for this patch for: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js
Attachment #8918906 - Attachment is obsolete: true
Comment on attachment 8918944 [details]
Bug 1407209 - Add observer for preference changes whilst extensions are being updated.

https://reviewboard.mozilla.org/r/189840/#review195476

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:338
(Diff revision 4)
> +  const CONTAINERS_PREF = "privacy.userContext.enabled";
> +  async function background() {
> +    let extensionInfo = await browser.management.getSelf();
> +    if (extensionInfo.version == "1.0.0") {
> +      const containers = await browser.contextualIdentities.query({});
> +      browser.test.assertEq(containers.length, 4, "We still have a original containers");

nit: a -> the

::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:346
(Diff revision 4)
> +        color: "red",
> +        icon: "circle",
> +      });
> +    }
> +    const containers = await browser.contextualIdentities.query({});
> +    browser.test.assertEq(containers.length, 5, "We still a new container");

nit: still -> have
Attachment #8918944 - Flags: review?(aswan) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/398dfb7487ae
Add observer for preference changes whilst extensions are being updated. r=aswan,baku,bsilverberg
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/398dfb7487ae
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8918944 [details]
Bug 1407209 - Add observer for preference changes whilst extensions are being updated.

Approval Request Comment
[Feature/Bug causing the regression]: https://bugzilla.mozilla.org/show_bug.cgi?id=1354602
[User impact if declined]: Containers users will have their tabs and containers reset on upgrade of any containers extension.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not required but given 57 the steps are in: https://bugzilla.mozilla.org/show_bug.cgi?id=1407209#c9
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: I don't think it is, given that we are just adding a message and a listener to web extension code. Non Containers users won't be impacted by any of this change besides two new messages that are sent on extension upgrades.
[String changes made/needed]: N/Z
Flags: needinfo?(amarchesini)
Attachment #8918944 - Flags: approval-mozilla-beta?
Comment on attachment 8918944 [details]
Bug 1407209 - Add observer for preference changes whilst extensions are being updated.

Must fix, beta57+
Attachment #8918944 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

I have verified this issue on Mac OS 10.12, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Nightly ( 	58.0a1-20171018100140) and it is no longer reproducible.
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

I have verified this issue on Mac OS 10.13, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Beta (57.0b10-20171019140425) and it is no longer reproducible.
Status: RESOLVED → VERIFIED
Depends on: 1549204
You need to log in before you can comment on or make changes to this bug.