Closed Bug 1454970 Opened 6 years ago Closed 6 years ago

Provide a nice hook to initialize RemoteSettings clients

Categories

(Firefox :: Remote Settings Client, enhancement)

Product:
Firefox
Component:
Remote Settings Client
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1454970 - Decouple Remote Settings synchronization from initialization order
59 bytes, text/x-review-board-request
glasserc
: review+
mgoodwin
: review+
Details 
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
Flags: needinfo?(florian)
Assignee: nobody → mathieu
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.
Flags: needinfo?(florian)
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?
Flags: needinfo?(florian)
Blocks: 1453692
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.
Flags: needinfo?(florian)
(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?
Flags: needinfo?(mathieu)
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 ;)
Flags: needinfo?(mathieu)
Attachment #8974099 - Flags: feedback?(eglassercamp)
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?
> 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
Flags: needinfo?(gijskruitbosch+bugs)
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.
See Also: → 1460628
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8974099 - Attachment is obsolete: true
Attachment #8974099 - Flags: feedback?(eglassercamp)
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 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 on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

https://reviewboard.mozilla.org/r/246610/#review252934
Attachment #8980438 - Flags: review?(eglassercamp) → review+
Blocks: 1464346
Keywords: checkin-needed
Cannot checkin without suitable reviewer :/
Keywords: checkin-needed
Blocks: 1397061
Attachment #8980438 - Flags: review?(mgoodwin)
Target Milestone: --- → Firefox 62
Version: 57 Branch → Trunk
Comment on attachment 8980438 [details]
Bug 1454970 - Decouple Remote Settings synchronization from initialization order

https://reviewboard.mozilla.org/r/246610/#review255040
Attachment #8980438 - Flags: review?(mgoodwin) → review+
Keywords: checkin-needed
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)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eadf17764c12
Decouple Remote Settings synchronization from initialization order r=glasserc,mgoodwin
Keywords: checkin-needed
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
Flags: needinfo?(mathieu)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5d6eaf83cf3f
Decouple Remote Settings synchronization from initialization order r=glasserc,mgoodwin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5d6eaf83cf3f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: