In bug 1309125, bug 1309668, and bug 1309660 we changed update mar download throttling to 0 which disables throttling and added support for remotely changing the throttling in case we found that we needed it as a "just in case". This was fixed in Firefox 50 so we've had a couple of releases where the download was not throttled. oremj has ok'd completely removing the throttling over email and the networking team has asked to move away from nsIIncrementalDownload. This is not critical but we should change app update to use the standard networking download code instead of nsIIncrementalDownload.

Matt, I split out your changes to remove nsIIncrementalDownload from bug 1343671 and put it in this bug so this can download while bug 1343671 is in progress and to make the patches simpler in bug 1343671.

Some of the client changes snuck into the test patch

Comment on attachment 8884594 [details] [diff] [review] patch xpc.msg part 2 I'm fine with this change but I'm not certain who can review this. I'll try Ehsan

Comment on attachment 8884594 [details] [diff] [review] patch xpc.msg part 2 Ehsan isn't accepting requests at this time. Could you ask on irc on Monday if someone else will need to r+ this besides myself?

Comment on attachment 8884603 [details] [diff] [review] patch client code part 1 ># HG changeset patch ># User Robert Strong <robert.bugzilla@gmail.com> ># Parent 79718ae1436da9f705b80c34606f572725423095 > >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 >+class CommonDownloader { <snip> >+ _verifyDownload(destination) { > LOG("Downloader:_verifyDownload called"); Change to CommonDownloader:_verifyDownload Also change other cases in CommonDownloader @@ -187,16 +187,18 @@ const APPID_TO_TOPIC = { // Instantbird "{33cb9019-c295-46dd-be21-8c4936574bee}": "xul-window-visible", }; var gUpdateMutexHandle = null; XPCOMUtils.defineLazyModuleGetter(this, "UpdateUtils", "resource://gre/modules/UpdateUtils.jsm"); +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil", + "resource://gre/modules/NetUtil.jsm"); It would be good to verify that this is the best method to use to accomplish this if you haven't already. >@@ -3221,31 +3204,114 @@ Downloader.prototype = { <snip> >+ /** >+ * Adds a listener to the download process >+ * @param listener >+ * A download listener, implementing nsIRequestObserver and >+ * nsIProgressEventSink >+ */ >+ addDownloadListener(listener) { >+ for (var i = 0; i < this._listeners.length; ++i) { nit: go ahead and change this to let while you are here >+ if (this._listeners[i] == listener) >+ return; nit: go ahead and add braces while you are here >+ } >+ this._listeners.push(listener); >+ } >+ >+ /** >+ * Removes a download listener >+ * @param listener >+ * The listener to remove. >+ */ >+ removeDownloadListener(listener) { >+ for (var i = 0; i < this._listeners.length; ++i) { nit: go ahead and change this to let while you are here >+ if (this._listeners[i] == listener) { >+ this._listeners.splice(i, 1); >+ return; >+ } >+ } >+ } >+} >+ >+/** >+ * Update downloader which uses an nsHttpChannel >+ */ >+class ChannelDownloader extends CommonDownloader { >+ constructor(background, updateService) { >+ LOG("Creating ChannelDownloader"); >+ >+ super(background, updateService); >+ >+ /** >+ * The channel object handling the download >+ */ >+ this._channel = null; >+ >+ /** >+ * Destination file output stream >+ */ >+ this._outStream = null; >+ >+ this.QueryInterface = XPCOMUtils.generateQI([Ci.nsIStreamListener, >+ Ci.nsIChannelEventSink, >+ Ci.nsIProgressEventSink, >+ Ci.nsIRequestObserver, >+ Ci.nsIInterfaceRequestor]); >+ } >+ >+ /** >+ * Verify the downloaded file. We assume that the download is complete at >+ * this point. >+ */ >+ _verifyDownload() { >+ if (!this._channel) { >+ AUSTLMY.pingDownloadCode(this.isCompleteUpdate, >+ AUSTLMY.DWNLD_ERR_VERIFY_NO_REQUEST); >+ return false; >+ } >+ let patchFile = getUpdatesDir().clone(); >+ patchFile.append(FILE_UPDATE_MAR); >+ return super._verifyDownload(patchFile); >+ } >+ >+ /** >+ * Cancels the active download. >+ */ >+ cancel(cancelError) { >+ LOG("Downloader: cancel"); Change to "ChannelDownloader:cancel" >+ if (cancelError === undefined) { >+ cancelError = Cr.NS_BINDING_ABORTED; >+ } >+ if (this._channel) { >+ this._channel.cancel(cancelError); >+ } >+ } > > /** > * Whether or not we are currently downloading something. > */ > get isBusy() { >- return this._request != null; >- }, >+ return this._channel != null; >+ } > > /** > * Download and stage the given update. > * @param update > * A nsIUpdate object to download a patch for. Cannot be null. > */ >- downloadUpdate: function Downloader_downloadUpdate(update) { >+ downloadUpdate(update) { > LOG("UpdateService:_downloadUpdate"); Change to "ChannelDownloader:downloadUpdate" > if (!update) { > AUSTLMY.pingDownloadCode(undefined, AUSTLMY.DWNLD_ERR_NO_UPDATE); > throw Cr.NS_ERROR_NULL_POINTER; > } > > var updateDir = getUpdatesDir(); > >@@ -3258,108 +3324,96 @@ Downloader.prototype = { > LOG("Downloader:downloadUpdate - no patch to download"); Change to ChannelDownloader:downloadUpdate > AUSTLMY.pingDownloadCode(undefined, AUSTLMY.DWNLD_ERR_NO_UPDATE_PATCH); > return readStatusFile(updateDir); > } > this.isCompleteUpdate = this._patch.type == "complete"; > > let patchFile = getUpdatesDir().clone(); > patchFile.append(FILE_UPDATE_MAR); >- update.QueryInterface(Ci.nsIPropertyBag); >- let interval = this.background ? update.getProperty("backgroundInterval") >- : DOWNLOAD_FOREGROUND_INTERVAL; > > LOG("Downloader:downloadUpdate - url: " + this._patch.URL + ", path: " + Change to ChannelDownloader:downloadUpdate >- patchFile.path + ", interval: " + interval); >+ patchFile.path); > var uri = Services.io.newURI(this._patch.URL); > >- this._request = Cc["@mozilla.org/network/incremental-download;1"]. >- createInstance(Ci.nsIIncrementalDownload); >- this._request.init(uri, patchFile, DOWNLOAD_CHUNK_SIZE, interval); >- this._request.start(this, null); >+ this._outStream = FileUtils.openFileOutputStream(patchFile); I suspect that this will get flagged as a perf problem. See bug 1361102. Either you or I can ask around how to best accomplish this while limiting jank. I would hope the download manager already has this figured out. >+ >+ this._channel = NetUtil.newChannel({uri, loadUsingSystemPrincipal: true}); >+ this._channel.notificationCallbacks = this; >+ this._channel.asyncOpen2(this.QueryInterface(Ci.nsIStreamListener)); > > writeStatusFile(updateDir, STATE_DOWNLOADING); > this._patch.QueryInterface(Ci.nsIWritablePropertyBag); Note: I think I have a patch in my queue that removes this since it shouldn't be needed. I'll clean it up in a different bug. > this._patch.state = STATE_DOWNLOADING; > var um = Cc["@mozilla.org/updates/update-manager;1"]. > getService(Ci.nsIUpdateManager); > um.saveUpdates(); > return STATE_DOWNLOADING; >- }, >- >- /** >- * An array of download listeners to notify when we receive >- * nsIRequestObserver or nsIProgressEventSink method calls. >- */ >- _listeners: [], >+ } >+ Please add a javadoc comment of /** * See nsIChannelEventSink.idl */ >+ asyncOnChannelRedirect(oldChannel, newChannel, flags, callback) { >+ LOG("Downloader: redirected from " + oldChannel.URI + " to " + newChannel.URI); Change to "ChannelDownloader:asyncOnChannelRedirect - redirected from " + oldChannel.URI + " to " + newChannel.URI >+ this._patch.finalURL = newChannel.URI; >+ callback.onRedirectVerifyCallback(Cr.NS_OK); >+ this._channel = newChannel; >+ } > > /** >- * Adds a listener to the download process >- * @param listener >- * A download listener, implementing nsIRequestObserver and >- * nsIProgressEventSink >+ * When we have new status text >+ * @param request >+ * The nsIRequest object for the transfer >+ * @param context >+ * Additional data >+ * @param status >+ * A status code >+ * @param statusText >+ * Human readable version of |status| > */ nit: go ahead and change this to /** * See nsIProgressEventSink.idl */ >- addDownloadListener: function Downloader_addDownloadListener(listener) { >- for (var i = 0; i < this._listeners.length; ++i) { >- if (this._listeners[i] == listener) >- return; >- } >- this._listeners.push(listener); >- }, >- >- /** >- * Removes a download listener >- * @param listener >- * The listener to remove. >- */ >- removeDownloadListener: function Downloader_removeDownloadListener(listener) { >- for (var i = 0; i < this._listeners.length; ++i) { >- if (this._listeners[i] == listener) { >- this._listeners.splice(i, 1); >- return; >+ onStatus(request, context, status, statusText) { >+ LOG("Downloader:onStatus - status: " + status + ", statusText: " + Change to ChannelDownloader:onStatus >+ statusText); >+ >+ for (let listener of this._listeners) { >+ if (listener instanceof Ci.nsIProgressEventSink) { >+ listener.onStatus(request, context, status, statusText); > } > } >- }, >+ } > > /** > * When the async request begins > * @param request > * The nsIRequest object for the transfer > * @param context > * Additional data > */ nit: go ahead and change this to /** * See nsIRequestObserver.idl */ >- onStartRequest: function Downloader_onStartRequest(request, context) { >- if (request instanceof Ci.nsIIncrementalDownload) >- LOG("Downloader:onStartRequest - original URI spec: " + request.URI.spec + >- ", final URI spec: " + request.finalURI.spec); >- // Always set finalURL in onStartRequest since it can change. >- this._patch.finalURL = request.finalURI.spec; >+ onStartRequest(request, context) { >+ LOG("Downloader:onStartRequest"); Change to "ChannelDownloader:onStartRequest" > var um = Cc["@mozilla.org/updates/update-manager;1"]. > getService(Ci.nsIUpdateManager); > um.saveUpdates(); > > var listeners = this._listeners.concat(); > var listenerCount = listeners.length; > for (var i = 0; i < listenerCount; ++i) > listeners[i].onStartRequest(request, context); >- }, >+ } > > /** > * When new data has been downloaded > * @param request > * The nsIRequest object for the transfer > * @param context > * Additional data > * @param progress > * The current number of bytes transferred > * @param maxProgress > * The total number of bytes that must be transferred > */ nit: go ahead and change this to /** * See nsIProgressEventSink.idl */ >- onProgress: function Downloader_onProgress(request, context, progress, >- maxProgress) { >+ onProgress(request, context, progress, maxProgress) { > LOG("Downloader:onProgress - progress: " + progress + "/" + maxProgress); Change to ChannelDownloader:onProgress > > if (progress > this._patch.size) { > LOG("Downloader:onProgress - progress: " + progress + Change to ChannelDownloader:onProgress > " is higher than patch size: " + this._patch.size); > AUSTLMY.pingDownloadCode(this.isCompleteUpdate, > AUSTLMY.DWNLD_ERR_PATCH_SIZE_LARGER); > this.cancel(Cr.NS_ERROR_UNEXPECTED); >@@ -3378,56 +3432,48 @@ Downloader.prototype = { > var listeners = this._listeners.concat(); > var listenerCount = listeners.length; > for (var i = 0; i < listenerCount; ++i) { > var listener = listeners[i]; > if (listener instanceof Ci.nsIProgressEventSink) > listener.onProgress(request, context, progress, maxProgress); > } > this.updateService._consecutiveSocketErrors = 0; >- }, >+ } > > /** >- * When we have new status text >+ * When the HTTP stream has new data > * @param request > * The nsIRequest object for the transfer > * @param context > * Additional data >- * @param status >- * A status code >- * @param statusText >- * Human readable version of |status| >+ * @param stream >+ * Input stream containing the new data >+ * @param offset >+ * Number of bytes downloaded prior to this chunk of data >+ * @param count >+ * Size of this chunk of data > */ nit: go ahead and change this to /** * See nsIStreamListener.idl */ >- onStatus: function Downloader_onStatus(request, context, status, statusText) { >- LOG("Downloader:onStatus - status: " + status + ", statusText: " + >- statusText); >- >- var listeners = this._listeners.concat(); >- var listenerCount = listeners.length; >- for (var i = 0; i < listenerCount; ++i) { >- var listener = listeners[i]; >- if (listener instanceof Ci.nsIProgressEventSink) >- listener.onStatus(request, context, status, statusText); >- } >- }, >+ onDataAvailable(request, context, stream, offset, count) { >+ let sis = Cc["@mozilla.org/scriptableinputstream;1"]. >+ createInstance(Ci.nsIScriptableInputStream); >+ sis.init(stream); >+ return this._outStream.write(sis.readBytes(count), count); >+ } > > /** > * When data transfer ceases > * @param request > * The nsIRequest object for the transfer > * @param context > * Additional data > * @param status > * Status code containing the reason for the cessation. > */ nit: go ahead and change this to /** * See nsIRequestObserver.idl */ >- onStopRequest: function Downloader_onStopRequest(request, context, status) { >- if (request instanceof Ci.nsIIncrementalDownload) >- LOG("Downloader:onStopRequest - original URI spec: " + request.URI.spec + >- ", final URI spec: " + request.finalURI.spec + ", status: " + status); >- >+ onStopRequest(request, context, status) { > // XXX ehsan shouldShowPrompt should always be false here. > // But what happens when there is already a UI showing? > var state = this._patch.state; > var shouldShowPrompt = false; > var shouldRegisterOnlineObserver = false; > var shouldRetrySoon = false; > var deleteActiveUpdate = false; > var retryTimeout = getPref("getIntPref", PREF_APP_UPDATE_SOCKET_RETRYTIMEOUT, >@@ -3550,17 +3596,19 @@ Downloader.prototype = { > if (!shouldRetrySoon && !shouldRegisterOnlineObserver) { > var listeners = this._listeners.concat(); > var listenerCount = listeners.length; > for (var i = 0; i < listenerCount; ++i) { > listeners[i].onStopRequest(request, context, status); > } > } > >- this._request = null; >+ this._channel = null; >+ this._outStream.close(); >+ this._outStream = null; > > if (state == STATE_DOWNLOAD_FAILED) { > var allFailed = true; > // Check if there is a complete update patch that can be downloaded. > if (!this._update.isCompleteUpdate && this._update.patchCount == 2) { > LOG("Downloader:onStopRequest - verification of patch failed, " + Change to ChannelDownloader:onStopRequest > "downloading complete update patch"); If there are any other LOG messages that should be changed that I missed please change those as well.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #9) > >- patchFile.path + ", interval: " + interval); > >+ patchFile.path); > > var uri = Services.io.newURI(this._patch.URL); > > > >- this._request = Cc["@mozilla.org/network/incremental-download;1"]. > >- createInstance(Ci.nsIIncrementalDownload); > >- this._request.init(uri, patchFile, DOWNLOAD_CHUNK_SIZE, interval); > >- this._request.start(this, null); > >+ this._outStream = FileUtils.openFileOutputStream(patchFile); > I suspect that this will get flagged as a perf problem. See bug 1361102. > Either you or I can ask around how to best accomplish this while limiting > jank. I would hope the download manager already has this figured out. I looked into what the download manager does, and it's handling file output on a separate thread by forwarding it through a purpose-built necko thing called a BackgroundFileSaver. I'll try adapting that method here.

Carrying over r+ from previous revision because this is just a rebase.

Above try push shows one of the chrome tests timing out on Linux and Mac. I'll investigate. It also shows this failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37717753a85ebaf9724c9df1cbf6241caf26b156&selectedJob=131137183 which does not look related to this bug, but should get looked into anyway. And there's an internal error in eslint there too, just for fun. Not sure what to do about that.

(In reply to Matt Howell [:mhowell] from comment #15) <snip> > And there's an internal error in eslint there too, just for fun. Not sure > what to do about that. I think you can just do what DownloadCore.jsm does as follows const BackgroundFileSaverStreamListener = Components.Constructor( "@mozilla.org/network/background-file-saver;1?mode=streamlistener", "nsIBackgroundFileSaver"); ... this._bkgFileSaver = new BackgroundFileSaverStreamListener();

(In reply to Matt Howell [:mhowell] from comment #15) ... > It also shows this failure: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=37717753a85ebaf9724c9df1cbf6241caf26b156&selectedJob=1 > 31137183 > which does not look related to this bug, but should get looked into anyway. I triggered 5 more runs and I suspect that is a pre-existing intermittent. I'll trigger a few more.

One of the things that is hard to catch are tests that start failing when they run parallel with other tests but typically pass during when they are automatically ran again by themselves. I have a bash script that does this and pipes the results to a file so I can check if a change has increased the parallel failure rate. It looks like these changes significantly increased the unit_aus_update parallel failures and I also had some failures when they ran again by themselves. The tests that failed the most often were downloadInterruptedRecoveryChannel.js failed 15 times parallel out 50 runs and also failed non parallel 2 out of 50 runs downloadInterruptedOffline.js failed 7 times parallel out 50 runs These can be extremely hard to diagnose since they don't have log output when they run in parallel. To deal with this I usually change the LOG function in nsUpdateService.js to also output to a file or override dump in xpcshellUtilsAUS.js so it outputs to a file as well as outputs to the original dump.

Main changes: - Applied the workaround from comment 16 to fix the eslint internal error - Wait for the BackgroundFileSaver to notify us that it's done writing instead of just assuming the file will be ready immediately after calling finish - Updated the cancel method, since I forgot to do that when I added the BackgroundFileSaver Robert, if this version doesn't do any better with your parallel runs, can you send me that bash script you're using?

The bash script is dirt simple and nothing special. It just makes it easy to run a bunch of tests without me having to babysit it. #!/bin/sh rm ../../_out END=50 if [ $# -eq 1 ]; then END=$1 fi for ((i=1;i<=END;i++)); do echo $i mach test toolkit/mozapps/update/tests/unit_aus_update/ >> ../../_out done

Test runs are looking good! The only one possibly related was one parallel failure out of 50 runs for downloadResumeForSameAppVersion.js which passed when it ran by itself.

I landed the latest patches on oak and got a failure https://queue.taskcluster.net/v1/task/JSNIyugRQC-9PUsDuYUn1Q/runs/0/artifacts/public/logs/live_backing.log

Looks like a failure due to this patch.

Comment on attachment 8910010 [details] [diff] [review] Part 1 - Client code ># HG changeset patch ># User Matt Howell <mhowell@mozilla.com> ># Date 1504904333 25200 ># Fri Sep 08 13:58:53 2017 -0700 ># Node ID 37880407f652fec351dd2a1999192535a2369dbe ># Parent ba69e294fc80f23d54fc68050f1b7d63d341aa9c >Bug 1348087 Part 1 - Stop using nsIIncrementalDownload for application update. r?rstrong > >MozReview-Commit-ID: DNANBCjrsyM > >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 <snip> >@@ -3379,229 +3364,304 @@ Downloader.prototype = { > > // Reset the Active Update object on the Update Manager and flush the > // Active Update DB. > var um = Cc["@mozilla.org/updates/update-manager;1"]. > getService(Ci.nsIUpdateManager); > um.activeUpdate = update; > > return selectedPatch; >- }, >+ } >+ >+ /** >+ * Adds a listener to the download process >+ * @param listener >+ * A download listener, implementing nsIRequestObserver and >+ * nsIProgressEventSink >+ */ >+ addDownloadListener(listener) { >+ for (let i = 0; i < this._listeners.length; ++i) { >+ if (this._listeners[i] == listener) { >+ return; >+ } >+ } >+ this._listeners.push(listener); >+ } >+ >+ /** >+ * Removes a download listener >+ * @param listener >+ * The listener to remove. >+ */ >+ removeDownloadListener(listener) { >+ for (let i = 0; i < this._listeners.length; ++i) { >+ if (this._listeners[i] == listener) { >+ this._listeners.splice(i, 1); >+ return; >+ } >+ } >+ } >+} >+ >+/** >+ * Update downloader which uses an nsHttpChannel >+ */ >+class ChannelDownloader extends CommonDownloader { >+ constructor(background, updateService) { >+ LOG("Creating ChannelDownloader"); >+ >+ super(background, updateService); >+ >+ /** >+ * Background file saver, for writing the downloaded file on another thread >+ */ >+ this._bkgFileSaver = null; >+ >+ /** >+ * The channel object handling the download >+ */ >+ this._channel = null; >+ >+ this.QueryInterface = XPCOMUtils.generateQI([Ci.nsIStreamListener, >+ Ci.nsIChannelEventSink, >+ Ci.nsIProgressEventSink, >+ Ci.nsIRequestObserver, >+ Ci.nsIInterfaceRequestor]); >+ } >+ >+ /** >+ * Verify the downloaded file. We assume that the download is complete at >+ * this point. >+ */ >+ _verifyDownload() { >+ if (!this._channel) { >+ AUSTLMY.pingDownloadCode(this.isCompleteUpdate, >+ AUSTLMY.DWNLD_ERR_VERIFY_NO_REQUEST); >+ return false; >+ } >+ let patchFile = getUpdatesDir().clone(); >+ patchFile.append(FILE_UPDATE_MAR); >+ return super._verifyDownload(patchFile); >+ } >+ >+ /** >+ * Cancels the active download. >+ */ >+ cancel(cancelError) { >+ LOG("ChannelDownloader: cancel"); >+ if (cancelError === undefined) { >+ cancelError = Cr.NS_BINDING_ABORTED; >+ } >+ if (this._bkgFileSaver) { >+ this._bkgFileSaver.finish(cancelError); >+ this._bkgFileSaver.observer = null; >+ this._bkgFileSaver = null; >+ } >+ if (this._channel) { >+ this._channel.cancel(cancelError); >+ this._channel = null; >+ } >+ let patchFile = getUpdatesDir().clone(); >+ patchFile.append(FILE_UPDATE_MAR); >+ try { >+ patchFile.remove(false); >+ } catch (e) { } Cancel didn't remove the file previously. This makes it so if a client exits while a download is happening the download starts over. I also noticed that the download progress updates is way too fast especially when compared to how it is without this patch.

Not sure what will need to be done to make it continue the download when starting again but I am sure it is possible one way or another.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #24) > Created attachment 8912059 [details] > mochitest-chrome log for failure seen with this patch > > Looks like a failure due to this patch. It is. I was seeing this failure locally until I made the changes to cancel(), so I thought it was gone. Having resume support will change how that works, so this might look different after that. (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #25) > I also noticed that the download progress updates is way too fast especially > when compared to how it is without this patch. Not certain what you mean here. This is in the about box? And it updates too rapidly to be able to read? (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #26) > Not sure what will need to be done to make it continue the download when > starting again but I am sure it is possible one way or another. I'll give it a shot. Looking at the download manager makes me think it's definitely possible and it _shouldn't_ be too hard...

> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #25) > > I also noticed that the download progress updates is way too fast especially > > when compared to how it is without this patch. > > Not certain what you mean here. This is in the about box? And it updates too > rapidly to be able to read? In the about dialog the download progress updates extremely fast. You can try it out on oak to see it.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #28) > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #25) > > > I also noticed that the download progress updates is way too fast especially > > > when compared to how it is without this patch. > > > > Not certain what you mean here. This is in the about box? And it updates too > > rapidly to be able to read? > In the about dialog the download progress updates extremely fast. You can > try it out on oak to see it. Okay, I understand. The incremental downloader gets around this by making the OnProgress calls itself instead of passing the channel's through, and throttling those down on a half-second interval. I'll see about replicating that.

Might want to check out how the download panel does it and go that route.

Throttled OnStatus to only call listeners every half second, and implemented resuming downloads; I've checked in the test logs to make sure that code is functioning, since otherwise it's kind of silent. Since I'm tired of the Linux chrome tests giving me trouble, I left them running in a loop overnight (that's 1,920 runs) and with this patch I have no failures; before I could get about 1 failure in 10 runs. Will also push to try in a bit.

(In reply to Matt Howell [:mhowell] from comment #31) > Throttled OnStatus to only call listeners every half second That should be OnProgress.

Comment on attachment 8913248 [details] [diff] [review] Part 1 - Client code This is why it is a good thing to manually verify the changes AUS:SVC CommonDownloader:_selectPatch - found existing patch with state: downloading AUS:SVC CommonDownloader:_selectPatch - resuming download AUS:SVC ChannelDownloader:downloadUpdate - url: https://archive.mozilla.org/pub/firefox/nightly/2017/09/2017-09-29-11-04-56-oak/firefox-58.0a1.en-US.win64.complete.mar, path: C:\Users\<username>\AppData\Local\Mozilla\updates\DA68BA2C2A44DE0B\updates\0\update.mar AUS:SVC ChannelDownloader:downloadUpdate - resuming previous download at 13596107 bytes AUS:SVC ChannelDownloader:finishDownload - status: 2152398850, current fail: 0, max fail: 10, retryTimeout: 2000 AUS:SVC ChannelDownloader:finishDownload - setting state to: downloading AUS:SVC ChannelDownloader:onStatus - status: 2152398860, statusText: archive.mozilla.org AUS:SVC ChannelDownloader:onStatus - status: 2152398861, statusText: archive.mozilla.org AUS:SVC ChannelDownloader:onStatus - status: 2152398858, statusText: archive.mozilla.org AUS:SVC ChannelDownloader:onStartRequest AUS:SVC ChannelDownloader:onStatus - status: 2152398854, statusText: archive.mozilla.org AUS:SVC ChannelDownloader:onProgress - progress: 17376/30178929 AUS:SVC ChannelDownloader:onProgress - maxProgress: 30178929 is not equal to expected patch size: 43775036 AUS:SVC ChannelDownloader: cancel AUS:SVC ChannelDownloader:finishDownload - status: 2147549183, current fail: 0, max fail: 10, retryTimeout: 2000 AUS:SVC ChannelDownloader:finishDownload - non-verification failure AUS:SVC getStatusTextFromCode - transfer error: Failed (unknown reason), default code: 2152398849 AUS:SVC ChannelDownloader:finishDownload - setting state to: download-failed AUS:SVC UpdateManager:_loadXMLFileIntoArray - XML file does not exist. path: C:\Users\<username>\AppData\Local\Mozilla\updates\DA68BA2C2A44DE0B\updates.xml AUS:SVC ChannelDownloader:finishDownload - notifying observers of error. topic: update-error, status: download-attempt-failed So, the new code expects the maxProgress to be from where it left off and hence resuming the download fails. Also, statusText is now just the hostname which doesn't seem right. Please also add a test that copies a portion of the complete mar to the test's update.mar and then resumes downloading the mar file. It can be based off of downloadResumeForSameAppVersion.js.

Fixed the expected file size calculation, and added correct message formatting in onStatus. Will request review if try push is good.

Added downloadResumeAfterRestart.js

I didn't push the above patches to oak, but I did run a test update from my local build, which downloaded (and applied) successfully. Here's the AUS log.

Comment on attachment 8915354 [details] [diff] [review] Part 1 - Client code Try push does look good.

Comment on attachment 8915354 [details] [diff] [review] Part 1 - Client code >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 >@@ -3379,229 +3368,347 @@ Downloader.prototype = { <snip> >- downloadUpdate: function Downloader_downloadUpdate(update) { >- LOG("UpdateService:_downloadUpdate"); >+ downloadUpdate(update) { >+ LOG("ChannelDownloader:downloadUpdate"); > if (!update) { > AUSTLMY.pingDownloadCode(undefined, AUSTLMY.DWNLD_ERR_NO_UPDATE); > throw Cr.NS_ERROR_NULL_POINTER; > } > >- var updateDir = getUpdatesDir(); >+ let updateDir = getUpdatesDir(); > > this._update = update; > > // This function may return null, which indicates that there are no patches > // to download. > this._patch = this._selectPatch(update, updateDir); > if (!this._patch) { >- LOG("Downloader:downloadUpdate - no patch to download"); >+ LOG("ChannelDownloader:downloadUpdate - no patch to download"); > AUSTLMY.pingDownloadCode(undefined, AUSTLMY.DWNLD_ERR_NO_UPDATE_PATCH); > return readStatusFile(updateDir); > } > this.isCompleteUpdate = this._patch.type == "complete"; >+ this._patch.QueryInterface(Ci.nsIWritablePropertyBag); > > let patchFile = getUpdatesDir().clone(); > patchFile.append(FILE_UPDATE_MAR); >- update.QueryInterface(Ci.nsIPropertyBag); >- let interval = this.background ? update.getProperty("backgroundInterval") >- : DOWNLOAD_FOREGROUND_INTERVAL; Intervals are no longer used after nsIIncrementalDownload is removed so the following should also be done: Remove the constants for DOWNLOAD_BACKGROUND_INTERVAL, DOWNLOAD_FOREGROUND_INTERVAL, and PREF_APP_UPDATE_BACKGROUNDINTERVAL. Remove the nsIUpdate backgroundInterval property. Remove the app.update.download.backgroundInterval preference from the Firefox preference file. Please file a releng bug to remove backgroundInterval support from balrog as well. >- >- LOG("Downloader:downloadUpdate - url: " + this._patch.URL + ", path: " + >- patchFile.path + ", interval: " + interval); >- var uri = Services.io.newURI(this._patch.URL); >- >- this._request = Cc["@mozilla.org/network/incremental-download;1"]. >- createInstance(Ci.nsIIncrementalDownload); >- this._request.init(uri, patchFile, DOWNLOAD_CHUNK_SIZE, interval); >- this._request.start(this, null); >+ >+ LOG("ChannelDownloader:downloadUpdate - url: " + this._patch.URL + >+ ", path: " + patchFile.path); >+ let uri = Services.io.newURI(this._patch.URL); >+ >+ let BackgroundFileSaver = Components.Constructor( >+ "@mozilla.org/network/background-file-saver;1?mode=streamlistener", >+ "nsIBackgroundFileSaver"); >+ this._bkgFileSaver = new BackgroundFileSaver(); >+ this._bkgFileSaver.QueryInterface(Ci.nsIStreamListener); >+ >+ this._channel = NetUtil.newChannel({uri, loadUsingSystemPrincipal: true}); >+ this._channel.notificationCallbacks = this; >+ this._channel.asyncOpen2(this.QueryInterface(Ci.nsIStreamListener)); >+ >+ if (this._channel instanceof Ci.nsIResumableChannel && >+ patchFile.exists()) { >+ try { >+ let entityID = this._patch.getProperty("entityID"); I'm tempted to say that this should be added to nsIUpdatePatch instead of it being a one-off property though I am very much on the fence. If this is what you are expecting to throw you could go with if default value then remove the file, etc. >+ let resumeFrom = patchFile.fileSize; >+ this._channel.resumeAt(resumeFrom, entityID); >+ this._bkgFileSaver.enableAppend(); >+ this._resumedFrom = resumeFrom; >+ LOG("ChannelDownloader:downloadUpdate - resuming previous download at " + >+ resumeFrom + " bytes"); Lots of calls within a try block tend to concern me. Could this be lessened or could you document this more thoroughly? >+ } catch (ex) { Since this isn't in another try block just use catch (e) >+ // This is expected if this is the first attempt to download this patch, >+ // so just log it and move on from the beginning of the file. It isn't expected if the file already exists though. Would be good to document how that state could happen. Since the patch file has to exist to get this far what do you think about removing the patch file since it has to start downloading from the beginning? This case should also be recorded in telemetry. >+ LOG("ChannelDownloader:downloadUpdate - couldn't resume download, " + >+ "caught exception: " + ex); >+ } >+ } >+ >+ this._bkgFileSaver.setTarget(patchFile, true); > > writeStatusFile(updateDir, STATE_DOWNLOADING); >- this._patch.QueryInterface(Ci.nsIWritablePropertyBag); > this._patch.state = STATE_DOWNLOADING; >- var um = Cc["@mozilla.org/updates/update-manager;1"]. >+ let um = Cc["@mozilla.org/updates/update-manager;1"]. > getService(Ci.nsIUpdateManager); > um.saveUpdates(); > return STATE_DOWNLOADING; >- }, >- >- /** >- * An array of download listeners to notify when we receive >- * nsIRequestObserver or nsIProgressEventSink method calls. >- */ >- _listeners: [], >+ } > > /** >- * Adds a listener to the download process >- * @param listener >- * A download listener, implementing nsIRequestObserver and >- * nsIProgressEventSink >+ * See nsIChannelEventSink.idl > */ >- addDownloadListener: function Downloader_addDownloadListener(listener) { >- for (var i = 0; i < this._listeners.length; ++i) { >- if (this._listeners[i] == listener) >- return; >- } >- this._listeners.push(listener); >- }, >+ asyncOnChannelRedirect(oldChannel, newChannel, flags, callback) { >+ LOG("ChannelDownloader: redirected from " + oldChannel.URI + >+ " to " + newChannel.URI); >+ this._patch.finalURL = newChannel.URI; >+ callback.onRedirectVerifyCallback(Cr.NS_OK); >+ this._channel = newChannel; >+ } > > /** >- * Removes a download listener >- * @param listener >- * The listener to remove. >+ * See nsIProgressEventSink.idl > */ >- removeDownloadListener: function Downloader_removeDownloadListener(listener) { >- for (var i = 0; i < this._listeners.length; ++i) { >- if (this._listeners[i] == listener) { >- this._listeners.splice(i, 1); >- return; >+ onStatus(request, context, status, statusText) { >+ LOG("ChannelDownloader:onStatus - status: " + status + ", message: " + >+ Services.strings.formatStatusMessage(status, statusText)); Even though the cost of calling formatStatusMessage is likely small I don't think it is worthwhile for this. I think it is better to just remove it. >- onStartRequest: function Downloader_onStartRequest(request, context) { >- if (request instanceof Ci.nsIIncrementalDownload) >- LOG("Downloader:onStartRequest - original URI spec: " + request.URI.spec + >- ", final URI spec: " + request.finalURI.spec); >- // Always set finalURL in onStartRequest since it can change. >- this._patch.finalURL = request.finalURI.spec; >+ onStartRequest(request, context) { >+ if (!this._channel || !this._bkgFileSaver) { >+ // Sometimes the channel calls onStartRequest after being canceled. >+ return; >+ } >+ >+ LOG("ChannelDownloader:onStartRequest"); >+ >+ this._bkgFileSaver.onStartRequest(request, context); >+ >+ if (request instanceof Ci.nsIResumableChannel) { >+ this._patch.setProperty("entityID", request.entityID); >+ } >+ > var um = Cc["@mozilla.org/updates/update-manager;1"]. > getService(Ci.nsIUpdateManager); > um.saveUpdates(); > > var listeners = this._listeners.concat(); > var listenerCount = listeners.length; > for (var i = 0; i < listenerCount; ++i) > listeners[i].onStartRequest(request, context); nit: go ahead and add curly braces >- }, >+ } > > /** >- * When new data has been downloaded >- * @param request >- * The nsIRequest object for the transfer >- * @param context >- * Additional data >- * @param progress >- * The current number of bytes transferred >- * @param maxProgress >- * The total number of bytes that must be transferred >+ * See nsIProgressEventSink.idl > */ >- onProgress: function Downloader_onProgress(request, context, progress, >- maxProgress) { >- LOG("Downloader:onProgress - progress: " + progress + "/" + maxProgress); >+ onProgress(request, context, progress, maxProgress) { >+ LOG("ChannelDownloader:onProgress - progress: " + progress + >+ "/" + maxProgress); > > if (progress > this._patch.size) { >- LOG("Downloader:onProgress - progress: " + progress + >+ LOG("ChannelDownloader:onProgress - progress: " + progress + > " is higher than patch size: " + this._patch.size); > AUSTLMY.pingDownloadCode(this.isCompleteUpdate, > AUSTLMY.DWNLD_ERR_PATCH_SIZE_LARGER); > this.cancel(Cr.NS_ERROR_UNEXPECTED); > return; > } > >- if (maxProgress != this._patch.size) { >- LOG("Downloader:onProgress - maxProgress: " + maxProgress + >+ if ((maxProgress + this._resumedFrom) != this._patch.size) { >+ LOG("ChannelDownloader:onProgress - maxProgress: " + >+ (maxProgress + this._resumedFrom) + > " is not equal to expected patch size: " + this._patch.size); > AUSTLMY.pingDownloadCode(this.isCompleteUpdate, > AUSTLMY.DWNLD_ERR_PATCH_SIZE_NOT_EQUAL); > this.cancel(Cr.NS_ERROR_UNEXPECTED); > return; > } > >- var listeners = this._listeners.concat(); >- var listenerCount = listeners.length; >- for (var i = 0; i < listenerCount; ++i) { >- var listener = listeners[i]; >- if (listener instanceof Ci.nsIProgressEventSink) >- listener.onProgress(request, context, progress, maxProgress); >+ let currentTime = Date.now(); >+ if ((currentTime - this._lastProgressTimeMs) > DOWNLOAD_PROGRESS_INTERVAL) { >+ this._lastProgressTimeMs = currentTime; >+ let listeners = this._listeners.concat(); >+ let listenerCount = listeners.length; >+ for (let i = 0; i < listenerCount; ++i) { >+ let listener = listeners[i]; >+ if (listener instanceof Ci.nsIProgressEventSink) { >+ listener.onProgress(request, context, progress, maxProgress); This will make it so on resume the listeners get progress and maxProgress that don't take into account what was previously downloaded. So, if there is a progress bar it could be close to 0 even when more than half of the update has been downloaded. >+ } >+ } > } >+ <snip> >+ * See nsIRequestObserver.idl > */ >- onStopRequest: function Downloader_onStopRequest(request, context, status) { >- if (request instanceof Ci.nsIIncrementalDownload) >- LOG("Downloader:onStopRequest - original URI spec: " + request.URI.spec + >- ", final URI spec: " + request.finalURI.spec + ", status: " + status); >- >+ onStopRequest(request, context, status) { >+ // this._bkgFileSaver may not exist anymore if we've been canceled. >+ if (this._bkgFileSaver) { >+ this._bkgFileSaver.onStopRequest(request, context, status); >+ } >+ if (Components.isSuccessCode(status)) { >+ this._bkgFileSaver.observer = { >+ onTargetChange() { }, >+ onSaveComplete: (aSaver, aStatus) => { >+ this._bkgFileSaver.observer = null; >+ this._finishDownload(request, context, aStatus); >+ } >+ }; >+ this._bkgFileSaver.finish(status); >+ } else { >+ this._finishDownload(request, context, status); >+ } >+ } >+ Please add a javadoc comment for this function >+ _finishDownload(request, context, status) { >@@ -3736,33 +3844,34 @@ Downloader.prototype = { > if (allFailed) { > if (getPref("getBoolPref", PREF_APP_UPDATE_DOORHANGER, false)) { > let downloadAttempts = getPref("getIntPref", PREF_APP_UPDATE_DOWNLOAD_ATTEMPTS, 0); > downloadAttempts++; > Services.prefs.setIntPref(PREF_APP_UPDATE_DOWNLOAD_ATTEMPTS, downloadAttempts); > let maxAttempts = Math.min(getPref("getIntPref", PREF_APP_UPDATE_DOWNLOAD_MAXATTEMPTS, 2), 10); > > if (downloadAttempts > maxAttempts) { >- LOG("Downloader:onStopRequest - notifying observers of error. " + >+ LOG("ChannelDownloader:finishDownload - notifying observers of error. " + > "topic: update-error, status: download-attempts-exceeded, " + > "downloadAttempts: " + downloadAttempts + " " + > "maxAttempts: " + maxAttempts); > Services.obs.notifyObservers(this._update, "update-error", "download-attempts-exceeded"); > } else { > this._update.selectedPatch.selected = false; >- LOG("Downloader:onStopRequest - notifying observers of error. " + >+ LOG("ChannelDownloader:finishDownload - notifying observers of error. " + > "topic: update-error, status: download-attempt-failed"); > Services.obs.notifyObservers(this._update, "update-error", "download-attempt-failed"); > } > } > > // If the update UI is not open (e.g. the user closed the window while > // downloading) and if at any point this was a foreground download > // notify the user about the error. If the update was a background > // update there is no notification since the user won't be expecting it. >+ this._update.QueryInterface(Ci.nsIPropertyBag); I could have sworn this wasn't necessary... please check > if (!Services.wm.getMostRecentWindow(UPDATE_WINDOW_NAME) && > this._update.getProperty("foregroundDownload") == "true") { > let prompter = Cc["@mozilla.org/updates/update-prompt;1"]. > createInstance(Ci.nsIUpdatePrompt); > prompter.showUpdateError(this._update); > } > > // Prevent leaking the update object (bug 454964).

Comment on attachment 8915356 [details] [diff] [review] Part 3 - Test changes >diff --git a/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeAfterRestart.js b/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeAfterRestart.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeAfterRestart.js >@@ -0,0 +1,61 @@ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. >+ */ >+ >+function run_test() { >+ setupTestCommon(); >+ start_httpserver(); >+ >+ debugDump("testing resuming a download on startup that was in progress " + >+ "when the application quit"); >+ >+ // Simulate a download that was partially completed in an earlier session >+ // by writing half of the final MAR into the download path. >+ const marBytesToPreCopy = SIZE_SIMPLE_MAR / 2; The mar file is an even number at this time but that could change. Would it make sense to just go with SIZE_SIMPLE_MAR - 200?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #40) > Remove the nsIUpdate backgroundInterval property. backgroundInterval already isn't in the IDL. > Please file a releng bug to remove backgroundInterval support from balrog as > well. Filed bug 1407392. > >+ let entityID = this._patch.getProperty("entityID"); > I'm tempted to say that this should be added to nsIUpdatePatch instead of it > being a one-off property though I am very much on the fence. If this is what > you are expecting to throw you could go with if default value then remove > the file, etc. I'd rather not add it to the interface, just because I think it's an implementation detail of the downloader and nothing else should need to know about it. > >+ let resumeFrom = patchFile.fileSize; > >+ this._channel.resumeAt(resumeFrom, entityID); > >+ this._bkgFileSaver.enableAppend(); > >+ this._resumedFrom = resumeFrom; > >+ LOG("ChannelDownloader:downloadUpdate - resuming previous download at " + > >+ resumeFrom + " bytes"); > Lots of calls within a try block tend to concern me. Could this be lessened > or could you document this more thoroughly? Only the first two lines might actually throw, I'll move the rest outside the try block. > >+ // This is expected if this is the first attempt to download this patch, > >+ // so just log it and move on from the beginning of the file. > It isn't expected if the file already exists though. Would be good to > document how that state could happen. You're right, I don't think this could happen unless something other than the update service created the file or something really odd and broken happens with the file system; I think I'll just take out the comment. > Since the patch file has to exist to get this far what do you think about > removing the patch file since it has to start downloading from the beginning? > This case should also be recorded in telemetry. I think deleting the patch file here isn't necessary because the BackgroundFileSaver always overwrites the target from the beginning if you don't call enableAppend. I'm not sure how to handle this for telemetry. The obvious thing is to add a new download status code, but I'm not sure how that would work because this is at a sort of intermediate stage that we didn't have before, and it isn't fatal to the download. Do you think that would make sense, or should I add a new histogram (or scalar maybe)? > >+ this._update.QueryInterface(Ci.nsIPropertyBag); > I could have sworn this wasn't necessary... please check Yeah, I put it in because I get failures in some chrome tests otherwise. There might be somewhere earlier that it makes sense to put this, I didn't really investigate that; I kind of like having it right where the interface is used anyway.

(In reply to Matt Howell [:mhowell] from comment #42) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #40) > > Remove the nsIUpdate backgroundInterval property. > > backgroundInterval already isn't in the IDL. https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1336 and elsewhere > > > Please file a releng bug to remove backgroundInterval support from balrog as > > well. > > Filed bug 1407392. > > > > >+ let entityID = this._patch.getProperty("entityID"); > > I'm tempted to say that this should be added to nsIUpdatePatch instead of it > > being a one-off property though I am very much on the fence. If this is what > > you are expecting to throw you could go with if default value then remove > > the file, etc. > > I'd rather not add it to the interface, just because I think it's an > implementation detail of the downloader and nothing else should need to know > about it. I'm ok with that > > > >+ let resumeFrom = patchFile.fileSize; > > >+ this._channel.resumeAt(resumeFrom, entityID); > > >+ this._bkgFileSaver.enableAppend(); > > >+ this._resumedFrom = resumeFrom; > > >+ LOG("ChannelDownloader:downloadUpdate - resuming previous download at " + > > >+ resumeFrom + " bytes"); > > Lots of calls within a try block tend to concern me. Could this be lessened > > or could you document this more thoroughly? > > Only the first two lines might actually throw, I'll move the rest outside > the try block. Please manually verify that does the right thing > > > >+ // This is expected if this is the first attempt to download this patch, > > >+ // so just log it and move on from the beginning of the file. > > It isn't expected if the file already exists though. Would be good to > > document how that state could happen. > > You're right, I don't think this could happen unless something other than > the update service created the file or something really odd and broken > happens with the file system; I think I'll just take out the comment. > > > > Since the patch file has to exist to get this far what do you think about > > removing the patch file since it has to start downloading from the beginning? > > This case should also be recorded in telemetry. > > I think deleting the patch file here isn't necessary because the > BackgroundFileSaver always overwrites the target from the beginning if you > don't call enableAppend. > I'm not sure how to handle this for telemetry. The obvious thing is to add a > new download status code, but I'm not sure how that would work because this > is at a sort of intermediate stage that we didn't have before, and it isn't > fatal to the download. Do you think that would make sense, or should I add a > new histogram (or scalar maybe)? iirc most of the download codes aren't fatal and I think it should be fine to report it as just a new code. I think that several of them are intermediate as well. > > > >+ this._update.QueryInterface(Ci.nsIPropertyBag); > > I could have sworn this wasn't necessary... please check > > Yeah, I put it in because I get failures in some chrome tests otherwise. > There might be somewhere earlier that it makes sense to put this, I didn't > really investigate that; I kind of like having it right where the interface > is used anyway. Good to know and thanks!

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #43) > > > >+ let resumeFrom = patchFile.fileSize; > > > >+ this._channel.resumeAt(resumeFrom, entityID); > > > >+ this._bkgFileSaver.enableAppend(); > > > >+ this._resumedFrom = resumeFrom; > > > >+ LOG("ChannelDownloader:downloadUpdate - resuming previous download at " + > > > >+ resumeFrom + " bytes"); > > > Lots of calls within a try block tend to concern me. Could this be lessened > > > or could you document this more thoroughly? > > > > Only the first two lines might actually throw, I'll move the rest outside > > the try block. > Please manually verify that does the right thing It wasn't doing the right thing. For some reason I thought getProperty would throw when the property wasn't found, but it actually returns null. That means that the entityID was implicitly optional, but I had intended for resuming to be skipped without it, as a way of being certain that we really were resuming the same patch. This was also hiding the fact that the entityID wasn't ever getting read out of the XML in the first place. :( I think I'll also add another test to check that the download proceeds normally without trying to resume when an entityID isn't present.

Not carrying over r+ from previous revision because I've added a new test (downloadResumeErrorNoEntityID.js).

Forgot to hg add the new file.

Don't forget (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8) > Comment on attachment 8884594 [details] [diff] [review] > patch xpc.msg part 2 > > Ehsan isn't accepting requests at this time. Could you ask on irc on Monday > if someone else will need to r+ this besides myself?

Comment on attachment 8917590 [details] [diff] [review] Part 1 - Client code Mostly nits. If you are fine with these changes I'm fine with r+'ing it. ># HG changeset patch ># User Matt Howell <mhowell@mozilla.com> >diff --git a/toolkit/mozapps/update/UpdateTelemetry.jsm b/toolkit/mozapps/update/UpdateTelemetry.jsm >--- a/toolkit/mozapps/update/UpdateTelemetry.jsm >+++ b/toolkit/mozapps/update/UpdateTelemetry.jsm >@@ -171,16 +171,17 @@ this.AUSTLMY = { > DWNLD_ERR_NO_UPDATE_PATCH: 6, > DWNLD_ERR_PATCH_SIZE_LARGER: 8, > DWNLD_ERR_PATCH_SIZE_NOT_EQUAL: 9, > DWNLD_ERR_BINDING_ABORTED: 10, > DWNLD_ERR_ABORT: 11, > DWNLD_ERR_DOCUMENT_NOT_CACHED: 12, > DWNLD_ERR_VERIFY_NO_REQUEST: 13, > DWNLD_ERR_VERIFY_PATCH_SIZE_NOT_EQUAL: 14, >+ DWNLD_RETRY_RESUME_FAILURE: 15, nit: please rename to DWNLD_RESUME_FAILURE since it is actually a resume and not a retry of a resume. >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 >@@ -1140,16 +1141,19 @@ function UpdatePatch(patch) { > this._properties = {}; > for (var i = 0; i < patch.attributes.length; ++i) { > var attr = patch.attributes.item(i); > attr.QueryInterface(Ci.nsIDOMAttr); > switch (attr.name) { > case "selected": > this.selected = attr.value == "true"; > break; >+ case "entityID": >+ this.setProperty("entityID", attr.value); >+ break; Fairly certain that the default case should handle this fine. e.g. this[attr.name] = attr.value; > case "size": > if (0 == parseInt(attr.value)) { > LOG("UpdatePatch:init - 0-sized patch!"); > throw Cr.NS_ERROR_ILLEGAL_VALUE; > } > // fall through > default: > this[attr.name] = attr.value; >@@ -1328,27 +1330,24 @@ function Update(update) { > this.errorCode = val; > } > } else if (attr.name == "isCompleteUpdate") { > this.isCompleteUpdate = attr.value == "true"; > } else if (attr.name == "promptWaitTime") { > if (!isNaN(attr.value)) { > this.promptWaitTime = parseInt(attr.value); > } >- } else if (attr.name == "backgroundInterval") { >- if (!isNaN(attr.value)) { >- this.backgroundInterval = parseInt(attr.value); >- } > } else if (attr.name == "unsupported") { > this.unsupported = attr.value == "true"; > } else { > this[attr.name] = attr.value; > > switch (attr.name) { > case "appVersion": >+ case "backgroundInterval": This shouldn't be necessary. > case "buildID": > case "channel": > case "displayVersion": > case "name": > case "previousAppVersion": > case "serviceURL": > case "statusText": > case "type": >@@ -3202,108 +3197,91 @@ Checker.prototype = { <snip> > /** > * Verify the downloaded file. We assume that the download is complete at > * this point. >+ * >+ * @param destination nit: what do you think of naming this patchFile since this is no longer using this._request.destination? I think that is more consistent with the variable name used elsewhere for the mar file in nsUpdateService.js. >+ * nsIFile representing the fully downloaded file > */ >- _verifyDownload: function Downloader__verifyDownload() { >- LOG("Downloader:_verifyDownload called"); >- if (!this._request) { >- AUSTLMY.pingDownloadCode(this.isCompleteUpdate, >- AUSTLMY.DWNLD_ERR_VERIFY_NO_REQUEST); >- return false; >- } >- >- let destination = this._request.destination; >+ _verifyDownload(destination) { >@@ -3379,229 +3357,367 @@ Downloader.prototype = { <snip> >+ >+ LOG("ChannelDownloader:downloadUpdate - url: " + this._patch.URL + >+ ", path: " + patchFile.path); >+ let uri = Services.io.newURI(this._patch.URL); >+ >+ let BackgroundFileSaver = Components.Constructor( >+ "@mozilla.org/network/background-file-saver;1?mode=streamlistener", >+ "nsIBackgroundFileSaver"); >+ this._bkgFileSaver = new BackgroundFileSaver(); >+ this._bkgFileSaver.QueryInterface(Ci.nsIStreamListener); >+ >+ this._channel = NetUtil.newChannel({uri, loadUsingSystemPrincipal: true}); >+ this._channel.notificationCallbacks = this; >+ this._channel.asyncOpen2(this.QueryInterface(Ci.nsIStreamListener)); >+ >+ if (this._channel instanceof Ci.nsIResumableChannel && >+ patchFile.exists()) { >+ let resumeFrom; >+ let entityID = this._patch.getProperty("entityID"); >+ if (!entityID) { >+ LOG("ChannelDownloader:downloadUpdate - failed to resume download, " + >+ "couldn't get entityID for the selected patch"); >+ } else { >+ try { >+ resumeFrom = patchFile.fileSize; >+ } catch (e) { >+ LOG("ChannelDownloader:downloadUpdate - failed to resume download, " + >+ "couldn't open partially downloaded file, exception: " + e); >+ } >+ } >+ >+ if (entityID && (resumeFrom != undefined)) { When checking for undefined it should be resumeFrom !== undefined Also, no need for the additional parentheses around resumeFrom !== undefined >+ this._channel.resumeAt(resumeFrom, entityID); >+ this._bkgFileSaver.enableAppend(); >+ this._resumedFrom = resumeFrom; >+ LOG("ChannelDownloader:downloadUpdate - resuming previous download " + >+ "starting after " + resumeFrom + " bytes"); >+ } else { >+ AUSTLMY.pingDownloadCode(this.isCompleteUpdate, >+ AUSTLMY.DWNLD_RETRY_RESUME_FAILURE); >+ } >+ }

Comment on attachment 8917889 [details] [diff] [review] Part 3 - Test changes diff --git a/toolkit/mozapps/update/tests/data/sharedUpdateXML.js b/toolkit/mozapps/update/tests/data/sharedUpdateXML.js >--- a/toolkit/mozapps/update/tests/data/sharedUpdateXML.js >+++ b/toolkit/mozapps/update/tests/data/sharedUpdateXML.js >@@ -280,20 +278,25 @@ function getLocalPatchString(aPatchProps > state: STATE_SUCCEEDED > }; > > for (let name in aPatchProps) { > patchProps[name] = aPatchProps[name]; > } > > let selected = "selected=\"" + patchProps.selected + "\" "; >- let state = "state=\"" + patchProps.state + "\"/>"; >+ let state = "state=\"" + patchProps.state + "\" "; >+ let entityID = aPatchProps.entityID ? >+ "entityID=\"" + aPatchProps.entityID + "\"" : >+ ""; > return getPatchString(patchProps) + " " + > selected + >- state; >+ state + >+ entityID + >+ " />"; You've added a couple of extra spaces here. When entityID doesn't exist there is the extra space after state and the one added above with " />". Instead, just add entityID before state and revert state above to how it was previously. >diff --git a/toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js b/toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js >--- a/toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js >+++ b/toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js >@@ -3676,30 +3677,38 @@ const downloadListener = { > }, > > QueryInterface: XPCOMUtils.generateQI([Ci.nsIRequestObserver, > Ci.nsIProgressEventSink]) > }; > > /** > * Helper for starting the http server used by the tests >+ * >+ * @param options.slowDownload >+ * When serving FILE_SIMPLE_MAR, hang until gSlowDownloadContinue is true > */ >-function start_httpserver() { >+function start_httpserver(options) { I'm lukewarm about using an object for options here. If you go with an object please fix the comment above so it reflects reality since options.errorDownload is also a valid param. > let dir = getTestDirFile(); > debugDump("http server directory path: " + dir.path); > > if (!dir.isDirectory()) { > do_throw("A file instead of a directory was specified for HttpServer " + > "registerDirectory! Path: " + dir.path); > } > > let { HttpServer } = Cu.import("resource://testing-common/httpd.js", {}); > gTestserver = new HttpServer(); > gTestserver.registerDirectory("/", dir); > gTestserver.registerPathHandler("/" + gHTTPHandlerPath, pathHandler); >+ if (options && options.slowDownload) { >+ gTestserver.registerPathHandler("/" + FILE_SIMPLE_MAR, marPathHandler); >+ } else if (options && options.errorDownload) { >+ gTestserver.registerPathHandler("/" + FILE_SIMPLE_MAR, marErrorPathHandler); >+ } >diff --git a/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js b/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js >rename from toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js >rename to toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js >--- a/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecovery.js >+++ b/toolkit/mozapps/update/tests/unit_aus_update/downloadInterruptedRecoveryChannel.js >@@ -1,59 +1,102 @@ <snip> >-/** >- * Called after the call to waitForUpdateXMLFiles finishes. >- */ >-function waitForUpdateXMLFilesFinished() { >- // The HttpServer must be stopped before calling do_test_finished >- stop_httpserver(doTestFinish); >-} Slightly concerned about not using the helpers that verify the xml files to be in a consistent state. If any of the tests without it start to intermittently fail it can be fixed in the intermittent bug. >diff --git a/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeForSameAppVersion.js b/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeForSameAppVersion.js >--- a/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeForSameAppVersion.js >+++ b/toolkit/mozapps/update/tests/unit_aus_update/downloadResumeForSameAppVersion.js >@@ -1,20 +1,21 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. > */ > > function run_test() { > setupTestCommon(); >+ start_httpserver(); > > debugDump("testing resuming an update download in progress for the same " + > "version of the application on startup (Bug 485624)"); > >- let patchProps = {state: STATE_DOWNLOADING}; >+ let patchProps = {state: STATE_DOWNLOADING, url: gURLData + FILE_SIMPLE_MAR}; This change shouldn't be necessary since that is the default value for url r=me

Just wanted to add that I like these patches quite a lot. Thanks!

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #50) > Don't forget > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #8) > > Comment on attachment 8884594 [details] [diff] [review] > > patch xpc.msg part 2 > > > > Ehsan isn't accepting requests at this time. Could you ask on irc on Monday > > if someone else will need to r+ this besides myself? Yeah, I had forgotten. I asked in #fx-team and didn't get a response, so I checked the commit log for xpc.msg and from that it doesn't look like anyone else's review is needed. (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #51) > >+ case "entityID": > >+ this.setProperty("entityID", attr.value); > >+ break; > Fairly certain that the default case should handle this fine. > e.g. this[attr.name] = attr.value; It doesn't, because entityID goes in the property bag and not directly in the object. I think this is the only place it really needs special attention for that reason, so I left it alone. (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #52) > > /** > > * Helper for starting the http server used by the tests > >+ * > >+ * @param options.slowDownload > >+ * When serving FILE_SIMPLE_MAR, hang until gSlowDownloadContinue is true > > */ > >-function start_httpserver() { > >+function start_httpserver(options) { > I'm lukewarm about using an object for options here. If you go with an > object please fix the comment above so it reflects reality since > options.errorDownload is also a valid param. I didn't want to start introducing new boolean positional arguments. I could switch to using a destructured object there instead to make it slightly more ergonomic, but it looks like a wash to me. I'll certainly fix the doc comment though.

Fixed nits, r+ carried over.

Fixed nits, r+ carried over.

(In reply to Matt Howell [:mhowell] from comment #54) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #50) > > Don't forget > > > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #8) > > > Comment on attachment 8884594 [details] [diff] [review] > > > patch xpc.msg part 2 > > > > > > Ehsan isn't accepting requests at this time. Could you ask on irc on Monday > > > if someone else will need to r+ this besides myself? > > Yeah, I had forgotten. I asked in #fx-team and didn't get a response, so I > checked the commit log for xpc.msg and from that it doesn't look like anyone > else's review is needed. > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #51) > > >+ case "entityID": > > >+ this.setProperty("entityID", attr.value); > > >+ break; > > Fairly certain that the default case should handle this fine. > > e.g. this[attr.name] = attr.value; > > It doesn't, because entityID goes in the property bag and not directly in > the object. I think this is the only place it really needs special attention > for that reason, so I left it alone. Hmmm... I think I know what is going on. Please hold off landing this until I get back to you about it.

Comment on attachment 8919503 [details] [diff] [review] Part 1 - Client code When I made it so an nsIUpdate could have and write arbitrary values I never made it so nsIUpdatePatch could have and write arbitrary values. I *think* I'd prefer that it can but I don't think that has to happen in this bug. Also, the this._update.QueryInterface(Ci.nsIPropertyBag) makes more sense to me now and I'll probably clean that up later.

Pushed by mhowell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/edfa340ec9fb Part 1 - Stop using nsIIncrementalDownload for application update. r=rstrong https://hg.mozilla.org/integration/mozilla-inbound/rev/fb54cd45fa10 Part 2 - Add a new XPC_MSG_DEF required by tests for this bug. r=rstrong https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2fcdddead1 Part 3 - Test changes for new app update downloader implementation. r=rstrong

https://hg.mozilla.org/mozilla-central/rev/edfa340ec9fb https://hg.mozilla.org/mozilla-central/rev/fb54cd45fa10 https://hg.mozilla.org/mozilla-central/rev/ae2fcdddead1

Jordan, how does this change block 1419189?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #61) > Jordan, how does this change block 1419189? I was trying to connect the two. It isn't blocking because it is resolved. Perhaps I should use see-also or else create a meta bug that tracks what broke in updates for 58.0b1 users. Does that make sense?

This was backed out happened on nightly in bug 1442407 and it was uplifted to beta so reopening.