1454970 - Provide a nice hook to initialize RemoteSettings clients

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement)

Description

[Mathieu Leplatre [:leplatrem]]

Currently, the RemoteSettings offers no clean way to initialize new clients.

What we did for blocklists was this:

```
// services/common/blocklists-clients.js

function initialize() {
   RemoteSettings("key")
      .on("change", ...);
}


// toolkit/mozapps/extensions/nsBlocklistService.js
XPCOMUtils.defineLazyGetter(this, "RemoteSettings", function() {
  // Instantiate blocklist clients.
  BlocklistClients.initialize();
  // Import RemoteSettings for ``pollChanges()``
  const { RemoteSettings } = ChromeUtils.import("resource://services-common/remote-settings.js", {});
  return RemoteSettings;
});
```

See https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/services/common/blocklist-clients.js#136

And https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/toolkit/mozapps/extensions/nsBlocklistService.js#37-43


Which is utterly ugly IMO.


What can we propose to our future RemoteSettings audience so that their clients get initialized lazily when the browser starts?

Ideally, in addition to migrating the current hack for blocklists, we'd like to suggest a clean and official way in our docs here:
https://firefox-source-docs.mozilla.org/services/common/services/RemoteSettings.html

Comment 1

[Florian Quèze [:florian]]

I'm not sure which problem you are trying to fix exactly. The only thing that seems obviously wrong to me here is the eager import of blocklist-clients.js at https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/toolkit/mozapps/extensions/nsBlocklistService.js#36 when it's only used from within the RemoteSettings lazy getter, which is itself triggered from what looks like a timer callback.

Comment 2

[Mathieu Leplatre [:leplatrem]]

Thanks for your feedback! The eager import could be fixed indeed. 

I don't consider it a good practice to have the initialization as some sort of «import side-effect» in another component.
One of the downsides for example is that we cannot have an exhaustive list of clients until every consuming component is loaded (for the blocklists it means until the first timer is called which can be like 24H).

In the future, we'll have more use-cases of RemoteSettings in several components, and I wish we could encourage one standard good practice to initialize their clients.

Something like a manifest entry that would hook their ``initialize()`` function cleanly. Is that hard to achieve?

Comment 3

[Florian Quèze [:florian]]

I'll be on PTO for a few weeks. If you want help during that time, I would suggest asking :Gijs. Otherwise we can discuss in person at the next all hands.

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Florian Quèze [:florian] from comment #1)
> I'm not sure which problem you are trying to fix exactly. The only thing
> that seems obviously wrong to me here is the eager import of
> blocklist-clients.js at
> https://searchfox.org/mozilla-central/rev/
> f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/toolkit/mozapps/extensions/
> nsBlocklistService.js#36 when it's only used from within the RemoteSettings
> lazy getter, which is itself triggered from what looks like a timer callback.

The eager import was fixed in bug 1456677.

Mathieu, do you think there's something else left to do here?

Comment 5

[Mathieu Leplatre [:leplatrem]]

I gave it some thought, and I think I have a plan to improve what we do now.

Currently, when we poll changes, we rely on the list of instantiated clients:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/services/common/remote-settings.js#511-519

That means that `BlocklistClients.initialize()` must be called once before the synchronization takes place otherwise the data is not fetched. I'm not a huge fan of this, but :florian didn't seem taken aback.

Anyway, we do it like that because we cannot just rely on the collection list from the server, since client synchronization relies on instantiation options like custom signer, custom buckets etc. 

But, for default use-cases where remote clients are instantiated without advanced options (those options are deliberately not documented BTW) we can rely on the collection list from the server and synchronize the collection data without having their related client instantiated ­— basically by using the default options (default signer and default bucket).

Thanks for your support, I'll give a try and get back to you ;)

Comment 6

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1454970 - Use list of RemoteSettings collection from server f?glasserc

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

Comment 7

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8974099 [details]
Bug 1454970 - Use list of RemoteSettings collection from server f?glasserc

https://reviewboard.mozilla.org/r/242400/#review248330

I think this makes sense and is an improvement over having to execute certain initialization code to start synchronizing data. However, this means that all data is always synchronized on all clients. Is that OK for our use case?

Comment 8

[Mathieu Leplatre [:leplatrem]]

> However, this means that all data is always synchronized on all clients. Is that OK for our use case?

Indeed, now that you ask, I realize that all data on all clients can be problematic. For example, we may not always want to download the remote settings data for a feature that the user has disabled. Or we may not want to synchronize QA collections, test pilot stuff, product specific like focus experiments, rocket stuff, etc. present in the main bucket [1]

My patch is thus worthless.

So... unless we have some "hook" mechanism, I don't see a way out of the two following strategies:
- rely on known instantiated clients and thus on a certain initialization order (like this ugly import side effect in blocklists)
- rely on the server list and thus potentially download useless data locally

For the "hook", I like the idea of a text file (~manifest) where developers using remotesettings could «declare» their collection name. 

But there might be other better options like (I'm inventing stuff here):

- an official startup/ready/first-paint/idle hook/event that we could recommend to developers so that they initialize their remotesettings clients, assuming that polling will always happen after and in order to pick all instantiated clients

- a new "remotesetting:polling:before" *synchronous* event that we would fire just before starting polling, so that every client can jump in before synchronization starts (initialization is already idempotent btw) 

- an official moz.build feature that would allow developers to populate something like a static global constant, containing a list of strings with collections names to synchronize. It would allow developers to select which product is concerned by each remotesettings collection (Fx, TB, SM, Fennec, ...) 


Gijs, does some of this makes sense to you? Do you think there's something out there that we don't know about but we could leverage?

Thanks a lot for your insights!


[1] https://settings.stage.mozaws.net/v1/buckets/main/collections?_fields=id

Comment 9

[Mathieu Leplatre [:leplatrem]]

After having discussed with Gijs, we have a plan: pollChanges() will synchronize the collections that exist on disk. For empty profiles, it can fallback to registered clients.

Lifecycle would be:
1) empty profile
2) some client registers for collection X somewhere
3) as of (2), we will sync collection X, one way or another (either because there's data on disk, or if not, because the client is registered)
4) if we stop using a client, unregister the instance and kill the db data

Question: Kinto uses a dedicated indexeddb store for each collection. Is it easy to get the list of indexeddb stores? If not, we can use a pref to «denormalize» the list of local databases.

Comment 10

[Mathieu Leplatre [:leplatrem]]

Listing local databases is done like that in devtools storage panel:
https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/devtools/server/actors/storage.js#2171-2220

Which in our case it would consist in listing:
$PROFILE_DIR/storage/permanent/chrome/idb/

```
➜  ~ sqlite3 ~/.mozilla/firefox/381kklwy.default/storage/permanent/chrome/idb/3345959086bslnoocdkdlaiFs2t%s.sqlite
SQLite version 3.22.0 2018-01-22 18:45:57
Enter ".help" for usage hints.
sqlite> SELECT name FROM database;
blocklists/addons
```

or

```
connection = await Sqlite.openConnection({ path });
let rows = await connection.execute("SELECT name FROM database");
let name = rows[0].getResultByName("name");
await connection.close();
```

Otherwise, spec is on the way:
https://github.com/w3c/IndexedDB/pull/240

Comment 11

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1454970 - Decouple Remote Settings synchronization from initialization order

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

Comment 12

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246610/diff/1-2/

Comment 13

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

https://reviewboard.mozilla.org/r/246610/#review252920

::: services/common/tests/unit/test_remote_settings_poll.js:80
(Diff revision 2)
>    // for a collection called 'test-collection'
> +  let maybeSyncCalled = false;
>    const c = RemoteSettings("test-collection", {
>      bucketName: "test-bucket",
>    });
> -  c.maybeSync = () => {};
> +  c.maybeSync = () => { maybeSyncCalled = true; };

Should we check that we didn't create a client/try to sync for the unknown collection?

::: services/common/tests/unit/test_remote_settings_poll.js:116
(Diff revision 2)
>    // Simulate a poll with up-to-date collection.
> -  Services.prefs.setIntPref(PREF_LAST_UPDATE, 0);
> +  const startHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
> +
> +  const serverTime = 4000;
> +  function server304(request, response) {
> +    if (request.hasHeader("if-none-match") && request.getHeader("if-none-match", "") == "\"1100\"") {

Nit: I don't think there's a second argument to `getHeader`

::: services/common/tests/unit/test_remote_settings_poll.js:134
(Diff revision 2)
> +  });
> +  c.maybeSync = () => { maybeSyncCalled = true; };
> +
>    await RemoteSettings.pollChanges();
> +
> +  Assert.ok(!maybeSyncCalled, "maybeSync should not be called");

Is there an expected behavior here for `remote-settings-changes-polled`?

Comment 14

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

https://reviewboard.mozilla.org/r/246610/#review252934

Comment 15

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246610/diff/2-3/

Comment 16

[Mathieu Leplatre [:leplatrem]]

Cannot checkin without suitable reviewer :/

Comment 17

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246610/diff/3-4/

Comment 18

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

https://reviewboard.mozilla.org/r/246610/#review255040

Comment 19

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ce3d42805d89fe6fba06a44bb8a0ead2f4afab2a -d aadcedf5a07e: rebasing 466774:ce3d42805d89 "Bug 1454970 - Decouple Remote Settings synchronization from initialization order r=glasserc,mgoodwin" (tip)
merging services/settings/remote-settings.js and services/common/remote-settings.js to services/settings/remote-settings.js
merging services/settings/test/unit/test_remote_settings_poll.js and services/common/tests/unit/test_remote_settings_poll.js to services/settings/test/unit/test_remote_settings_poll.js
warning: conflicts while merging services/settings/remote-settings.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging services/settings/test/unit/test_remote_settings_poll.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 20

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246610/diff/4-5/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eadf17764c12
Decouple Remote Settings synchronization from initialization order r=glasserc,mgoodwin

Comment 22

[Dorel Luca [:dluca]]

Backed out changeset eadf17764c12 (bug 1454970) for XPCShell Failure on services/settings/test/unit/test_remote_settings_poll.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=181803355&repo=autoland&lineNumber=1327

TEST-PASS | services/settings/test/unit/test_remote_settings.js | took 25670ms
[task 2018-06-04T23:21:27.070Z] 23:21:27     INFO -  TEST-START | services/settings/test/unit/test_remote_settings_poll.js
[task 2018-06-04T23:21:37.110Z] 23:21:37  WARNING -  TEST-UNEXPECTED-FAIL | services/settings/test/unit/test_remote_settings_poll.js | xpcshell return code: 0
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  TEST-INFO took 10036ms
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  >>>>>>>
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | xpcw: cd /sdcard/tests/xpc/services/settings/test/unit
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/sdcard/tests/xpc/services/settings/test/unit/../../../common/tests/unit/head_global.js", "/sdcard/tests/xpc/services/settings/test/unit/../../../common/tests/unit/head_helpers.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_remote_settings_poll.js"]; -e const _TEST_NAME = "services/settings/test/unit/test_remote_settings_poll.js" -e _execute_test(); quit(0);
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | JavaScript strict warning: resource://services-common/kinto-http-client.js, line 1639: ReferenceError: reference to undefined property "initializer"
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | JavaScript strict warning: resource://services-common/kinto-http-client.js, line 1102: ReferenceError: reference to undefined property "initializer"
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | JavaScript strict warning: resource://services-common/kinto-http-client.js, line 347: ReferenceError: reference to undefined property "initializer"
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  running event loop
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "initializer"" {file: "resource://services-common/kinto-http-client.js" line: 1639}]"
[task 2018-06-04T23:21:37.110Z] 23:21:37     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "initializer"" {file: "resource://services-common/kinto-http-client.js" line: 1102}]"
[task 2018-06-04T23:21:37.111Z] 23:21:37     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "initializer"" {file: "resource://services-common/kinto-http-client.js" line: 347}]"
[task 2018-06-04T23:21:37.111Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | Starting clear_state
[task 2018-06-04T23:21:37.111Z] 23:21:37     INFO -  (xpcshell/head.js) | test clear_state pending (2)
[task 2018-06-04T23:21:37.112Z] 23:21:37     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-06-04T23:21:37.112Z] 23:21:37     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2018-06-04T23:21:37.112Z] 23:21:37     INFO -  (xpcshell/head.js) | test clear_state finished (2)
[task 2018-06-04T23:21:37.113Z] 23:21:37     INFO -  services/settings/test/unit/test_remote_settings_poll.js | Starting test_check_success
[task 2018-06-04T23:21:37.114Z] 23:21:37     INFO -  (xpcshell/head.js) | test test_check_success pending (2)
[task 2018-06-04T23:21:37.114Z] 23:21:37     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2018-06-04T23:21:37.115Z] 23:21:37     INFO -  TEST-PASS | services/settings/test/unit/test_remote_settings_poll.js | test_check_success - [test_check_success : 101] maybeSync was called - true == true

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=eadf17764c12fdc70a560a549e649ee4960b0128&filter-searchStr=Android%204.3%20API16%20%20opt%20Xpcshell%20tests%20test-android-4.3-arm7-api-16%2Fopt-xpcshell-6%20X(X6)

Backout:
https://hg.mozilla.org/integration/autoland/rev/cba6d551e43e6a96eee178b5342225368a4b3130

Comment 23

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246610/diff/5-6/

Comment 24

[Mathieu Leplatre [:leplatrem]]

Tests fixed on Android:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3deb531a2e03b615d1e2bddce5f4ec3b91c112d6

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d6eaf83cf3f
Decouple Remote Settings synchronization from initialization order r=glasserc,mgoodwin

Comment 26

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/5d6eaf83cf3f