Closed Bug 353804 Opened 18 years ago Closed 4 years ago

Multiple update downloads: automatic update does not check for newer versions if a previous update is still in progress or staged

Categories

(Toolkit :: Application Update, defect, P2)

Product:
Toolkit
Component:
Application Update
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox60 --- verified disabled
firefox61 --- wontfix
firefox62 --- wontfix
firefox86 --- fixed

People

(Reporter: whimboo, Assigned: bytesized)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [fxgrowth][iu_tracking])

Attachments

(10 files, 11 obsolete files)

Bug 353804 - Enable downloading multiple updates per Firefox session r=mhowell!
(deleted), text/x-phabricator-request
Details
Bug 353804 - String changes needed for multiple updates per Firefox session r=mhowell!,flod!
(deleted), text/x-phabricator-request
Details
Bug 353804 - Add update swap handling to UpdateListener and AppUpdater r=mhowell!
(deleted), text/x-phabricator-request
Details
Bug 353804 - Add telemetry for states introduced by allowing multiple updates per session r=mhowell!,chutten!
(deleted), text/x-phabricator-request
Details
Bug 353804 - Fix test broken by enabling multiple update downloads per session r=mhowell!
(deleted), text/x-phabricator-request
Details
Bug 353804 - Fix a function that expects AUS.downloadUpdate to return a state r=mhowell!
(deleted), text/x-phabricator-request
Details
Bug 353804 - Add tests of the AUS changes to allow multiple downloads per Firefox session r=mhowell!
(deleted), text/x-phabricator-request
Details
Bug 353804 - Add UI tests for the multiple update downloads per session functionality r=mhowell!
(deleted), text/x-phabricator-request
Details
multiupdate_data_review.md
(deleted), text/plain
chutten
: data-review+
Details
Bug 353804 - Fix intermittent failure in added tests r=agashlin
(deleted), text/x-phabricator-request
Details
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.
Depends on: 350920
No longer depends on: 350920
Depends on: 407508
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
Blocks: 394021
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
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
Attached patch patch in progress (obsolete) (deleted) — — 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?
(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) (deleted) — — Splinter Review
Attachment #369232 - Attachment is obsolete: true
Attached patch little better (obsolete) (deleted) — — Splinter Review
Attachment #369463 - Attachment is obsolete: true
Attached patch almost there (obsolete) (deleted) — — 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
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.
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.
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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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) (deleted) — — 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 v7 (obsolete) (deleted) — — Splinter 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.
(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.
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
(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.
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).
Whiteboard: [fxgrowth]
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 )
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)
status-firefox60: --- → verified disabled
status-firefox61: --- → wontfix
status-firefox62: --- → affected
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?

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)
Priority: P2 → P3

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)
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
Assignee: nalexander → ksteuber

:rachel, would it be possible to make this document publicly accessible?

(In reply to Dirkjan Ochtman (:djc) from comment #116)

:rachel, would it be possible to make this document publicly accessible?

yes, but give us some time to hash a few things out internally first. If it's not public in a week or two, please feel free to ni? me and I'll follow up.

Attachment #543082 - Attachment is obsolete: true
Depends on: 1666319

I've made a publicly-viewable version of the document that Rachel linked to earlier. I don't expect any more changes to the original document, but if any are made, I'll make sure to keep this version synchronized with it.

https://docs.google.com/document/d/15RBrDjIthmuQwr2II2BBzYKXhG0TMrWQM2jL1WNco5Y/edit?usp=sharing

Depends on: 1668096
Depends on: 1672267

UpdateListener and AppUpdater need to know when a downloading update is transitioning to being a ready update so they don't prompt the user to restart Firefox while updates are still staging (which would result in them not being installed).

Depends on D95820

Attachment #9185637 - Attachment description: Bug 353804 - Enable downloading multiple updates per Firefox session r=mhowell → Bug 353804 - Enable downloading multiple updates per Firefox session r=mhowell!
Attachment #9185638 - Attachment description: Bug 353804 - String changes needed for multiple updates per Firefox session r=mhowell,flod → Bug 353804 - String changes needed for multiple updates per Firefox session r=mhowell!,flod!
Attachment #9185639 - Attachment description: Bug 353804 - Add update swap handling to UpdateListener and AppUpdater r=mhowell → Bug 353804 - Add update swap handling to UpdateListener and AppUpdater r=mhowell!

In Bug 1672267, AUS.downloadUpdate was changed to return a success boolean, but it looks like I missed a function that is still expecting it to return a state.

Depends on D98536

Attached file multiupdate_data_review.md (deleted) —
Attachment #9190914 - Flags: data-review?(chutten)

Comment on attachment 9190914 [details]
multiupdate_data_review.md

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, :bytesized is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

Result: datareview+

Attachment #9190914 - Flags: data-review?(chutten) → data-review+

It looks like some of my tests end up opening the doorhanger more quickly than this test framework expects. We can solve this problem by checking to see if the doorhanger is already open before we wait for it to open.

Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7f97797ad25a Enable downloading multiple updates per Firefox session r=mhowell https://hg.mozilla.org/integration/autoland/rev/6829658e6744 String changes needed for multiple updates per Firefox session r=mhowell,flod https://hg.mozilla.org/integration/autoland/rev/2a7e6d0f28c7 Add update swap handling to UpdateListener and AppUpdater r=mhowell https://hg.mozilla.org/integration/autoland/rev/419c64b004ae Add telemetry for states introduced by allowing multiple updates per session r=mhowell,chutten https://hg.mozilla.org/integration/autoland/rev/e1cfdae21a1e Fix test broken by enabling multiple update downloads per session r=mhowell https://hg.mozilla.org/integration/autoland/rev/469a1a2cedd0 Fix a function that expects AUS.downloadUpdate to return a state r=mhowell https://hg.mozilla.org/integration/autoland/rev/2ceebfe59f1a Add tests of the AUS changes to allow multiple downloads per Firefox session r=mhowell https://hg.mozilla.org/integration/autoland/rev/39c59ddcc24b Add UI tests for the multiple update downloads per session functionality r=mhowell https://hg.mozilla.org/integration/autoland/rev/4e0dc2aaaaef Fix intermittent failure in added tests r=agashlin
Summary: Automatic update does not check for newer versions if a previous update is still in progress or staged → Multiple update downloads: automatic update does not check for newer versions if a previous update is still in progress or staged
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: