The "cancel" method accepts an aOptions argument, where the keepPartFile option can be used to determine if the download will restart from the point where it currently stopped, if supported by the saver object. The "target.partFile" property keeps a reference to the partially downloaded file. If the partially downloaded file exists, it will be considered when the "start" method is called, if the source supports resuming.

If the source does not support resuming from partially downloaded files, then the file should not be kept, even if requested.

Hi guys. Can I get more info on what part of the code is involved and how to implement this? Thanks for your help. Marcos.

(In reply to Marcos Aruj from comment #2) > Can I get more info on what part of the code is involved and how to > implement this? Hello Marcos! This bug is about adding off-main-thread resuming support to the JavaScript API for downloads. It can be divided in two main parts. The first involves adding resuming support to the BackgroundFileSaver component. The component is written in C++, you can see its interface defined in "BackgroundFileSaver.idl" and the implementation in "BackgroundFileSaver.cpp". The second part, to be done after the first one, involves changes to the code in the "toolkit/components/jsdownloads" folder, which is not built by default now, but will be available in Nightly in a few days from now. There is still some time to be spent to better understand how resuming would work in the new API, before getting to coding it. The C++ part, however, can already be compiled and if you can run tests from the "test_backgroundfilesaver.js" file, you have what you need to move forward on implementing that part. If you're interested, let me know and I can file a new dependent bug to let you get started there. Alternatively, there are some other JavaScript bugs that may be worked on in the new API, and require less time to complete. I can point you to a few of them if you're interested.

Thanks Paolo for the info. I think I would then prefer the alternative bugs you mention if that's OK. :) Please point me to them. Kind regards.

Supporting partial download data in a fully asynchronous environment where we may have different views controlling the same download is not easy, but I think this interface is simple enough and prevents race conditions. To do this, I had to write an interface that is slightly different from comment 0. Please take a look at this interface, and let me know if you see any possible race conditions or a use case that is not covered. This interface supports the current pause/resume logic, but is more clear about what "pausing" means (in fact, it does not suspend the request, it just cancels the request and keeps any partial data). So, if hasPartial is true, it means the download can be "paused": you can call cancel() and start() to actually pause and resume it. To completely stop a download, you can call cancel() and then removePartial() immediately. But, probably, we don't want to do that, because the stop button should still preserve partial data where possible, until the download is removed from the list. (Once stopping does the right thing automatically, we'll unify the concepts in the user interface also.) On shutdown, we will just cancel() all the downloads, and this will do the right thing, keeping the partial data if possible, to use when the download is restarted when the browser is reopened.

Comment on attachment 774667 [details] [diff] [review] Interface definition + * + * To have any effect, this property must be set before starting the download. + * Setting this property to false after the download already started will not + * remove any partial data. + * after the download *has* already started >+ * @return {Promise} >+ * @resolves When the partial data has been successfully removed. >+ * @rejects JavaScript exception if the operation could not be completed. >+ */ >+ removePartial: function () >+ { >+ return Promise.resolve(); >+ }, So if someone wants to restart a download fresh, do they need to call this method first? >+ /** >+ * Prevents the download from starting again after having been stopped. This >+ * method does not cancel the download in case it has already started. >+ * >+ * This method is used while shutting down or disposing of the download >+ * object, to prevent other callers to interfere with the operation. This is >+ * required because cancellation and other operations are asynchronous. >+ */ >+ preventStart: function () >+ { This name is a bit confusing. If it is intended for cleanup, it is private to the download manager implementation or would other callers use it? > this.remove(download); >+ // Since the download state may have changed meanwhile, we fully >+ // execute the procedure that ensures that the download is stopped and >+ // no partial data is kept. We don't need to wait for the procedure >+ // to be complete before processing the other downloads in the list. >+ download.preventStart(); >+ download.cancel(); >+ download.removePartial(); Could we combine these into a single call that just stops and cleans up?

(In reply to Neil Deakin from comment #6) > >+ removePartial: function () > >+ { > >+ return Promise.resolve(); > >+ }, > > So if someone wants to restart a download fresh, do they need to call this > method first? Yes, if they want to restart the same download with the same target file, but without using any previously stored partial data. > >+ preventStart: function () > >+ { > > This name is a bit confusing. If it is intended for cleanup, it is private > to the download manager implementation or would other callers use it? > > >+ download.preventStart(); > >+ download.cancel(); > >+ download.removePartial(); > > Could we combine these into a single call that just stops and cleans up? Since the two cases when preventStart is currently used are before throwing away a download and before shutting down the browser, maybe we might define a download.shutdown(bool removePartial) method that calls preventStart plus "cancel", and optionally calls removePartial, returning a promise resolved when the operation is completed. This would replace the need for a public preventStart method. What do you think?

> Since the two cases when preventStart is currently used are before throwing > away a download and before shutting down the browser, maybe we might define > a download.shutdown(bool removePartial) method that calls preventStart plus > "cancel", and optionally calls removePartial, returning a promise resolved > when the operation is completed. This would replace the need for a public > preventStart method. What do you think? preventStart isn't a clear api name (nor is removePartial) so I think that would be better. I probably wouldn't call it 'shutdown' though as it doesn't have to be called at shutdown. Maybe 'reset' or 'cleanup'.

what about just download.finalize()?

Comment on attachment 774667 [details] [diff] [review] Interface definition Review of attachment 774667 [details] [diff] [review]: ----------------------------------------------------------------- I think covered pretty well the request, just a couple notes (and my suggested API name is in previous commment) ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +417,5 @@ > + * download failed or has been canceled, using the removePartial method. If > + * this property is still true, partial data will be retained during the next > + * download attempt. > + */ > + tryKeepPartial: true, I see you used this in SimpleDownload, but do we have a real use case for it? What's the downside if SimpleDownload would just not care and we'd not expose this option? ::: toolkit/components/jsdownloads/src/DownloadList.jsm @@ +221,5 @@ > + // no partial data is kept. We don't need to wait for the procedure > + // to be complete before processing the other downloads in the list. > + download.preventStart(); > + download.cancel(); > + download.removePartial(); do we already handle PB somewhere? cause there we should remove the partial.

that was "I think Neil covered pretty well the request..."

(In reply to Marco Bonardo [:mak] from comment #10) > > + tryKeepPartial: true, > > I see you used this in SimpleDownload, but do we have a real use case for it? > What's the downside if SimpleDownload would just not care and we'd not > expose this option? Consumers of simpleDownload would leak partial files on disk on failure. Setting tryKeepPartial to false is also useful for those cases where we want to handle a full download with progress report (that is, not a simpleDownload) but we just discard the result on failure (for example, this would allow us to move add-on XPI downloads off the main thread). Actually, since tryKeepPartial is only safe to use if the download is going to be persisted across sessions or the caller invokes removePartial before shutdown, I wonder if we should just have the default to false, and set it to true only where we add the download to the list (DownloadLegacySaver and panel front-end code). > ::: toolkit/components/jsdownloads/src/DownloadList.jsm > do we already handle PB somewhere? cause there we should remove the partial. Yes, we should remove partial data when downloads are canceled when exiting private browsing mode. That's covered by bug 836443.

(In reply to Neil Deakin from comment #8) > preventStart isn't a clear api name (nor is removePartial) Would removePartialData be clearer than removePartial? (In reply to Marco Bonardo [:mak] from comment #9) > what about just download.finalize()? finalize looks good to me, with an aRemovePartialData argument.

This bug will make it possible to restart downloads previously started through a DownloadLegacySaver. This makes the behavior of this saver interchangeable with DownloadCopySaver. I've thus refactored the tests so that they are the same for both components. For the moment, I've disabled the restart tests, that will be enabled in a separate patch on this bug. Unfortunately, since with the legacy saver we cannot get a reference to the Download object before starting the download, some tests have a slightly different control flow between the two versions. This patch contains a rename of test_DownloadCore.js to the new name common_test_Download.js, with the modifications to include all the former test_DownloadLegacy.js tests.

This patch contains the updated interface definitions, as well as the first implementation of resuming for DownloadCopySaver. This will need more tests for all the new methods in the interface, but there should be enough material for a preliminary review. Marco, I think I'll do the DownloadError constructor refactoring we talked about, since it seems we have more cases where we need to set the properties manually, and not based on the current constructor parameters. Given the complexity of this patch alone, I think I'll add resuming support for DownloadLegacySaver in a third, separate patch.

With the refactoring, some tests now failed more consistently for the same causes as the intermittent tests in bug 865364. This patch makes each request independent from the others. This fixed the tests and will probably also have the effect of resolving the intermittent failures.

This is how the third and last patch in the series would look like.

Comment on attachment 778498 [details] [diff] [review] Part 1 of 2 - Unify DownloadLegacy tests in preparation for resuming support >+ let persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"] >+ .createInstance(Ci.nsIWebBrowserPersist); >+ >+ // Apply decoding if required by the "Content-Encoding" header. >+ persist.persistFlags &= ~Ci.nsIWebBrowserPersist.PERSIST_FLAGS_NO_CONVERSION; >+ >+ // We must create the nsITransfer implementation using its class ID because >+ // the "@mozilla.org/transfer;1" contract is currently implemented in >+ // "toolkit/components/downloads". When the other folder is not included in >+ // builds anymore (bug 851471), we'll be able to use the contract ID. >+ let transfer = >+ Components.classesByID["{1b4c85df-cbdd-4bb6-b04e-613caece083c}"] >+ .createInstance(Ci.nsITransfer); >+ >+ if (aOptions) { >+ aOptions.outPersist = persist; >+ } Move these outPersist lines above to where persist is set up. >- * Executes a download, started by constructing the simplest Download object. >+ * Executes a download and checks its basic properties after constructions. 'construction' >-add_task(function test_download_construction() >+add_task(function test_basic() > { >- let targetPath = getTempFile(TEST_TARGET_FILE_NAME).path; >+ let tempDirectory = FileUtils.getDir("TmpD", []); tempDirectory is unused. >+ // The download is already started, wait for completion and report errors. >+ if (!download.stopped) { >+ yield download.start(); >+ } What is this condition checking for? If you want the test to verify that download.stopped must be false, the test should check that. Are there cases where download.stopped could be true here? Same with a number of other places where this condition appears. >-add_task(function test_download_final_state_notified() >+add_task(function test_final_state_notified() > { >- let download = yield promiseSimpleDownload(); >+ let deferResponse = deferNextResponse(); >+ >+ let download = yield promiseStartDownload(httpUrl("interruptible.txt")); > ... >- // Starts the download and waits for completion. >- yield download.start(); I'm a bit confused here. Doesn't promiseStartDownload end up calling start() already? It's a bit confusing that some of the tests use promiseStartDownload, while others check gUseLegacySaver and set things up separately. Some comments as to why this is needed is those cases would be helpful.

Comment on attachment 779715 [details] [diff] [review] Fix tests - Add the ability to resume to DownloadCopySaver. >+ * Indicates whether any partially downloaded data should be retained, to use >+ * it when restarting a failed or canceled download. The default is false. remove 'it' >+ * If this property is set to true, care should be taken that partial data is >+ * removed before the reference to the download is discarded. This can be >+ * done using the removePartialData or the "finalize" methods. >+ */ >+ tryKeepPartialData: false, Maybe better as 'tryToKeepPartialData' > /** >+ * String containing the path of the ".part" file containing the data >+ * downloaded so far, or null to disable the use of a ".part" file to keep >+ * partially downloaded data. >+ */ >+ partFilePath: null, >+ >+ /** So to retain partial files, does one need to set both target.partFilePath and download.tryKeepPartialData? Seems like only one field should be needed to be set. >+ * >+ * This method never called until the promise returned by "execute" is either >+ * resolved or rejected, and the "execute" method is not called again until >+ * the promise returned by this method is resolved or rejected. This method *is* never called... >- // Set the target file, that will be deleted if the download fails. >- backgroundFileSaver.setTarget(new FileUtils.File(download.target.path), >- false); The old code sets the target here before everything starts, but the new code doesn't set it until onStartRequest is called. Is that ok? >+ // Create a placeholder for the final target file immediately. If the >+ // file already exist, don't delete its contents yet. file already *exists*, ... Maybe explain why we need to create the file right away. >+ if (this._canceled) { >+ // Don't create the BackgroundFileSaver object if we have been >+ // canceled meanwhile. >+ throw new DownloadError(Cr.NS_ERROR_FAILURE, "Saver canceled."); >+ } Is canceling a download an error, or does this error not propagate? >+ let deferCancel = Promise.defer(); >+ download.onchange = function () { >+ if (!download.stopped && !download.canceled && download.progress == 50) { >+ // After we receive the progress notification, we should wait some time >+ // for the worker thread of BackgroundFileSaver to receive the data to >+ // be written to disk. We don't have control over when this happens. >+ do_timeout(50, () => { >+ deferCancel.resolve(download.cancel()); >+ }); This will fail if the test happens to take longer that 50ms, so it should probably iterate. Is the promiseNextRequestReceived method still needed? Not sure I like the names blockResponses and unblockResponses. These are used just to delay a response part way and start it again right? Perhaps 'mustPauseResponse' and 'continueResponse' would be clearer.

Comment on attachment 780284 [details] [diff] [review] Work in progress - Add the ability to resume to DownloadLegacySaver. Both DownloadCopySaver.removePartialData and DownloadLegacySaver.removePartialData are the same code. Could they be shared or just use an inherited implementation or will they differ?

(In reply to Neil Deakin from comment #18) > >+ // The download is already started, wait for completion and report errors. > >+ if (!download.stopped) { > >+ yield download.start(); > >+ } > > What is this condition checking for? > > It's a bit confusing that some of the tests use promiseStartDownload, while > others check gUseLegacySaver and set things up separately. Some comments as > to why this is needed is those cases would be helpful. To clarify the tests, I've added a promiseDownloadStopped function, renamed promiseSimpleDownload to promiseNewDownload, added comments when we do things differently for DownloadLegacySaver, and refactored some tests to include the DownloadLegacySaver case in error state checks. It should be much more readable overall, let me know if there is something else than needs clarification.

(In reply to Neil Deakin from comment #19) > So to retain partial files, does one need to set both target.partFilePath > and download.tryKeepPartialData? Seems like only one field should be needed > to be set. Currently, partFilePath may be set even if tryKeepPartialData is false, to use a ".part" file instead of the final target while downloading. In case tryKeepPartialData is false, the ".part" file is always deleted. tryKeepPartialData instead controls whether partial data should be kept, for all saver types. A DownloadDOMDocumentSaver might for example use this flag to determine if the responses should be kept using the browser cache, without actually needing the partFilePath to be set. Currently, both DownloadCopySaver and DownloadLegacySaver also need partFilePath to be set, to be able to keep partial data. We may make it so that if tryKeepPartialData is true and partFilePath is null, a default value is used, but then, since tryKeepPartialData may be set to false later, the removePartialData function would always need to check if the file with the default name exists and delete it, even if we didn't want to use a part file in the first place. What do you think? > The old code sets the target here before everything starts, but the new code > doesn't set it until onStartRequest is called. Is that ok? Yes, this should work correctly. > >+ if (this._canceled) { > >+ // Don't create the BackgroundFileSaver object if we have been > >+ // canceled meanwhile. > >+ throw new DownloadError(Cr.NS_ERROR_FAILURE, "Saver canceled."); > >+ } > > Is canceling a download an error, or does this error not propagate? The Download object handles cancellation by requesting the DownloadSaver object to terminate the "execute" method with an exception that is then ignored, so it does not propagate. > >+ let deferCancel = Promise.defer(); > >+ download.onchange = function () { > >+ if (!download.stopped && !download.canceled && download.progress == 50) { > >+ // After we receive the progress notification, we should wait some time > >+ // for the worker thread of BackgroundFileSaver to receive the data to > >+ // be written to disk. We don't have control over when this happens. > >+ do_timeout(50, () => { > >+ deferCancel.resolve(download.cancel()); > >+ }); > > This will fail if the test happens to take longer that 50ms, so it should > probably iterate. I've implemented an iterative approach that should be more robust. I wasn't able to test it as the actual code flow depends on timing, and it seems that the simple call to OS.File.exists makes the timeout unnecessary, probably because it posts events between threads. > Not sure I like the names blockResponses and unblockResponses. These are > used just to delay a response part way and start it again right? Perhaps > 'mustPauseResponse' and 'continueResponse' would be clearer. I've gone for mustInterruptResponses and continueResponses (plural since they apply to all the requests at the same time).

(In reply to Neil Deakin from comment #20) > Both DownloadCopySaver.removePartialData and > DownloadLegacySaver.removePartialData are the same code. Could they be > shared or just use an inherited implementation or will they differ? They'll be the same, but the function won't apply to other saver types, so I've just done "return DownloadCopySaver.prototype.removePartialData.call(this);".

Marco, let me know if you have any feedback on any of the three patches.

sure, sorry I had some issues keeping up with requests this week.

This patch adds the missing tests and should be ready for review.

This completes the implementation of resuming support in DownloadLegacySaver, and adds tests for downloads started through nsIExternalHelperAppService.

Comment on attachment 780859 [details] [diff] [review] Part 1 of 3 - Unify DownloadLegacy tests in preparation for resuming support The comments here and the promiseDownloadStopped method make what's happening here much clearer. Thanks! The only change is that the comment below, the last comma should be removed, or the sentence reworded: + // When testing DownloadCopySaver, we have control over the download, thus + // we can hook its onchange callback, that will be notified when the + // download starts. And in the next comment: + // is created, and it may already have notified all its property changes + // until now, thus there I can't quite parse this line. Do you mean 'may have already made all needed property change notifications'?

Comment on attachment 781083 [details] [diff] [review] Part 2 of 3 - Add the ability to resume a download from where it stopped to DownloadCopySaver. Review of attachment 781083 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +417,5 @@ > return this._promiseCanceled; > }, > > /** > + * Indicates whether any partially downloaded data should be retained, to use maybe "to be used"? @@ +425,5 @@ > + * download source, and may not be known before the download is started. > + * > + * To have any effect, this property must be set before starting the download. > + * Resetting this property to false after the download has already started > + * will not remove any partial data. is that an expected use-case? otherwise you may replace the value with a getter once the download has started, to prevent setting it. @@ +429,5 @@ > + * will not remove any partial data. > + * > + * If this property is set to true, care should be taken that partial data is > + * removed before the reference to the download is discarded. This can be > + * done using the removePartialData or the "finalize" methods. either both should have apices, or none, there's nothing special about finalize. @@ +444,5 @@ > + /** > + * Removes any partial data kept as part of a canceled or failed download. > + * > + * If the download is not canceled or failed, this method has no effect, and > + * it returns a resolved promise. If the "cancel" method was called but the I wonder if that should be considered an error and rejected, like removing partial data of an in-progress download. Couldn't just resolving hide mistakes like that? @@ +727,5 @@ > } else if (aSerializable instanceof Ci.nsIURI) { > source.url = aSerializable.spec; > } else { > + // Convert String objects to primitive strings at this point. > + source.url = "" + aSerializable.url; nit: it may be more polite to explicitly use .toString() @@ +801,5 @@ > target.path = aSerializable.path; > } else { > // Read the "path" property of the serializable DownloadTarget > + // representation, converting String objects to primitive strings. > + target.path = "" + aSerializable.path; ditto @@ +1008,5 @@ > * Implements "DownloadSaver.execute". > */ > execute: function DCS_execute(aSetProgressBytesFn) > { > + let self = this; s/self/copySaver/ ? ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +96,5 @@ > + * saved to disk. You must call "continueResponses" to allow the interruptible > + * request to continue. > + * > + * TODO: This function uses either DownloadCopySaver or DownloadLegacySaver > + * based on the current test run. I'm not sure what's the todo part in that phrase.

I quickly skimmed through the patches and the approach and naming looks good to me, didn't go for a deep line-by-line review though.

(In reply to Marco Bonardo [:mak] from comment #30) > > + * Indicates whether any partially downloaded data should be retained, to use > > maybe "to be used"? I think the grammar is correct per comment 19, right? > @@ +425,5 @@ > > + * download source, and may not be known before the download is started. > > + * > > + * To have any effect, this property must be set before starting the download. > > + * Resetting this property to false after the download has already started > > + * will not remove any partial data. > > is that an expected use-case? otherwise you may replace the value with a > getter once the download has started, to prevent setting it. This gives control on whether to keep partial data when restarting downloads originally started from DownloadLegacySaver, we have a test case for that. > @@ +429,5 @@ > > + * will not remove any partial data. > > + * > > + * If this property is set to true, care should be taken that partial data is > > + * removed before the reference to the download is discarded. This can be > > + * done using the removePartialData or the "finalize" methods. > > either both should have apices, or none, there's nothing special about > finalize. Well, finalizeHasNoInterCaps, this is why I've added quotes to mark it clearly as an identifier. When translating to formatted text, identifiers become monospace, but in the text-only version I preferred not to use "quotes" or |pipes| where avoidable without ambiguity. > @@ +444,5 @@ > > + /** > > + * Removes any partial data kept as part of a canceled or failed download. > > + * > > + * If the download is not canceled or failed, this method has no effect, and > > + * it returns a resolved promise. If the "cancel" method was called but the > > I wonder if that should be considered an error and rejected, like removing > partial data of an in-progress download. Couldn't just resolving hide > mistakes like that? The idea is that the download might have been restarted by another listener while we are processing a completion notification, and this shouldn't result in a possibly intermittent error condition that is visible to the user. This is the same logic used by other methods on the download, it isn't an error if calls arrive in a different order. As long as we keep a consistent internal state, and properly notify changes in hasPartialData and other properties to the views, whether the download still has or hasn't performed the requested action because of a conflicting command coming from another listener should be secondary. > @@ +727,5 @@ > > } else if (aSerializable instanceof Ci.nsIURI) { > > source.url = aSerializable.spec; > > } else { > > + // Convert String objects to primitive strings at this point. > > + source.url = "" + aSerializable.url; > > nit: it may be more polite to explicitly use .toString() I'll update the code. > ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js > @@ +96,5 @@ > > + * saved to disk. You must call "continueResponses" to allow the interruptible > > + * request to continue. > > + * > > + * TODO: This function uses either DownloadCopySaver or DownloadLegacySaver > > + * based on the current test run. > > I'm not sure what's the todo part in that phrase. In fact, it's implemented in the following patch. (In reply to Marco Bonardo [:mak] from comment #31) > I quickly skimmed through the patches and the approach and naming looks good > to me, didn't go for a deep line-by-line review though. Thanks a lot for the feedback!

Made changes from comment 29 and landed part 1 of 3 to fx-team: https://hg.mozilla.org/integration/fx-team/rev/8c3ee4235ec1

(In reply to Paolo Amadini [:paolo] from comment #33) > Made changes from comment 29 and landed part 1 of 3 to fx-team: > > https://hg.mozilla.org/integration/fx-team/rev/8c3ee4235ec1 https://hg.mozilla.org/mozilla-central/rev/8c3ee4235ec1

While working on this, I found that bug 899102 is affecting the reliability of these tests. I think we should try to land this patch despite the issue, so that we can build upon it, without blocking on bug 899102. So, I've added the following workaround after every cancellation in the legacy nsExternalHelperAppService tests: // This time-based solution is a workaround to avoid intermittent failures, // and will be removed when bug 899102 is resolved. if (gUseLegacySaver) { yield promiseTimeout(250); }

Comment on attachment 781228 [details] [diff] [review] Part 3 of 3 - Add the ability to resume a download from where it stopped to DownloadLegacySaver. >+ throw new Components.Exception("Asynchronous version implemented.", >+ Cr.NS_ERROR_NOT_AVAILABLE); This is a confusing error message. It would make more sense to say 'Synchronous promptForSaveToFile not implemented.' >+ let legacyDownload = yield promiseStartLegacyDownload(); >+ yield legacyDownload.cancel(); >+ listForSave.add(legacyDownload); Why do you cancel this download first, but not the others?

(In reply to Neil Deakin from comment #36) > >+ let legacyDownload = yield promiseStartLegacyDownload(); > >+ yield legacyDownload.cancel(); > >+ listForSave.add(legacyDownload); > > Why do you cancel this download first, but not the others? We cannot create legacy downloads without starting them, while when we use the other functions that create normal downloads (Downloads.createDownload and promiseNewDownload) we can just avoid to start them.

This is the rebased version of the same patch.

I'm working on resolving some intermittent failures that appeared on Windows, I hope this version of the patch will address them.

Intermittent failures disappeared with this new version of the patch: https://tbpl.mozilla.org/?tree=Try&rev=a6f34758d89a If any other intermittent failure appears, it should be filed as a separate bug.

https://hg.mozilla.org/integration/fx-team/rev/5913eecc1447 https://hg.mozilla.org/integration/fx-team/rev/9b8995108081

https://hg.mozilla.org/mozilla-central/rev/5913eecc1447 https://hg.mozilla.org/mozilla-central/rev/9b8995108081

Marking as [qa-] since there are unit tests for both pushlogs.