Closed
Bug 1515611
Opened 6 years ago
Closed 6 years ago
Fix some issues in the update service code
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
Bug 1514210 exposed a few things here.
Assignee | ||
Comment 1•6 years 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•6 years 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•6 years 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•6 years 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
Comment 5•6 years ago
|
||
(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.
Comment 6•6 years ago
|
||
(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)
Comment 7•6 years ago
|
||
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
Updated•6 years ago
|
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•6 years 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.
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dab6389dca55 https://hg.mozilla.org/mozilla-central/rev/74bbbdf69467
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•