375596 - Didn't update extension over installed disabled previous version

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Modex]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9a4pre) Gecko/20070327 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9a4pre) Gecko/20070327 Minefield/3.0a4pre

If I have installed and disabled extension (e.g. 0.5) and I install from disk new version (e.g. 0.6). After that enable still old version (0.5) in Addons Manager and reboot browser.

Reproducible: Always

Steps to Reproduce:
1. Install new version of extension
2. Enable old version of installed extension
3. Reboot browser
Actual Results:  
After reboot enabled old (0.5) version of extension.

Expected Results:  
After reboot must be new version of extension installed and enabled.
Or after install extension it must enabled automatically.

I use Brief extension for test:
http://people.mozilla-russia.org/modex/extensions/BRIEF.rar
In archive 0.5 and 0.6

Comment 1

[Merlyel]

Same on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 ID:2007030919

But if I updated a bit differently:
1. Enable old version of extension
2. Update extension
3. Restart browser
i.e. change order of updating steps, extension is updated and enabled.

Comment 2

[Samuel Sidler (old account; do not CC)]

I can confirm this. Seems like an edge case, but it definitely happens. I can't find a dupe, so, confirming.

Comment 3

[Dave Townsend [:mossop]]

Alternate STR:

With an extension disabled:

1. Install an update to the extension.
2. Click to enable the extension.
3. Click to disable the extension.

The extension is no longer in the "Will be updated on restart" state and indeed it doesn't on restart.

Essentially currently the EM can only keep track of one operation to perform to an extension on the next restart, as soon as we tell it that the operation is to enable the extension, it forgets about the operation to update it.

bug 337909 could alleviate this particular case. Equally we could just grey out the enable button (like the disable button is currently greyed out on an extension being updated).

Comment 4

[[:Aleksej]]

Confirmed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070627 Minefield/3.0a6pre. OS → All, version → Trunk, blocking-firefox3 → ?.

Comment 5

[Dave Townsend [:mossop]]

Comment 1 would be solved by bug 342579

Rob, what do you think to disabling the enable/disable buttons for extensions pending install/upgrade. Or possibly make install/upgrade like uninstall in that there is just a cancel button to cancel that operation?

Comment 6

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

My initial thoughts on this were to make it like uninstall with just a cancel button.

Comment 7

[Dave Townsend [:mossop]]

Thinking of reducing the silly number of states here, I wonder if just making any addon with a pending operation have a cancel button to undo that so if the add-on will be disabled/enabled/installed/upgraded/uninstalled then we have a cancel button to cancel that action?

Comment 8

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

LOL... I hear ya!

The enable / disable and uninstall cases are already flushed out so I don't think it would be any easier than just displaying the cancel button for this instance.

Comment 9

[Dave Townsend [:mossop]]

Ok here's my ascii mockup of what I think we move to here:

Default (as current):

+-------------------------------------------+
|Name                                       |
|Desc                                       |
|<Preferences>       <En/Disable><Uninstall>|
+-------------------------------------------+


needs-install (changed), needs-upgrade (changed), needs-uninstall (as current)

+-------------------------------------------+
|Name                                       |
|Desc                                       |
|<Preferences>                      <Cancel>|
+-------------------------------------------+


needs-enable (minor change):

+-------------------------------------------+
|Name                                       |
|Desc                                       |
|<Preferences>        <Disable><-Uninstall->|
+-------------------------------------------+
(Uninstall greyed out)


needs-disable (minor change):

+-------------------------------------------+
|Name                                       |
|Desc                                       |
|<Preferences>         <Enable><-Uninstall->|
+-------------------------------------------+
(Uninstall greyed out)

The only missing situation is for when the user manually installs a new version of an extension that is set to be disabled/enabled/uninstalled. We could:

* Just override in that case (forgetting the previous state)
* Warn of what is about to happen
* Not allow the install (I think not)

I'm thinking the first case but need to confirm UI on all this.

Comment 10

[Madhava Enros [:madhava]]

My amendment here would be that there's no need to disable the Uninstall button after a user has clicked enable/disable.  While it's true that, if he or she did so we wouldn't remember what enabling/disabling/updating her or she had done before, the Un-installation makes that action unimportant.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch hides the enable/disable buttons in the event that an install/upgrade is pending and provides a button to cancel that operation. A function to do this is added to nsIExtensionManager which safely cleans up the staged file etc.

Some of the confirmation code in extensions.js is split out since it is used in a few places now.

Comment 12

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

I'm wondering about bug 136041 / Bug 386578 and if there is a way to mitigate that for this patch?

Comment 13

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

btw: The install update checkbox on each item in the updates view also fell victim to bug 136041... I dynamically remove and add the accesskey as needed.

Comment 14

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

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

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.133
>diff -u8 -p -r1.133 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	10 Aug 2007 21:11:20 -0000	1.133
>+++ toolkit/mozapps/extensions/content/extensions.js	14 Aug 2007 00:42:00 -0000	
>...
> function buildContextMenu(aEvent)
>@@ -1136,17 +1137,21 @@ function buildContextMenu(aEvent)
>   var menuitem_about = document.getElementById("menuitem_about_clone");
>   var name = selectedItem ? selectedItem.getAttribute("name") : "";
>   menuitem_about.setAttribute("label", getExtensionString("aboutAddon", [name]));
> 
>   // When performing update or install tasks we don't support uninstall
Please update this comment to reflect the changes.

>   switch (gView) {
>   case "extensions":
>     canEnable = gExtensionsViewController.isCommandEnabled("cmd_reallyEnable");
>     document.getElementById("menuitem_enable_clone").hidden = !canEnable;
>     document.getElementById("menuitem_disable_clone").hidden = canEnable;
>     document.getElementById("menuitem_useTheme_clone").hidden = true;

Not part of this bug but could you also change the locales and plugins case as follows?
  case "plugins":
    document.getElementById("menuitem_about_clone").hidden = true;
    document.getElementById("menuitem_uninstall_clone").hidden = true;
    document.getElementById("menuitem_checkUpdate_clone").hidden = true;
  case "locales":
    canEnable = gExtensionsViewController.isCommandEnabled("cmd_reallyEnable");
    document.getElementById("menuitem_enable_clone").hidden = !canEnable;
    document.getElementById("menuitem_disable_clone").hidden = canEnable;
    document.getElementById("menuitem_useTheme_clone").hidden = true;
    document.getElementById("menuitem_options_clone").hidden = true;
    break;

>@@ -1602,16 +1628,20 @@ var gExtensionsViewController = {
>              selectedItem.type == nsIUpdateItem.TYPE_THEME &&
>              selectedItem.getAttribute("internalName") != gDefaultTheme) &&
>              selectedItem.opType != OP_NEEDS_UNINSTALL &&
>              selectedItem.getAttribute("locked") != "true" &&
>              canWriteToLocation(selectedItem) &&
>              !gExtensionsView.hasAttribute("update-operation");
>     case "cmd_cancelUninstall":
>       return selectedItem.opType == OP_NEEDS_UNINSTALL;
>+    case "cmd_cancelInstall":
>+      return selectedItem.opType == OP_NEEDS_INSTALL;
>+    case "cmd_cancelUpgrade":
>+      return selectedItem.opType == OP_NEEDS_UPGRADE;
Any reason not to just have one command (see below)?

>@@ -1832,46 +1848,66 @@ var gExtensionsViewController = {
>     {
>       gExtensionManager.cancelUninstallItem(getIDFromResourceURI(aSelectedItem.id));
>       gExtensionsViewController.onCommandUpdate();
>       updateGlobalCommands();
>       gExtensionsView.selectedItem.focus();
>       updateOptionalViews();
>     },
> 
>+    cmd_cancelInstall: function (aSelectedItem)
>+    {
>+      // Confirm cancelling the install
>+      var name = aSelectedItem.getAttribute("name");
>+      var result = confirmOperation(name, "cancelInstallTitle", "cancelInstallQueryMessage",
>+                                    "cancelInstallButton", "cancelCancelInstallButton",
>+                                    null, []);
>+      if (!result)
>+        return;
>+
>+      gExtensionManager.cancelInstallItem(getIDFromResourceURI(aSelectedItem.id));
>+      gExtensionsViewController.onCommandUpdate();
>+      updateGlobalCommands();
>+      gExtensionsView.selectedItem.focus();
>+      updateOptionalViews();
>+    },
>+
>+    cmd_cancelUpgrade: function (aSelectedItem)
>+    {
>+      // Confirm cancelling the upgrade
>+      var name = aSelectedItem.getAttribute("name");
>+      var result = confirmOperation(name, "cancelUpgradeTitle", "cancelUpgradeQueryMessage",
>+                                    "cancelUpgradeButton", "cancelCancelUpgradeButton",
>+                                    null, []);
>+      if (!result)
>+        return;
>+
>+      gExtensionManager.cancelInstallItem(getIDFromResourceURI(aSelectedItem.id));
>+      gExtensionsViewController.onCommandUpdate();
>+      updateGlobalCommands();
>+      gExtensionsView.selectedItem.focus();
>+      updateOptionalViews();
>+    },
Seems like you can just use one function for both and use the opType to set the confirmOperation message... similar to how you have the EM function. I'd suggest just naming it cmd_cancelInstall so it is consistent with the EM.

btw: nice cleanup with confirmOperation.

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.234
>diff -u8 -p -r1.234 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	12 Aug 2007 04:02:58 -0000	1.234
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	14 Aug 2007 00:42:00 -0000	
>@@ -4993,16 +4995,66 @@ ExtensionManager.prototype = {
>       // ... just remove it from the list. 
>       ds.removeCorruptDLItem(id);
>     }
>     
>     this._notifyAction(id, EM_ITEM_UNINSTALLED);
>   },
> 
>   /**
>+   * Cancels a pending install or upgrade of an item
>+   * @param   id
>+   *          The ID of the item.
>+   */
nit: I'm leaning towards changing all the comments that are duplicated in the idl to
/* See nsIExtensionManager.idl */
Might as well start here and I'll get the rest in bug 392419.

>+  cancelInstallItem: function(id) {
>+    var ds = this.datasource;
>+    var opType = ds.getItemProperty(id, "opType");
>+    if (opType == OP_NEEDS_UPGRADE || opType == OP_NEEDS_INSTALL) {
Under what conditions will one of these not be evaluate to true?
If there is a case where it doesn't how is cancelUninstallItem and uninstallItem protected from the same case?
If there is such a condition might as well return early

>+      ds.updateDownloadState(PREFIX_ITEM_URI + id, null);
>+      var installLocation = this.getInstallLocation(id);
>+      // Removes any staged xpis for this item.
>+      var stageFile = installLocation.getStageFile(id);
>+      if (stageFile)
>+        installLocation.removeFile(stageFile);
>+      // Addons with an opType of OP_NEEDS_INSTALL only have a staged xpi file
>+      // and just need to be removed completely from the ds.
>+      if (opType == OP_NEEDS_INSTALL) {
>+        ds.removeItemMetadata(id);
>+        ds.removeItemFromContainer(id);
>+        ds.updateVisibleList(id, null, true);
>+        StartupCache.clearEntry(installLocation, id);
>+        this._updateManifests(false);
>+        this._notifyAction(id, EM_ITEM_CANCEL);
>+      }
>+      else {
>+        // Clear upgrade information and reset any request to enable/disable.
>+        ds.setItemProperty(id, EM_R("newVersion"), null);
>+        var appDisabled = ds.getItemProperty(id, "appDisabled");
>+        var userDisabled = ds.getItemProperty(id, "userDisabled");
>+        if (appDisabled == "true" || appDisabled == OP_NONE && userDisabled == OP_NONE) {
>+          this._setOp(id, OP_NONE);
>+          this._notifyAction(id, EM_ITEM_CANCEL);
>+        }
>+        else if (appDisabled == OP_NEEDS_DISABLE || userDisabled == OP_NEEDS_DISABLE) {
>+          this._setOp(id, OP_NEEDS_DISABLE);
>+          this._notifyAction(id, EM_ITEM_DISABLED);
>+        }
>+        else if (appDisabled == OP_NEEDS_ENABLE || userDisabled == OP_NEEDS_ENABLE) {
>+          this._setOp(id, OP_NEEDS_ENABLE);
>+          this._notifyAction(id, EM_ITEM_ENABLED);
>+        }
>+        else {
>+          this._setOp(id, OP_NONE);
>+          this._notifyAction(id, EM_ITEM_CANCEL);
>+        }
I wish I had documented why I did this in this manner in cancelUninstallItem. ;)
I'll try to decipher it out over the weekend.

Next one should do it... thanks Dave!

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch rev 2

(In reply to comment #14)
> >+    case "cmd_cancelInstall":
> >+      return selectedItem.opType == OP_NEEDS_INSTALL;
> >+    case "cmd_cancelUpgrade":
> >+      return selectedItem.opType == OP_NEEDS_UPGRADE;
> Any reason not to just have one command (see below)?

Seemed cleaner to me since we have multiple menu items, multiple buttons etc to have multiple commands. In this patch I've made the actual action of the command the same, but if you still think it's better to go with one then I can move the optype checks into buildContextMenu etc.

> >+  cancelInstallItem: function(id) {
> >+    var ds = this.datasource;
> >+    var opType = ds.getItemProperty(id, "opType");
> >+    if (opType == OP_NEEDS_UPGRADE || opType == OP_NEEDS_INSTALL) {
> Under what conditions will one of these not be evaluate to true?
> If there is a case where it doesn't how is cancelUninstallItem and
> uninstallItem protected from the same case?
> If there is such a condition might as well return early

There isn't (or at least shouldn't be) such a case in our code. I was more thinking that since the EM is a public API that a simple sanity check that we weren't being called incorrectly wouldn't be a bad thing. I've made it just return early though maybe we should think about throwing in that situation. I can also add similar checks to cancelUninstallItem and uninstallItem if you like, was just hitting things as I touched them ;)

> >+        var appDisabled = ds.getItemProperty(id, "appDisabled");
> >+        var userDisabled = ds.getItemProperty(id, "userDisabled");
> >+        if (appDisabled == "true" || appDisabled == OP_NONE && userDisabled == OP_NONE) {
> >+          this._setOp(id, OP_NONE);
> >+          this._notifyAction(id, EM_ITEM_CANCEL);
> >+        }
> >+        else if (appDisabled == OP_NEEDS_DISABLE || userDisabled == OP_NEEDS_DISABLE) {
> >+          this._setOp(id, OP_NEEDS_DISABLE);
> >+          this._notifyAction(id, EM_ITEM_DISABLED);
> >+        }
> >+        else if (appDisabled == OP_NEEDS_ENABLE || userDisabled == OP_NEEDS_ENABLE) {
> >+          this._setOp(id, OP_NEEDS_ENABLE);
> >+          this._notifyAction(id, EM_ITEM_ENABLED);
> >+        }
> >+        else {
> >+          this._setOp(id, OP_NONE);
> >+          this._notifyAction(id, EM_ITEM_CANCEL);
> >+        }
> I wish I had documented why I did this in this manner in cancelUninstallItem.
> ;)
> I'll try to decipher it out over the weekend.

Yeah it's a bit of a weird sequence that basically just caches whether the preceding optype was an enable or disable it seems.

Comment 16

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Wrong file...

Comment 17

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

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

>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v
>retrieving revision 1.137
>diff -u8 -p -r1.137 extensions.js
>--- toolkit/mozapps/extensions/content/extensions.js	25 Aug 2007 14:26:44 -0000	1.137
>+++ toolkit/mozapps/extensions/content/extensions.js	25 Aug 2007 16:21:55 -0000	
>@@ -1169,21 +1170,26 @@ function buildContextMenu(aEvent)
>     popup.appendChild(clonedMenu);
>   }
> 
>   // All views support about
>   var menuitem_about = document.getElementById("menuitem_about_clone");
>   var name = selectedItem ? selectedItem.getAttribute("name") : "";
>   menuitem_about.setAttribute("label", getExtensionString("aboutAddon", [name]));
> 
>-  // When performing update or install tasks we don't support uninstall
>+  // When pending an update or install tasks we don't support uninstall, but can
>+  // cancel te update or install.
ewww... :P Not sure if this is complete but how about:
When an update or install is pending allow canceling the update or install and don't allow uninstall. When an uninstall is pending allow canceling the uninstall.

>@@ -1195,26 +1201,26 @@ function buildContextMenu(aEvent)
>       // don't let the user activate incompatible themes, but show a (disabled) Enable
>       // menuitem to give visual feedback; it's disabled because cmd_enable returns false
>       enableMenu.hidden = false;
>     else
>       enableMenu.hidden = true;
>     document.getElementById("menuitem_options_clone").hidden = true;
>     document.getElementById("menuitem_disable_clone").hidden = true;
>     break;
>-  case "locales":
>   case "plugins":
>+    document.getElementById("menuitem_about_clone").hidden = true;
>+    document.getElementById("menuitem_uninstall_clone").hidden = true;
>+    document.getElementById("menuitem_checkUpdate_clone").hidden = true;
>+  case "locales":
>     canEnable = gExtensionsViewController.isCommandEnabled("cmd_reallyEnable");
>     document.getElementById("menuitem_enable_clone").hidden = !canEnable;
>     document.getElementById("menuitem_disable_clone").hidden = canEnable;
>     document.getElementById("menuitem_useTheme_clone").hidden = true;
>     document.getElementById("menuitem_options_clone").hidden = true;
>-    document.getElementById("menuitem_about_clone").hidden = true;
>-    document.getElementById("menuitem_uninstall_clone").hidden = true;
>-    document.getElementById("menuitem_checkUpdate_clone").hidden = true;
Thank you

>@@ -1871,46 +1888,65 @@ var gExtensionsViewController = {
...
>+    cmd_cancelUpgrade: function (aSelectedItem)
>+    {
>+      this.cmd_cancelInstall(aSelectedItem);
>+    },
Good choice

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v
>retrieving revision 1.49
>diff -u8 -p -r1.49 nsIExtensionManager.idl
>--- toolkit/mozapps/extensions/public/nsIExtensionManager.idl	20 Aug 2007 08:02:22 -0000	1.49
>+++ toolkit/mozapps/extensions/public/nsIExtensionManager.idl	25 Aug 2007 15:48:14 -0000	
>@@ -192,17 +192,17 @@ interface nsIInstallLocation : nsISupports
> 
> /**
>  * Interface representing a system for the installation and management of
>  * Extensions, Themes etc. 
>  *
>  * XXXben - Some of this stuff should go into a management-ey interface, 
>  *          some into an app-startup-ey interface.
>  */
>-[scriptable, uuid(c2aef3c3-8ce1-4470-94e4-1167e5ca8928)]
>+[scriptable, uuid(f5c34a77-3487-4b70-a65a-6727fa066324)]
> interface nsIExtensionManager : nsISupports
> {
>   /**
>    * Constants representing types of update checks.
>    */
>   const unsigned long UPDATE_CHECK_NEWVERSION    = 0;
>   const unsigned long UPDATE_CHECK_COMPATIBILITY = 1;
>   const unsigned long UPDATE_SYNC_COMPATIBILITY  = 2;
>@@ -402,16 +402,23 @@ interface nsIExtensionManager : nsISupports
>    * @param   movingID
>    *          The ID of an item to be moved.
>    * @param   destinationID
>    *          The ID of an item to move an item to.
>    */
>   void moveToIndexOf(in AString movingID, in AString destinationID);
> 
>   /**
>+   * Cancels a pending install or upgrade of an item
>+   * @param   id
>+   *          The ID of the item.
>+   */
>+  void cancelInstallItem(in AString id);
Please make note that this is a noop if the item doesn't have a pending install or upgrade.

Could you also file a followup bug about making the other actions noop's so they are consistent?

r=me with those items addressed and you seriously do rock!

Comment 18

[Dave Townsend [:mossop]]

Attachment: Ready for checkin

Patch for checkin. Carrying over review. Filed bug 393719 for the other cleanup work.

Comment 19

[Dave Townsend [:mossop]]

Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v  <--  extensions.dtd
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.36; previous revision: 1.35
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.138; previous revision: 1.137
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.45; previous revision: 1.44
done
Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.61; previous revision: 1.60
done
Checking in toolkit/mozapps/extensions/public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v  <--  nsIExtensionManager.idl
new revision: 1.50; previous revision: 1.49
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.239; previous revision: 1.238
done
Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.27; previous revision: 1.26
done
Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v  <--  extensions.css
new revision: 1.31; previous revision: 1.30
done

Comment 20

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2