Closed Bug 1397230 Opened 7 years ago Closed 6 years ago

Generalize Kinto blocklist code as remote settings

Categories

(Toolkit :: Blocklist Implementation, enhancement, P3)

57 Branch
enhancement

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).
Does this really need tracking for Firefox 57?
Flags: needinfo?(mathieu)
Priority: -- → P3
> Does this really need tracking for Firefox 57?

No it does not.
Flags: needinfo?(mathieu)
Attachment #8959179 - Attachment is obsolete: true
Attachment #8959180 - Attachment is obsolete: true
Attachment #8959181 - Attachment is obsolete: true
Attachment #8959182 - Attachment is obsolete: true
Attachment #8959178 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → mathieu
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)
(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 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]
> > 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)
(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)
(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)
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 :)
Attachment #8959178 - Attachment is obsolete: true
Attachment #8959178 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1447332
(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)
Thanks Mark! I fixed it by cleaning everything out. And Try is now green :)
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 on attachment 8959177 [details]
Bug 1397230 - Generalize blocklist clients to remote settings clients

https://reviewboard.mozilla.org/r/228046/#review235820

Looks good to me.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/fc6d9baa6cc9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1450998
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: