1343671 - Update agent - file download

Status: RESOLVED WONTFIX

(Toolkit :: Application Update, enhancement, P3)

Description

[Molly Howell (she/her) (no longer active)]

The update agent needs to be able to download update files in the background, such that they can be resumed if the agent crashes/is killed or if the network drops out.

Comment 1

[Molly Howell (she/her) (no longer active)]

Attachment: Patch - Phase 1 update agent for Windows only

Comment 2

[Molly Howell (she/her) (no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99fb87225c40bd9e9a668bb6a25211789c18473

Comment 3

[Molly Howell (she/her) (no longer active)]

Attachment: Part 1 - Agent program

Comment 4

[Molly Howell (she/her) (no longer active)]

Attachment: Part 2 - Convert update service downloader to use the agent on Windows, and an nsIHttpChannel elsewhere

Comment 5

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

Pushed to oak

Comment 6

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

Looks like there is a browser test failure on oak

Comment 7

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

Several test failures on oak

Comment 8

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

Comment on attachment 8882240 [details] [diff] [review]
Part 2 - Convert update service downloader to use the agent on Windows, and an nsIHttpChannel elsewhere

Talked with mhowell on irc and he is going to resubmit with fixes for the tests

Comment 9

[Mark Smith [:mcs]]

Can you please provide a preference to allow use of the download agent to be disabled? I did not see support for that in the patches, although I may have missed it. In Tor Browser, we won't be able to easily use an external agent for downloads because we need to have the download use the Tor Network.

Comment 10

[Molly Howell (she/her) (no longer active)]

Yes, I can absolutely add a pref. Thanks for taking a look and pointing that out, Mark.

Comment 11

[Molly Howell (she/her) (no longer active)]

Attachment: Part 2 - Convert update service downloader to use the agent on Windows, and an nsIHttpChannel elsewhere

Changes in this revision:

1) Hopefully fixed test failures by resolving the following issues:
  a) Forgot to include updateagent.exe in the package manifest.
  b) Fixed broken logic for finding updateagent.exe at runtime.
  c) Chnaged chrome and browser tests to download MAR's from 127.0.0.1 instead of from example.com, since the agent doesn't get the proxy magic that makes that address work.
2) Separated test changes into a third patch (coming up right after this one).
3) Added a pref app.update.agent.enable which defaults to true.

Comment 12

[Molly Howell (she/her) (no longer active)]

Attachment: Part 3 - Test changes

Comment 13

[Molly Howell (she/her) (no longer active)]

Attachment: Part 1 - Agent program

Revised to hopefully resolve static analysis issues.

Comment 14

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

Pushed to oak

Comment 15

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

A couple of findings while testing this.

Trying to download a mar that was previously downloaded and then deleted doesn't download the mar and the updateagent.exe returns 0. I also tried to do this directly from the command line with the same result.

I had to compile updateagent.exe with a manifest otherwise it requires elevation likely due to "update" being in the name.

Running this from the app dir will prevent uninstall. Perhaps copy it to the updates root dir and run it from there. I think there will need to be code in the uninstaller to kill the process for this case.

Why didn't you go with a scheduled task? That way it will resume after an OS reboot.

Comment 16

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15)
> A couple of findings while testing this.
> 
> Trying to download a mar that was previously downloaded and then deleted
> doesn't download the mar and the updateagent.exe returns 0. I also tried to
> do this directly from the command line with the same result.
> 
> I had to compile updateagent.exe with a manifest otherwise it requires
> elevation likely due to "update" being in the name.
> 
> Running this from the app dir will prevent uninstall. Perhaps copy it to the
> updates root dir and run it from there. I think there will need to be code
> in the uninstaller to kill the process for this case.
With code in the uninstaller to kill the process copying it wouldn't be necessary.

Comment 17

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

It appears that manual updates don't get status and leave the UI in a bad state.

Comment 18

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

BTW: changing the download url allowed me to download a different mar. This time deleting it allowed me to redownload it so I suspect I ended up putting BITS into a weird state. Would be a good thing to have a fallback for this case.

Comment 19

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

Attachment: Part 1-2 - add manifest

Since I have this locally I am attaching it.

Comment 20

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

Attachment: Part 2 - Add update service downloader for the agent on Windows

Since I split out the conversion of the nsIIncrementalDownloader to use nsIHttpChannel into bug 1348087 I updated this patch for the removal. I also fixed the pref in firefox.js while I was doing this.

Comment 21

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

Attachment: Part 3 - Test changes

I changed a couple of the tests so they don't use the downloader. I think it might be better not to rename downloadInterruptedRecovery.js and to have a separate directory for the agent tests.

Comment 22

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

I've had some time to look into BITS and realized that a scheduled task isn't needed for this.

Some of my notes:
* Verify BITS can be used (possibly with the XPCOM component mentioned later) and fallback to using Firefox app update download code if not.
* On non-transient BITS error fallback to using Firefox download code. When Firefox isn't running this could be accomplished by setting the status file to an error code to denote that BITS should not be used. nsUpdateService,js reads this on startup and would then do the right thing including recording this state so it knows whether to use it in the future. Will likely need retries as well.
* What should be done for transient BITS errors when running Firefox and UI is displayed? Possibly a message that the download will continue shortly? Might want to try to continue the BITS download if possible.
* Download should be async so it is possible to continue download after Firefox exits.
* Add an XPCOM component that creates BITS jobs, connects to BITS job and uses callbacks to get transfer completed (no UI) and bytes transferred (UI). This will need a Job name that is well known and unique for each installation possibly using the CityHash id that is also used for TaskBar IDs and for the updates directory.
* Background downloads should be low priority and downloads with UI should be foreground priority. If possible (I think it is) the priority should be adjusted based on whether there is UI shown. I believe we should continue at foreground priority if UI has been shown since there is a user expectation at that point. The XPCOM component can be used to accomplish this.

Comment 23

[Bruno Marsal (Bullja)]

If BITS is used, maybe add an option disabling downloading updates when the device is on a metered connection. See: How to control whether a BITS job is allowed to download over an expensive connection - https://msdn.microsoft.com/en-us/library/windows/desktop/hh994437(v=vs.85).aspx

Comment 24

[Alessio Placitelli [:Dexter]]

Hey Robert, this is post-58, correct?

Comment 25

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

Hi Alessio, this is for 58. Any particular reason this should be post 58?

Comment 26

[Alessio Placitelli [:Dexter]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #25)
> Hi Alessio, this is for 58. Any particular reason this should be post 58?

This has some impact on the "Internet Connection Quality" measurement project [1], unless we find a way to gather some statistics from BIT.
Maybe we can find a way to collect the same data using BITS? That would be ideal, since that's the future for Windows AFAICT.
The crucial bits of information we would need are:

- Is the update download being throttled?
- The amount of data transferred every X seconds and at the end of the download.

[1] - https://docs.google.com/document/d/1f3_E4k4cUbhuNb2Nr2mxlCNJCAUKZrifSwjVOlTLP20/edit#heading=h.o0xb26mxhwoo

Comment 27

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

Agreed with this impacting that project as we discussed on July 12th when I first heard about this project and I informed you that using the "update blob might not be the best choice".

The update will be throttled depending on whether UI is being shown or not.

I'm just learning about BITS as I go along and docs are available on msdn.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa362708(v=vs.85).aspx

Comment 28

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #26)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #25)
> > Hi Alessio, this is for 58. Any particular reason this should be post 58?
> 
> This has some impact on the "Internet Connection Quality" measurement
> project [1], unless we find a way to gather some statistics from BIT.

Let me be more explicit on this, as I now understand that it might have been a bit confusing: the update agent change is *not blocking* ICQ nor it should be held back because of ICQ.

I only wanted to understand the schedule of this and if it would still be possible to get some statistics about the update download.

Comment 29

[Dave Townsend [:mossop]]

With the updated approach for the background update agent I don't think this is relevant anymore. Please re-open if I am mistaken.