802434 - Support resetting preferences when disabling blocklisted add-ons

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Kris Maglione [:kmag]]

Based on bug 784189 comment 2, and Asa's comments in a meeting, we should make it possible to reset preference changes made by add-ons when blocking them based on data in the blocklist entry. We might want to opt for something more general, such as including a fragment of cleanup code for a particular add-on, or otherwise just include a list of preferences (or other types of settings) which should be reset.

There would need to be a separate addons.mozilla.org bug to provide an interface for these changes.

Comment 1

[Sachin Hosmani [:sachin]]

Attachment: Resets prefs when blocklisted add-on is disabled.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 771340 [details] [diff] [review]
Resets prefs when blocklisted add-on is disabled.

>+    let prefs = [];
>+
>+    for (let x = 0; x < childNodes.length; x++) {
>+      var childElement = childNodes.item(x);
>+      if (!(childElement instanceof Ci.nsIDOMElement))
>         continue;
>-
>-      blockEntry.versions.push(new BlocklistItemData(versionRangeElement));
>+      if (childElement.localName === "prefs") {
>+        let prefElements = childElement.childNodes;
>+        for (let i = 0; i < prefElements.length; i++) {
>+          let prefElement = prefElements.item(i);
>+          if (!(prefElement instanceof Ci.nsIDOMElement) ||
>+              prefElement.localName !== "pref")
>+            continue;
>+          prefs.push(prefElement.textContent);

For consistency, I think it's best to push directly to blockEntry.prefs in the loop, like it's done with blockEntry.versions.

> 
>+        if (state === Ci.nsIBlocklistService.STATE_BLOCKED) {
>+          // It's a hard block. We must reset certain preferences.
>+          let prefs = self._getAddonPrefs(addon.id);
>+          resetPrefs(prefs);
>+         }
>+

We want to be able to do this for softblocks also.

Comment 3

[Sachin Hosmani [:sachin]]

(In reply to Jorge Villalobos [:jorgev] from comment #2)
> Comment on attachment 771340 [details] [diff] [review]
> Resets prefs when blocklisted add-on is disabled.
> 
> We want to be able to do this for softblocks also.

It works for softblocks also. See here:
https://bugzilla.mozilla.org/attachment.cgi?id=771340&action=diff#a/toolkit/mozapps/extensions/nsBlocklistService.js_sec6

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

Oh, okay. I guess the comment needs some clarification, because it sounds like we're only doing it for hard blocks.

Comment 5

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 774546 [details] [diff] [review]
Addresses the comments.

Looks good to me, but needs additional review.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 774546 [details] [diff] [review]
Addresses the comments.

Review of attachment 774546 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. 

Really needs a test though. See test_blocklistchange.js and test_blocklist_regexp.js for inspiration (add a new test file - test_blocklistchange.js is already too big).

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +918,5 @@
>  
> +    // A helper function that reverts the prefs passed to default values.
> +    function resetPrefs(prefs) {
> +      for (let i = 0; i < prefs.length; i++) {
> +        let pref = prefs[i];

New code should use for-of loops. ie:
  for (let pref of prefs) {
    ...
  }

@@ +920,5 @@
> +    function resetPrefs(prefs) {
> +      for (let i = 0; i < prefs.length; i++) {
> +        let pref = prefs[i];
> +        let branch = gPref.getBranch(pref);
> +        branch.clearUserPref("");

Don't need to get the branch for every pref (it has a performance cost). Just gPrefs.clearUserPref() will do.

Comment 8

[Sachin Hosmani [:sachin]]

Attachment: Includes test case

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 804916 [details] [diff] [review]
Includes test case

Review of attachment 804916 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go!

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/a7d9f7af109f

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a7d9f7af109f