1426487 - Add method for adding download listeners without pausing and restarting the download

Status: NEW

(Toolkit :: Application Update, defect, P2)

Description

[Robert Strong (they/them - no direct email)]

Currently it is necessary to stop the update download (e.g. pauseDownload) and restart it again with downloadUpdate to remove the background download listener and add the foreground download listener. This shouldn't be necessary and causes bugs like bug 1423571.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

I don't think new methods will be needed. Pushed to oak so I can manually test things.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Robert, do we still need this for 58 as well as 59?

Comment 3

[Robert Strong (they/them - no direct email)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #2)
> Robert, do we still need this for 58 as well as 59?
No, bug 1423967 was identified as the bug that regressed / caused bug 1423571 and bug 1423967 was backed out of beta and aurora (dev edition) which is 58.

Comment 4

[Robert Strong (they/them - no direct email)]

Correction, Bug 1348087 was the actual cause which is stated as being backed out in bug 1423967

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, I saw that, but it looks like testing failed in https://bugzilla.mozilla.org/show_bug.cgi?id=1423571#c20

Comment 6

[Robert Strong (they/them - no direct email)]

I suspect that was cause by the backout not being included in the build that was tested that failed. Could you check if that is the case?

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Sure, I asked QA to confirm which version they were testing or to re-verify with a later build.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Gerry, can you follow up here to make sure all is well for 58 ? Thanks.

Comment 9

[Gerry Chang [:gchang]]

According to comment #26 in bug 1423571. This should not be an issue in 58. It's no longer a blocking. Mark 58 won't fix.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

I pushed this patch to oak. I've seen some problems with the TV test runs but think I have them fixed (fingers crossed).

Comment 11

[Robert Strong (they/them - no direct email)]

The TV test runs are passing.

Comment 12

[Robert Strong (they/them - no direct email)]

I've also manually tested repeatedly opening the about window and restarting Firefox while the download is in progress.

Comment 13

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8941666 [details] [diff] [review]
patch rev1

Review of attachment 8941666 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/nsUpdateService.js
@@ +3595,5 @@
> +    if (requestEntityID && requestEntityID != patchEntityID) {
> +      this._patch.QueryInterface(Ci.nsIWritablePropertyBag);
> +      this._patch.setProperty("entityID", requestEntityID);
> +      Cc["@mozilla.org/updates/update-manager;1"].
> +      getService(Ci.nsIUpdateManager).saveUpdates();

If the entityID has changed (and was previously set), then we should abort the request and start over without trying to resume, because that means that either the last modified time or the file size has changed since the last download attempt, so we're now downloading a different file than we were before.
The channel handled this for us by failing the request with NS_ERROR_ENTITY_CHANGED if the new entityID doesn't match the one passed to resumeAt, but since this patch no longer passes that parameter we should take care of it here.

@@ +3631,5 @@
>  
>      let currentTime = Date.now();
>      if ((currentTime - this._lastProgressTimeMs) > DOWNLOAD_PROGRESS_INTERVAL) {
> +      LOG("ChannelDownloader:onProgress - progress: " + progress +
> +          "/" + maxProgress);

This change is actually addressing bug 1417254, which is fine since it's so small, just need to remember to also close that one out.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Robert, Matt, since this update issue affects 59, do you think we should hold back 59.0b1? It is built and tested and was scheduled to be released today.     We could try to land a fix, test it, and go to build for 59.0b2 by tomorrow or Thursday.

Comment 15

[Robert Strong (they/them - no direct email)]

I think it would be safer to backout the changes on beta as was done in bug 1423967.

Comment 16

[Molly Howell (she/her) (mostly inactive)]

One backout bug and patch coming right up.

Comment 17

[anhtuanho]

Comment on attachment 8941666 [details] [diff] [review]
patch rev1

Review of attachment 8941666 [details] [diff] [review]:
-----------------------------------------------------------------

on the bug

Comment 18

[Julien Cristau [:jcristau]]

Robert, is this still expected to make 60?  If not should we at some point back bug 1348087 out of central until we get this sorted?

Comment 20

[Robert Strong (they/them - no direct email)]

The backout happened on nightly in bug 1442407 and it was uplifted to beta so no need to block.