648599 - When an extension changes from being softblocked to unblocked it should become enabled

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

When an add-on is softblocked we automatically disabled it unless the user takes action in the UI. In this case if the add-on later becomes unblocked we should re-enable it. The add-on can become unblocked for the following reasons:

* App updates so the blocklist entry no longer applies
* Add-on updates to a safe version
* Blocklist entry is removed

If the user manually enables/disables the add-on then we should not re-enable it later.

Comment 1

[Tony Mechelynck [:tonymec]]

(In reply to comment #0)
[...]
> If the user manually enables/disables the add-on then we should not re-enable
> it later.

Indeed. I think a general rule should be: "An auto-disable (for any reason, also e.g. version compatibility) should be reversed as soon as the reason for it disappears; a manual disable can only be reversed by a manual enable."

IMO there should be some indication in the EM of whether the disable was manual (and won't be reversed behind the user's back, but can presumably be manually reversed at will) or automatic (and may be reversed with no user action if the reason for it disappears, but can presumably not be reversed by "only" a manual enable). But maybe this should be a different bug, with "enhancement" severity and "polish" keyword.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This adds a softDisabled property for add-ons in the database. When the startup code or blocklist UI disables an add-on for being softblocked it sets this to true. If a user manually disabled/enables an add-on the softDisabled property gets lost. If something causes an add-on to change to unblocked and it still has softDisabled set then it will become enabled again.

The code has to start remembering the old versions of the application and platform and passing them through the startup code since we have to do different things depending on whether an add-on was blocked in the previous app or not.

Rather long testcase covering every scenario I could find, has some notes at the top for what is going on but basically runs the same test for different ways that add-ons can become blocked/unblocked.

Also added an observer notification for when the blocklist service is done updating so I can just watch for that in test rather than hacks that I've used previously.

All of this only works for XPI add-ons, it doesn't work for plugins at the moment.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 525200 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -408,16 +409,71 @@ function findClosestLocale(aLocales) {
>     // If we found a valid match for this locale return it
>     if (bestmatch)
>       return bestmatch;
>   }
>   return null;
> }
> 
> /**
>+ * Updates the softDisabled and userDisabled properties of a new add-on
>+ * depending on how the blocklist state has changed. The new add-on and old
>+ * add-on may be the same version or event the same object if the application
>+ * has changed.
huh? "be the same version or event the same object"

>+ *
>+ * @param  aOldAddon
>+ *         The old version of the add-on
>+ * @param  aNewAddon
>+ *         The new version of the add-on
>+ * @param  aAppVersion
>+ *         The application version to use when checking the blocklist or
>+ *         undefined to use the current application
>+ * @param  aPlatformVersion
>+ *         The platform version to use when checking the blocklist or
>+ *         undefined to use the current platform
>+ */
>+function applyBlocklistChanges(aOldAddon, aNewAddon, aOldAppVersion,
>+                               aOldPlatformVersion) {
>+  // Carry over the userDisabled setting by default
>+  aNewAddon.userDisabled = aOldAddon.userDisabled;
>+
>+  let bs = Cc["@mozilla.org/extensions/blocklist;1"].
>+           getService(Ci.nsIBlocklistService);
>+
>+  let oldBlocklistState = bs.getAddonBlocklistState(aOldAddon.id,
>+                                                    aOldAddon.version,
>+                                                    aOldAppVersion,
>+                                                    aOldPlatformVersion)
>+  let newBlocklistState = bs.getAddonBlocklistState(aNewAddon.id,
>+                                                    aNewAddon.version)
>+  LOG(aOldAddon.id + " " + oldBlocklistState + " " + newBlocklistState + " " + aOldAddon.userDisabled + " " + aOldAddon.softDisabled);
wrap please... this msg doesn't contain enough info for me to understand what it is trying to tell me. Example output:
myaddon@myorg true false true false

>+  // If the new add-on is not softblocked then it cannot be softDisabled
>+  if (newBlocklistState != Ci.nsIBlocklistService.STATE_SOFTBLOCKED) {
>+    // If the old add-on was softDisabled then also unset userDisabled
>+    if (aOldAddon.softDisabled)
>+      aNewAddon.userDisabled = false;
>+
>+    aNewAddon.softDisabled = false;
>+  }
>+  else if (oldBlocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED ||
>+           aOldAddon.userDisabled) {
>+    // If the old add-on was softblocked or was userDisabled then just copy the
>+    // softDisabled flag
>+    aNewAddon.softDisabled = aOldAddon.softDisabled;
>+  }
>+  else {
>+    // The add-on has become softblocked so set the softBlocked flag
>+    aNewAddon.softDisabled = aNewAddon.userDisabled = true;
>+  }
>+
>+  LOG(aNewAddon.userDisabled + " " + aNewAddon.softDisabled);
same here

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 525200 [details] [diff] [review]
patch rev 1

Just talked with Dave about concerns I have with softDisabled setting userDisabled and suggested the two remain separate similar to the old extension manager. He agreed and will be resubmitting a new patch so clearing review request.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Ok this is the new patch where we actually store the softDisabled flag completely separately.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 529870 [details] [diff] [review]
patch rev 2

Sorry for taking so long... looks good overall

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -408,16 +409,63 @@ function findClosestLocale(aLocales) {
>     // If we found a valid match for this locale return it
>     if (bestmatch)
>       return bestmatch;
>   }
>   return null;
> }
> 
> /**
>+ * Updates the softDisabled and userDisabled properties of a new add-on
>+ * depending on how the blocklist state has changed. The new add-on and old
>+ * add-on may be the same version or event the same object if the application
>+ * has changed.
This comment is a tad confusing. It starts with stating "of a new add-on" and then states the "new add-on and old add-on may be the same version". Can't the new add-on and old add-on be the same? Also, "or event the same object if the application has changed" doesn't make sense to me. I think you mean they can be the same add-on when the application version has changed.

>+ *
>+ * @param  aOldAddon
>+ *         The old version of the add-on
>+ * @param  aNewAddon
>+ *         The new version of the add-on
>+ * @param  aAppVersion
>+ *         The application version to use when checking the blocklist or
>+ *         undefined to use the current application
>+ * @param  aPlatformVersion
>+ *         The platform version to use when checking the blocklist or
>+ *         undefined to use the current platform
>+ */
note that the last two are optional here and elsewhere.

>+function applyBlocklistChanges(aOldAddon, aNewAddon, aOldAppVersion,
>+                               aOldPlatformVersion) {
>...
>@@ -699,19 +751,24 @@ function loadManifestFromRDF(aUri, aStre
>   // A theme's userDisabled value is true if the theme is not the selected skin
>   // or if there is an active lightweight theme. We ignore whether softblocking
>   // is in effect since it would change the active theme.
>   if (addon.type == "theme") {
>     addon.userDisabled = !!LightweightThemeManager.currentTheme ||
>                          addon.internalName != XPIProvider.selectedSkin;
>   }
>   else {
>-    addon.userDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
>+    addon.userDisabled = false;
>   }
> 
>+  if (!addon.userDisabled)
>+    addon.softDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
>+  else
>+    addon.softDisabled = false;
Have theme's been taken into account?

>@@ -952,17 +1009,18 @@ function verifyZipSigning(aZip, aPrincip
>  *         An optional number representing the type of update, only applicable
>  *         when creating a url for retrieving an update manifest
>  * @param  aAppVersion
>  *         The optional application version to use for %APP_VERSION%
>  * @return the appropriately escaped uri.
>  */
> function escapeAddonURI(aAddon, aUri, aUpdateType, aAppVersion)
> {
>-  var addonStatus = aAddon.userDisabled ? "userDisabled" : "userEnabled";
>+  var addonStatus = aAddon.userDisabled || aAddon.softDisabled ? "userDisabled"
>+                                                               : "userEnabled";
I almost want softDisabled added to the url but I think this is ok.

>@@ -2054,28 +2119,36 @@ var XPIProvider = {
>    * @param  aState
>    *         The array of current install location states
>    * @param  aManifests
>    *         A dictionary of cached AddonInstalls for add-ons that have been
>    *         installed
>    * @param  aUpdateCompatibility
>    *         true to update add-ons appDisabled property when the application
>    *         version has changed
>+   * @param  aOldAppVersion
>+   *         The version of the application last run with this profile or null
>+   *         if it is a new profile or the version is unknown
>+   * @param  aOldPlatformVersion
>+   *         The version of the platform last run with this profile or null
>+   *         if it is a new profile or the version is unknown
>    * @param  aMigrateData
>    *         an object generated from a previous version of the database
>    *         holding information about what add-ons were previously userDisabled
>    *         and updated compatibility information if present
>    * @param  aActiveBundles
>    *         When performing recovery after startup this will be an array of
>    *         persistent descriptors of add-ons that are known to be active,
>    *         otherwise it will be null
>    * @return true if a change requiring a restart was detected
>    */
>   processFileChanges: function XPI_processFileChanges(aState, aManifests,
>                                                       aUpdateCompatibility,
>+                                                      aOldAppVersion,
>+                                                      aOldPlatformVersion,
>                                                       aMigrateData,
>                                                       aActiveBundles) {
>     let visibleAddons = {};
>     let oldBootstrappedAddons = this.bootstrappedAddons;
>     this.bootstrappedAddons = {};
> 
>     /**
>      * Updates an add-on's metadata and determines if a restart of the
>@@ -2098,18 +2171,18 @@ var XPIProvider = {
>       // Check if there is an updated install manifest for this add-on
>       let newAddon = aManifests[aInstallLocation.name][aOldAddon.id];
> 
>       try {
>         // If not load it
>         if (!newAddon) {
>           let file = aInstallLocation.getLocationForID(aOldAddon.id);
>           newAddon = loadManifestFromFile(file);
>-          // Carry over the userDisabled setting for add-ons that just appeared
>-          newAddon.userDisabled = aOldAddon.userDisabled;
>+
nit: extra line not needed

>+          applyBlocklistChanges(aOldAddon, newAddon);
>...
>@@ -3267,39 +3371,53 @@ var XPIProvider = {
>...
>   updateAddonDisabledState: function XPI_updateAddonDisabledState(aAddon,
>-                                                                  aUserDisabled) {
>+                                                                  aUserDisabled,
>+                                                                  aSoftDisabled) {
>     if (!(aAddon instanceof DBAddonInternal))
>       throw new Error("Can only update addon states for installed addons.");
> 
>     if (aUserDisabled === undefined)
>       aUserDisabled = aAddon.userDisabled;
>+    // If enabling the add-on then remove softDisabled
>+    else if (!aUserDisabled)
>+      aSoftDisabled = false;
really don't like multi-lines without braces

>+
>+    // If not changing softDisabled or the add-on is already userDisabled then
>+    // use the existing value for softDisabled
>+    if (aSoftDisabled === undefined || aUserDisabled)
>+      aSoftDisabled = aAddon.softDisabled;
Is it possible for a user to disable an add-on and soft disable to change at the same time?

>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>...
>@@ -854,29 +855,31 @@ Blocklist.prototype = {
>         if (oldAddonEntries)
>           oldState = self._getAddonBlocklistState(addons[i].id, addons[i].version,
>                                                   oldAddonEntries);
>         let state = self.getAddonBlocklistState(addons[i].id, addons[i].version);
> 
>         LOG("Blocklist state for " + addons[i].id + " changed from " +
>             oldState + " to " + state);
> 
>-        // Don't warn about add-ons becoming unblocked.
>-        if (state == 0)
>-          continue;
>-
>         // We don't want to re-warn about add-ons
>         if (state == oldState)
>           continue;
> 
>+        // Don't warn about add-ons becoming unblocked.
>+        if (state == 0) {
Ci.nsIBlocklistService.STATE_NOT_BLOCKED

>+          addons[i].softDisabled = false;
>+          continue;
Seems like the hard block state change is skipped?

>+        }
>+
>         // If an add-on has dropped from hard to soft blocked just mark it as
>         // user disabled and don't warn about it.
>         if (state == Ci.nsIBlocklistService.STATE_SOFTBLOCKED &&
>             oldState == Ci.nsIBlocklistService.STATE_BLOCKED) {
>-          addons[i].userDisabled = true;
>+          addons[i].softDisabled = true;
>           continue;
>         }
> 
>         // If the add-on is already disabled for some reason then don't warn
>         // about it
>         if (!addons[i].isActive)
>           continue;

Still reviewing the tests but minusing now so you get notified.

Comment 7

[Robert Strong (they/them - no direct email)]

Tests look good

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #6)
> >   else {
> >-    addon.userDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
> >+    addon.userDisabled = false;
> >   }
> > 
> >+  if (!addon.userDisabled)
> >+    addon.softDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
> >+  else
> >+    addon.softDisabled = false;
> Have theme's been taken into account?

Need to work out exactly what is going on with themes so I'm adding some tests for them.

> >+    // If not changing softDisabled or the add-on is already userDisabled then
> >+    // use the existing value for softDisabled
> >+    if (aSoftDisabled === undefined || aUserDisabled)
> >+      aSoftDisabled = aAddon.softDisabled;
> Is it possible for a user to disable an add-on and soft disable to change at
> the same time?

Not at exactly the same time no. I've added a check to the start of the function to ensure we never accidentally call it with both set at the same time since it won't work out right in that case.

Comment 9

[Dave Townsend [:mossop]]

For the time being this is only going to work for extensions, filed bug 657520 for themes and bug 657522 for plugins. The latest patch also fixes bug

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This addresses the comments and also makes some tweaks to the theme handling. It is looking too complex at the moment to make themes work in exactly the same way, nor does it seem to be ideal to have a theme suddenly re-enable after an update or something so for the time being the default theme just gets re-enabled when the current theme gets soft blocked and the user would have to re-enable it manually even if the theme became unblocked. It seems unlikely that we'll soft-block a theme and also unlikely that one would be installed by a third party so I don't think it is worth the work to try to fix this right now. I've added to the tests to ensure that themes behave as expected currently.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 532807 [details] [diff] [review]
patch rev 3

Per ur discussion, might be good to file a followup to add a property for disabledBy.

>diff --git a/toolkit/mozapps/extensions/AddonUpdateChecker.jsm b/toolkit/mozapps/extensions/AddonUpdateChecker.jsm
>--- a/toolkit/mozapps/extensions/AddonUpdateChecker.jsm
>+++ b/toolkit/mozapps/extensions/AddonUpdateChecker.jsm
>...
>@@ -679,20 +680,27 @@ var AddonUpdateChecker = {
>   getNewestCompatibleUpdate: function AUC_getNewestCompatibleUpdate(aUpdates,
>                                                                     aAppVersion,
>                                                                     aPlatformVersion) {
>     if (!aAppVersion)
>       aAppVersion = Services.appinfo.version;
>     if (!aPlatformVersion)
>       aPlatformVersion = Services.appinfo.platformVersion;
> 
>+    let blocklist = Cc["@mozilla.org/extensions/blocklist;1"].
>+                    getService(Ci.nsIBlocklistService);
>+
>     let newest = null;
>     for (let i = 0; i < aUpdates.length; i++) {
>       if (!aUpdates[i].updateURL)
>         continue;
>+      let state = blocklist.getAddonBlocklistState(aUpdates[i].id, aUpdates[i].version,
>+                                                   aAppVersion, aPlatformVersion);
>+      if (state != Ci.nsIBlocklistService.STATE_NOT_BLOCKED)
>+        continue;
Previously all updates were applied even ones that were blocked which is probably bad since a non-blocked add-on can be updated to a blocked add-on. Now blocked add-ons that have an update are not updated. I prefer the new behavior but I think this would be better if existing an softblocked add-on could be updated to a newer softblocked version. I'll leave it up to you if you'd like to file a followup.

>       if ((newest == null || (Services.vc.compare(newest.version, aUpdates[i].version) < 0)) &&
>           matchesVersions(aUpdates[i], aAppVersion, aPlatformVersion))
>         newest = aUpdates[i];
>     }
>     return newest;
>   },
> 
>   /**
>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -408,16 +409,71 @@ function findClosestLocale(aLocales) {
>     // If we found a valid match for this locale return it
>     if (bestmatch)
>       return bestmatch;
>   }
>   return null;
> }
> 
> /**
>+ * Sets the userDisabled and softDisabled properties of an add-on based on what
>+ * values those properties had for a previous instance of the add-on. The
>+ * previous instance may be a previous install or in the case of application
>+ * upgrade the same add-on.
nit: doesn't this also apply to downgrading? If so, how about "in the case of an application version change the same add-on"?

>+ *
>+ * @param  aOldAddon
>+ *         The previous instance of the add-on
>+ * @param  aNewAddon
>+ *         The new instance of the add-on
>+ * @param  aAppVersion
>+ *         The optional application version to use when checking the blocklist
>+ *         or undefined to use the current application
>+ * @param  aPlatformVersion
>+ *         The optional platform version to use when checking the blocklist or
>+ *         undefined to use the current platform
>+ */
>+function applyBlocklistChanges(aOldAddon, aNewAddon, aOldAppVersion,
>+                               aOldPlatformVersion) {
>...
>@@ -699,17 +759,22 @@ function loadManifestFromRDF(aUri, aStre
>   // A theme's userDisabled value is true if the theme is not the selected skin
>   // or if there is an active lightweight theme. We ignore whether softblocking
>   // is in effect since it would change the active theme.
>   if (addon.type == "theme") {
>     addon.userDisabled = !!LightweightThemeManager.currentTheme ||
>                          addon.internalName != XPIProvider.selectedSkin;
>   }
>   else {
>-    addon.userDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
>+    addon.userDisabled = false;
>+
>+    if (!addon.userDisabled)
When will addon.userDisabled ever be anything other than false give the statement previous to this one?

minusing because of this

>+      addon.softDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
>+    else
>+      addon.softDisabled = false;
>   }
>...
>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>@@ -854,29 +855,33 @@ Blocklist.prototype = {
>         if (oldAddonEntries)
>           oldState = self._getAddonBlocklistState(addons[i].id, addons[i].version,
>                                                   oldAddonEntries);
>         let state = self.getAddonBlocklistState(addons[i].id, addons[i].version);
> 
>         LOG("Blocklist state for " + addons[i].id + " changed from " +
>             oldState + " to " + state);
> 
>+        // We don't want to re-warn about add-ons
>+        if (state == oldState)
>+          continue;
>+
>+        // Ensure that softDisabled is false if the add-on is not soft blocked
>+        if (state != Ci.nsIBlocklistService.STATE_SOFTBLOCKED)
>+          addons[i].softDisabled = false;
nit: perhaps just as you do for plugins
addons[i].softDisabled = state == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;

>         // Don't warn about add-ons becoming unblocked.
>         if (state == 0)
>           continue;
please change to the following as noted in comment #6
if (state == Ci.nsIBlocklistService.STATE_NOT_BLOCKED)

>-        // We don't want to re-warn about add-ons
>-        if (state == oldState)
>-          continue;
>-
>         // If an add-on has dropped from hard to soft blocked just mark it as
>         // user disabled and don't warn about it.
This comment should be updated since it now soft disables instead of user disables.

Next patch should be good to go and we can probably work out the main issue in person if you want

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Spinning this back over try one more time but hopefully this is golden.

(In reply to comment #11)
> Comment on attachment 532807 [details] [diff] [review] [review]
> patch rev 3
> 
> Per ur discussion, might be good to file a followup to add a property for
> disabledBy.

Filed bug 657810

> Previously all updates were applied even ones that were blocked which is
> probably bad since a non-blocked add-on can be updated to a blocked add-on.
> Now blocked add-ons that have an update are not updated. I prefer the new
> behavior but I think this would be better if existing an softblocked add-on
> could be updated to a newer softblocked version. I'll leave it up to you if
> you'd like to file a followup.

Filed bug 657809

> >   else {
> >-    addon.userDisabled = addon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;
> >+    addon.userDisabled = false;
> >+
> >+    if (!addon.userDisabled)
> When will addon.userDisabled ever be anything other than false give the
> statement previous to this one?

Removed, pretty sure this was just left over from an older version of the patch.

> >+        // Ensure that softDisabled is false if the add-on is not soft blocked
> >+        if (state != Ci.nsIBlocklistService.STATE_SOFTBLOCKED)
> >+          addons[i].softDisabled = false;
> nit: perhaps just as you do for plugins
> addons[i].softDisabled = state == Ci.nsIBlocklistService.STATE_SOFTBLOCKED;

It isn't always the case that we'll want to set softDisabled=true for STATE_SOFTBLOCKED, we only do if the user didn't uncheck the box in the UI we show. I want to avoid setting it before we know that for sure.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 533114 [details] [diff] [review]
patch rev 4

I guess this is failing on tinderbox for some reason, will have to investigate tomorrow.

Comment 14

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Ok the only problem was I didn't include some files in the patch, this should be good now and is passing on try.

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 533450 [details] [diff] [review]
patch rev 4

Just skimmed over the changes... r=me

Comment 16

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/f594c196fac7

Would really like this to be covered by a manual test as well as the automated tests.

Comment 17

[Vlad [QA]]

Can you please post some clear STR in order to verify this.
Thanks.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Vlad from comment #17)
> Can you please post some clear STR in order to verify this.
> Thanks.

You would have to install an add-on, update the blocklist to one where that add-on is softblocked, accept the dialog to disable it, then update the blocklist to one where the add-on is unbloked. The add-on should become enabled again.