Closed Bug 1321570 Opened 4 years ago Closed 4 years ago

Cleanups and refactors to ext-storage-sync code

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: glasserc, Assigned: glasserc)

Details

(Whiteboard: triaged)

Attachments

(4 files)

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.
Assignee: nobody → eglassercamp
Whiteboard: triaged
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 on attachment 8825894 [details]
Bug 1321570 - Move ExtensionStorageSync crypto out of services/sync,

https://reviewboard.mozilla.org/r/103966/#review108378
Attachment #8825894 - Flags: review?(kmaglione+bmo) → review+
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?
Attachment #8825895 - Flags: review?(kmaglione+bmo)
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.
Attachment #8825896 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8829983 [details]
Bug 1321570: Remove duplicate tests,

https://reviewboard.mozilla.org/r/106936/#review108396
Attachment #8829983 - Flags: review?(kmaglione+bmo) → review+
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.
(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 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.
Attachment #8825895 - Flags: review?(kmaglione+bmo) → review+
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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.