In Bug 1224467 a preference was introduced to migrate OneCRL from the blocklist XML to the new remote settings solution (aka. Kinto). The new system has been default for a while now. In order to avoid having to test many preferences combinations when working on Bug 1257565, I suggest that we get rid of this preference — it is very unlikely that we switch back to the XML system (:mgoodwin agrees).

I made my best to rewrite ``security/manager/ssl/tests/unit/test_cert_blocklist.js`` to rely on JSON instead of XML. Unfortunately I couldn't go through everything. Mainly two things: - With XML there were a way to have several "serialNumber" children. With JSON, it does not make sense anymore IMO. But some tests still assert some behaviour around that. - When relying on ``blocklist.xml``, we knew that when the "blocklist-updated" event was received, the whole XML had been processed. When relying on Kinto, we have no event that tells when synchronization and its ``updateCertBlocklist()`` callback is finished. Mark, would you have some time to adjust the test suite and land? Thanks a lot!

Comment on attachment 8861481 [details] Bug 1359428 - Remove preference to select OneCRL update mechanism https://reviewboard.mozilla.org/r/133458/#review138496 This looks good to me; it might be worth getting a review from Keeler too (since you're touching some PSM code and tests).

(In reply to Mathieu Leplatre (:leplatrem) from comment #4) > Mark, would you have some time to adjust the test suite and land? I misread; yes. I can do this.

Mark, I rebased and commented out the failing parts of the tests. I tried my best to fix them myself but I got lost :| If you have some bandwidth, I'd gladly welcome help here :) Thanks!

This blocks a Quantum Flow P1 bug and we're wondering what the status is. Any updates you can share? Thank you.

Mat; I've made the necessary changes to the tests but I can't review these myself. I r? keeler since the majority of the changes are in test_cert_blocklist.js and I r+ed the rest in https://reviewboard.mozilla.org/r/133458/diff/5#index_header (reviewboard apparently isn't able to reconcile the two as a single set of changes).

Comment on attachment 8904577 [details] Bug 1359428 - Remove preference to select OneCRL update mechanism https://reviewboard.mozilla.org/r/176422/#review181394 LGTM - just some minor fixups. ::: security/manager/ssl/CertBlocklist.cpp:649 (Diff revision 1) > auto blocklist = static_cast<CertBlocklist*>(aClosure); > MutexAutoLock lock(blocklist->mMutex); > > MOZ_LOG(gCertBlockPRLog, LogLevel::Warning, > ("CertBlocklist::PreferenceChanged %s changed", aPref)); > - if (strcmp(aPref, PREF_BACKGROUND_UPDATE_TIMER) == 0) { > + if (strcmp(aPref, PREF_BLOCKLIST_ONECRL_CHECKED) == 0) { Is the PREF_BACKGROUND_UPDATE_TIMER pref still relevant? (If not, we should probably at least stop observing it, right?) ::: security/manager/ssl/tests/unit/test_cert_blocklist.js:46 (Diff revision 1) > .createInstance(Ci.nsIFileOutputStream); > stream.init(blockFile, > FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | FileUtils.MODE_TRUNCATE, > FileUtils.PERMS_FILE, 0); > > -var data = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + > +var emptyBlocklistXML = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + Do we even still need to serve an empty blocklist.xml file? ::: security/manager/ssl/tests/unit/test_cert_blocklist.js:121 (Diff revision 1) > - // In this case, the issuer name and the valid serialNumber correspond > + // In this case, the issuer name and the valid serialNumber correspond > - // to other-test-ca.pem in bad_certs/ (for testing root revocation) > + // to other-test-ca.pem in bad_certs/ (for testing root revocation) > - "<certItem issuerName='MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E='>" + > - "<serialNumber>exJUIJpq50jgqOwQluhVrAzTF74=</serialNumber></certItem>" + > - // This item corresponds to an entry in sample_revocations.txt where: > + ` { > + "id": "6", > + "last_modified": 100000000000000000005, > + "issuerName": "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=", Shouldn't this be "MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E="? ::: security/manager/ssl/tests/unit/test_cert_blocklist.js:150 (Diff revision 1) > - " pubKeyHash='VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8='>" + > - "</certItem></certItems></blocklist>"; > - > -var updatedBlocklist = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" + > - "<blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\">" + > - "<certItems>" + > + "id": "9", > + "last_modified": 100000000000000000007, > + "subject": "MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5", > + "pubKeyHash": "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=" > + },` + > + // This item revokes other-issuer-ee.pem by issuer and serial number. If the above entry is fixed, I don't think this one is necessary. ::: services/common/blocklist-updater.js:13 (Diff revision 1) > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > Cu.importGlobalProperties(["fetch"]); > XPCOMUtils.defineLazyModuleGetter(this, "UptakeTelemetry", > "resource://services-common/uptake-telemetry.js"); I suppose you'll need a services/toolkit peer to review these files.

Comment on attachment 8904577 [details] Bug 1359428 - Remove preference to select OneCRL update mechanism https://reviewboard.mozilla.org/r/176422/#review181706 ::: services/common/blocklist-updater.js:21 (Diff revision 1) > const PREF_SETTINGS_SERVER_BACKOFF = "services.settings.server.backoff"; > const PREF_BLOCKLIST_CHANGES_PATH = "services.blocklist.changes.path"; > const PREF_BLOCKLIST_LAST_UPDATE = "services.blocklist.last_update_seconds"; > const PREF_BLOCKLIST_LAST_ETAG = "services.blocklist.last_etag"; > const PREF_BLOCKLIST_CLOCK_SKEW_SECONDS = "services.blocklist.clock_skew_seconds"; > +const PREF_BLOCKLIST_LOAD_DUMP = "services.blocklist.load_dump"; Leftover? ::: services/common/blocklist-updater.js:182 (Diff revision 1) > // Save current Etag for next poll. > if (currentEtag) { > Services.prefs.setCharPref(PREF_BLOCKLIST_LAST_ETAG, currentEtag); > } > + > + Services.obs.notifyObservers(null, "blocklist-updater-versions-checked"); Could you please add an explicit mention of this in `test_blocklist_updater.js`? I know it's tested in `test_cert_blocklist.js` but I believe it's a good practice when each module feature has specific assertions in its test file :)

(In reply to Mathieu Leplatre (:leplatrem) from comment #14) > > +const PREF_BLOCKLIST_LOAD_DUMP = "services.blocklist.load_dump"; > > Leftover? I thought this was one of your changes... > Could you please add an explicit mention of this in > `test_blocklist_updater.js`? > I know it's tested in `test_cert_blocklist.js` but I believe it's a good > practice when each module feature has specific assertions in its test file :) Good call. I'll add an explicit test in there.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13) > > -var data = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + > > +var emptyBlocklistXML = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + > > Do we even still need to serve an empty blocklist.xml file? No. But we do need to keep the clobbering of extensions.blocklist.url pref around - for now.

Comment on attachment 8904577 [details] Bug 1359428 - Remove preference to select OneCRL update mechanism https://reviewboard.mozilla.org/r/176422/#review187660

Pushed by mgoodwin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a889835e4003 Remove preference to select OneCRL update mechanism r=keeler,leplatrem,rhelmer