Bug 585339: Uninstalling disabled add-ons cannot be undone. r=robstrong, a=blocking-b6

toolkit/mozapps/extensions/LightweightThemeManager.jsm

@@ -46,6 +46,7 @@ Components.utils.import("resource://gre/modules/Services.jsm");
 const ID_SUFFIX              = "@personas.mozilla.org";
 const PREF_LWTHEME_TO_SELECT = "extensions.lwThemeToSelect";
 const PREF_GENERAL_SKINS_SELECTEDSKIN = "general.skins.selectedSkin";
+const PREF_EM_DSS_ENABLED    = "extensions.dss.enabled";
 const ADDON_TYPE             = "theme";
 
 const DEFAULT_MAX_USED_THEMES_COUNT = 30;
@@ -441,6 +442,18 @@ function AddonWrapper(aTheme, aBeingEnabled) {
   });
 
   this.__defineGetter__("operationsRequiringRestart", function() {
+    // If a non-default theme is in use then a restart will be required to
+    // enable lightweight themes unless dynamic theme switching is enabled
+    if (Services.prefs.prefHasUserValue(PREF_GENERAL_SKINS_SELECTEDSKIN)) {
+      try {
+        if (Services.prefs.getBoolPref(PREF_EM_DSS_ENABLED))
+          return AddonManager.OP_NEEDS_RESTART_NONE;
+      }
+      catch (e) {
+      }
+      return AddonManager.OP_NEEDS_RESTART_ENABLE;
+    }
+
     return AddonManager.OP_NEEDS_RESTART_NONE;
   });
 

toolkit/mozapps/extensions/XPIProvider.jsm

@@ -2264,11 +2264,21 @@ var XPIProvider = {
     if (!this.extensionsActive)
       return false;
 
-    // If the theme we're enabling is the skin currently selected then it doesn't
+    // Anything that is active is already enabled
-    // require a restart to enable it.
+    if (aAddon.active)
-    if (aAddon.type == "theme")
+      return false;
-      return aAddon.internalName != this.currentSkin &&
+
-             !Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
+    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;
+    }
 
     return !aAddon.bootstrap;
   },
@@ -2286,13 +2296,30 @@ var XPIProvider = {
     if (!this.extensionsActive)
       return false;
 
-    // This sounds odd but it is correct. Themes are only ever asked to disable
+    // Anything that isn't active is already disabled
-    // after another theme has been enabled. Disabling the theme only requires
+    if (!aAddon.active)
-    // a restart if enabling the other theme does too. If the selected skin doesn't
+      return false;
-    // match the current skin then a restart is necessary.
+
-    if (aAddon.type == "theme")
+    if (aAddon.type == "theme") {
-      return this.selectedSkin != this.currentSkin &&
+      // If dynamic theme switching is enabled then switching themes does not
-             !Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
+      // 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;
+    }
 
     return !aAddon.bootstrap;
   },
@@ -2310,12 +2337,33 @@ var XPIProvider = {
     if (!this.extensionsActive)
       return false;
 
-    // Themes not currently in use can be installed immediately
+    // Add-ons that are already installed don't require a restart to install.
-    if (aAddon.type == "theme")
+    // This wouldn't normally be called for an already installed add-on (except
-      return aAddon.internalName == this.currentSkin ||
+    // for forming the operationsRequiringRestart flags) so is really here as
-             Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
+    // a safety measure.
+    if (aAddon instanceof DBAddonInternal)
+      return false;
 
-    return !aAddon.bootstrap;
+    // 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;
   },
 
   /**
@@ -2331,12 +2379,9 @@ var XPIProvider = {
     if (!this.extensionsActive)
       return false;
 
-    // Themes not currently in use can be uninstalled immediately
+    // If the add-on can be disabled without a restart then it can also be
-    if (aAddon.type == "theme")
+    // uninstalled without a restart
-      return aAddon.internalName == this.currentSkin ||
+    return this.disableRequiresRestart(aAddon);
-             Prefs.getBoolPref(PREF_EM_DSS_ENABLED);
-
-    return !aAddon.bootstrap;
   },
 
   /**
@@ -2559,7 +2604,7 @@ var XPIProvider = {
       throw new Error("Cannot uninstall addons from locked install locations");
 
     // Inactive add-ons don't require a restart to uninstall
-    let requiresRestart = aAddon.active && this.uninstallRequiresRestart(aAddon);
+    let requiresRestart = this.uninstallRequiresRestart(aAddon);
 
     if (requiresRestart) {
       // We create an empty directory in the staging directory to indicate that
@@ -4736,12 +4781,6 @@ AddonInstall.prototype = {
     let isUpgrade = this.existingAddon &&
                     this.existingAddon._installLocation == this.installLocation;
     let requiresRestart = XPIProvider.installRequiresRestart(this.addon);
-    // Restarts is required if the existing add-on is active and disabling it
-    // requires a restart
-    if (!requiresRestart && this.existingAddon) {
-      requiresRestart = this.existingAddon.active &&
-                        XPIProvider.disableRequiresRestart(this.existingAddon);
-    }
 
     LOG("Starting install of " + this.sourceURI.spec);
     AddonManagerPrivate.callAddonListeners("onInstalling",

toolkit/mozapps/extensions/test/xpcshell/test_disable.js

@@ -56,6 +56,8 @@ function run_test() {
       do_check_true(isExtensionInAddonsList(profileDir, newa1.id));
       do_check_true(hasFlag(newa1.permissions, AddonManager.PERM_CAN_DISABLE));
       do_check_false(hasFlag(newa1.permissions, AddonManager.PERM_CAN_ENABLE));
+      do_check_eq(newa1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_DISABLE |
+                                                    AddonManager.OP_NEEDS_RESTART_UNINSTALL);
       do_check_in_crash_annotation(addon1.id, addon1.version);
 
       run_test_1();
@@ -80,6 +82,8 @@ function run_test_1() {
     do_check_eq(a1.iconURL, "chrome://foo/content/icon.png");
     do_check_false(hasFlag(a1.permissions, AddonManager.PERM_CAN_DISABLE));
     do_check_true(hasFlag(a1.permissions, AddonManager.PERM_CAN_ENABLE));
+    do_check_eq(a1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_DISABLE |
+                                               AddonManager.OP_NEEDS_RESTART_UNINSTALL);
     do_check_in_crash_annotation(addon1.id, addon1.version);
 
     ensure_test_completed();
@@ -100,6 +104,7 @@ function run_test_1() {
         do_check_false(isExtensionInAddonsList(profileDir, newa1.id));
         do_check_false(hasFlag(newa1.permissions, AddonManager.PERM_CAN_DISABLE));
         do_check_true(hasFlag(newa1.permissions, AddonManager.PERM_CAN_ENABLE));
+        do_check_eq(newa1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_ENABLE);
         do_check_not_in_crash_annotation(addon1.id, addon1.version);
 
         run_test_2();
@@ -123,6 +128,7 @@ function run_test_2() {
     do_check_eq(a1.iconURL, gIconURL.spec);
     do_check_true(hasFlag(a1.permissions, AddonManager.PERM_CAN_DISABLE));
     do_check_false(hasFlag(a1.permissions, AddonManager.PERM_CAN_ENABLE));
+    do_check_eq(a1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_ENABLE);
 
     ensure_test_completed();
 
@@ -142,6 +148,8 @@ function run_test_2() {
         do_check_true(isExtensionInAddonsList(profileDir, newa1.id));
         do_check_true(hasFlag(newa1.permissions, AddonManager.PERM_CAN_DISABLE));
         do_check_false(hasFlag(newa1.permissions, AddonManager.PERM_CAN_ENABLE));
+        do_check_eq(newa1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_DISABLE |
+                                                      AddonManager.OP_NEEDS_RESTART_UNINSTALL);
         do_check_in_crash_annotation(addon1.id, addon1.version);
 
         run_test_3();

toolkit/mozapps/extensions/test/xpcshell/test_dss.js

@@ -623,7 +623,8 @@ function run_test_10() {
 
       prepare_test({
         "theme2@tests.mozilla.org": [
-          "onUninstalling",
+          ["onUninstalling", false],
+          "onUninstalled"
         ],
         "default@tests.mozilla.org": [
           ["onEnabling", false],
@@ -660,7 +661,8 @@ function run_test_11() {
 
     prepare_test({
       "theme1@tests.mozilla.org": [
-        "onInstalling"
+        ["onInstalling", false],
+        "onInstalled"
       ]
     }, [
       "onInstallStarted",
@@ -702,7 +704,8 @@ function run_test_12() {
 
     prepare_test({
       "theme1@tests.mozilla.org": [
-        "onInstalling"
+        ["onInstalling", false],
+        "onInstalled"
       ]
     }, [
       "onInstallStarted",

toolkit/mozapps/extensions/test/xpcshell/test_install.js

@@ -67,8 +67,8 @@ function run_test_1() {
     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 &
+    do_check_true(hasFlag(install.addon.operationsRequiringRestart,
-                 AddonManager.OP_NEEDS_RESTART_INSTALL, 0);
+                          AddonManager.OP_NEEDS_RESTART_INSTALL));
     let file = do_get_addon("test_install1");
     let uri = Services.io.newFileURI(file).spec;
     do_check_eq(install.addon.getResourceURI("install.rdf").spec, "jar:" + uri + "!/install.rdf");
@@ -204,6 +204,8 @@ function check_test_2(install) {
   do_check_eq(install.name, "Real Test 2");
   do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
   do_check_eq(install.addon.install, install);
+  do_check_true(hasFlag(install.addon.operationsRequiringRestart,
+                        AddonManager.OP_NEEDS_RESTART_INSTALL));
   do_check_eq(install.iconURL, null);
 
   // Pause the install here and start it again in run_test_3
@@ -298,6 +300,8 @@ function check_test_4(install) {
   do_check_neq(install.existingAddon);
   do_check_eq(install.existingAddon.id, "addon2@tests.mozilla.org");
   do_check_eq(install.addon.install, install);
+  do_check_true(hasFlag(install.addon.operationsRequiringRestart,
+                        AddonManager.OP_NEEDS_RESTART_INSTALL));
 
   run_test_5();
   // Installation will continue when there is nothing returned.
@@ -610,6 +614,8 @@ function run_test_11() {
     do_check_eq(installs[0].version, "1.0");
     do_check_eq(installs[0].name, "Multi Test 1");
     do_check_eq(installs[0].state, AddonManager.STATE_DOWNLOADED);
+    do_check_true(hasFlag(installs[0].addon.operationsRequiringRestart,
+                          AddonManager.OP_NEEDS_RESTART_INSTALL));
 
     // Comes from addon5.jar and is compatible by default
     do_check_eq(installs[1].sourceURI, install.sourceURI);
@@ -618,6 +624,8 @@ function run_test_11() {
     do_check_eq(installs[1].version, "3.0");
     do_check_eq(installs[1].name, "Multi Test 2");
     do_check_eq(installs[1].state, AddonManager.STATE_DOWNLOADED);
+    do_check_true(hasFlag(installs[1].addon.operationsRequiringRestart,
+                          AddonManager.OP_NEEDS_RESTART_INSTALL));
 
     // Comes from addon6.xpi and is incompatible
     do_check_eq(installs[2].sourceURI, install.sourceURI);
@@ -626,6 +634,8 @@ function run_test_11() {
     do_check_eq(installs[2].version, "2.0");
     do_check_eq(installs[2].name, "Multi Test 3");
     do_check_eq(installs[2].state, AddonManager.STATE_DOWNLOADED);
+    do_check_false(hasFlag(installs[2].addon.operationsRequiringRestart,
+                           AddonManager.OP_NEEDS_RESTART_INSTALL));
 
     // Comes from addon7.jar and is made compatible by an update check
     do_check_eq(installs[3].sourceURI, install.sourceURI);
@@ -634,6 +644,8 @@ function run_test_11() {
     do_check_eq(installs[3].version, "5.0");
     do_check_eq(installs[3].name, "Multi Test 4");
     do_check_eq(installs[3].state, AddonManager.STATE_DOWNLOADED);
+    do_check_true(hasFlag(installs[3].addon.operationsRequiringRestart,
+                          AddonManager.OP_NEEDS_RESTART_INSTALL));
 
     AddonManager.getAllInstalls(function(aInstalls) {
       do_check_eq(aInstalls.length, 4);
@@ -646,7 +658,8 @@ function run_test_11() {
           "onInstalling"
         ],
         "addon6@tests.mozilla.org": [
-          "onInstalling"
+          ["onInstalling", false],
+          "onInstalled"
         ],
         "addon7@tests.mozilla.org": [
           "onInstalling"
@@ -664,8 +677,13 @@ function run_test_11() {
 
       installs[0].install();
       installs[1].install();
-      installs[2].install();
       installs[3].install();
+
+      // Note that we install addon6 last. Since it doesn't need a restart to
+      // install it completes asynchronously which would otherwise make the
+      // onInstallStarted/onInstallEnded events go out of sequence unless this
+      // is the last install operation
+      installs[2].install();
     });
   });
 }
@@ -717,7 +735,8 @@ function run_test_12() {
         "onInstalling"
       ],
       "addon6@tests.mozilla.org": [
-        "onInstalling"
+        ["onInstalling", false],
+        "onInstalled"
       ],
       "addon7@tests.mozilla.org": [
         "onInstalling"
@@ -865,7 +884,8 @@ function check_test_13(install) {
     do_check_true(hasFlag(olda2.pendingOperations, AddonManager.PENDING_UPGRADE));
     do_check_eq(olda2.pendingUpgrade, install.addon);
 
-    do_check_true(hasFlag(install.addon.pendingOperations, AddonManager.PENDING_INSTALL));
+    do_check_true(hasFlag(install.addon.pendingOperations,
+                          AddonManager.PENDING_INSTALL));
 
     prepare_test({
       "addon2@tests.mozilla.org": [

toolkit/mozapps/extensions/test/xpcshell/test_theme.js

@@ -110,6 +110,8 @@ function run_test() {
     do_check_true(isThemeInAddonsList(profileDir, t1.id));
     do_check_false(hasFlag(t1.permissions, AddonManager.PERM_CAN_DISABLE));
     do_check_false(hasFlag(t1.permissions, AddonManager.PERM_CAN_ENABLE));
+    do_check_eq(t1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_UNINSTALL |
+                                               AddonManager.OP_NEEDS_RESTART_DISABLE);
 
     do_check_neq(t2, null);
     do_check_true(t2.userDisabled);
@@ -120,6 +122,7 @@ function run_test() {
     do_check_false(isThemeInAddonsList(profileDir, t2.id));
     do_check_false(hasFlag(t2.permissions, AddonManager.PERM_CAN_DISABLE));
     do_check_true(hasFlag(t2.permissions, AddonManager.PERM_CAN_ENABLE));
+    do_check_eq(t2.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_ENABLE);
 
     run_test_1();
   });
@@ -168,6 +171,7 @@ function check_test_1() {
     do_check_false(isThemeInAddonsList(profileDir, t1.id));
     do_check_false(hasFlag(t1.permissions, AddonManager.PERM_CAN_DISABLE));
     do_check_true(hasFlag(t1.permissions, AddonManager.PERM_CAN_ENABLE));
+    do_check_eq(t1.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_ENABLE);
 
     do_check_neq(t2, null);
     do_check_false(t2.userDisabled);
@@ -176,6 +180,8 @@ function check_test_1() {
     do_check_true(isThemeInAddonsList(profileDir, t2.id));
     do_check_false(hasFlag(t2.permissions, AddonManager.PERM_CAN_DISABLE));
     do_check_false(hasFlag(t2.permissions, AddonManager.PERM_CAN_ENABLE));
+    do_check_eq(t2.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_UNINSTALL |
+                                               AddonManager.OP_NEEDS_RESTART_DISABLE);
     do_check_false(gLWThemeChanged);
 
     run_test_2();
@@ -673,6 +679,7 @@ function run_test_11() {
     do_check_eq(install.name, "Test Theme 1");
     do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
     do_check_true(install.addon.skinnable, true);
+    do_check_false(hasFlag(install.addon.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_INSTALL));
 
     prepare_test({
       "theme1@tests.mozilla.org": [
@@ -716,6 +723,7 @@ function run_test_12() {
     do_check_eq(install.version, "1.0");
     do_check_eq(install.name, "Test Theme 1");
     do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
+    do_check_false(hasFlag(install.addon.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_INSTALL));
 
     prepare_test({
       "theme1@tests.mozilla.org": [
@@ -767,6 +775,7 @@ function run_test_13() {
       do_check_eq(install.version, "1.0");
       do_check_eq(install.name, "Test Theme 1");
       do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
+      do_check_true(hasFlag(install.addon.operationsRequiringRestart, AddonManager.OP_NEEDS_RESTART_INSTALL));
 
       prepare_test({
         "theme1@tests.mozilla.org": [