Add incompatible update start and done states to nsIAddonUpdateListener

RESOLVED FIXED in mozilla1.9beta5

Status

()

P4
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: rstrong, Assigned: mossop)

Tracking

Trunk
mozilla1.9beta5
Points:
---
Bug Flags:
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Flags: blocking-firefox3?
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.
Flags: blocking-firefox3? → blocking-firefox3+

Updated

11 years ago
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Priority: -- → P2

Updated

11 years ago
Target Milestone: Firefox 3 M10 → Firefox 3 M11

Updated

11 years ago
Priority: P2 → P4
(Assignee)

Comment 2

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

Updated

11 years ago
Blocks: 408116
(Assignee)

Updated

11 years ago
No longer blocks: 408116
Not going to continue to block on this, please renominate with reasons why we can't ship with this in final
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
(Assignee)

Updated

11 years ago
Blocks: 414936

Comment 4

11 years ago
Drivers: bug 414936 is blocking b3, and is marked as a dependency on this bug.  Can we renominate this bug for blocking beta 3?
Flags: blocking-firefox3- → blocking-firefox3?
(Assignee)

Comment 5

11 years ago
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.
Assignee: robert.bugzilla → dtownsend

Comment 6

11 years ago
ah okay, i stand corrected.   Feel free to remove the flag request.  thanks
(Assignee)

Updated

11 years ago
Flags: blocking-firefox3?
(Assignee)

Comment 7

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

Updated

11 years ago
Whiteboard: [has patch]
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?
(Assignee)

Comment 9

11 years ago
(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.
thanks... interesting that it was being sent in the install.rdf case previously.
(Assignee)

Comment 11

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

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #301868 - Attachment is obsolete: true
Attachment #301868 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 13

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

Comment 16

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

Updated

11 years ago
Attachment #309550 - Flags: approval1.9?
Attachment #309550 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 17

11 years ago
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
No longer blocks: 414936
Flags: in-testsuite+
Whiteboard: [has patch]
(Assignee)

Updated

11 years ago
Target Milestone: Firefox 3 beta3 → Firefox 3 beta5
Can we close this one out?
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

11 years ago
Filed bug 433034 for the minor follow up work
Product: Firefox → Toolkit
Depends on: 494391
You need to log in before you can comment on or make changes to this bug.