Closed
Bug 1397230
Opened 7 years ago
Closed 6 years ago
Generalize Kinto blocklist code as remote settings
Categories
(Toolkit :: Blocklist Implementation, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: leplatrem, Assigned: leplatrem)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
Originally we leveraged Kinto in Firefox for the blocklists synchronization. But now (with Bug 1336208) the mechanism is used for other use-cases. It's time to rename the files and class names to something more generic like *remote settings* (same wording as Bug 1254099).
Comment 1•7 years ago
|
||
Does this really need tracking for Firefox 57?
Flags: needinfo?(mathieu)
Priority: -- → P3
Assignee | ||
Comment 2•7 years ago
|
||
> Does this really need tracking for Firefox 57?
No it does not.
Flags: needinfo?(mathieu)
Updated•7 years ago
|
status-firefox57:
affected → ---
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8959179 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8959180 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8959181 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8959182 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8959178 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mathieu
Assignee | ||
Comment 14•6 years ago
|
||
Mark, when checking out the first commit, all tests pass except one: security/manager/ssl/tests/unit/test_cert_blocklist.js with an error that I don't understand: pid:6091 1521216809096 addons.xpi WARN Failed to add built-in install location app-system-defaults: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/FileUtils.jsm :: FileUtils_getDir :: line 64" data: no] Stack trace: FileUtils_getDir()@resource://gre/modules/FileUtils.jsm:64 < addBuiltInInstallLocation()@resource://gre/modules/addons/XPIProvider.jsm:2049 < startup()@resource://gre/modules/addons/XPIProvider.jsm:2126 < callProvider()@resource://gre/modules/AddonManager.jsm:258 < _startProvider()@resource://gre/modules/AddonManager.jsm:733 < startup()@resource://gre/modules/AddonManager.jsm:897 < startup()@resource://gre/modules/AddonManager.jsm:2972 < observe()@addonManager.js:65 < /home/mathieu/Code/Mozilla/firefox/obj-x86_64-pc-linux-gnu/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:161 < load_file()@/home/mathieu/Code/Mozilla/firefox/testing/xpcshell/head.js:650 < _load_files()@/home/mathieu/Code/Mozilla/firefox/testing/xpcshell/head.js:662 < _execute_test()@/home/mathieu/Code/Mozilla/firefox/testing/xpcshell/head.js:526 < -e:1 Do you have any idea what could cause that? Gijs, I don't know what is missing for the second commit to work. First, ``mach eslint`` returns a lot of errors (like 'add_task' is not defined.) And `NS_ERROR_FILE_NOT_FOUND` for the ChromeUtils imports of `services-blocklists/clients` in the tests... Do you spot anything obvious? Thank you both in advance for your insights!
Flags: needinfo?(mgoodwin)
Flags: needinfo?(gijskruitbosch+bugs)
Comment 15•6 years ago
|
||
(In reply to Mathieu Leplatre (:leplatrem) from comment #14) > Gijs, I don't know what is missing for the second commit to work. First, > ``mach eslint`` returns a lot of errors (like 'add_task' is not defined.) You probably need to `hg copy` whatever .eslintrc.js file is in the directory where the tests originally lived into the directory where they live now. You probably need to also update https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/modules.json#17 . > And `NS_ERROR_FILE_NOT_FOUND` for the ChromeUtils imports of > `services-blocklists/clients` in the tests... It looks like you fixed this? I don't see anything off-hand in the last version of the commit that'd explain this. I'm afraid the review might have to wait until Monday, I'm a bit swamped. Do let me know if you're still seeing issues with this. Did you push to try?
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8959177 [details] Bug 1397230 - Generalize blocklist clients to remote settings clients https://reviewboard.mozilla.org/r/228046/#review234610 Code analysis found 5 defects in this patch: - 5 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/common/blocklist-clients.js:137 (Diff revision 5) > > -var OneCRLBlocklistClient = new BlocklistClient( > - Services.prefs.getCharPref(PREF_BLOCKLIST_ONECRL_COLLECTION), > - PREF_BLOCKLIST_ONECRL_CHECKED_SECONDS, > - updateCertBlocklist, > - Services.prefs.getCharPref(PREF_BLOCKLIST_BUCKET), > +let OneCRLBlocklistClient; > +let AddonBlocklistClient; > +let PluginBlocklistClient; > +let GfxBlocklistClient; > +let GfxBlocklistClient; Error: Parsing error: identifier 'gfxblocklistclient' has already been declared [eslint: None] ::: services/common/remote-settings.js:9 (Diff revision 5) > + > +"use strict"; > + > +var EXPORTED_SYMBOLS = ["remoteSettingsClient", "pollChanges"]; > + > +const { classes: Cc, interfaces: Ci, utils: Cu } = Components; Error: Cc is now defined in global scope, a separate definition is no longer necessary. [eslint: mozilla/no-define-cc-etc] ::: services/common/remote-settings.js:9 (Diff revision 5) > + > +"use strict"; > + > +var EXPORTED_SYMBOLS = ["remoteSettingsClient", "pollChanges"]; > + > +const { classes: Cc, interfaces: Ci, utils: Cu } = Components; Error: Ci is now defined in global scope, a separate definition is no longer necessary. [eslint: mozilla/no-define-cc-etc] ::: services/common/remote-settings.js:9 (Diff revision 5) > + > +"use strict"; > + > +var EXPORTED_SYMBOLS = ["remoteSettingsClient", "pollChanges"]; > + > +const { classes: Cc, interfaces: Ci, utils: Cu } = Components; Error: Cu is now defined in global scope, a separate definition is no longer necessary. [eslint: mozilla/no-define-cc-etc] ::: services/common/remote-settings.js:70 (Diff revision 5) > + // All existing records are replaced by the version from the server. > + changes.forEach((record) => records[record.id] = record); > + > + return Object.values(records) > + // Filter out deleted records. > + .filter((record) => record.deleted != true) Error: Don't compare for inexact equality against boolean literals [eslint: mozilla/no-compare-against-boolean-literals]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•6 years ago
|
||
> > Gijs, I don't know what is missing for the second commit to work. First, > > ``mach eslint`` returns a lot of errors (like 'add_task' is not defined.) > > You probably need to `hg copy` whatever .eslintrc.js file is in the directory > where the tests originally lived into the directory where they live now. You > probably need to also update https://dxr.mozilla.org/mozilla- > central/source/tools/lint/eslint/modules.json#17 . > Thanks! That worked nicely for the tests symbols. Unfortunately it wasn't enough for the symbols defined in `services/common/tests/unit/head_helpers.js` that I «include» via `head = ` in xpcshell.ini. > > And `NS_ERROR_FILE_NOT_FOUND` for the ChromeUtils imports of > `services- > blocklists/clients` in the tests... > > It looks like you fixed this? I don't see anything off-hand in the last version > of the commit that'd explain this. > I don't unfortunately. I rebased to latest central and tried many different things this morning, in vain :| In the first place, do you think the separation I did makes sense? I'd like to leave in the `services/common` folder components that are reusable and shared. I don't think blocklist belongs to that category. > I'm afraid the review might have to wait until Monday, I'm a bit swamped. Do let > me know if you're still seeing issues with this. Did you push to try? I just did, but I don't think the errors come from my local setup :(
Flags: needinfo?(gijskruitbosch+bugs)
Comment 24•6 years ago
|
||
(In reply to Mathieu Leplatre (:leplatrem) from comment #23) > > > Gijs, I don't know what is missing for the second commit to work. First, > > > ``mach eslint`` returns a lot of errors (like 'add_task' is not defined.) > > > > You probably need to `hg copy` whatever .eslintrc.js file is in the directory > > where the tests originally lived into the directory where they live now. You > > probably need to also update https://dxr.mozilla.org/mozilla- > > central/source/tools/lint/eslint/modules.json#17 . > > > > Thanks! That worked nicely for the tests symbols. > > Unfortunately it wasn't enough for the symbols defined in > `services/common/tests/unit/head_helpers.js` > that I «include» via `head = ` in xpcshell.ini. I think you need some kind of 'import globals' syntax for this, see examples at: https://dxr.mozilla.org/mozilla-central/search?q=import%20global%20from%20file%3A.js&=mozilla-central :Standard8 can hopefully confirm if there's a better way to do this. > > > And `NS_ERROR_FILE_NOT_FOUND` for the ChromeUtils imports of > `services- > > blocklists/clients` in the tests... > > > > It looks like you fixed this? I don't see anything off-hand in the last version > > of the commit that'd explain this. > > > > I don't unfortunately. I rebased to latest central and tried many different > things this morning, in vain :| Huh. Locally, if you open: resource://services-blocklists/ do you see what you expect? If you open resource://gre/modules/, do you see a 'services-blocklists' directory? It's probably worth looking at where this is going wrong that way, ie which moving part is broken. I'm about to head out of the house and won't be able to look at this again for a few hours, but that's where I'd start digging. Please do ping me again end of day today or tomorrow morning (either on here or IRC or whatever) if you can't figure it out. People in #developers who know about the build system might also be able to spot what's missing... it's not obvious to me, either, at least not from a quick look. Perhaps you also need to make sure that the directory itself is being processed through services/moz.build ? Maybe the manifest file needs to be unique (so rename servicesComponents.manifest to blocklistComponents.manifest or whatever, make sure to update moz.build as well) because it's all ending up in one directory, or something? > In the first place, do you think the separation I did makes sense? > I'd like to leave in the `services/common` folder components that are > reusable and shared. I don't think blocklist belongs to that category. I think this is probably a question for the folks who know about services/common more than I do. :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(standard8)
Comment 25•6 years ago
|
||
(In reply to :Gijs from comment #24) > (In reply to Mathieu Leplatre (:leplatrem) from comment #23) > > > > Gijs, I don't know what is missing for the second commit to work. First, > > > > ``mach eslint`` returns a lot of errors (like 'add_task' is not defined.) > > > > > > You probably need to `hg copy` whatever .eslintrc.js file is in the directory > > > where the tests originally lived into the directory where they live now. You > > > probably need to also update https://dxr.mozilla.org/mozilla- > > > central/source/tools/lint/eslint/modules.json#17 . > > > > > > > Thanks! That worked nicely for the tests symbols. > > > > Unfortunately it wasn't enough for the symbols defined in > > `services/common/tests/unit/head_helpers.js` > > that I «include» via `head = ` in xpcshell.ini. > > I think you need some kind of 'import globals' syntax for this, see examples > at: > > https://dxr.mozilla.org/mozilla-central/ > search?q=import%20global%20from%20file%3A.js&=mozilla-central > > :Standard8 can hopefully confirm if there's a better way to do this. Thanks for the ping. Taking a look around at the existing test imports for head files, it became clear that if the head file isn't in the same directory, then ESLint wasn't detecting it. We can do better! I've just filed bug 1447034 for this, and I've already got an experimentation patch locally. The very short-term workaround is for the import-globals-from, however, after bug 1447034 that hopefully won't be needed at all for xpcshell-tests.
Flags: needinfo?(standard8)
Assignee | ||
Comment 26•6 years ago
|
||
Thanks Gijs and Mark for your help! I could fix the ESLint errors, and now understand why the module is not at the right place. In Bug 1224528 and Bug 1351675 we packaged blocklist data using `DIST_SUBDIR = 'browser'` The `services-blocklists/clients.js` is thus in `obj-x86_64-pc-linux-gnu/dist/bin/browser/modules/services-blocklists/clients.js` as explicitly instructed in the `moz.build` file. I'll see what I can do :)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8959178 -
Attachment is obsolete: true
Attachment #8959178 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•6 years ago
|
||
(In reply to Mathieu Leplatre (:leplatrem) from comment #14) > Mark, when checking out the first commit, all tests pass except one: > security/manager/ssl/tests/unit/test_cert_blocklist.js with an error that I > don't understand: > Do you have any idea what could cause that? No; if you're still having this problem I'm happy to help tracking it down?
Flags: needinfo?(mgoodwin)
Assignee | ||
Comment 33•6 years ago
|
||
Thanks Mark! I fixed it by cleaning everything out. And Try is now green :)
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8959177 [details] Bug 1397230 - Generalize blocklist clients to remote settings clients https://reviewboard.mozilla.org/r/228046/#review235812 ::: security/manager/ssl/tests/unit/test_cert_blocklist.js:217 (Diff revision 10) > let blocklist = Cc["@mozilla.org/extensions/blocklist;1"] > .getService(Ci.nsITimerCallback); > > return new Promise((resolve) => { > - let certblockObserver = { > + const e = "remote-settings-changes-polled"; > + const certblockObserver = { Nit: Maybe this should also be renamed to reflect the new naming?
Attachment #8959177 -
Flags: review?(mgoodwin) → review+
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8959177 [details] Bug 1397230 - Generalize blocklist clients to remote settings clients https://reviewboard.mozilla.org/r/228046/#review235820 Looks good to me.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 37•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fc6d9baa6cc9 Generalize blocklist clients to remote settings clients r=mgoodwin
Keywords: checkin-needed
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc6d9baa6cc9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in
before you can comment on or make changes to this bug.
Description
•