Mathieu Leplatre [:leplatrem] Reporter
	Description • 9 years ago

      No description provided.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 2 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3685dbd5b4f9

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 4 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68da25cd27d1

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 5 • 9 years ago

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

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 6 • 9 years ago

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

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 7 • 9 years ago

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

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 8 • 9 years ago

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

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 9 • 9 years ago

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

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 10 • 8 years ago

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/

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 11 • 8 years ago

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/

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 12 • 8 years ago

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/

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 13 • 8 years ago

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/

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 14 • 8 years ago

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/

	Matthew N. [:MattN]
	Comment 15 • 8 years ago

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`

	Matthew N. [:MattN]
	Comment 16 • 8 years ago

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?

	Matthew N. [:MattN]
	Comment 23 • 8 years ago

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.

	Dave Townsend [:mossop]
	Comment 24 • 8 years ago
mozreview-review

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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 31 • 8 years ago

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!

	Dave Townsend [:mossop]
	Comment 37 • 8 years ago
mozreview-review

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

	Dave Townsend [:mossop]
	Comment 38 • 8 years ago
mozreview-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.

	Dave Townsend [:mossop]
	Comment 39 • 8 years ago
mozreview-review

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.

	Florian Quèze [:florian]
	Comment 40 • 8 years ago

(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.

	Dave Townsend [:mossop]
	Comment 44 • 8 years ago

Waiting on a response to comment 38 here

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 49 • 8 years ago

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!

	Dave Townsend [:mossop]
	Comment 50 • 8 years ago
mozreview-review

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.

	Dave Townsend [:mossop]
	Comment 51 • 8 years ago
mozreview-review

Comment on attachment 8743787 [details]
Bug 1257565 - Reload from disk when kinto blocklist was updated

https://reviewboard.mozilla.org/r/48007/#review136472

	Florian Quèze [:florian]
	Comment 52 • 8 years ago

(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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 53 • 8 years ago
mozreview-review

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

	Florian Quèze [:florian]
	Comment 54 • 8 years ago

Please run:

./mach eslint toolkit/mozapps/extensions

It gives 63 errors for me with the patches applied.

	Dave Townsend [:mossop]
	Comment 58 • 8 years ago

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?

	Florian Quèze [:florian]
	Comment 59 • 8 years ago

(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.

	Dave Townsend [:mossop]
	Comment 60 • 8 years ago

(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.

	Dave Townsend [:mossop]
	Comment 61 • 8 years ago
mozreview-review

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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 67 • 8 years ago
mozreview-review-reply

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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 71 • 8 years ago
mozreview-review-reply

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

	Dave Townsend [:mossop]
	Comment 72 • 8 years ago
mozreview-review

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.

	Andrew Overholt [:overholt]
	Comment 81 • 7 years ago

Does this still need bug 1359428?

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 82 • 7 years ago

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)

	Florian Quèze [:florian]
	Comment 83 • 7 years ago

(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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 88 • 7 years ago

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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 89 • 7 years ago

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?

	Jean Gong :jgong
	Comment 90 • 7 years ago

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.

	Mike Conley (:mconley) (:⚙️)
	Comment 91 • 7 years ago

As far as I know, this still affects start-up. Does comment 89 still apply, or is there time to work on this now?

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 92 • 7 years ago

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)

	Mike Conley (:mconley) (:⚙️)
	Comment 93 • 7 years ago

Okay, waiting on feedback from kmag / florian.

	Mike Conley (:mconley) (:⚙️)
	Comment 94 • 7 years ago

Ping?

	Florian Quèze [:florian]
	Comment 95 • 7 years ago

Redirecting my needinfo to Gijs who works on blocklist perf problems these days.

	:Gijs (he/him) Assignee
	Comment 96 • 7 years ago

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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 97 • 6 years ago

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

	:Gijs (he/him) Assignee
	Comment 99 • 6 years ago

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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 100 • 6 years ago
mozreview-review

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.

	Kris Maglione [:kmag]
	Comment 101 • 6 years ago

(In reply to Mathieu Leplatre (:leplatrem) from comment #100)
> I am impressed, this is finally happening! GG :clap:

Concur. Beers are owed.

	Kris Maglione [:kmag]
	Comment 102 • 6 years ago
mozreview-review

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()?

	:Gijs (he/him) Assignee
	Comment 103 • 6 years ago

(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.

	Mathieu Leplatre [:leplatrem] Reporter
	Comment 105 • 6 years ago
mozreview-review

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 :)

	Doug Thayer [:dthayer] (he/him)
	Comment 106 • 6 years ago
mozreview-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?