Fix some issues in the update service code

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

5 months ago
Bug 1514210 exposed a few things here.
Assignee

Comment 1

5 months ago
The code currently accesses it using getProperty("backgroundInterval"), but it
really is just a property like the others so we should define and use it like
that. This is needed for the next patch.
Assignee

Comment 2

5 months ago
If we QI nsIPropertyBag on an XPCWrappedNative wrapping an XPCWrappedJS, calling
the getProperty method might incorrectly end up calling .getProperty on the
XPCWrappedJS itself, because it also implements nsIPropertyBag.

This became a problem with same-compartment chrome globals because if we no longer
cross a compartment boundary, we don't create a new WrappedNative and can now end up
seeing the nsIPropertyBag if it got queried before nsIWritablePropertyBag.

Depends on D15076
Assignee

Comment 3

5 months ago
I'm happy to remove the backgroundInterval property completely, but do you mind explaining why we can do this now? Then I can add that to the commit message :)
Flags: needinfo?(robert.strong.bugs)
Assignee

Comment 4

5 months ago
Also what should this code look like? Do we always want to use DOWNLOAD_FOREGROUND_INTERVAL or should we keep the ternary somehow?

https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/mozapps/update/nsUpdateService.js#3556-3557
(In reply to Jan de Mooij [:jandem] from comment #4)
> Also what should this code look like? Do we always want to use
> DOWNLOAD_FOREGROUND_INTERVAL or should we keep the ternary somehow?
> 
> https://searchfox.org/mozilla-central/rev/
> 9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/mozapps/update/
> nsUpdateService.js#3556-3557
Remove both
const DOWNLOAD_BACKGROUND_INTERVAL  = 600; // seconds
const DOWNLOAD_FOREGROUND_INTERVAL  = 0;

and for the code here
https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/mozapps/update/nsUpdateService.js#3556-3557

Just set interval to 0 with a comment that "The interval is 0 since there is no need to throttle downloads." or something similar.
(In reply to Jan de Mooij [:jandem] from comment #3)
> I'm happy to remove the backgroundInterval property completely, but do you
> mind explaining why we can do this now? Then I can add that to the commit
> message :)
For the commit message something like
Removed backgroundInterval. It was added in case there were CDN issues with downloading unthrottled and since there haven't been any issues it is no longer needed.

backgroundInterval was originally added because we overloaded the download mirrors (provided mainly by universities) we had way back when and at some point we changed to using the CDN we have today. We wanted to move to an interval of 0 to improve update rates but there were some concerns that this might have issues as well so we went with the ability to configure it via the update advertisement. This is why it was added to the update xml provided by the update server. Since there haven't been any issues we got the ok to remove it and go with a value of 0. The plan was to remove it when the code was rewritten to use standard networking code but that isn't resourced as of yet.
Flags: needinfo?(robert.strong.bugs)
If you want to just remove it from nsIUpdate. Then we'll remove it entirely when the networking code is changed to use standard networking code instead of nsIIncrementalDownload
Attachment #9032646 - Attachment description: Bug 1515611 part 1 - Add backgroundInterval property to nsIUpdate. r?rstrong → Bug 1515611 part 1 - Remove backgroundInterval from nsIUpdate. r?rstrong
Assignee

Comment 8

5 months ago
Ok thanks. I had already written the patch to remove it earlier today so I just submitted that with some tweaks based on your suggestions.

Comment 9

5 months ago
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dab6389dca55
part 1 - Remove backgroundInterval from nsIUpdate. r=rstrong
https://hg.mozilla.org/integration/autoland/rev/74bbbdf69467
part 2 - QI nsIWritablePropertyBag instead of nsIPropertyBag on nsIUpdate. r=rstrong

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dab6389dca55
https://hg.mozilla.org/mozilla-central/rev/74bbbdf69467
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.