Open Bug 353804 Opened 14 years ago Updated 4 months ago

Automatic update does not check for newer versions if a previous update is still in progress or staged

Categories

(Toolkit :: Application Update, defect, P2)

defect

Tracking

()

ASSIGNED
Tracking Status
firefox60 --- verified disabled
firefox61 --- wontfix
firefox62 --- wontfix

People

(Reporter: whimboo, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxgrowth][iu_tracking])

Attachments

(1 file, 10 obsolete files)

I have seen this behavior by updating an old Firefox installation to the newest one. Each of our users don't have admin rights on the computers. So automatic update doesn't work for them. When I logged in as administrator and started Firefox 1.5.0.3 to access "Help | Update Firefox" I have seen that it was already downloading the version 1.5.0.4. Also after I stopped the download process and restarted Firefox the newest 1.5.0.7 version isn't downloaded.

For my case I had to download 6.1MB for version 1.5.0.4 and afterwards additionally 6.1MB for the upgrade to version 1.5.0.7.

To circumvent such a behavior we have to check anytime for the newest version. Also if a software download was started or continued. Its a hassle for users with a small bandwidth to download two update packages.
Doesn't this just have to do with what AUS is offering?
(In reply to comment #1)
> Doesn't this just have to do with what AUS is offering?

I have done this today. By upgrading from 1.5.0.3 -> 1.5.0.4 -> 1.5.0.7 AUS offers the newest version. But it is not recognized by the client while an former upgrade to 1.5.0.4 was still in action for several month.
rhelmer:

Seems like this could explain some of the whacky bouncer numbers you were noticing... although, I don't know why it would be stuck on 1.5.0.4.

Maybe Henrik has millions of users? :-)
Not really. :) I think this applies especially for companies. Due to the missing msi installer updates have to be installed manually. This is done after longer periods which could be the cause why we see this older versions.
I was able to see that behavior today while updating a computer with the older 2.0.0.1 release. There was started a full download of Fx 2.0.0.4 some times ago. But it was not finished. When hitting the update button it restarts to download the 2.0.0.4 release and applied it. Afterwards I had to check and update again to get the 2.0.0.5 release. I think this should also apply for other OS but wasn't able to reproduce it there. I will try it with a nightly build later today.
No longer depends on: 350920
Duplicate of this bug: 384132
Same happened to me today with a nightly build from 03/04. The download stalled in background and I manually started the update check. So the started one was finally downloaded and my build got upgraded to 03/11. Now it downloads the next complete mar file to the 03/18 nighly version. Seen this with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031104 Minefield/3.0b5pre
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
Product: Firefox → Toolkit
Rob, I thought that I had cc'ed you to this bug. But it doesn't look like. Will there be a way to fix it? I was running into this problem very often in the last days while having a running download stuck in the background (bug 407508). And on slow connections its annoying to have to download 17MB twice.
Hey Henrik, I watch this component and have seen this bug previously. It is pretty low on my priority list atm so I am unsure if I will get to this for 3.1... more likely 3.2
Duplicate of this bug: 431779
Duplicate of this bug: 474251
It just happened again.  Minutes ago, I downloaded the following:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090317 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090317044128


Now it wants me to download 3.6a1 2009017101214
Duplicate of this bug: 458737
Attached patch patch in progress (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
(In reply to comment #12)
> It just happened again.  Minutes ago, I downloaded the following:
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090317
> Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090317044128
> 
> 
> Now it wants me to download 3.6a1 2009017101214
Peter S., by any chance are you using zip builds and extracting on top of an existing build?
Duplicate of this bug: 463170
Duplicate of this bug: 417802
(In reply to comment #15)
> (In reply to comment #12)
> > It just happened again.  Minutes ago, I downloaded the following:
> > Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090317
> > Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090317044128
> > 
> > 
> > Now it wants me to download 3.6a1 2009017101214
> Peter S., by any chance are you using zip builds and extracting on top of an
> existing build?

Whenever I download something, it's either through the help->check for updates option or the "installer.exe" files in the nightly build threads.
Attached patch need to think on this some more (obsolete) — Splinter Review
Attachment #369232 - Attachment is obsolete: true
Attached patch little better (obsolete) — Splinter Review
Attachment #369463 - Attachment is obsolete: true
Attached patch almost there (obsolete) — Splinter Review
Attachment #369474 - Attachment is obsolete: true
Filed spinoff bug 485493 to fix displaying updates that are downloading in the history ui. For now I'm going to not display them since we don't have strings for the update downloading history ui.
Split off the canceling of the download instead of resuming on startup to bug 485624. Most of the bugs duped to this bug fit into this new bug.
Depends on: 485624
Summary: Automatic update does not check for newer versions before continuing a running software download → Automatic update does not check for newer versions before resuming an update download on startup
Duplicate of this bug: 437969
Robert, do you have a new ETA for this bug? I got this situation a couple of times the last days because I haven't run the update check daily.
Duplicate of this bug: 537121
Wow.. this an old bug.. Anyway, it just occured:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100618 Firefox/3.6.4 notfox ID:20100618040705

This build was installed on Saturday, June 19th at 2:20 AM.


However, the below build:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100618 Firefox/3.6.4 notfox ID:201006187161137

was installed just minutes prior.

The previous build before that was installed on the 17th in the afternoon in US Eastern Time.

I believe the 17th had a re-spin, but it still should have grabbed the most recent build without having me download two builds in rapid succession.
Sorry for the double-post, but here's a bit more info, which could be pertinent to the above post.

http://forums.mozillazine.org/viewtopic.php?f=23&t=1925201&start=45

Wildcat Ray says:
"Yes, there was another build for, I think, 3 total yesterday. The last was 20100617161137."

Apparenty, there were issues with the FTP site, according to a post at that mozillazine link.
Sure and though this is an old bug it doesn't happen very often especially for builds other than nightly builds for obvious reasons and there are more serious bugs to spend time on.
Duplicate of this bug: 638999
Tim, could you take a look at this one before bug 657472 since we'll want this for bug 657472 as well?
Assignee: robert.bugzilla → tim.taubert
Attached patch patch v1 (obsolete) — Splinter Review
Hey Robert,

I didn't quite understand what your WIP patch actually does so I decided to just take a stab how I would do it from reading the existing code to get familiar with it.

The test I wrote is working but some others (like three) are broken that would need to be fixed because they're expecting the download to happen right after initializing the service and not just after checking for a newer update.

Looking forward to your feedback :)
Attachment #533273 - Flags: feedback?(robert.bugzilla)
Comment on attachment 533273 [details] [diff] [review]
patch v1

(In reply to comment #33)
>...
> I didn't quite understand what your WIP patch actually does so I decided to
> just take a stab how I would do it from reading the existing code to get
> familiar with it.
I'm glad you did... that patch is less than useful now.

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -1294,16 +1290,71 @@ UpdateService.prototype = {
>       }
>       update.QueryInterface(Ci.nsIWritablePropertyBag);
>       update.setProperty("patchingFailed", oldType);
>       prompter.showUpdateError(update);
>     }
>   },
> 
>   /**
>+   * TODO
Be sure to note that this is only used for background downloads.

>+   * @param   update
>+   *          TODO
>+   */
>+  _resumeDownload: function AUS__resumeDownload(update) {
>+    LOG("UpdateService:_resumeDownload");
>+    let self = this;
>+
>+    let downloadUpdate = function () {
>+      let status = self.downloadUpdate(update, true);
>+      if (status == STATE_NONE)
Since this is being moved please rename status to state.

>diff --git a/toolkit/mozapps/update/test/unit/test_bug353804.js b/toolkit/mozapps/update/test/unit/test_bug353804.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/test/unit/test_bug353804.js
>@@ -0,0 +1,109 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+let gNextRunFunc;
>+let gResponseBody;
>+
>+function run_test() {
>+  do_test_pending();
>+  do_register_cleanup(end_test);
>+
>+  setUpdateChannel();
>+  setUpdateURLOverride();
>+
>+  // The mock XMLHttpRequest is MUCH faster
>+  overrideXHR(callHandleEvent);
>+  // The HTTP server is only used for the mar file downloads which is slow
>+  start_httpserver(URL_PATH);
>+
>+  standardInit();
>+
>+  // TODO
>+  runTest("1.0", function run_test_v10() {
>+    logTestInfo("Testing: activeUpdate should not equal null");
>+    do_check_neq(gUpdateManager.activeUpdate, null);
>+    logTestInfo("Testing: activeUpdate.state should equal STATE_DOWNLOADING");
>+    do_check_eq(gUpdateManager.activeUpdate.state, STATE_DOWNLOADING);
>+    logTestInfo("Testing: activeUpdate.appVersion should equal 1.0");
>+    do_check_eq(gUpdateManager.activeUpdate.appVersion, "1.0");
>+
>+    logTestInfo("Testing: Update should be currently downloaded");
nit: Update should be downloading

>+    do_check_eq(gAUS.isDownloading, true);
>+    gAUS.pauseDownload();
>+
>+    // TODO
>+    runTest("1.1", function run_test_v11() {
>+      logTestInfo("Testing: activeUpdate should not equal null");
>+      do_check_neq(gUpdateManager.activeUpdate, null);
>+      logTestInfo("Testing: activeUpdate.state should equal STATE_DOWNLOADING");
>+      do_check_eq(gUpdateManager.activeUpdate.state, STATE_DOWNLOADING);
>+      logTestInfo("Testing: activeUpdate.appVersion should equal 1.1");
>+      do_check_eq(gUpdateManager.activeUpdate.appVersion, "1.1");
>+
>+      logTestInfo("Testing: Update should be currently downloaded");
nit: Update should be downloading

There are a couple of edgecases where we might want to do this as well such as when an update fails to apply.

Looks good and thanks!
Attachment #533273 - Flags: feedback?(robert.bugzilla) → feedback+
You probably thought of this already, but ... will the new code cope with the rapid-release model where the version may not be changing, but the buildID will be ?
Thought about it a little. Since we have had the build id format change a few times in the past we can only check if the value is different vs. greater.
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #34)
> >   /**
> >+   * TODO
> Be sure to note that this is only used for background downloads.

Done.

> >+    let downloadUpdate = function () {
> >+      let status = self.downloadUpdate(update, true);
> >+      if (status == STATE_NONE)
> Since this is being moved please rename status to state.

Fixed.

> >+    logTestInfo("Testing: Update should be currently downloaded");
> nit: Update should be downloading
> >+      logTestInfo("Testing: Update should be currently downloaded");
> nit: Update should be downloading

Fixed.

> There are a couple of edgecases where we might want to do this as well such
> as when an update fails to apply.

Are there any specific edge cases I should also check? When an update fails to apply the complete update is picked in Downloader._selectPatch(), isn't it? If the partial update failed to apply and there is a newer one it's chosen before entering this method.

(In reply to comment #36)
> Thought about it a little. Since we have had the build id format change a
> few times in the past we can only check if the value is different vs.
> greater.

The patch does now choose the remote update if the remote version is equal but the buildID differs.
Attachment #369636 - Attachment is obsolete: true
Attachment #533273 - Attachment is obsolete: true
Attachment #533927 - Flags: review?(robert.bugzilla)
Comment on attachment 533927 [details] [diff] [review]
patch v2

Looks fine and thanks!
Attachment #533927 - Flags: review?(robert.bugzilla) → review+
(In reply to comment #37)
>...
> > There are a couple of edgecases where we might want to do this as well such
> > as when an update fails to apply.
> 
> Are there any specific edge cases I should also check? When an update fails
> to apply the complete update is picked in Downloader._selectPatch(), isn't
> it? If the partial update failed to apply and there is a newer one it's
> chosen before entering this method.
I suspect there are a couple of cases but this handles the case that is important and if there are other cases they can be fixed later.
Comment on attachment 533927 [details] [diff] [review]
patch v2

I believe there is one other case that needs to be taken care of. If there is an update already downloaded and waiting to be applied the next time an update check is performed it should check if there is a newer update available and if there is it should discard the existing update and download the newer one
Attachment #533927 - Flags: review+ → review-
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to comment #40)
> I believe there is one other case that needs to be taken care of. If there
> is an update already downloaded and waiting to be applied the next time an
> update check is performed it should check if there is a newer update
> available and if there is it should discard the existing update and download
> the newer one

Done. I completely restructured the patch and I needed some time to understand why those tests were failing... That's why it took a bit longer :)
Attachment #533927 - Attachment is obsolete: true
Attachment #535632 - Flags: review?(robert.bugzilla)
This patch leaks

== BloatView: ALL (cumulative) LEAK STATISTICS, default process 3816

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          22     9048    15843      243 (  358.66 +/-   562.43)    33257      247 ( 1359.68 +/-  2109.38)
   2 BackstagePass                                  20       20        1        1 (    1.00 +/-     0.00)       65        4 (   20.28 +/-     9.53)
  24 Monitor                                         4        4       22        1 (    8.51 +/-     4.50)        0        0 (    0.00 +/-     0.00)
  25 Mutex                                           4        8       60        2 (   30.40 +/-    16.95)        0        0 (    0.00 +/-     0.00)
  31 SharedScriptableHelperForJSIID                  8        8        1        1 (    1.00 +/-     0.00)      212        2 (    6.66 +/-     2.14)
  41 XPCNativeScriptableInfo                         8       16      233        2 (  117.00 +/-    67.05)        0        0 (    0.00 +/-     0.00)
  42 XPCNativeScriptableShared                     160     1440      254        9 (   12.83 +/-     4.16)        0        0 (    0.00 +/-     0.00)
  45 XPCWrappedNative                               52     4108      488       79 (  262.05 +/-   132.44)     1279       79 (  247.31 +/-   139.88)
  46 XPCWrappedNativeProto                          32      448       96       14 (   51.26 +/-    26.17)        0        0 (    0.00 +/-     0.00)
 118 nsJSCID                                        44      704       41       16 (   23.65 +/-    10.96)      224       16 (   68.94 +/-    36.80)
 119 nsJSID                                         32       64       55        2 (   28.00 +/-    15.67)      190        2 (   56.99 +/-    31.24)
 120 nsJSIID                                        20      980      102       49 (   59.54 +/-    27.92)      670       49 (  190.15 +/-    98.26)
 164 nsStringBuffer                                  8      136     3765       17 ( 1189.53 +/-   616.11)    12162       18 ( 3540.54 +/-  2140.85)
 173 nsSystemPrincipal                              32       32        1        1 (    1.00 +/-     0.00)      105        1 (    7.05 +/-     1.45)
 187 nsVoidArray                                     4        4      449        1 (   75.14 +/-    30.87)        0        0 (    0.00 +/-     0.00)
 193 nsXPCComponents                                56      112       17        2 (    9.00 +/-     4.71)      136        4 (   41.16 +/-    21.93)
 194 nsXPCComponents_Classes                        16       32        7        2 (    4.00 +/-     1.86)      106        4 (   16.93 +/-    10.08)
 198 nsXPCComponents_Interfaces                     24       48        8        2 (    4.50 +/-     2.14)      141        4 (   19.08 +/-    12.51)
 199 nsXPCComponents_Results                        16       32        6        2 (    3.50 +/-     1.58)       50        4 (   16.90 +/-     8.42)
 200 nsXPCComponents_Utils                          20       20        8        1 (    4.27 +/-     2.25)       69        1 (   18.62 +/-     8.93)
 203 nsXPCWrappedJS                                 60       60       31        1 (   15.43 +/-     8.41)      241        2 (   52.55 +/-    26.60)
 204 nsXPCWrappedJSClass                            40       40       22        1 (   10.98 +/-     5.89)      143        1 (   20.78 +/-     8.52)
 210 xptiInterfaceInfo                              16      560     1160       35 (  154.81 +/-   128.58)     3763       54 (  297.38 +/-   188.70)
 211 xptiInterfaceInfoManager                       96       96        1        1 (    1.00 +/-     0.00)      737        2 (   10.88 +/-     1.12)
 212 xptiWorkingSet                                 76       76        1        1 (    1.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)

nsTraceRefcntImpl::DumpStatistics: 212 entries
That was from running test_0064_manager.js but a couple of others leak as well likely for the same reason
Comment on attachment 535632 [details] [diff] [review]
patch v3

>   /**
>+   * Returns true if the given <candidate> is newer than <update>.
>+   * @param   candidate
>+   *          The candidate for which to check if it's newer than <update>.
>+   * @param   update
>+   *          The update to compare the <candidate> to.
>+   */
>+  _updateIsNewerThan: function AUS__updateIsNewerThan(candidate, update) {
>+    if (candidate.buildID == update.buildID)
>+      return false;
>+
>+    // The buildIDs differ but the appVersions are equal so <candidate> is
>+    // considered newer than <update>.
>+    if (candidate.appVersion == update.appVersion)
>+      return true;
>+
>+    return Services.vc.compare(candidate.appVersion, update.appVersion) > 0;
Might as well just go with the following and remove the equals check
return Services.vc.compare(candidate.appVersion, update.appVersion) >= 0;

If you'd like to shorten the anonymous function names please go for it. For example,
AUS__fetchNewestUpdate_onCheckComplete
could be written as
AUS__fnu_onCheckComplete

Mainly minusing because of the leaks
Attachment #535632 - Flags: review?(robert.bugzilla) → review-
Attached patch patch v4 (obsolete) — Splinter Review
(In reply to comment #45)
> Might as well just go with the following and remove the equals check
> return Services.vc.compare(candidate.appVersion, update.appVersion) >= 0;

Yeah, of course :) Fixed.

> If you'd like to shorten the anonymous function names please go for it. For
> example,
> AUS__fetchNewestUpdate_onCheckComplete
> could be written as
> AUS__fnu_onCheckComplete

Fixed.

> Mainly minusing because of the leaks

Fixed.
Attachment #535632 - Attachment is obsolete: true
Attachment #540072 - Flags: review?(robert.bugzilla)
Comment on attachment 540072 [details] [diff] [review]
patch v4

There is one thing that this breaks. If the update is for a newer version updating to it might disable add-ons that are compatible with the update that was originally accepted by the user. With the rapid release cycle this is even more of a problem since a 5.0 user might get an unprompted download for 6.0, not run Firefox for a while, and get 7.0 which would then disable some of their add-ons.

I think the flow for this case should be:
if the version is the same and the build id is different download the update
if the version is different get a list of add-ons that are incompatible with the new update and the old update. If the list is different continue on with the old update otherwise download the new update.

The patch looks fine otherwise. Sorry about this :(
Attachment #540072 - Flags: review?(robert.bugzilla) → review-
Attached patch patch v5 (obsolete) — Splinter Review
(In reply to comment #48)
> There is one thing that this breaks. If the update is for a newer version
> updating to it might disable add-ons that are compatible with the update
> that was originally accepted by the user. With the rapid release cycle this
> is even more of a problem since a 5.0 user might get an unprompted download
> for 6.0, not run Firefox for a while, and get 7.0 which would then disable
> some of their add-ons.
> 
> I think the flow for this case should be:
> if the version is the same and the build id is different download the update
> if the version is different get a list of add-ons that are incompatible with
> the new update and the old update. If the list is different continue on with
> the old update otherwise download the new update.

Yeah, good catch... fixed. Do you have any idea how to test this? The only add-on related thing I could find is "gDisableNoUpdateAddon" but this flag would have to be switched between those incompatibility checks.
Attachment #540072 - Attachment is obsolete: true
Attachment #542126 - Flags: review?(robert.bugzilla)
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #542126 - Attachment is obsolete: true
Attachment #542127 - Flags: review?(robert.bugzilla)
Attachment #542126 - Flags: review?(robert.bugzilla)
What are the odds this would make the cutoff for aurora next week?
Very good... finishing the review of the existing patch hopefully today and though I am not positive of the best way to test it I see a couple of ways it can be tested.
Comment on attachment 542127 [details] [diff] [review]
patch v6

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -1355,23 +1429,113 @@ UpdateService.prototype = {

>+  /*
>+   * Checks whether the given <addon> is incompatible with <update>.
>+   * @param update
>+   *        The update to which <addon> should be compatible.
>+   * @param addon
>+   *        The addon to check compatibility for.
>+   * @returns Whether <addon> is incompatible with <update>.
nit: line these up and use @return on all new code. The pre-existing @returns are actually incorrect.

>+   * @param  addon
>+   *         The addon to check compatibility for.
>+   * @return Whether <addon> is incompatible with <update>.


>+   */
>+  _isAddonIncompatibleWithUpdate:
>+    function AUS__isAddonIncompatibleWithUpdate(update, addon) {
>+
>+    // Protect against code that overrides the add-ons manager and doesn't
>+    // implement the isCompatibleWith or the findUpdates method.
>+    if (!("isCompatibleWith" in addon) || !("findUpdates" in addon)) {
>+      let errMsg = "Add-on doesn't implement either the isCompatibleWith " +
>+                   "or the findUpdates method!";
>+      if (addon.id)
>+        errMsg += " Add-on ID: " + addon.id;
>+      Components.utils.reportError(errMsg);
>+      return false;
>+    }
>+
>+    // If an add-on isn't appDisabled and isn't userDisabled then it is
>+    // either active now or the user expects it to be active after the
>+    // restart. If that is the case and the add-on is not installed by the
>+    // application and is not compatible with the new application version
>+    // then the user should be warned that the add-on will become
>+    // incompatible. If an addon's type equals plugin it is skipped since
>+    // checking plugins compatibility information isn't supported and
>+    // getting the scope property of a plugin breaks in some environments
>+    // (see bug 566787).
>+    try {
>+       return addon.type != "plugin" &&
>+              !addon.appDisabled && !addon.userDisabled &&
>+              addon.scope != AddonManager.SCOPE_APPLICATION &&
>+              addon.isCompatible &&
>+              !addon.isCompatibleWith(update.appVersion, update.platformVersion);
>+    }
>+    catch (e) {
>+      Components.utils.reportError(e);
>+      return false;
>+    }
nit: I personally prefer putting the return at the very end so it is obvious what the return value is.

>+  },
>+
>+  /**
>    * Determine the update from the specified updates that should be offered.
>    * If both valid major and minor updates are available the minor update will
>    * be offered.
>    * @param   updates
>    *          An array of available nsIUpdate items
>    * @returns The nsIUpdate to offer.
>    */
>   selectUpdate: function AUS_selectUpdate(updates) {

Still thinking of the best way to test this
Attachment #542127 - Flags: review?(robert.bugzilla) → review+
I think the best way to test the addons case will be with mochitest-chrome tests though I suspect it will still be a pita. I'll take a stab at writing those tests tonight.
Attached patch patch v7Splinter Review
(In reply to comment #54)
> >+  /*
> >+   * Checks whether the given <addon> is incompatible with <update>.
> >+   * @param update
> >+   *        The update to which <addon> should be compatible.
> >+   * @param addon
> >+   *        The addon to check compatibility for.
> >+   * @returns Whether <addon> is incompatible with <update>.
> nit: line these up and use @return on all new code. The pre-existing
> @returns are actually incorrect.

Fixed.

> >+    try {
> >+       return addon.type != "plugin" &&
> >+              !addon.appDisabled && !addon.userDisabled &&
> >+              addon.scope != AddonManager.SCOPE_APPLICATION &&
> >+              addon.isCompatible &&
> >+              !addon.isCompatibleWith(update.appVersion, update.platformVersion);
> >+    }
> >+    catch (e) {
> >+      Components.utils.reportError(e);
> >+      return false;
> >+    }
> nit: I personally prefer putting the return at the very end so it is obvious
> what the return value is.

Fixed.
Attachment #542127 - Attachment is obsolete: true
I have a couple of the tests written and am working on finishing them up. I've also given Mossop a heads up regarding reviewing the tests which will hopefully be done by tomorrow.
bah... I tried to go the xpcshell path since it is cleaner and spent several hours on this before realizing that the overridden xhr makes this impossible without changing the add-ons mgr's use of xhr. Back to mochitest-chrome
After writing some of the additional tests over the weekend I decided that it would be best to let this land on the trunk early after the merge this week so it has time to bake before hitting aurora.
The last comment is from July. Has this already landed? Is it fixed?
No. There is a bug in the current implementation that breaks update and I need to find time to delve into why it breaks
From a mirror management point-of-view I'm really looking forward to this change, as it will mean that we'll have to meet less requests for old versions.
Robert, can you upload the tests you wrote so I could maybe have a look at this again and make them work? IIRC you found some issue that isn't covered by this patch - but I can't remember what exactly it was.
Note that this will probably affect bug 307181 in many ways, so I'd rather the work on this bug happen on top of my patches there, so that I wouldn't have to rewrite that code for a third time.  :-)
Also, with regard to the automated tests in the patch, you probably want to add a Win32 Maintenance Service version of the test(s) in the test_svc directory as well.
(In reply to Ehsan Akhgari [:ehsan] from comment #64)
> Note that this will probably affect bug 307181 in many ways, so I'd rather
> the work on this bug happen on top of my patches there, so that I wouldn't
> have to rewrite that code for a third time.  :-)

Yes, we should probably wait for bug 307181 to be fixed.
Duplicate of this bug: 733986
Duplicate of this bug: 763206
(In reply to Tim Taubert [:ttaubert] from comment #66)
> (In reply to Ehsan Akhgari [:ehsan] from comment #64)
> > Note that this will probably affect bug 307181 in many ways, so I'd rather
> > the work on this bug happen on top of my patches there, so that I wouldn't
> > have to rewrite that code for a third time.  :-)
> 
> Yes, we should probably wait for bug 307181 to be fixed.

Looks like that bug has been fixed. Any word on when this one might follow suit?

(BTW, I think the summary of this bug should be changed: It specifically mentions checking for new updates while a previous update is still downloading, but most of the bugs duped against it a referring to updates that have already been downloaded.)
(In reply to Gordon P. Hemsley [:gphemsley] from comment #69)
> (In reply to Tim Taubert [:ttaubert] from comment #66)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #64)
> > > Note that this will probably affect bug 307181 in many ways, so I'd rather
> > > the work on this bug happen on top of my patches there, so that I wouldn't
> > > have to rewrite that code for a third time.  :-)
> > 
> > Yes, we should probably wait for bug 307181 to be fixed.
> 
> Looks like that bug has been fixed. Any word on when this one might follow
> suit?
There is still fallout from that bug which needs to be fixed first and there are also other bugs we are working on.
Duplicate of this bug: 759791
Summary: Automatic update does not check for newer versions before resuming an update download on startup → Automatic update does not check for newer versions if a previous update is still in progress or staged
(In reply to Robert Strong [:rstrong] (do not email) from comment #70)
> (In reply to Gordon P. Hemsley [:gphemsley] from comment #69)
> > (In reply to Tim Taubert [:ttaubert] from comment #66)
> > > (In reply to Ehsan Akhgari [:ehsan] from comment #64)
> > > > Note that this will probably affect bug 307181 in many ways, so I'd rather
> > > > the work on this bug happen on top of my patches there, so that I wouldn't
> > > > have to rewrite that code for a third time.  :-)
> > > 
> > > Yes, we should probably wait for bug 307181 to be fixed.
> > 
> > Looks like that bug has been fixed. Any word on when this one might follow
> > suit?
> There is still fallout from that bug which needs to be fixed first and there
> are also other bugs we are working on.

rstrong: what bug# is tracking fallout/cleanup work here? Or more specifically, whats left to do before ttaubert's patch can be landed?
joduinn, dependencies on bug 307181 and most of the fallout has been cleaned up. As stated somewhere else in this bug there was a problem with the patch under some conditions. Also, at this time we are busy with other high priority work.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 857657
Duplicate of this bug: 861464
(In reply to Robert Strong [:rstrong] (do not email) from comment #73)
> joduinn, dependencies on bug 307181 and most of the fallout has been cleaned
> up. As stated somewhere else in this bug there was a problem with the patch
> under some conditions. Also, at this time we are busy with other high
> priority work.

ping?
(In reply to John O'Duinn [:joduinn] from comment #76)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #73)
> > joduinn, dependencies on bug 307181 and most of the fallout has been cleaned
> > up. As stated somewhere else in this bug there was a problem with the patch
> > under some conditions. Also, at this time we are busy with other high
> > priority work.
> 
> ping?
With this primarily affecting users of channels other than release it keeps getting pushed to the back of the queue as other work that affects release users comes up. So, no progress on this.
Duplicate of this bug: 893775
First time I got this over a version major number bump... I don't run nightly/Win7/VBox/Linux very often.  Started it and nightly was still on 25.0a1. Auto-downloaded 26.0a1 (2013-08-14), restarted and then 26.0a1 (2013-08-21).
Duplicate of this bug: 685499
Whiteboard: [fxgrowth]
Duplicate of this bug: 797121
Duplicate of this bug: 559708
Duplicate of this bug: 1145309
Duplicate of this bug: 695629
Duplicate of this bug: 1144648
Comment taken from bug 861464:
"On a not so recent Nightly, you have to restart several times until the browser is fully updated. There should be a way to jump straight to the latest nightly, or maybe that should even be the default."
Note: the most you will have to restart is twice.
I stared 45.0.1, it informed me that a new version is available (some dialog). I closed it and a few minutes later opened the about dialog. There it downloaded about 50MB and prompted for restart.
The version was now 47.0.2 and it downloaded another update, this time 50.1.0

Windows 10
(In reply to David Balažic from comment #88)
> I stared 45.0.1, it informed me that a new version is available (some
> dialog). I closed it and a few minutes later opened the about dialog. There
> it downloaded about 50MB and prompted for restart.
> The version was now 47.0.2 and it downloaded another update, this time 50.1.0
> 
> Windows 10

That is expected behavior and is unrelated to this bug. 47.0.2 has to be updated to before updating to the latest.
47.0.2 - websense detection ( bug 1305847 ) and SSE2 requirement ( bug 1271755 )
Duplicate of this bug: 1336925
Duplicate of this bug: 1337165
Duplicate of this bug: 1146422
I wonder if it makes sense to revive this? With Nightly updates twice a day we'd make a lot of people happier if we fixed this.
Indeed, I think a lot of nightly users would appreciate that.
:ttaubert do you know who you could assign to review your patch?
(In reply to Rémy Hubscher (:natim) from comment #95)
> :ttaubert do you know who you could assign to review your patch?

The patch will at least need to be rebased, but I suspect it needs to be rewritten.
Priority: -- → P2
Whiteboard: [fxgrowth] → [fxgrowth][nightly-only]
Is it really a nightly-only bug? I mean, of course nightly is much more affected, but stable could also either have close updates (security fixes) or have users restarting the browser very rarely, right?
(In reply to Clément Lefèvre from comment #97)
> Is it really a nightly-only bug? I mean, of course nightly is much more
> affected, but stable could also either have close updates (security fixes)
> or have users restarting the browser very rarely, right?

This bug can happen on any channel. It's entirely possible that a user could download a release channel update, not restart for an entire 6 weeks, and then be unable to apply the next released version because of the pending update. Beta & DevEdition also update twice a week these days, and I imagine it's not uncommon to hit this on those channels.
(In reply to Ben Hearsum (:bhearsum) from comment #98)
> (In reply to Clément Lefèvre from comment #97)
> > Is it really a nightly-only bug? I mean, of course nightly is much more
> > affected, but stable could also either have close updates (security fixes)
> > or have users restarting the browser very rarely, right?
> 
> This bug can happen on any channel. It's entirely possible that a user could
> download a release channel update, not restart for an entire 6 weeks, and
> then be unable to apply the next released version because of the pending
> update. Beta & DevEdition also update twice a week these days, and I imagine
> it's not uncommon to hit this on those channels.

Yup, I was just saying that regarding the whiteboard :jimm added ([nightly-only]). Won't remove it myself though, only him can judge I guess.
(In reply to Ben Hearsum (:bhearsum) from comment #98)
> (In reply to Clément Lefèvre from comment #97)
> > Is it really a nightly-only bug? I mean, of course nightly is much more
> > affected, but stable could also either have close updates (security fixes)
> > or have users restarting the browser very rarely, right?
> 
> This bug can happen on any channel. It's entirely possible that a user could
> download a release channel update, not restart for an entire 6 weeks, and
> then be unable to apply the next released version because of the pending
> update. Beta & DevEdition also update twice a week these days, and I imagine
> it's not uncommon to hit this on those channels.

FWIW, I see this all the time on my family members' machines, where I have beta installed. Very often they're more than a few beta versions behind. Sometimes they're more than a few major releases behind :) The best was when one machine was behind a watershed, and so I had to restart Firefox 3 times to get them up-to-date.
Hey Rob, do you know if we have telemetry on this?
Flags: needinfo?(robert.strong.bugs)
With the two nightlies per day, it would be indeed interesting to fix that.
(removing the nightly-only as it is also impacting other branches)
Whiteboard: [fxgrowth][nightly-only] → [fxgrowth]
So I had a Nightly installed from October 3rd, and then was away all the last week. Today I was informed of an update, and I downloaded it. After restarting Firefox the update was not applied, and the software update dialog appeared immediately, and forced me to do another restart of Firefox. then the update was applied.

Is that a behavior which is related to this bug?
Duplicate of this bug: 1521470

Any news here?
I had my PC off for several months.
When I started it today, I opened Firefox and immediately opened the about dialog.
The installed version was 64.0.
It started downloading an update.
After restarting FF, I opened the about dialog again, no it showed version 65.0.2 and again started downloading an update.
After another restart, it was at version 66.0(not downloading another version)

On Windows 8.1

Flags: needinfo?(robert.strong.bugs)
Duplicate of this bug: 1180764
Priority: P2 → P3
Duplicate of this bug: 1560279

Hi Robert, how feasible would it be to make this happen? I imagine it might not be a trivial project.

Aside from the mild inconvenience for Nightly users, this might help with getting more people in the general release population onto the most current releases, faster (and with less confusion).

Flags: needinfo?(robert.strong.bugs)

Hi all - This bug is urgent / critical as we want to do as much as we can to hit the relationship KPI. Being able to reach everyone with the WNP would help us tremendously. Thanks so much for your help!

Severity: major → critical
Priority: P3 → P1

(In reply to Marissa (Reese) Wood from comment #109)

Hi all - This bug is urgent / critical as we want to do as much as we can to hit the relationship KPI. Being able to reach everyone with the WNP would help us tremendously. Thanks so much for your help!

Can I get some clarification on why this was marked P1? FYI being able to jump over releases during updates generally isn't a major issue in terms of getting users who run release builds updated. It's a bit of an annoyance for Nightly users but not for other releases.

Our longer term solution here is the background updater. I'd rather work on that because it's a really good solution to this vs. what's proposed here.

Flags: needinfo?(robert.strong.bugs) → needinfo?(marissawood)
Priority: P1 → P3

I marked this as P1 because we are closing in on the end of the year and need to meet our relationship KPI. Reaching 17M people (or not) could help us a lot.

Flags: needinfo?(marissawood)

Oh geez! I'm so embarrassed. I have two bugs open and commented on the wrong one. Thanks so much for calling this out, Jim. You can reduce priority.

Flags: needinfo?(jmathies)

(In reply to Jim Mathies [:jimm] from comment #110)

Can I get some clarification on why this was marked P1? FYI being able to jump over releases during updates generally isn't a major issue in terms of getting users who run release builds updated. It's a bit of an annoyance for Nightly users but not for other releases.

It can also be the case for non-nightly users who doesn't restart their computer and browser really often. This way if there are often released versions due to some fixes, they can be late of one or two updates.

Flags: needinfo?(jmathies)
Duplicate of this bug: 1608238
Severity: critical → normal
Whiteboard: [fxgrowth] → [fxgrowth][iu_tracking]

This can be particularly troublesome if there are emergency updates, causing updates to come down the pipe more frequently than normal. And with the faster release cadence, it's even more likely to happen than it used to be, with users that rarely if ever restart. I go weeks at a time between browser restarts sometimes.

(Side note: it's a little bit fun to note this has had patches submitted over ten years now and still is open) 😁

Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Priority: P3 → P2
You need to log in before you can comment on or make changes to this bug.