Update agent - file download

ASSIGNED
Assigned to

Status

()

Toolkit
Application Update
P1
enhancement
ASSIGNED
11 months ago
8 days ago

People

(Reporter: mhowell, Assigned: rstrong)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

11 months ago
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.
(Reporter)

Comment 1

7 months ago
Created attachment 8875032 [details] [diff] [review]
Patch - Phase 1 update agent for Windows only
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Attachment #8875032 - Flags: review?(robert.strong.bugs)
Priority: -- → P2
(Reporter)

Comment 3

7 months ago
Created attachment 8882239 [details] [diff] [review]
Part 1 - Agent program
Attachment #8875032 - Attachment is obsolete: true
Attachment #8875032 - Flags: review?(robert.strong.bugs)
Attachment #8882239 - Flags: review?(robert.strong.bugs)
(Reporter)

Comment 4

7 months ago
Created attachment 8882240 [details] [diff] [review]
Part 2 - Convert update service downloader to use the agent on Windows, and an nsIHttpChannel elsewhere
Attachment #8882240 - Flags: review?(robert.strong.bugs)
Pushed to oak
Looks like there is a browser test failure on oak
Several test failures on oak
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
Attachment #8882240 - Flags: review?(robert.strong.bugs)

Comment 9

6 months ago
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.
(Reporter)

Comment 10

6 months ago
Yes, I can absolutely add a pref. Thanks for taking a look and pointing that out, Mark.
(Reporter)

Comment 11

6 months ago
Created attachment 8884385 [details] [diff] [review]
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.
Attachment #8882240 - Attachment is obsolete: true
Attachment #8884385 - Flags: review?(robert.strong.bugs)
(Reporter)

Comment 12

6 months ago
Created attachment 8884386 [details] [diff] [review]
Part 3 - Test changes
Attachment #8884386 - Flags: review?(robert.strong.bugs)
(Reporter)

Comment 13

6 months ago
Created attachment 8884388 [details] [diff] [review]
Part 1 - Agent program

Revised to hopefully resolve static analysis issues.
Attachment #8882239 - Attachment is obsolete: true
Attachment #8882239 - Flags: review?(robert.strong.bugs)
Attachment #8884388 - Flags: review?(robert.strong.bugs)
Pushed to oak
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.
(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.
It appears that manual updates don't get status and leave the UI in a bad state.
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.
Created attachment 8884599 [details] [diff] [review]
Part 1-2 - add manifest

Since I have this locally I am attaching it.
Created attachment 8884600 [details] [diff] [review]
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.
Attachment #8884385 - Attachment is obsolete: true
Attachment #8884385 - Flags: review?(robert.strong.bugs)
Created attachment 8884601 [details] [diff] [review]
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.
Attachment #8884386 - Attachment is obsolete: true
Attachment #8884386 - Flags: review?(robert.strong.bugs)
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.
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
Hey Robert, this is post-58, correct?
Flags: needinfo?(robert.strong.bugs)
Hi Alessio, this is for 58. Any particular reason this should be post 58?
Flags: needinfo?(robert.strong.bugs) → needinfo?(alessio.placitelli)
(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
Flags: needinfo?(alessio.placitelli) → needinfo?(robert.strong.bugs)
Flags: needinfo?(robert.strong.bugs)
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
(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.

Updated

3 months ago
Assignee: mhowell → robert.strong.bugs

Updated

3 months ago
Priority: P2 → P1

Updated

3 months ago
QA Contact: amasresha
(Reporter)

Updated

8 days ago
Attachment #8884388 - Flags: review?(robert.strong.bugs)
You need to log in before you can comment on or make changes to this bug.