Uninstalling disabled add-ons cannot be undone

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({regression})

Trunk
mozilla2.0b7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
blocking2.0: --- → beta4+
(Assignee)

Comment 1

7 years ago
Getting this right for themes is really complicated until bug 520124 is fixed.
blocking2.0: beta4+ → beta5+
Depends on: 520124
(Assignee)

Updated

7 years ago
Blocks: 563971
(Assignee)

Updated

7 years ago
Blocks: 577063
(Assignee)

Updated

7 years ago
Blocks: 580298
(Assignee)

Updated

7 years ago
No longer blocks: 563971
(Assignee)

Updated

7 years ago
No longer blocks: 577063
(Assignee)

Updated

7 years ago
No longer blocks: 580298
Duplicate of this bug: 585465
Keywords: regression
(Assignee)

Comment 3

7 years ago
Created attachment 468190 [details] [diff] [review]
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.
Attachment #468190 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 590096
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
Attachment #468190 - Flags: review?(robert.bugzilla) → review+
(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.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 590509
Ready for checkin?
(Assignee)

Comment 9

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

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

Updated

7 years ago
blocking2.0: beta5+ → beta6+
(Assignee)

Updated

7 years ago
No longer depends on: 520124
(Assignee)

Comment 10

7 years ago
Created attachment 470907 [details] [diff] [review]
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.
Attachment #468190 - Attachment is obsolete: true
Attachment #470907 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #470907 - Flags: review? → review?(robert.bugzilla)
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 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 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.
Attachment #470907 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 14

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/6654555a3ca7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Depends on: 592947
No longer depends on: 592947
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
Blocks: 590508
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.