Closed Bug 1700156 Opened 3 years ago Closed 3 years ago

Allow to exit AppUpdate.jsm update cycle when BITS job is kicked off

Categories

(Toolkit :: Application Update, task)

task

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox89 --- fixed

People

(Reporter: nalexander, Assigned: bytesized)

References

(Regressed 1 open bug)

Details

Attachments

(3 files)

For the --backgroundtask backgroundupdate, we'd like to be able to exit the task as soon as a BITS job is started. Right now, the AppUpdate.jsm state machine transitions to DOWNLOADING before continuing on; in some way, we want the task to not wait for the BITS job. This might require some small tweaks to the Application Update Service to handle the asynchrony.

Assignee: nobody → ksteuber

My initial thought was that we could just assume that we are done when we see the DOWNLOADING state, here. But it looks like we can't do something quite that simple. Here is what the flow of events could look like if we did that:

  1. Background Update Task runs, kicks off the BITS download, sees the DOWNLOADING state and exits. So far so good.
  2. The Background Update Task runs again. But the Update Service may not have had time to transition the the update status from "downloading" to "pending" yet. So the Background Update Task may see that the state is DOWNLOADING and immediately exit rather than proceeding with staging.

I think, however, that there is an easy solution to this. When AppUpdater propagates onProgress to its listeners (here), it gives the listener the amount of progress that has been made on the download. So instead of just exiting when we see the DOWNLOADING state, I think that we should exit when we see the DOWNLOADING state with aProgress != aProgressMax. That will allow us to distinguish between an in-progress download and a completed download.

There is one other detail that I think should also be added here. If the Background Update Task is downloading during this session, I think that we should lock the Update Service's ability to stage during this session. I'm concerned about the possibility that an extremely fast update download could cause the Update Service to start staging right before the Background Update Task shuts down. This could result in some unpleasant race conditions that I think are best avoided.

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #1)

My initial thought was that we could just assume that we are done when we see the DOWNLOADING state, here. But it looks like we can't do something quite that simple. Here is what the flow of events could look like if we did that:

  1. Background Update Task runs, kicks off the BITS download, sees the DOWNLOADING state and exits. So far so good.
  2. The Background Update Task runs again. But the Update Service may not have had time to transition the the update status from "downloading" to "pending" yet. So the Background Update Task may see that the state is DOWNLOADING and immediately exit rather than proceeding with staging.

I think, however, that there is an easy solution to this. When AppUpdater propagates onProgress to its listeners (here), it gives the listener the amount of progress that has been made on the download. So instead of just exiting when we see the DOWNLOADING state, I think that we should exit when we see the DOWNLOADING state with aProgress != aProgressMax. That will allow us to distinguish between an in-progress download and a completed download.

This sounds good. I was not aware of progress max -- are we confident that we always know the max, i.e., that we always have Content-Length or its equivalent?

There is one other detail that I think should also be added here. If the Background Update Task is downloading during this session, I think that we should lock the Update Service's ability to stage during this session. I'm concerned about the possibility that an extremely fast update download could cause the Update Service to start staging right before the Background Update Task shuts down. This could result in some unpleasant race conditions that I think are best avoided.

If we can do this, I would like to. The state machine of notifications feels like a train that's hard to get off the tracks to me, anything to have more station-stops sounds good.

(In reply to Nick Alexander :nalexander [he/him] from comment #2)

I was not aware of progress max -- are we confident that we always know the max, i.e., that we always have Content-Length or its equivalent?

We actually require that the file size be reported by the server, and we won't even download the file if the size reported isn't the size we expect. The only exception (which I'll be sure to handle in this implementation) is when BITS shows that the download job exists, but hasn't connected yet. During that time, the total file size is reported as -1. See here for details on all this.

Also, because the AppUpdater implementation gives very similar notifications here and here, I'll have to be careful in the implementation here that I check that those values are defined before I use them for anything.

The background update task will exit when downloading begins. We do not want the update process to proceed beyond downloading at this time, both for consistency and to prevent any potential race condiditions around staging while we are shutting down. The new nsIApplicationUpdateService.onlyDownloadUpdatesThisSession interface will allow us to prevent updates from proceeding past the "downloading" stage for the remainder of the session.

Depends on D110647

Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1e9a1bd16a6
Add AUS.onlyDownloadUpdatesThisSession r=nalexander
https://hg.mozilla.org/integration/autoland/rev/5273d9bd86d9
Test for AUS.onlyDownloadUpdatesThisSession r=nalexander
https://hg.mozilla.org/integration/autoland/rev/b95c7ed878ee
Exit the Background Update Task once a new update download has started r=nalexander
Regressions: 1707020
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: