Closed Bug 1257565 Opened 9 years ago Closed 6 years ago

Allow switching blocklist from XML to Remote Settings diff-based sync

Categories

(Toolkit :: Blocklist Implementation, defect, P2)

Product:
Toolkit
Component:
Blocklist Implementation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Project Flags:
Performance Impact low
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox68 --- fixed

People

(Reporter: leplatrem, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Keywords: perf, perf:responsiveness, Whiteboard: [fxperf:p2])

Attachments

(8 files, 11 obsolete files)

Bug 1257565 - switch gfx blocklist over to kinto-based storage,
59 bytes, text/x-review-board-request
alexical
: review+
leplatrem
: review+
Details
Bug 1257565 - wip - move plugin and addon stuff to kinto
59 bytes, text/x-review-board-request
Details
Bug 1257565 - move blocklist tests to their own dir so we can easily keep them working and separate from tests for the new implementation, r?aswan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1257565 - create a pref to phase out use of the XML blocklist, r?aswan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1257565 - switch gfx blocklist over to kinto-based storage, r?leplatrem!,aswan!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1257565 - add remote settings support for plugin and addon blocklist, r?aswan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1257565 - move addon blocklist tests from services/common to the blocklist dir, r?leplatrem!
47 bytes, text/x-phabricator-request
Details | Review
Node script to split addon/plugin blocklist xml files into json ones
4.48 KB, application/x-javascript
Details
No description provided.
Attached patch Load blocklists from JSON (obsolete) — — Splinter Review
See Also: → 1254099, 1224528, 1224531, 1227956
Attached patch Load blocklists from JSON (obsolete) — — Splinter Review
Attachment #8732120 - Attachment is obsolete: true
Comment on attachment 8743783 [details] Bug 1257565 - Refactor to (pre)load multiple files Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47673/diff/1-2/
Attachment #8743784 - Attachment description: MozReview Request: Bug 1257565 - Remove coupling of certificates invalidation and XML parsing → MozReview Request: Bug 1257565 - Remove coupling betweeen XML and Gfx
Comment on attachment 8743784 [details] MozReview Request: Bug 1257565 - Remove coupling betweeen XML and Gfx Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47893/diff/1-2/
Comment on attachment 8743785 [details] Bug 1257565 - Load addons/plugins/gfx blocklist from JSON files Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48003/diff/1-2/
Comment on attachment 8743786 [details] MozReview Request: Bug 1257565 - Import initial JSON files in browser folder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48005/diff/1-2/
Comment on attachment 8743787 [details] Bug 1257565 - Reload from disk when kinto blocklist was updated Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48007/diff/1-2/
Attachment #8743783 - Flags: review?(MattN+bmo)
Attachment #8743784 - Flags: review?(MattN+bmo)
Comment on attachment 8743783 [details] Bug 1257565 - Refactor to (pre)load multiple files https://reviewboard.mozilla.org/r/47673/#review53336 Could you flag mdeboer instead as I have too much else going on. ::: toolkit/mozapps/extensions/nsBlocklistService.js:725 (Diff revision 2) > } > > let telemetry = Services.telemetry; > > - if (this._isBlocklistPreloaded()) { > + // Check if preloaded content exists for this file. > + if (this._preloadedBlocklistContent.hasOwnProperty(file.path)) { `_preloadedBlocklistContent` should be a `Map`
Attachment #8743783 - Flags: review?(MattN+bmo)
Comment on attachment 8743784 [details] MozReview Request: Bug 1257565 - Remove coupling betweeen XML and Gfx https://reviewboard.mozilla.org/r/47893/#review53338 Perhaps mgoodwin or someone else could review this change instead?
Attachment #8743784 - Flags: review?(MattN+bmo)
Attachment #8743784 - Attachment is obsolete: true
Attachment #8743786 - Attachment is obsolete: true
Attachment #8743783 - Flags: review?(dtownsend)
Comment on attachment 8743783 [details] Bug 1257565 - Refactor to (pre)load multiple files I don't know much about nsBlocklistService.js so redirecting to Mossop who I think has touched this before. I'm not sure if the other commits are ready for review or not since they're not flagged.
Attachment #8743783 - Flags: review?(MattN+bmo)
Comment on attachment 8743783 [details] Bug 1257565 - Refactor to (pre)load multiple files https://reviewboard.mozilla.org/r/47673/#review118066 I think this looks ok but I'd like to go back over it once the rest of the queue is ready for review. ::: toolkit/mozapps/extensions/nsBlocklistService.js:704 (Diff revision 3) > > - /** > -# The blocklist XML file looks something like this: > -# > -# <blocklist xmlns="http://www.mozilla.org/2006/addons-blocklist"> > -# <emItems> > + _loadBlocklistFromFile(filename) { > + let file = FileUtils.getFile(KEY_PROFILEDIR, [filename]); > + if (!file.exists()) { > + let appFile = FileUtils.getFile(KEY_APPDIR, [filename]); > + if (appFile.exists()) { You should bail out if this doesn't exist. ::: toolkit/mozapps/extensions/nsBlocklistService.js:719 (Diff revision 3) > - this._loadBlocklistFromString(this._preloadedBlocklistContent); > - delete this._preloadedBlocklistContent; > - return; > + const text = this._preloadedBlocklistContent.get(file.path); > + delete this._preloadedBlocklistContent.delete(file.path); > + return text; > } > > if (!file.exists()) { We've already done this existance check above.
Attachment #8743783 - Flags: review?(dtownsend)
Attachment #8732256 - Attachment is obsolete: true
Assignee: nobody → mathieu
Attachment #8841936 - Attachment is obsolete: true
I took some time to rebase and rework this patch. This improvement may even be considered as a «quantum flow» bug: When enabled, loading from JSON files instead of XML from the main thread is a lot more efficient. Dave, if you have some bandwidth to review or give your feedback, that would be awesome :) Thanks a lot in advance!
Blocks: 1252456
Comment on attachment 8743783 [details] Bug 1257565 - Refactor to (pre)load multiple files https://reviewboard.mozilla.org/r/47673/#review135382 ::: toolkit/mozapps/extensions/nsBlocklistService.js:698 (Diff revision 6) > > - if (this._isBlocklistPreloaded()) { > + // Check if preloaded content exists for this file. > + if (this._preloadedBlocklistContent.has(file.path)) { > telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false); > - this._loadBlocklistFromString(this._preloadedBlocklistContent); > - delete this._preloadedBlocklistContent; > + const text = this._preloadedBlocklistContent.get(file.path); > + delete this._preloadedBlocklistContent.delete(file.path); You don't need the first delete here
Attachment #8743783 - Flags: review?(dtownsend) → review+
Comment on attachment 8743785 [details] Bug 1257565 - Load addons/plugins/gfx blocklist from JSON files https://reviewboard.mozilla.org/r/48003/#review135386 I'm a little concerned by this change. Switching from loading one file to loading three seems likely to cause a performance regression particularly if it happens during startup which it will sometimes. Have you done any performance measurements on this change? ::: toolkit/mozapps/extensions/nsBlocklistService.js:223 (Diff revision 8) > > /** > * Checks whether this blocklist element is valid for the current OS and ABI. > * If the element has an "os" attribute then the current OS must appear in > * its comma separated list for the element to be valid. Similarly for the > * xpcomabi attribute. Update this comment please ::: toolkit/mozapps/extensions/nsBlocklistService.js:226 (Diff revision 8) > * If the element has an "os" attribute then the current OS must appear in > * its comma separated list for the element to be valid. Similarly for the > * xpcomabi attribute. > */ > -function matchesOSABI(blocklistElement) { > - if (blocklistElement.hasAttribute("os")) { > +function matchesOSABI(os, xpcomabi) { > + if (os && os.split(",").indexOf(gApp.OS) < 0) { You can use array.includes here ::: toolkit/mozapps/extensions/nsBlocklistService.js:229 (Diff revision 8) > - if (blocklistElement.hasAttribute("os")) { > + if (os && os.split(",").indexOf(gApp.OS) < 0) { > - var choices = blocklistElement.getAttribute("os").split(","); > - if (choices.length > 0 && choices.indexOf(gApp.OS) < 0) > - return false; > + return false; > } > - > + if (xpcomabi && xpcomabi.split(",").indexOf(gApp.XPCOMABI) < 0) { And here ::: toolkit/mozapps/extensions/nsBlocklistService.js:1172 (Diff revision 8) > + continue; > + } > + let parsed; > + try { > + parsed = JSON.parse(content); > + this[dest] = parsed.data.map((entry) => handle(entry)).filter((entry) => !!entry); You shouldn't need the double negation here.
Comment on attachment 8743787 [details] Bug 1257565 - Reload from disk when kinto blocklist was updated https://reviewboard.mozilla.org/r/48007/#review135402 ::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_kinto.js:343 (Diff revision 8) > + > + // Send message. > + yield Services.cpmm.sendAsyncMessage("Blocklist:reload-from-disk"); > + > + // Since message is loaded asynchronously, wait for its reception. > + yield new Promise((resolve) => setTimeout(() => resolve(), 100)); Please use the blocklist-updated observer notification instead of a timeout.
Attachment #8743787 - Flags: review?(dtownsend)
(In reply to Mathieu Leplatre (:leplatrem) from comment #31) > I took some time to rebase and rework this patch. > > This improvement may even be considered as a «quantum flow» bug: When > enabled, loading from JSON files instead of XML from the main thread is a > lot more efficient. It's definitely something we care about for performance. Parsing the current XML file is very expensive. Here is a startup profile on Mac where _loadBlocklistFromString (ie. processing the content of the xml file once we are done reading it off the disk) is 7.1% of the time spent before the first browser window is shown: https://perfht.ml/2pPT7Gp Here a similar profile on Windows where it's 4.9% of the time spent before showing the first window: https://perfht.ml/2pPXu4t This code runs early during startup, so we should ensure we make it as fast as possible.
Whiteboard: [qf]
Waiting on a response to comment 38 here
Flags: needinfo?(mathieu)
Thanks Dave for your review! I took your comments into accounts. I'm sorry for the noise with moz-review here. I did something wrong while rebasing my local bookmarks and pushed a wrong version of the code. That explains the gap in revisions in interdiff :( Regarding performance, on my hardware I divide by 2 or 3 the time spent in parsing the blocklist. I didn't run a full perf test, I was just dumping the elapsed time in console: 30-40ms to parse the three JSON files against 90-130ms to parse one XML file. Since I have a SSD, the hard-drive latency might not be representative of most users' though. Florian would you be able to run the perf test with the perf ``security.blocklist.via.amo = false`` please? I am at your disposal (vidyo whatever) if you need more info from me :) Thanks!
Flags: needinfo?(mathieu)
Flags: needinfo?(florian)
Flags: needinfo?(dtownsend)
Depends on: 1359428
Depends on: 1359434
Comment on attachment 8743785 [details] Bug 1257565 - Load addons/plugins/gfx blocklist from JSON files https://reviewboard.mozilla.org/r/48003/#review136462 ::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_json.js:361 (Diff revision 10) > + blocklist._preloadedBlocklistContent.set(addonsAppPath, '{>invalid}'); > + > + blocklist._loadBlocklist(); > + > + // XXX: What should we do? Fallback on release file? Raise loudly? > + // Current behaviour with XML is to give up like this: Falling back to the release file is probably the right call, let's file a follow-up bug. ::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_json.js:374 (Diff revision 10) > + yield OS.File.writeAtomic(path, '{>invalid}', {tmpPath: path + ".tmp"}); > + > + const blocklist = Blocklist(); > + try { > + blocklist._loadBlocklist(); > + // XXX: What should we do? Fallback on release file? Raise loudly? Same here.
Attachment #8743785 - Flags: review?(dtownsend) → review+
Comment on attachment 8743787 [details] Bug 1257565 - Reload from disk when kinto blocklist was updated https://reviewboard.mozilla.org/r/48007/#review136472
Attachment #8743787 - Flags: review?(dtownsend) → review+
(In reply to Mathieu Leplatre (:leplatrem) from comment #49) > Florian would you be able to run the perf test with the perf > ``security.blocklist.via.amo = false`` please? Here is a profile https://perfht.ml/2pfP7MN showing 85ms spent during startup on my very fast macbook. 25ms are spent loading resource://services-common/blocklist-clients.js and its dependencies (I suspect an easy win could be to convert some of the Cu.import calls in there to lazy getters). It's not clear to me why the blocklist service is created very early using the profile-after-change category; this makes XPIProvider.jsm's lazy service getter a bit moot (http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/toolkit/mozapps/extensions/internal/XPIProvider.jsm#65)... but that probably doesn't change the total time much. 40ms are spent in _loadBlocklist (this was 76ms in the profile from comment 40 captured on the same machine). Maybe this is a naive question, but it's not clear to me why this needs to do more than a simple JSON.parse(). Isn't the point of using JSON here to have the data in a directly usable format? Finally there's 12ms spent in _getAddonBlocklistState. This could probably do with some optimization too. The loop at http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/toolkit/mozapps/extensions/nsBlocklistService.js#463 seems suspicious. But this isn't caused by your patches.
Flags: needinfo?(florian)
Comment on attachment 8743785 [details] Bug 1257565 - Load addons/plugins/gfx blocklist from JSON files https://reviewboard.mozilla.org/r/48003/#review136824 ::: toolkit/mozapps/extensions/nsBlocklistService.js:17 (Diff revision 10) > > Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > Components.utils.import("resource://gre/modules/Services.jsm"); > Components.utils.import("resource://gre/modules/AppConstants.jsm"); > > +const BlocklistClients = Components.utils.import("resource://services-common/blocklist-clients.js", {}); This import should be lazy
Please run: ./mach eslint toolkit/mozapps/extensions It gives 63 errors for me with the patches applied.
See Also: → 1357116
I probably should have removed my review flags yesterday after I saw Florian's discussion, I don't think this should have landed with open questions about the performance and implementation. By landing without the lazy getter changes I suspect we've regressed performance here. Can you also address Florian's question, particularly about why this isn't a simple JSON.parse?
Flags: needinfo?(dtownsend) → needinfo?(mathieu)
(In reply to Dave Townsend [:mossop] from comment #58) > I don't think this should have landed with open > questions about the performance and implementation. It doesn't seem to have landed yet. > By landing without the > lazy getter changes I suspect we've regressed performance here. There's some work happening in bug 1357116 to use lazy getters. > Can you also address Florian's question, particularly about why this isn't a > simple JSON.parse? Mathieu and I had a vidyo discussion today. One of the goal here was to have a drop-in replacement for the existing xml file, so most of the issues with the xml file (minus the time it takes to parse it) remain. We agreed to work together soon on designing an efficient new solution in a follow-up.
(In reply to Florian Quèze [:florian] [:flo] from comment #59) > (In reply to Dave Townsend [:mossop] from comment #58) > > I don't think this should have landed with open > > questions about the performance and implementation. > > It doesn't seem to have landed yet. Oops, without my morning caffeine I read the new patches as landings, my bad! > > By landing without the > > lazy getter changes I suspect we've regressed performance here. > > There's some work happening in bug 1357116 to use lazy getters. Excellent > > Can you also address Florian's question, particularly about why this isn't a > > simple JSON.parse? > > Mathieu and I had a vidyo discussion today. One of the goal here was to have > a drop-in replacement for the existing xml file, so most of the issues with > the xml file (minus the time it takes to parse it) remain. We agreed to work > together soon on designing an efficient new solution in a follow-up. Cool, thanks for doing that.
Flags: needinfo?(mathieu)
Comment on attachment 8861413 [details] Bug 1257565 - Perform HEAD request on XML blocklist when using JSON https://reviewboard.mozilla.org/r/133402/#review137038 ::: toolkit/mozapps/extensions/nsBlocklistService.js:211 (Diff revision 2) > + * @returns A Promise resolved on load or rejected on error. > + */ > +function fetchXML(uri, options = {}) { > + return new Promise((resolve, reject) => { > + const { method = "GET" } = options; > + LOG(`Blocklist::notify: Requesting ${method} ${uri}`); s/notify/fetchXML/ ::: toolkit/mozapps/extensions/nsBlocklistService.js:219 (Diff revision 2) > + request.channel.notificationCallbacks = new gCertUtils.BadCertHandler(); > + request.overrideMimeType("text/xml"); > + request.setRequestHeader("Cache-Control", "no-cache"); > + request.QueryInterface(Components.interfaces.nsIJSXMLHttpRequest); > + request.addEventListener("load", (event) => { > + const requestReponse = event.target; This variable is just request from the outer scope isn't it? No need to re-assign in that case. ::: toolkit/mozapps/extensions/nsBlocklistService.js:238 (Diff revision 2) > + resolve(requestReponse); > + }); > + request.addEventListener("error", (event) => { > + let requestReponse, status; > + try { > + requestReponse = event.target; Same again here ::: toolkit/mozapps/extensions/nsBlocklistService.js:677 (Diff revision 2) > - request.open("GET", uri.spec, true); > - request.channel.notificationCallbacks = new gCertUtils.BadCertHandler(); > - request.overrideMimeType("text/xml"); > - request.setRequestHeader("Cache-Control", "no-cache"); > - request.QueryInterface(Components.interfaces.nsIJSXMLHttpRequest); > - > + // We still do a HEAD request, since ADU metrics rely on this blocklist. > + if (gBlocklistFromXML) { > + fetchXML(uri.spec) > + .then(request => this.onXMLLoad(request)) > + .catch(error => { > + LOG("Blocklist:onError: There was an error fetching the blocklist\r\n" + s/onError/notify/ ::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_json.js:283 (Diff revision 2) > const blocklist = Blocklist(); > strictEqual(blocklist._isBlocklistLoaded(), false); > - // When managed with Kinto, nothing is loaded/downloaded on notify. > + // When managed with Kinto, blocklist.xml is reached with a HEAD request. > blocklist.notify(null); > strictEqual(blocklist._isBlocklistLoaded(), false); > }); You need to check that your path handler has actually been called.
Depends on: 1357116
Whiteboard: [qf] → [qf:p1]
Comment on attachment 8861413 [details] Bug 1257565 - Perform HEAD request on XML blocklist when using JSON https://reviewboard.mozilla.org/r/133402/#review137038 > Same again here I was not sure in this case, because of the ``requestReponse = request.channel.QueryInterface(Ci.nsIRequest);`` when getting the status throws.
Blocks: 1360576
Comment on attachment 8743785 [details] Bug 1257565 - Load addons/plugins/gfx blocklist from JSON files https://reviewboard.mozilla.org/r/48003/#review136462 > Falling back to the release file is probably the right call, let's file a follow-up bug. Created Bug 1360576
Comment on attachment 8861413 [details] Bug 1257565 - Perform HEAD request on XML blocklist when using JSON https://reviewboard.mozilla.org/r/133402/#review137832 ::: toolkit/mozapps/extensions/nsBlocklistService.js:237 (Diff revisions 2 - 5) > - } > + } > - resolve(requestReponse); > + } > + resolve(request); > }); > request.addEventListener("error", (event) => { > let requestReponse, status; Rename requestReponse to requestResponse please. ::: toolkit/mozapps/extensions/nsBlocklistService.js:218 (Diff revision 5) > + request.open(method, uri, true); > + request.channel.notificationCallbacks = new gCertUtils.BadCertHandler(); > + request.overrideMimeType("text/xml"); > + request.setRequestHeader("Cache-Control", "no-cache"); > + request.QueryInterface(Components.interfaces.nsIJSXMLHttpRequest); > + request.addEventListener("load", (event) => { The argument event is unnecessary now. ::: toolkit/mozapps/extensions/nsBlocklistService.js:236 (Diff revision 5) > + reject(new Error("Invalid XML")); > + } > + } > + resolve(request); > + }); > + request.addEventListener("error", (event) => { Same here.
Attachment #8861413 - Flags: review?(dtownsend) → review+
Status: NEW → ASSIGNED
Attachment #8879992 - Attachment is obsolete: true
Summary: Load blocklists from Kinto on startup → Get rid of blocklist.xml download in favor of Kinto diff-based sync
Does this still need bug 1359428?
Flags: needinfo?(mathieu)
I'm not sure anymore if this bug deserves the Quantum Flow tag or not. :kmag submitted a patch for Bug 1358907 which caused the addons database to be loaded too soon in the startup process. Loading the blocklist may now be out of the startup path. (The work going on here would still relevant but shouldn't affect performance as much if synchronous parsing is out of the main thread)
Flags: needinfo?(mathieu) → needinfo?(kmaglione+bmo)
(In reply to Mathieu Leplatre (:leplatrem) from comment #82) > I'm not sure anymore if this bug deserves the Quantum Flow tag or not. > > :kmag submitted a patch for Bug 1358907 which caused the addons database to > be loaded too soon in the startup process. Loading the blocklist may now be > out of the startup path. Telemetry still queries the plugins at startup, causing the blocklist to load before first paint. See bug 1371888.
Is this ready to land? It looks like these patches could speed things up in bug 1371888, but I can't tell if they're waiting for re-review or what.
Flags: needinfo?(mathieu)
This patch is not ready to land, because the approach has to be changed. Indeed parsing JSON is a lot faster than parsing XML. But on the other hand, we now have 3 JSON files with blocking I/O. It completely cancels out the benefits of switching to JSON. A better way to get rid of blocklist.xml would be: * To change the nsBlocklistService interface to asynchronous methods * Instead of loading the whole blocklist into memory, query the local database (requires async) It would also be a great opportunity to simplify and modernize that part of the code base. But as I explained to :florian a while ago, I don't have the skills/time for the first part. Especially because there are tons of tests to understand and rewrite. I can definitely help for the second part. If I remember well, :florian and :kmag had the intuition that loading blocklist could be done off the main thread, out of startup. Hence Bug 1371888 and Bug 1358907. I hope this helps :| What do you think?
Flags: needinfo?(mathieu)
There alot of work and risk involved with this bug hence I am moving it from qf:p1 to qf:p2 for post 57 work.
Whiteboard: [qf:p1] → [qf:p2]
Keywords: perf
As far as I know, this still affects start-up. Does comment 89 still apply, or is there time to work on this now?
Flags: needinfo?(mathieu)
Yes, comment 89 still applies. Naively, I would think that someone could take care of making the nsBlocklistService async, and then I could take over and get rid of the XML part (revamp the approach from the previous patches). I also would like to hear what :florian and :kmag think about this approach (and about comments from Bug 1371888)
Flags: needinfo?(mconley)
Flags: needinfo?(mathieu)
Flags: needinfo?(florian)
Okay, waiting on feedback from kmag / florian.
Flags: needinfo?(mconley)
Whiteboard: [qf:p2] → [qf:p1]
Whiteboard: [qf:p1] → [qf:p1] [fxperf]
Redirecting my needinfo to Gijs who works on blocklist perf problems these days.
Flags: needinfo?(florian) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P2
Depends on: 1439519
I think the long-term plan here is to wean plugins and gfx off sync access to the blocklist, then create async access APIs, update the callsites, remove the sync access APIs, and then switch to JSON. I've tried to start the plugins stuff in bug 1371888.
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [qf:p1] [fxperf] → [qf:p1] [fxperf:p2]
See Also: → 1447332
Flags: needinfo?(kmaglione+bmo)
Depends on: 1447680
See Also: → 1458917
See Also: → 1458920
We discussed with Gijs about the next steps. Basically they would be: - Get rid of XML download and parsing - Keep a HEAD request on the XML endpoint (ADI count) (see Bug 1359434, and about Socorro dropping ADI integration https://bugzilla.mozilla.org/show_bug.cgi?id=1369498#c19) - Query local data with RemoteSettings API now that get entry/state methods are async in Blocklist (done in Bug 1447680) - Rewrite all the tests that rely on XML content :( - Fix up (or remove) the data copy in https://dxr.mozilla.org/mozilla-central/rev/f877359308b17e691209e1afb7193b8e86f175ce/toolkit/mozapps/extensions/AddonManager.jsm#565-639 - RemoteSettings .get() filtering by property value might have to be improved, based on the required blocklist lookups (:leplatrem or :glasserc would be in charge) - :leplatrem will implement client-side filtering of target apps within the RemoteSettings API in order to mimic what was done on the server in Bug 1434302. See Bug 1458920 Related tasks / Stretch goals: - Get rid of JSON dumps in profiles (useless now that we want to use indexeddb from nsBlocklistService). Basically remove this https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/services/common/blocklist-clients.js#112-128 - Currently, the RemoteSettings polling is done from nsBlocklistService::notify (for no good reason appart lack of skills in Gecko/XPCOM). It would be better to decorelate the two, and let the RemoteSettings polling have its own "schedule". See Bug 1458917 - Currently, the RemoteSettings polling requires that each consumer makes a prelimary call to RemoteSettings("key") to be aware of the collections to be synchronized. See Bug 1454970 - Bot to update JSON dumps in source tree. See Bug 1451040
Assignee: mathieu → gijskruitbosch+bugs
Blocks: 1425602
Depends on: 1458920
See Also: 1458920 →
Attachment #8861413 - Attachment is obsolete: true
Attachment #8743787 - Attachment is obsolete: true
Attachment #8743785 - Attachment is obsolete: true
Attachment #8743783 - Attachment is obsolete: true
Just to clarify a bit now that I've attached a first patch... I intend to migrate 1 consumer per commit (so gfx first, and plugins + addons in 1 commit each), fixing their tests in the same commit (which in a way is the largest "bit"). Then I expect I'll do some smaller/easier cleanup commits to tidy up some loose ends (e.g. hopefully removing blocklist-clients.js, maybe the HEAD request for the XML file in its own commit, potentially removing this bit ( https://dxr.mozilla.org/mozilla-central/rev/380cf87c1ee3966dd94499942b73085754dc4824/toolkit/mozapps/extensions/AddonManager.jsm#562 ) of AM code, maybe removing the in-profile JSON dumping if that isn't done before then, etc.) Given the short cycle (soft code freeze is while we're all at all hands, and I have 7 days of PTO before then, too, so practically speaking there's just over 10 working days left for me) in I'm on the fence about whether I will manage to write it all + get reviews in time for this to ride 62. I don't want to have a pref because I don't want to maintain 2 implementations any longer than necessary, and I don't think it's a good idea for us to keep/have half the stuff using kinto, half of it using the XML file. I'm uploading the first of these patches now even if I won't land it without the others (as yet not finished) because I think it'll help hash out approaches with reviewers, and I expect to need a succession of try pushes to get any test bustage sorted out anyway (some of these tests aren't marked as disabled on opt/mac/whatever in the manifest, but silently exit early without doing any testing on some platforms/configurations, so that's a pain to figure out without just pushing to try and seeing what breaks). Given that I want to land everything in 1 go, I figured doing it in here rather than separate deps seemed easier. Doug, let me know if you would like me to find a different reviewer for this stuff, but I figured you might be interested. :-) For this particular patch, I think things should be reasonably straightforward. The gfx code currently depends on the blocklist being created through other callsites and then accidentally notifying gfx that it's got gfx blocklist data. I'm trying to make that a bit more explicit because in kinto the gfx data is in its own collection anyway. A potential follow-up could be not actually calling .get() and checking the entries (and reporting them to the gfxinfo stuff) if nothing's changed (instead of always doing it in an idle task off startup), as well as teaching kinto about the platform interpretations so it can filter out relevant entries itself. However, I think that needs some discussion with the gfx team and for this bug I'd like to keep things as straightforward as possible - there are enough moving parts as it is.
Comment on attachment 8976265 [details] Bug 1257565 - switch gfx blocklist over to kinto-based storage, https://reviewboard.mozilla.org/r/244450/#review250456 I am impressed, this is finally happening! GG :clap: ::: services/common/blocklist-clients.js:217 (Diff revision 1) > signerName: Services.prefs.getCharPref(PREF_BLOCKLIST_PINNING_SIGNER), > }); > PinningBlocklistClient.on("sync", updatePinningList); > } > + > +let KintoBlocklist = { I would like to avoid mentioning Kinto if possible. And use RemoteSettings instead :) (kinto is the underlying tech, remote settings is the service ;)) ::: toolkit/mozapps/extensions/Blocklist.jsm:91 (Diff revision 1) > + signerName: Services.prefs.getCharPref(PREF_BLOCKLIST_GFX_SIGNER), > + filterFunc: KintoBlocklist.targetAppFilter, > + }); > + this._client.on("sync", v => { > + this.checkForEntries(); > + KintoBlocklist.updateJSONBlocklist(this._client, v); Quick note: actually dumping JSON on disk became useless now. It could be tackled in another patch though. ::: toolkit/mozapps/extensions/Blocklist.jsm:120 (Diff revision 1) > + if (typeof val == "string") { > + val = trim(val); > + } else if (p == "devices") { > + let invalidDevices = []; > + let validDevices = []; > + val.forEach(v => v.includes(",") ? invalidDevices.push(v) : validDevices.push(v)); I imagine that you ported some old code, but in the current blocklist dataset, no entry seems affected. ``` curl https://firefox.settings.services.mozilla.com/v1/buckets/blocklists/collections/gfx/records | jq '.data[] | .devices' ``` ::: toolkit/mozapps/extensions/Blocklist.jsm:140 (Diff revision 1) > + maxVersion: trim(entry.versionRange.maxVersion) || "*", > + }; > + } > + return rv; > + }); > + this._entries = entries; Is this used in tests only? Maybe worth a comment ;) ::: toolkit/mozapps/extensions/Blocklist.jsm:162 (Diff revision 1) > + } > + return key + ":" + value; > + }).join("\t"); > + }).join("\n"); > + Services.obs.notifyObservers(null, "blocklist-data-gfxItems", payload); > + }, Now that the code is all gathered in one place, I think it could almost make sense to merge `checkForEntries` and `notifyEntries`. Appart from unit tests, the list of "post-processed" entries is not used anywhere else. ::: toolkit/mozapps/extensions/Blocklist.jsm:430 (Diff revision 1) > break; > } > }, > > + checkForGfxBlocklistItems() { > + GfxBlocklist.init(); I found this method name a bit confusing, since the init() only registers the sync event handler, and does not explicitly check anything.
Attachment #8976265 - Flags: review?(mathieu)
(In reply to Mathieu Leplatre (:leplatrem) from comment #100) > I am impressed, this is finally happening! GG :clap: Concur. Beers are owed.
Comment on attachment 8976265 [details] Bug 1257565 - switch gfx blocklist over to kinto-based storage, https://reviewboard.mozilla.org/r/244450/#review250480 ::: browser/components/nsBrowserGlue.js:1229 (Diff revision 1) > LanguagePrompt.init(); > }); > > Services.tm.idleDispatchToMainThread(() => { > Blocklist.loadBlocklistAsync(); > + Blocklist.checkForGfxBlocklistItems(); Drive-by nit: Having multiple operations in a single idle callback isn't really ideal, since they can overrun the idle slice more easily that way. These should really all be using ChromeUtils.idleDispatch, which, aside from being cheaper, actually guarantees all callbacks have a 50ms idle slice available before running them. ::: toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js:72 (Diff revision 1) > // have processed the gfxItems event. > executeSoon(checkBlacklist); > }, "blocklist-data-gfxItems"); > > - load_blocklist("test_gfxBlacklist.xml"); > + let scope = {}; > + Services.scriptloader.loadSubScript(Services.io.newFileURI(do_get_file("data/test_gfxBlacklist.js")).spec, scope); Any reason these can't just be JSON that you can fetch()?
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #102) > toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js:72 > > + Services.scriptloader.loadSubScript(Services.io.newFileURI(do_get_file("data/test_gfxBlacklist.js")).spec, scope); > > Any reason these can't just be JSON that you can fetch()? The original test code loaded the XML sync, which is why I went this route, but I think you're right and because of the observer pattern it uses, it can just fetch them async, making it easier to use JSON. (In reply to Mathieu Leplatre (:leplatrem) from comment #100) > > + KintoBlocklist.updateJSONBlocklist(this._client, v); > > Quick note: actually dumping JSON on disk became useless now. It could be > tackled in another patch though. I think I can just remove it here, then - the services/common/tests/unit/test_blocklist_clients.js test will need updating for that, but it already needs fixing anyway. Because it checks all the clients I think I'll do that once I've migrated the other clients as well (and move it into the toolkit/mozapps/extensions/ dir because that's where the code-under-test will live). It's orange right now, so I can't forget. :-) > ::: toolkit/mozapps/extensions/Blocklist.jsm:120 > > + } else if (p == "devices") { > > + let invalidDevices = []; > > + let validDevices = []; > > + val.forEach(v => v.includes(",") ? invalidDevices.push(v) : validDevices.push(v)); > > I imagine that you ported some old code, but in the current blocklist > dataset, no entry seems affected. This is a safety feature that I ported from the old code, yes. We serialize the blocklist results to the gfx C++ code in some kind of ad-hoc text/plain \n, \t and comma-separated list. Because of this, the device names themselves can't contain commas - that would break the serialization/deserialization. I think the code is here to avoid clients breaking if someone ever puts the wrong type of data in the blocklist - and there are tests verifying this behavior, so I've kept it, even if we'll never hit it with the current values in the blocklist. > ::: toolkit/mozapps/extensions/Blocklist.jsm:140 > > + this._entries = entries; > > Is this used in tests only? Maybe worth a comment ;) Yes. Per your other comment about merging the methods, I've done that and now I just return `entries` from there, and make the test catch that, and added a comment to that effect. > ::: toolkit/mozapps/extensions/Blocklist.jsm:430 > (Diff revision 1) > > break; > > } > > }, > > > > + checkForGfxBlocklistItems() { > > + GfxBlocklist.init(); > > I found this method name a bit confusing, since the init() only registers > the sync event handler, and does not explicitly check anything. I've updated the naming a bit and just made the entry check method initialize things if required. This will probably be closer to how the plugins/addons code will want to work, too, so that's good.
Comment on attachment 8976265 [details] Bug 1257565 - switch gfx blocklist over to kinto-based storage, https://reviewboard.mozilla.org/r/244450/#review250666 ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1164 (Diff revision 2) > +async function mockGfxBlocklistItems(items) { > + let bsPass = ChromeUtils.import("resource://gre/modules/Blocklist.jsm", {}); > + bsPass.GfxBlocklist._mockedItems = items; > + let rv = await bsPass.GfxBlocklist.checkForEntries(); > + delete bsPass.GfxBlocklist._mockedItems; > + return rv; Instead of having this custom `_mockedItems`, another approach would be to fill the local storage with fake data. It would be done like this: ```js const client = RemoteSettings("gfx", { bucketName: "blocklists" }); const collection = await client.openCollection(); await collection.clear(); await collection.loadDump(items); ``` I don't say that you should change it, but at least wanted to make sure that you were aware of this possibility :)
Attachment #8976265 - Flags: review?(mathieu) → review+
Comment on attachment 8976265 [details] Bug 1257565 - switch gfx blocklist over to kinto-based storage, https://reviewboard.mozilla.org/r/244450/#review250944 Hooray! Looks good to me. ::: toolkit/mozapps/extensions/Blocklist.jsm (Diff revision 1) > - // <driverVersionComparator> LESS_THAN_OR_EQUAL </driverVersionComparator> > - // <model>foo</model> > - // <product>foo</product> > - // <manufacturer>foo</manufacturer> > - // <hardware>foo</hardware> > - // </gfxBlacklistEntry> Nit: I kind of find this example entry handy when looking at the code that processes it. Was it a decision to not bring it over de-XML-ified, or did it just not survive the translation?
Attachment #8976265 - Flags: review?(dothayer) → review+
I'm progressing with moving the plugin code to kinto... First, a basic question: How do I find out what the equivalent fields are for plugin/addon entries, esp. wrt capitalization and so on? It seems the current plugin entries do not have this field, so it's hard to know how it gets serialized. I expect there may be other such fields... Otherwise, it seems it's a bit tricky because of the blocklist-updated notification. There used to be just 1 update notification, but now the add-on, gfx and plugin ones get updated separately. This was fine for gfx, but the observers of blocklist-updated are going to be unhappy. So I'm splitting them out to care about what they ought to care about, and having 2 different notification topics. That would be fine, except for the blocklist notification dialogs. Those currently get a combination of blocklisted add-ons and plugins. I haven't examined the code in detail yet, but I expect that I may need to write some shenanigans to deal with more blocklisted things coming in while the dialog is already up, or something. If people have clever ideas about how to do this I'm all ears.
Flags: needinfo?(mathieu)
(In reply to :Gijs (he/him) from comment #107) > That would be fine, except for the blocklist notification dialogs. Those > currently get a combination of blocklisted add-ons and plugins. We don't show blocklist dialogs for extensions anymore. They were only ever showed for extensions that required a browser restart in order to disable. Those don't exist anymore.
> How do I find out what the equivalent fields are for plugin/addon entries The XML file is generated from those JSON entries, so we should be fine. You can check out the code if there are any doubts. The only tricky transformation I found was this one: https://github.com/mozilla-services/amo2kinto/blob/f8273a732e3afa914c3f9539c655ec8b9408d68e/amo2kinto/exporter.py#L182-L194 > There used to be just 1 update notification, but now the add-on, gfx and plugin ones get updated separately. The alternative is to listen to the ``remote-settings-changes-polled`` event. It will be sent everytime we poll for changes, and not only when changes were found. But we could enrich it and add a payload with the list of collections that were updated or something like that if that's necessary. Have the same debounced listener on addons + plugins sync could do the trick, but that would be hackish :/
Flags: needinfo?(mathieu)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #108) > (In reply to :Gijs (he/him) from comment #107) > > That would be fine, except for the blocklist notification dialogs. Those > > currently get a combination of blocklisted add-ons and plugins. > > We don't show blocklist dialogs for extensions anymore. They were only ever > showed for extensions that required a browser restart in order to disable. > Those don't exist anymore. I'm don't see that implemented. Maybe I'm missing something? As far as I can tell, once the blocklist updates, we run this method: https://dxr.mozilla.org/mozilla-central/rev/51f2535c797495f1f4e864072c2449b7c28669de/toolkit/mozapps/extensions/Blocklist.jsm#1300 which at the end of the add-ons loop adds items to `addonList` (which AIUI will happen if they changed from not-blocked to hard-blocked), then calls AddonManagerPrivate.updateAddonAppDisabledStates() (which can't alter `addonList`), then checks plugins (adding new blocked entries to `addonList`, too), then (if `addonList` is non-empty), checks for a component we only implement on mobile (and is scheduled to be removed in bug 1462796) and then opens the blocklist dialog https://dxr.mozilla.org/mozilla-central/rev/51f2535c797495f1f4e864072c2449b7c28669de/toolkit/mozapps/extensions/Blocklist.jsm#1476-1479 .
Flags: needinfo?(kmaglione+bmo)
(In reply to :Gijs (he/him) from comment #110) > (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're > blocked) from comment #108) > > (In reply to :Gijs (he/him) from comment #107) > > > That would be fine, except for the blocklist notification dialogs. Those > > > currently get a combination of blocklisted add-ons and plugins. > > > > We don't show blocklist dialogs for extensions anymore. They were only ever > > showed for extensions that required a browser restart in order to disable. > > Those don't exist anymore. > > I'm don't see that implemented. Maybe I'm missing something? As far as I can > tell, once the blocklist updates, we run this method: See https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/toolkit/mozapps/extensions/Blocklist.jsm#1350-1360 We only notify if the add-on is still active after updating its blocklist state, which is never the case for restartless add-ons. Yes, that means a huge chunk of this method is dead code.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1463782
Whiteboard: [qf:p1] [fxperf:p2] → [qf:p1:f64] [fxperf:p2]
Unfortunately finishing this work is blocked on concerns about how to decom the existing blocklist server in a sustainable fashion, so I'm going to unassign to get this out of my queue.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
Depends on: 1469525
(In reply to :Gijs (he/him) from comment #115) > Unfortunately finishing this work is blocked on concerns about how to decom > the existing blocklist server in a sustainable fashion, so I'm going to > unassign to get this out of my queue. Why is that a blocker?
See Also: → 1471522
Just checkpointing this after rebasing across bug 1454378...
(In reply to Kris Maglione [:kmag] from comment #116) > (In reply to :Gijs (he/him) from comment #115) > > Unfortunately finishing this work is blocked on concerns about how to decom > > the existing blocklist server in a sustainable fashion, so I'm going to > > unassign to get this out of my queue. > > Why is that a blocker? Essentially, the blocklist server feeds into metrics - metrics we've had for literally more than 10 years, and which are apparently being relied on by more teams in Mozilla than one might expect. We're moving people away from this metric but of course this takes time. In the meantime we're reluctant to change things in the client where it might impact that metric. Of course, we need to balance this with the fact that, from some quick checks, it seems fixing this patch gives us a 10ms win (on average, warm startup - cold startup is likely more!) on the reference device. 10ms is a pretty significant win for startup, and we should really try to get this moving again as soon as we can.
Summary: Get rid of blocklist.xml download in favor of Kinto diff-based sync → Get rid of blocklist.xml download in favor of Remote Settings diff-based sync
Pushing this out to target Firefox 67 for now - though hopefully it happens sooner.
Whiteboard: [qf:p1:f64] [fxperf:p2] → [qf:p1:f67][fxperf:p2]
Component: Blocklist Policy Requests → Blocklist Implementation
Flags: needinfo?(mconley)
Whiteboard: [qf:p1:f67][fxperf:p2] → [qf:p3:responsiveness][fxperf:p2]
Flags: needinfo?(mconley)
See Also: → 1534603

Glutton for punishment, I guess...

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

MozReview-Commit-ID: AiGycyhGUta

Depends on D29833

Depends on: 1549548
Depends on: 1549550
Summary: Get rid of blocklist.xml download in favor of Remote Settings diff-based sync → Allow switching blocklist from XML to Remote Settings diff-based sync
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9ba2aa4a6143 move blocklist tests to their own dir so we can easily keep them working and separate from tests for the new implementation, r=aswan https://hg.mozilla.org/integration/autoland/rev/f960290161ab create a pref to phase out use of the XML blocklist, r=aswan https://hg.mozilla.org/integration/autoland/rev/77ec8a41ee73 switch gfx blocklist over to kinto-based storage, r=leplatrem,aswan https://hg.mozilla.org/integration/autoland/rev/c4f071c98871 add remote settings support for plugin and addon blocklist, r=aswan https://hg.mozilla.org/integration/autoland/rev/bb4e78986eed move addon blocklist tests from services/common to the blocklist dir, r=leplatrem
Depends on: 1549858
Depends on: 1557790
Depends on: 1559353
Depends on: 1580353
Depends on: 1602655
See Also: → 1714673
Performance Impact: --- → P3
Keywords: perf:responsiveness
Whiteboard: [qf:p3:responsiveness][fxperf:p2] → [fxperf:p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: