585339 - Uninstalling disabled add-ons cannot be undone

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

The option to undo add-ons that are disabled doesn't show up because of a problem with the API introduced in bug 573149. The API says that disabled add-ons require a restart to uninstall when in fact they do not so the frontend gets confused.

The tests in bug 553494 don't pick this up because we use mock objects for the test cases.

Comment 1

[Dave Townsend [:mossop]]

Getting this right for themes is really complicated until bug 520124 is fixed.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This corrects some of the logic for when operations require restarts. Note that it is intended to be applied on top of the patch in bug 520124 and probably doesn't make sense without that.

Comment 5

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

Comment on attachment 468190 [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

>   /**
>    * Tests whether installing an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   installRequiresRestart: function XPI_installRequiresRestart(aAddon) {
>     // If the platform couldn't have activated up extensions then we can make
>     // changes without any restart.
>     if (!this.extensionsActive)
>       return false;
> 
>-    // Themes not currently in use can be installed immediately
>-    if (aAddon.type == "theme")
>-      return aAddon.internalName == this.currentSkin ||
>-             Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
>-
>-    return !aAddon.bootstrap;
>+    // Add-ons that are already installed don't require a restart to install.
>+    if (aAddon instanceof DBAddonInternal)
>+      return false;
>+
>+    // If we have an AddonInstall for this add-on then we can see if there is
>+    // an existing installed add-on with the same ID
>+    if ("_install" in aAddon && aAddon._install) {
>+      // If there is an existing installed add-on and uninstalling it would
>+      // require a restart then installing the update will also require a
>+      // restart
>+      let existingAddon = aAddon._install.existingAddon;
>+      if (existingAddon && this.uninstallRequiresRestart(existingAddon))
>+        return true;
>+    }
>+
>+    // If the add-on is not going to be active after installation then it
>+    // doesn't require a restart to install.
>+    if (aAddon.userDisabled || aAddon.appDisabled)
>+      return false;
>+
>+    // Themes will require a restart (even if dynamic switching is enabled due
>+    // to some caching issues) and non-bootstrapped add-ons will require a
>+    // restart
>+    return aAddon.type == "theme" || !aAddon.bootstrap;
Seems like you would want to respect dynamic switching even though it has the caching issues.

r=me with me enlightened regarding the question

Comment 6

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

(In reply to comment #5)
>...
> r=me with me enlightened regarding the question
consider me enlightened - I forgot the chrome registration needs to happen which currently requires a restart.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Ready for checkin?

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> Ready for checkin?

No, this patch depends on others that look like they may slip past b5

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This is a patch that solves the bulk of the problems without needing bug 520124. There is one bug left, operationsRequiringRestart for the default theme generally says it doesn't need a restart to disable. The problem is that for the default theme you don't know whether disabling it needs a restart without knowing if you're disabling it in order to enable a lightweight theme or to enable a different theme. This code allows the events to send the correct information and makes the UI work since we never actually consider disabling or uninstalling the default theme.

Comment 11

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

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

>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
>@@ -2258,89 +2258,133 @@ var XPIProvider = {
>    * @return true if the operation requires a restart
>    */
>   enableRequiresRestart: function XPI_enableRequiresRestart(aAddon) {
>     // If the platform couldn't have activated extensions then we can make
>     // changes without any restart.
>     if (!this.extensionsActive)
>       return false;
> 
>-    // If the theme we're enabling is the skin currently selected then it doesn't
>-    // require a restart to enable it.
>-    if (aAddon.type == "theme")
>-      return aAddon.internalName != this.currentSkin &&
>-             !Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
>-
>-    return !aAddon.bootstrap;
>+    // Anything that is active is already enabled
>+    if (aAddon.active)
>+      return false;
>+
>+    if (aAddon.type == "theme") {
>+      // If dynamic theme switching is enabled then switching themes does not
>+      // require a restart
>+      if (Prefs.getBoolPref(PREF_EM_DSS_ENABLED))
>+        return false;
>+
>+      // If the theme is already the theme in use then no restart is necessary.
>+      // This covers the case where the default theme is in use but a
>+      // lightweight theme is considered active.
>+      return aAddon.internalName != this.currentSkin;
>+    }
>+    else {
>+      return !aAddon.bootstrap;
>+    }
No need for the else

>   },
> 
>   /**
>    * Tests whether disabling an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   disableRequiresRestart: function XPI_disableRequiresRestart(aAddon) {
>     // If the platform couldn't have activated up extensions then we can make
>     // changes without any restart.
>     if (!this.extensionsActive)
>       return false;
> 
>-    // This sounds odd but it is correct. Themes are only ever asked to disable
>-    // after another theme has been enabled. Disabling the theme only requires
>-    // a restart if enabling the other theme does too. If the selected skin doesn't
>-    // match the current skin then a restart is necessary.
>-    if (aAddon.type == "theme")
>-      return this.selectedSkin != this.currentSkin &&
>-             !Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
>-
>-    return !aAddon.bootstrap;
>+    // Anything that isn't active is already disabled
>+    if (!aAddon.active)
>+      return false;
>+
>+    if (aAddon.type == "theme") {
>+      // If dynamic theme switching is enabled then switching themes does not
>+      // require a restart
>+      if (Prefs.getBoolPref(PREF_EM_DSS_ENABLED))
>+        return false;
>+
>+      // Non-default themes always require a restart to disable since it will
>+      // be switching from one theme to another or to the default theme and a
>+      // lightweight theme.
>+      if (aAddon.internalName != this.defaultSkin)
>+        return true;
>+
>+      // The default theme requires a restart to disable if we are in the
>+      // process of switching to a different theme. Note that this makes the
>+      // disabled flag of operationsRequiringRestart incorrect for the default
>+      // theme (it will be false most of the time). Bug 520124 would be required
>+      // to fix it. For the UI this isn't a problem since we never try to
>+      // disable or uninstall the default theme.
>+      return this.selectedSkin != this.currentSkin;
>+    }
>+    else {
>+      return !aAddon.bootstrap;
>+    }
No need for the else

Comment 12

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

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

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_install.js b/toolkit/mozapps/extensions/test/xpcshell/test_install.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_install.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_install.js
>@@ -62,18 +62,17 @@ function run_test_1() {
>     do_check_eq(install.linkedInstalls, null);
>     do_check_eq(install.type, "extension");
>     do_check_eq(install.version, "1.0");
>     do_check_eq(install.name, "Test 1");
>     do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
>     do_check_true(install.addon.hasResource("install.rdf"));
>     do_check_eq(install.addon.install, install);
>     do_check_eq(install.addon.size, ADDON1_SIZE);
>-    do_check_neq(install.addon.operationsRequiringRestart &
>-                 AddonManager.OP_NEEDS_RESTART_INSTALL, 0);
>+    do_check_true(hasFlag(install.addon.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_INSTALL));
newline after the comma etc. here and elsewhere for the long lines

Comment 13

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

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

>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
>@@ -2258,89 +2258,133 @@ var XPIProvider = {
>...
>   /**
>    * Tests whether installing an add-on will require a restart.
>    *
>    * @param  aAddon
>    *         The add-on to test
>    * @return true if the operation requires a restart
>    */
>   installRequiresRestart: function XPI_installRequiresRestart(aAddon) {
>     // If the platform couldn't have activated up extensions then we can make
>     // changes without any restart.
>     if (!this.extensionsActive)
>       return false;
> 
>-    // Themes not currently in use can be installed immediately
>-    if (aAddon.type == "theme")
>-      return aAddon.internalName == this.currentSkin ||
>-             Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
>-
>-    return !aAddon.bootstrap;
>+    // Add-ons that are already installed don't require a restart to install.
Could you add to the comment that this is a protective measure and shouldn't get called for an add-on that is already installed? Here and elsewhere.

Comment 14

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/6654555a3ca7

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Except bug 590508 everything looks good now. Removal of disabled extensions can now be undone. Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b6pre) Gecko/20100903 Firefox/4.0b6pre