It would be desirable if the kinto version of the OneCRL Certificate blocklist did not need to fetch initial data from the kinto server. The blocklist.xml used by the AMO version of OneCRL is shipped with products; we need a similar mechanism which allows initial versions to be read from a user's profile.

This makes use of the kinto.js loadDump feature to populate the OneCRL collection with initial data from application defaults. This also includes a modified moz-kinto-client.js because there was an issue with the loadDump implementation in the FirefoxStorage adapter that caused breakage with empty collections. Review commit: https://reviewboard.mozilla.org/r/35623/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35623/

This is more a feedback? than r? since I know I have to provide initial data for, e.g. fennec. Re. the loadDump issue, more information can be found here: https://github.com/Kinto/kinto.js/pull/342

Comment on attachment 8721280 [details] MozReview Request: Bug 1224528 - Provide a mechanism to populate the OneCRL kinto collection with initial records r=rnewman https://reviewboard.mozilla.org/r/35623/#review32561 ::: browser/app/collections/moz.build:7 (Diff revision 1) > +FINAL_TARGET_FILES.defaults.collections.blocklists += ['certificates.json'] Can you document here, or in a README next to the file: * What this file is * When and how new versions are added * Risks of making changes * How to verify that a change is correct ? ::: services/common/KintoCertificateBlocklist.js:93 (Diff revision 1) > + const collectionFile = FileUtils.getFile("CurProcD", Should this be CurProcD or XCurProcD? The latter is overrideable. And should this be based on GreD instead? I think CurProcD will not be what you want sometimes.

This makes use of the kinto.js loadDump feature to populate the OneCRL collection with initial data from application defaults. This also includes a modified moz-kinto-client.js because there was an issue with the loadDump implementation in the FirefoxStorage adapter that caused breakage with empty collections. Review commit: https://reviewboard.mozilla.org/r/50181/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50181/

Review commit: https://reviewboard.mozilla.org/r/50183/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50183/

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50181/diff/1-2/

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review47227 ::: browser/app/collections/moz.build:7 (Diff revision 2) > +# vim: set filetype=python: > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +FINAL_TARGET_FILES.defaults.collections.blocklists += ['certificates.json'] I think this will work fine for Firefox - we also need to think about Fennec. ::: services/common/KintoBlocklist.js:82 (Diff revision 2) > + * cold start. > + * > + * If an error occurs this method does nothing. > + */ > + loadFromDump(collection) { > + const collectionFile = FileUtils.getFile("CurProcD", There were some unaddressed issues in rnewman's original review response (including the use of CurProcD here). Could you take a look at those, please?

Review commit: https://reviewboard.mozilla.org/r/52443/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52443/

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50181/diff/2-3/

Comment on attachment 8752160 [details] MozReview Request: Bug 1224528 - WIP Android support https://reviewboard.mozilla.org/r/52443/#review49475 LGTM

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review49471 ::: services/common/tests/unit/test_kintoCertBlocklist.js:80 (Diff revision 3) > // Our test data has a single record; it should be in the local collection > let collection = do_get_kinto_collection("certificates"); > yield collection.db.open(); > let list = yield collection.list(); > + // We know there will be initial values; just not how many. > + do_check_true(list.data.length >= 113); Is it worth updating the comment above to state we're assuming there will be at least 113 entries?

https://reviewboard.mozilla.org/r/50179/#review50316 ::: services/common/KintoBlocklist.js:85 (Diff revision 3) > + * For Bug 1257565 this method will have to try to load the file from the profile, > + * in order to leverage the updateJSONBlocklist() below, which writes a new > + * dump each time the collection changes. > + */ > + loadDumpFile() { > + const filePath = ["defaults", "collections", "blocklists", `${this.collectionName}.json`]; This should be factorized with this.FILENAME_ADDONS_JSON, this.FILENAME_GFX_JSON, this.FILENAME_PLUGINS_JSON

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review50312 ::: browser/app/moz.build:6 (Diff revision 3) > > -DIRS += ['profile/extensions'] > +DIRS += ['collections', 'profile/extensions'] > As we discussed, we may want to move this into services/blocklist ::: browser/app/moz.build:7 (Diff revision 3) > # vim: set filetype=python: > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > -DIRS += ['profile/extensions'] > +DIRS += ['collections', 'profile/extensions'] Could you add a README or a script to help others in the future update this preloaded dump? This could be done in a follow-up. ::: services/common/KintoBlocklist.js:19 (Diff revision 3) > > const { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.importGlobalProperties(['fetch']); Nit: double quotes ::: services/common/KintoBlocklist.js:77 (Diff revision 3) > + * Load the the JSON file distributed with the release for this blocklist. > + * > + * For Bug 1257565 this method will have to try to load the file from the profile, > + * in order to leverage the updateJSONBlocklist() below, which writes a new > + * dump each time the collection changes. > + */ > + loadDumpFile() { Don't we want to update the preference file in order to avoid a sync altogether if all the data is up-to-date?

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50181/diff/3-4/

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50181/diff/4-5/

Matt, I moved the dump files to `services/blocklist/` but I can't figure out to specify the destination in `moz.build`. In the current patch, the files land in `dist/bin/defaults/collections/blocklists/`. How can I specify a path that would carry the app name (browser/thunderbird/...) so that they land in `dist/bin/browser/defaults/collections/blocklists/` for example? (Otherwise I copied the files by hand and the tests pass)

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50181/diff/5-6/

Regarding load JSON dumps on Fennec: this is tricky: - Shipping JSON dumps will grow the android package. An effort is already being made to ship assets like fonts (via Kinto ^^) for this reason! - Shipping no dump will oblige fennec clients to download more data on first blocklist synchronization. Before tackling this: what is currently being done about blocklist.xml on Fennec? The whole file is downloaded every day like elsewhere?

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50181/diff/6-7/

(In reply to Mathieu Leplatre (:leplatrem) from comment #16) > Matt, I moved the dump files to `services/blocklist/` but I can't figure out > to specify the destination in `moz.build`. > > In the current patch, the files land in > `dist/bin/defaults/collections/blocklists/`. > > How can I specify a path that would carry the app name > (browser/thunderbird/...) so that they land in > `dist/bin/browser/defaults/collections/blocklists/` for example? > > (Otherwise I copied the files by hand and the tests pass) I don't have time to investigate. Please ask someone more involved in the build system / packaging such as glandium.

(In reply to Mathieu Leplatre (:leplatrem) from comment #18) > - Shipping JSON dumps will grow the android package. An effort is already > being made to ship assets like fonts (via Kinto ^^) for this reason! How much does it grow? Currently we monitor changes larger than 100kb. If it's more in the are of a couple of kB then this is negligible. Otherwise we need to balance between security and size concerns. > - Shipping no dump will oblige fennec clients to download more data on first > blocklist synchronization. Of course it depends on the size but as the other option is to download this with the APK there shouldn't be much difference from the size of the data. > Before tackling this: what is currently being done about blocklist.xml on > Fennec? The whole file is downloaded every day like elsewhere? I actually don't know. From the code I've seen I assume there's no difference between desktop and Fennec and we use the same code. But this is more or less a guess. I flagged mfinkle and margaret. They probably know or know who knows. :)

(In reply to Sebastian Kaspari (:sebastian) from comment #21) > (In reply to Mathieu Leplatre (:leplatrem) from comment #18) > > - Shipping JSON dumps will grow the android package. An effort is already > > being made to ship assets like fonts (via Kinto ^^) for this reason! > > How much does it grow? Currently we monitor changes larger than 100kb. If > it's more in the are of a couple of kB then this is negligible. Otherwise we > need to balance between security and size concerns. > > > - Shipping no dump will oblige fennec clients to download more data on first > > blocklist synchronization. > > Of course it depends on the size but as the other option is to download this > with the APK there shouldn't be much difference from the size of the data. > > > Before tackling this: what is currently being done about blocklist.xml on > > Fennec? The whole file is downloaded every day like elsewhere? > > I actually don't know. From the code I've seen I assume there's no > difference between desktop and Fennec and we use the same code. But this is > more or less a guess. I flagged mfinkle and margaret. They probably know or > know who knows. :) We don't ship this on Fennec, see bug 1235596.

Mike, I need to move some JSON files from `services/blocklist/` to `dist/bin/browser/defaults/collections/blocklists/` during build. In the current patch, I used ``FINAL_TARGET_FILES.defaults.collections.blocklists += ['addons.json', ...]``, and the files land in ``dist/bin/defaults/collections/blocklists/``. How can I specify a path that would target the app name folder (browser/thunderbird/...) so that they land in `dist/bin/browser/defaults/collections/blocklists/` ? (If it is even possible) Thanks for your insights!

Gregory, Tarek suggested me to needinfo you too for the above question :) Maybe you have some advice or feedback to share with regards to the approach... Many thanks in advance!

FINAL_TARGET_FILES prepends the DIST_SUBDIR or XPI_NAME moz.build variable to the destination directory. e.g. inside browser/ moz.build files, things will get installed to browser/* because browser/moz.build has "DIST_SUBDIR = 'browser'". CONFIG['MOZ_APP_NAME'] contains the name of the current application. In theory you could reference that when assigning to FINAL_TARGET_FILES (pretty sure you can use [] in addition to . for hierarchical assignment). Instead of using FINAL_TARGET_FILES, have you considered a jar.mn file? Historically files under services/ are using a packaging method different from the rest of Firefox. This has been cargo culted over the years. I would suggest using jar.mn files like most things.

Note that Thunderbird doesn't use a separate app directory like Firefox does. Also note that using jar.mn wouldn't solve the app directory problem. You still need to set DIST_SUBDIR. See devtools/moz.build.

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review69582 Let me know if you need more guidance.

:glandium helped me fix the packaging issue! Thank you again! So in the last version of this patch, the server json dumps are shipped in omni.ja and accessed via resource:// with fetch(). Since there is a work-in-progress regarding the content of omni.ja on Fennec (https://bugzilla.mozilla.org/show_bug.cgi?id=1044079), I only set it up for the `browser` target. This should be ready for review/merge :)

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review98382 ::: services/blocklist/readme.md:3 (Diff revision 8) > +The blocklist entries are synchronized locally from the Firefox Settings service. > + > +https://firefox.settings.services.mozilla.com Is /services/ supposed to be generic to support non-Firefox applications? Will Firefox on Android use the exact same files even though some of the gfx and plugins ones are probably irrelevant? ::: services/blocklist/readme.md:24 (Diff revision 8) > +curl "$SERVICE_URL/buckets/blocklists/collections/$COLLECTION/records?" > services/blocklist/$COLLECTION.json > +``` > + > +## TODO > + > +- Setup a bot to update it regurlarly. Nit: regularly ::: services/moz.build:7 (Diff revision 8) > DIRS += [ > + 'blocklist', Only run this on desktop for now so we don't ship extra data in irrelevant applications especially filesize sensitive ones like FxAndroid: `if CONFIG['MOZ_BUILD_APP'] == 'browser':`

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist I think it would be good to get another pass by mgoodwin.

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review98382 > Is /services/ supposed to be generic to support non-Firefox applications? Will Firefox on Android use the exact same files even though some of the gfx and plugins ones are probably irrelevant? I used /services/ because it was suggested in a previous comment. I also thought we could later move the blocklist clients and tests in that folder also.

/services/ is about as generic as toolkit is -- which is to say, in theory it's supposed to be all cross-platform, and in practice it's not necessarily so. Most notably Sync's desktop implementation lives in services, and is only shipped on desktop.

I rebased and made some changes to the patch to generalize it a bit: we now support certificate pinning (Bug 1306470), and now load from folders whose name is the remote bucket name. Is there any chance that we can land this please? It's been here for a long time and it paves the way for Bug 1257565 Thanks for your feedback!

Since comment 31 this was waiting on review from mgoodwin but you cleared the flag without explanation. I'm fine with you landing after this review of the code since he has more context.

Comment on attachment 8748326 [details] Bug 1224528 - Load initial JSON files for blocklist https://reviewboard.mozilla.org/r/50181/#review114146

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/63d0ef447e81 Load initial JSON files for blocklist r=mgoodwin

Oops. Backed out in https://hg.mozilla.org/mozilla-central/rev/a9ec72f82299250e6023988e238931bbca0ef7fa because that actually made Android xpcshell permaorange like https://treeherder.mozilla.org/logviewer.html#?job_id=77770672&repo=mozilla-central but if you give us an even vaguely matching intermittent-failure bug suggestion, we'll just star permaorange all day.

Hi Phil! I'm sorry :/ Thanks for your patience... We can skip the test file on Android, since we don't ship the files (see #c22). How do I proceed to update my patch now that it has been backed-out? MozReview suggests me to file a new bug (!?). It would just consist of adding ``skip-if = os == "android"`` in xpcshell.ini Thanks for your insights!

The way I understand it, you just click the "Reopen" in "This change has been marked as submitted. Reopen" at the top of https://reviewboard.mozilla.org/r/50181/ (though that's just what I've overheard, since I've never been on that side of a backout, only the backing-out side).

I updated my patch, and submitted a new Try build on linux+android. Everything looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f107ea09a92a

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/735f81d9fd96 Load initial JSON files for blocklist r=mgoodwin