1407067 - Move Sync's validation and repair into a system addon

Status: RESOLVED WONTFIX

(Firefox :: Sync, enhancement, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

We only run validation and repair down to the beta channel and only when there are < 1k bookmarks because (a) we have no way to enable or disable it if we find issues and (b) waiting for the trains makes for a very poor feedback loop.

A reasonable solution to this might be to ship these components as a system addon, which means we can release changes directly to the release channel and also remotely enable or disable it.

There are a few unknowns here, but there are enough system addons around that we can stand on the shoulders of giants.

This will almost certainly turn into a meta-bug.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

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

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

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

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

This is mostly complete. The tests all pass and it seems to work for me when I run a local build of it. The biggest remaining thing is that I haven't put much thought into how we're doing preferences. (I think we want to specify them inside the addon, but I don't know the best way to do that right now. We also might want or need them to be in the branch extension.sync-repair or something.)

I also don't know how the "live" update process works with extensions. (since they can be updated without a restart, no?) Regardless, I believe this code should work fine, but it's possible I have to do something to get a new version of various jsms. That still needs to be looked into.

The files are now `.jsm`s so that I can glob them with *.jsm. It's possible I could have put them in a subdirectory, but I don't entirely understand the syntax of the jar.mn file, and kind of stole what's there from formautofill and other addons nearby (This is true for some other build configuration type things as well). 

The diff looks larger than it is since I changed a function called by most tests so that it still works and more accurately reflects what it does (especially after my changes). Specifically enableValidationPrefs() => forceBookmarkValidation().

I think those are all of the comments I had about the current state.

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

I forgot to add the [go-faster-system-addon] whiteboard tag back when I wrote the intent to implement [0]. Hopefully nobody minds too much given that there wasn't a lot of change on it until now. (Sorry though!)

[0]: https://mail.mozilla.org/pipermail/gofaster/2017-October/000791.html

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Oh, I also forgot to update TPS. But I'll get to that tomorrow or sometime soon.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

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

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

Same as before but now with the necessary TPS changes.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

https://reviewboard.mozilla.org/r/197416/#review203642

Awesome! I haven't looked too deeply at this, but wanted to say something before I took off for the weekend :) It LGTM at a quick glance, and I'll consider the other issues you raise next week (but in the meantime, feel free to charge ahead :)

::: browser/extensions/sync-repair/bookmark_repair.jsm:488
(Diff revision 3)
> +    return null;
> +  }
> +
> +  /* Is the passed client record suitable as a repair responder?
> +  */
> +  _isSuitableClient(client) {

This raises an interesting question - we will only be able to choose clients with the addon available, right? We probably need to consider how to handle that (eg, maybe we will want to extend the client record with "capabilities" or something?)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

https://reviewboard.mozilla.org/r/197416/#review203644

(or maybe the responder should remain in the core?)

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

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

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8926166 [details]
Bug 1407067 - Move syncs validation and repair code into a system addon. f?markh

I wish feedback requests weren't cleared for updates via mozreview. Oh well, maybe someday Phabricator will become a reality.

Main change is that this actually unloads the jsms on shutdown. Apparently this is necessary. I opted not to hard code a list of our modules since I don't trust myself to keep it up to date.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Thanks Thom, but as per the email discussions, we aren't going to move ahead with this currently (but we can always revisit it later!)