396129 - Add incompatible update start and done states to nsIAddonUpdateListener

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P4)

Description

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

Add states for checking compatibility information during an incompatible update check in nsIAddonUpdateListener. nsIAddonUpdateListener currently just forwards the states from nsIXPIProgressDialog which isn't sufficient for the extension manager since we can successfully install and add-on based on its update datasource's compatibility info. Without this we are unable to determine when an installations has actually finished which causes the following with current trunk builds:

1) Restart button is enabled before the installation has actually finished during normal operations.
2) Continue button is enabled before the installation has actually finished during startup.
3) Tests have to use a timeout (see bug 395430).

We also display an additional add-on in the ui during an incompatible update check which I plan on also fixing in this bug.

Comment 1

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

I have a patch that works but I'm going to rewrite nsIAddonUpdateListener so it uses similar listener callbacks as add-on updates except for add-on installs. I'm very tempted to just rename it to nsIAddonInstallListener.

Comment 2

[Dave Townsend [:mossop]]

Rob, one of the problems I'm hitting in bug 404024 is that while I can trigger an install and see when it ends, I can't easily find out the ID of the add-on that was installed. The current notifications all use the xpiURL as the nsIUpdateItem.id in the callbacks. Is this bug likely to help matters and if so do you have an eta for it?

Comment 3

[Mike Connor [:mconnor]]

Not going to continue to block on this, please renominate with reasons why we can't ship with this in final

Comment 4

[Tony Chung [:tchung]]

Drivers: bug 414936 is blocking b3, and is marked as a dependency on this bug.  Can we renominate this bug for blocking beta 3?

Comment 5

[Dave Townsend [:mossop]]

This does not really need to block beta 3, I just believe that the best way to resolve bug 414936 will involve fixing this which I am working on now.

Comment 6

[Tony Chung [:tchung]]

ah okay, i stand corrected.   Feel free to remove the flag request.  thanks

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This removes nsIAddonUpdateListener and replaces it with nsIAddonInstallListener which has multiple methods for different notifications. As well as the previous notifications there are also compatibiltiy check notifications. Additionally the final notification (onInstallsCompleted) is only dispatched when the EM has completely finished all current downloads and installs that it knows about (previously you could get into a case of receiving DIALOG_CLOSE for one download while a second was in progress). The installListeners are also notifies for all installs now including those started with installItemFromFile, this was really necessary since the end notifications have to be dispatched from there anyway.

This lets us simplify the frontend code, we don't need to know how many installs are in progress, just that there are some and when they have all finished.

The backend has some cleanup in IncompatibleObserver, it had some properties that were no longer being used. It now keeps a count of the compatibility checks running so that we can suitably fire onInstallsComplete.

Included are a set of unit tests that check an installListener receives appropriate notifications from various installs of compatible, incompatible, incompatible made compatible by update check and incompatible still incompatible after update check, running through installItemFromFile and addDownloads.

Comment 8

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

Dave, I'm mainly concerned with the following:

>     case nsIXPIProgressDialog.INSTALL_DONE:
>       --this._downloadCount;
>       // From nsInstall.h
>       // SUCCESS        = 0
>-      // REBOOT_NEEDED  = 999
>       // USER_CANCELLED = -210
>-      if (value != 0 && value != 999 && value != -210 && id != addon.xpiURL) {
>+      if (value != 0 && value != -210 && id != addon.xpiURL) {
The REBOOT_NEEDED check was needed when the same extension was installed a second time. Are you sure this change is the right thing to do?

>         ds.updateDownloadState(id, "failure");
>         ds.updateDownloadProgress(id, null);
>       }
>       transaction.removeDownload(addon.xpiURL);
>+      // A successful install will be passing notifications via installItemFromFile
>+      if (value != 0) {
>+        for (var i = 0; i < this._installListeners.length; ++i)
>+          this._installListeners[i].onInstallEnded(addon, value);
>+      }
What about a successful install that is reported as REBOOT_NEEDED?

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> Dave, I'm mainly concerned with the following:
> 
> >     case nsIXPIProgressDialog.INSTALL_DONE:
> >       --this._downloadCount;
> >       // From nsInstall.h
> >       // SUCCESS        = 0
> >-      // REBOOT_NEEDED  = 999
> >       // USER_CANCELLED = -210
> >-      if (value != 0 && value != 999 && value != -210 && id != addon.xpiURL) {
> >+      if (value != 0 && value != -210 && id != addon.xpiURL) {
> The REBOOT_NEEDED check was needed when the same extension was installed a
> second time. Are you sure this change is the right thing to do?

Now that install.js has been removed completely from xpinstall the REBOOT_NEEDED status can never be sent by xpinstall any longer.

Comment 10

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

thanks... interesting that it was being sent in the install.rdf case previously.

Comment 11

[Dave Townsend [:mossop]]

Well the EM monitors installs from any XPI files regardless of the install script so it would previously get the return code from the install.js if that happened to be the case.

Comment 12

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

I am fairly certain that I used to get this return code with ext's that only had an install.rdf when installing it a second time a couple of years ago and I'll check it out. The install.js case is totally understandable.

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This is pretty much as the last patch with some comment changes and moves the onInstallsComplete sending out of installItemFromFileInternal to make sure we catch all return cases from there.

Comment 14

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

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

>diff -r 32d658c33a2c toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js	Mon Feb 25 01:08:37 2008 -0800
>+++ b/toolkit/mozapps/extensions/content/extensions.js	Mon Feb 25 11:45:46 2008 +0000
>@@ -1240,72 +1241,77 @@ XPInstallDownloadManager.prototype = {
>...
>+  onInstallsCompleted: function()
>+  {
>+    gInstalling = false;
>+    gExtensionManager.sortTypeByProperty(nsIUpdateItem.TYPE_ADDON, "name", true);
Shouldn't this be TYPE_ANY now that bug 406184 has landed?

>diff -r 32d658c33a2c toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>--- a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl	Mon Feb 25 01:08:37 2008 -0800
>+++ b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl	Mon Feb 25 11:45:46 2008 +0000
>@@ -389,30 +389,30 @@ interface nsIExtensionManager : nsISuppo

>@@ -579,43 +579,92 @@ interface nsIUpdateItem : nsISupports

>-   * Install/Download state has changed
>+   * Called when an add-on starts to be downloaded. This will be called for every
>+   * new add-on downloaded including those started by add-on update.
nit: that's a tad clunky. How about?
Called when an add-on download starts. This will be called for every add-on downloaded including those started by an add-on update.

>    * @param   addon
>-   *          The addon that state changed for
>-   * @param   state
>-   *          The new state. States are defined in nsIXPIProgressDialog
>-   * @param   value
>-   *          Some data about the new state
>+   *          The add-on that started downloading
nit: The add-on that is being downloaded

>+  /**
>+   * Called when an add-on download is complete. This will be called for every
>+   * new add-on downloaded.
nit: 'new' isn't necessary here is it?

>+   * @param   addon
>+   *          The add-on that finished downloading
>+   */
>+  void onDownloadEnded(in nsIUpdateItem addon);
>+
>+  /**
>+   * Called when the extension manager starts to install an add-on either
>+   * through a call to installItemFromFile or from a webpage triggered install.
>+   * @param   addon
>+   *          The add-on started installing
nit: 'The add-on being installed' or similar

>+  /**
>+   * Called when an add-on compatibility check starts. This will be called 
>+   * during install if an add-on appears to be incompatible according to it's
nit: 'during an install if an add-on is incompatible according to it's'

>+   * install.rdf.
>+   * @param   addon
>+   *          The add-on that the compatibility check will be for
nit: The add-on that is having its compatibility checked

>+  /**
>+   * Called at the end of an add-on compatibility check. The status will be one
>+   * of the results from nsIAddonUpdateCheckListener to indicate whether new
>+   * information was found or not.
>+   * @param   addon
>+   *          The add-on that the compatibility check was for
>+   * @param   status
>+   *          The status code of the update operation
nit: The status code from nsIAddonUpdateCheckListener for the update operation

>+
>+  /**
>+   * Called when an add-on install completes. The status will be 0 on success.
>+   * any other value constitutes a failed install.
>+   * @param   addon
>+   *          The add-on that finished installing
>+   * @param   status
>+   *          The status of the installation
nit: please call out where the status code comes from

>diff -r 32d658c33a2c toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in	Mon Feb 25 01:08:37 2008 -0800
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in	Mon Feb 25 11:45:46 2008 +0000
>...
>@@ -5574,80 +5613,95 @@ ExtensionManager.prototype = {
>...
>     case nsIXPIProgressDialog.DIALOG_CLOSE:
>       for (var i = 0; i < this._transactions.length; ++i) {
>         if (this._transactions[i].id == transaction.id) {
>           this._transactions.splice(i, 1);
>           // Remove the observers when all transactions have completed.
>           if (this._transactions.length == 0) {
>             gOS.removeObserver(this, "offline-requested");
>             gOS.removeObserver(this, "quit-application-requested");
Can you move the removal of these observers so they are only removed when all of the installs have completed? 

r=me with those items addressed

Comment 15

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

(In reply to comment #14)
> >@@ -5574,80 +5613,95 @@ ExtensionManager.prototype = {
> >...
> >     case nsIXPIProgressDialog.DIALOG_CLOSE:
> >       for (var i = 0; i < this._transactions.length; ++i) {
> >         if (this._transactions[i].id == transaction.id) {
> >           this._transactions.splice(i, 1);
> >           // Remove the observers when all transactions have completed.
> >           if (this._transactions.length == 0) {
> >             gOS.removeObserver(this, "offline-requested");
> >             gOS.removeObserver(this, "quit-application-requested");
> Can you move the removal of these observers so they are only removed when all
> of the installs have completed? 

btw: I'm ok with this being a followup if it is difficult

Comment 16

[Dave Townsend [:mossop]]

Attachment: patch for checkin

(In reply to comment #14)
> >+    gInstalling = false;
> >+    gExtensionManager.sortTypeByProperty(nsIUpdateItem.TYPE_ADDON, "name", true);
> Shouldn't this be TYPE_ANY now that bug 406184 has landed?

Yep, hadn't realised this had bitrotted, that was the only change though.

>           if (this._transactions.length == 0) {
> > >             gOS.removeObserver(this, "offline-requested");
> > >             gOS.removeObserver(this, "quit-application-requested");
> > Can you move the removal of these observers so they are only removed when all
> > of the installs have completed? 
> 
> btw: I'm ok with this being a followup if it is difficult

Ok, will do a follow-up for that. Will need to add some flag on the EM that it is currently observing I think.

Comment 17

[Dave Townsend [:mossop]]

Checked in, no longer blocks bug 414936, holding open for that final change.

Checking in content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.173; previous revision: 1.172
done
Checking in public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v  <--  nsIExtensionManager.idl
new revision: 1.58; previous revision: 1.57
done
Checking in src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.281; previous revision: 1.280
done
Checking in test/unit/head_extensionmanager.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js,v  <--  head_extensionmanager.js
new revision: 1.3; previous revision: 1.2
done
Checking in test/unit/test_bug299716.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug299716.js,v  <--  test_bug299716.js
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug396129.js,v
done
Checking in test/unit/test_bug396129.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug396129.js,v  <--  test_bug396129.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_a_1/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_a_1/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_a_1/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_a_2/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_a_2/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_a_2/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_b_1/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_b_1/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_b_1/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_b_2/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_b_2/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_b_2/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_c_1/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_c_1/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_c_1/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_c_2/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_c_2/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_c_2/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_d_1/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_d_1/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_d_1/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_d_2/install.rdf,v
done
Checking in test/unit/addons/test_bug396129_d_2/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug396129_d_2/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
Checking in test/unit/data/test_bug394300.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug394300.rdf,v  <--  test_bug394300.rdf
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug396129.rdf,v
done
Checking in test/unit/data/test_bug396129.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug396129.rdf,v  <--  test_bug396129.rdf
initial revision: 1.1
done

Comment 18

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

Can we close this one out?

Comment 19

[Matthew Gregan [:kinetik]]

Patch landed, bug is showing up on the radar since it's still open and has a patch with a1.9.  I can't see any reason to keep this open.  Resolving.

Comment 20

[Dave Townsend [:mossop]]

Filed bug 433034 for the minor follow up work