1321570 - Cleanups and refactors to ext-storage-sync code

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Ethan Glasser-Camp (:glasserc)]

1. Right now, to not break Android, we conditionally-define some stuff in ExtensionStorageSync because it depends on stuff that I put in extension-storage.js (which isn't included on Android). Move that code from services/sync/extension-storage.js into ExtensionStorageSync so that it's all in one place. This should simplify some code, even though it still won't work on Android.

2. Right now, because it seems like the "done thing", we define `_fxaService` attributes which point to the `fxAccounts` singleton. This is not ideal because it means that testing anything that depends on `fxAccounts` requires monkeypatching. Instead, take a reference to an `fxAccounts` at construction time, and test an `ExtensionStorageSync` instance. (We can still provide a singleton ExtensionStorageSync since we need to communicate between sync code and webextensions code.

3. The test_ext_storage_sync.js has a mock Kinto server which is stupid and ugly. Rather than trying to simulate what a Kinto server might do, it provides a bunch of hacks like "serve a record once" or "serve a record but only if a certain query argument is missing". This could be cleaned up if the server was a bit smarter. In particular, it should support an explicit notion of "time". Records appear at a certain time. Treat _since correctly. Set the server's notion of time accordingly.

Comment 1

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6fe29a8fc8

Comment 2

[Ethan Glasser-Camp (:glasserc)]

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

Comment 3

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

Since services/sync doesn't ship on Android, this meant
conditionally-defining some variables such as `cryptoCollection` and
`CollectionKeyEncryptionRemoteTransformer` depending on whether or not
we were on Android. However, none of these definitions really rely on
functionality that isn't present on Android (although you can't really
use them yet either). Move the dependency together with the dependant
code so we can simplify a bit. This lets us remove conditional uses of
`cryptoCollection` and `CollectionKeyEncryptionRemoteTransformer`.

Because the WebExtensions source directory has more stringent eslint
rules, we end up reformatting and commenting a bit in addition to
moving.

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

Comment 4

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1321570 - use dependency injection for fxAccounts,

Get rid of the ugly hack where test code monkeypatches a singleton to
convince the rest of the ExtensionStorageSync code that a fake user is
logged in. Instead, take a handle to the fxAccounts service at
construction time. Test code can provide any kind of fxAccounts it
wants, including one that has a hard-coded user.

This provokes a bunch of changes:

- ExtensionStorageSync is now no longer a singleton, but a class. You
  have to instantiate it in order to do anything with it. A global
  instance extensionStorageSync is provided for use by Sync code as
  well as WebExtension code.

- CryptoCollection is now also a class, and each ExtensionStorageSync
  instance has its own CryptoCollection. This dependency should maybe
  also be injected, but for the time being it doesn't provide us any
  value to do so.

- Converting singletons with asynchronous methods to classes is a pain
  in the neck. We convert async method foo from `foo:
  Task.async(....)` to `get foo() { return Task.async(....) }` and
  then reindenting the method. This is semantically equivalent, but
  quite ugly. Hopefully we get async/await at some point and we can
  clean this up.

- Update Sync code and WebExtension code to use extensionStorageSync.

- There's a cyclic dependency where CryptoCollection#sync depends on
  ExtensionStorageSync#_syncCollection which depends on
  CryptoCollection#getKeyRing. As a short-term hack, we now require an
  ExtensionStorageSync argument to CryptoCollection#sync.

- KeyRingEncryptionRemoteTransformer now takes a handle to fxAccounts
  on construction time as well. Because this is the only
  EncryptionRemoteTransformer subclass that accesses fxAccounts, we
  can get rid of the hack where the tests monkeypatch something in the
  EncryptionRemoteTransformer prototype.

- CollectionKeyEncryptionRemoteTransformer now takes a handle to a
  CryptoCollection, rather than relying on a global one.

- A bunch of methods that previously assumed access to fxAccounts now
  check if fxAccounts is present (i.e. if we're on Android). Strictly
  speaking, this isn't required by this change, but it helps make
  certain kinds of failure a little easier to diagnose.

- Update tests, including extracting a domain-specific helper method
  to hide the use of CollectionKeyEncryptionRemoteTransformer. We now
  no longer monkeypatch in our mock fxaService, but pass it to the
  test so that it can do whatever it wants with it. We also provide an
  ExtensionStorageSync instance for convenience. Access to the global
  cryptoCollection is now done through an ExtensionStorageSync
  instance.

To summarize, we've gone from a situation where a bunch of singletons
had implicit dependencies on other singletons in a shared global
namespace, to a situation where dependencies are provided explicitly
using method/constructor arguments. This highlights some of the
dependencies present:

- ExtensionStorageSync depends on CryptoCollection and fxAccounts if
  it needs to sync

- Every collection created via openCollection needs a handle to
  CryptoCollection so it can correctly create its remote transformers

- CryptoCollection needs a handle to fxAccounts so it can create its
  own remote transformer for its special collection

Most of this is only possible, or at least much easier, because we no
longer try to juggle Sqlite connections but just keep one around
forever.

However, please note:

- CryptoCollection needs a handle to ExtensionStorageSync to actually
  perform syncing logic because that's where we foolishly put the
  logic to make requests

- There's still a backing Sqlite store which is shared by everything

- There's still a singleton tracking contexts that opened extensions
  which we manage to try to clean contexts up correctly

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

Comment 5

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1321570 - Rewrite KintoServer in tests,

Allow individual records to decide whether or not they want to appear
in a certain request, and be more explicit with our handling of time,
since the client is already tracking this using etag and providing it
to us using _since. This allows us to get rid of hacks like
`encryptAndAddRecordOnlyOnce` and `addRecordInPast`. While we're here,
change the method signatures for `addRecord` to make them more
flexible. Also, make the server agnostic about "collections" and instead
focus only on records.

Conflicts are still something we have to handle explicitly because in
order to trigger conflict behavior in the client, they have to appear
between two client requests (i.e. within a sync() call). So we don't
show them on GET requests but surface them on POST requests.

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

Comment 6

[Ethan Glasser-Camp (:glasserc)]

OK, here's a patch series. I realize most of the commits are pretty big and I normally would have split them into smaller ones, but I wasn't sure the changes made sense individually and Mozilla seems to prefer them a little larger. Anyhow, let me know if you want me to change them.

Comment 7

[Ethan Glasser-Camp (:glasserc)]

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

Comment 8

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825894 [details]
Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

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

Comment 9

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

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

Comment 10

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

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

Comment 11

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2100f225f719

Comment 12

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825894 [details]
Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

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

Comment 13

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

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

Comment 14

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

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

Comment 15

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6240ac3dac04

Comment 16

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

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

Comment 17

[Ethan Glasser-Camp (:glasserc)]

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

Comment 18

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1321570: Remove duplicate tests,

These tests are essentially the same as the tests of the same name in
test_ext_storage.js. These tests could maybe be valuable if they were
content script tests instead of background script tests, but they
aren't, so let's delete them.

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

Comment 19

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825894 [details]
Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

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

Comment 20

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

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

Comment 21

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

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

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8825894 [details]
Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

https://reviewboard.mozilla.org/r/103966/#review108378

Comment 23

[Kris Maglione [:kmag]]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

https://reviewboard.mozilla.org/r/103968/#review108382

Looks like a significant improvement, but some minor issues.

::: services/sync/tests/unit/test_extension_storage_engine.js:49
(Diff revision 4)
> -  let oldSync = ExtensionStorageSync.syncAll;
> -  let syncMock = ExtensionStorageSync.syncAll = mock({returns: Promise.resolve()});
> +  let oldSync = ExtensionStorageSync.prototype.syncAll;
> +  let syncMock = ExtensionStorageSync.prototype.syncAll = mock({returns: Promise.resolve()});

Is there a reason we can't just change this on the instance rather than the prototype?

::: toolkit/components/extensions/ExtensionStorageSync.jsm:38
(Diff revision 4)
>  const KINTO_REQUEST_TIMEOUT = 30000;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  const {
> +  ExtensionUtils,
>    runSafeSyncWithoutClone,

We shouldn't be doing any destructuring here at all. `runSafeSyncWithoutClone` should be destructured from `ExtensionUtils` below.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:278
(Diff revision 4)
> +    const self = this;
>      return Task.spawn(function* () {

Please either make this an async function, or add `.bind(this)` to the function you pass to `Task.spawn`. Same elsewhere.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:416
(Diff revision 4)
> +  get getCollection() {
> +    return Task.async(function* () {

Please just use `async getCollection() {...}` here. If that's not an option, for some reason, just make this an ordinary method and `return Task.spawn(...)`

Same elsewhere.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:825
(Diff revision 4)
> +      if (!this._fxaService) {
> +        throw new ExtensionError("Use of chrome.storage.sync \"keyring\" impossible because " +
> +                                 "FXAccounts is not available; are you on Android?");
> +      }

You do this in quite a lot of places. Please add a helper method.

Also, I'm not sure we want an `ExtensionError` for this. This is more of an internal error.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:902
(Diff revision 4)
>  <span class="indent">>></span>      // changes is when a new keyring is uploaded, which only happens
>  <span class="indent">>></span>      // after a server wipe. So when we get a "conflict" (resolved by
>  <span class="indent">>></span>      // server_wins), we check whether the server version has a new
>  <span class="indent">>></span>      // UUID. If so, reset our sync status, so that we'll reupload
>  <span class="indent">>></span>      // everything.
> -      const result = yield cryptoCollection.sync();
> +        const result = yield this.cryptoCollection.sync(this);

Why don't we just pass `this` to the `CryptoCollection` constructor and store it?

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

https://reviewboard.mozilla.org/r/103970/#review108394

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:51
(Diff revision 5)
>      this.httpServer = new HttpServer();
>      this.httpServer.start(-1);
>  
> -    // Map<CollectionId, Set<Object>> corresponding to the data in the
> -    // Kinto server
> -    this.collections = new Map();
> +    // Set<Object> corresponding to records that might be served.
> +    // The format of these objects is defined in the documentation for #addRecord.
> +    this.records = new Set();

Why is this a `Set`? It looks like it would work just as well an array, which would remove the need for the `Array.from` call below.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8829983 [details]
Bug 1321570: Remove duplicate tests,

https://reviewboard.mozilla.org/r/106936/#review108396

Comment 26

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

https://reviewboard.mozilla.org/r/103968/#review108382

> Is there a reason we can't just change this on the instance rather than the prototype?

No, apparently not.

> Please either make this an async function, or add `.bind(this)` to the function you pass to `Task.spawn`. Same elsewhere.

As discussed on IRC, I will hold off on making any other changes like this to code that isn't touched in this commit. That conversion can happen as a follow-up bug.

> Please just use `async getCollection() {...}` here. If that's not an option, for some reason, just make this an ordinary method and `return Task.spawn(...)`
> 
> Same elsewhere.

Thanks, I didn't realize we had async/await already!

> Why don't we just pass `this` to the `CryptoCollection` constructor and store it?

There probably isn't a good reason, but here are my bad ones:

- This is the only method that actually needs access to an ExtensionStorageSync object, so storing it makes for a tighter coupling than is necessary.
- I don't actually want there to be a dependency on ExtensionStorageSync, so formalizing the link between the two objects is the opposite of what I want.
- Cycles are just instinctively scary to me.

Comment 27

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825894 [details]
Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

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

Comment 28

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

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

Comment 29

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

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

Comment 30

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8829983 [details]
Bug 1321570: Remove duplicate tests,

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

Comment 31

[Kris Maglione [:kmag]]

(In reply to Ethan Glasser-Camp (:glasserc) from comment #26)
> - Cycles are just instinctively scary to me.

Cycles aren't a problem for for the JS GC. All that matters is whether the object is reachable from a root, and cycles don't contribute to that one way or the other.

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

https://reviewboard.mozilla.org/r/103968/#review114268

::: toolkit/components/extensions/ExtensionStorageSync.jsm:268
(Diff revision 5)
> +    super();
> +    this._fxaService = fxaService;
> +  }
> +
>    getKeys() {
> +    complainIfNoFxA(this._fxaService, "encrypting chrome.storage.sync records");

This should be `throwIf...` or `assert...`. `complain` makes it sound like it just logs.

::: toolkit/components/extensions/ExtensionStorageSync.jsm:442
(Diff revision 5)
> +    if (!this._fxaService) {
> +      throw new Error("tried to access cryptoCollection without fxaService; are you on Android?");
> +    }

Please use helper for this.

Comment 33

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

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

Comment 34

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825896 [details]
Bug 1321570 - Rewrite KintoServer in tests,

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

Comment 35

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8829983 [details]
Bug 1321570: Remove duplicate tests,

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

Comment 36

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8825895 [details]
Bug 1321570 - use dependency injection for fxAccounts,

https://reviewboard.mozilla.org/r/103968/#review108382

> There probably isn't a good reason, but here are my bad ones:
> 
> - This is the only method that actually needs access to an ExtensionStorageSync object, so storing it makes for a tighter coupling than is necessary.
> - I don't actually want there to be a dependency on ExtensionStorageSync, so formalizing the link between the two objects is the opposite of what I want.
> - Cycles are just instinctively scary to me.

It seems like this is OK with :John-Galt (per communication on IRC), even though cycles aren't scary unless you're a Python developer.

Comment 37

[Ethan Glasser-Camp (:glasserc)]

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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca00e2c5420d
Move ExtensionStorageSync crypto out of services/sync, r=kmag
https://hg.mozilla.org/integration/autoland/rev/28fdc9533636
use dependency injection for fxAccounts, r=kmag
https://hg.mozilla.org/integration/autoland/rev/98f976334918
Rewrite KintoServer in tests, r=kmag
https://hg.mozilla.org/integration/autoland/rev/0af3de0a7fe9
Remove duplicate tests, r=kmag

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca00e2c5420d
https://hg.mozilla.org/mozilla-central/rev/28fdc9533636
https://hg.mozilla.org/mozilla-central/rev/98f976334918
https://hg.mozilla.org/mozilla-central/rev/0af3de0a7fe9