1286915 - Implement a validator for the sync addon engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

Similar to the bookmark validator from bug 1265419. This should integrate with telemetry in the same way as the bookmark validator does in bug 1267917.

This seems like a natural first step after the bookmark validator, given that the addons engine seems to have a number of bugs associated with it, and is also quite visible.

Comment 1

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

Attachment: Bug 1286915 - Implement a validator for the sync addons engine

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

Comment 2

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

Initial version, needs tests. You can check that it works (let me know if it doesn't, it seems to be reporting legitimate issues for me) using https://github.com/mhammond/aboutsync/pull/8.

I'm also unsure about some of the logic for when something should/shouldn't be synced.

Comment 3

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

https://reviewboard.mozilla.org/r/66230/#review64734

This looks fine to me - it looks a little over-wrought just for addons, but I'm sure it will become clearer once we have some other validators too.

::: services/sync/modules/collection_validator.js:30
(Diff revision 1)
> +    this.serverUnexpected = [];
> +    this.differences = [];
> +  }
> +}
> +
> +class CollectionValidator {

I thnk we should get some brief comments for each of the functions here, and the args to the ctor.

::: services/sync/modules/collection_validator.js:130
(Diff revision 1)
> +      }
> +    }
> +
> +    for (let [id, { server, client }] of allRecords) {
> +      if (!client && !server) {
> +        throw new Error("Impossible: no client or server record for "+id);

spaces :)

::: services/sync/modules/engines/addons.js:737
(Diff revision 1)
> +      "source"
> +    ]);
> +    this.engine = engine;
> +  }
> +
> +  getClientItems() {

This doesn't seem used in this patch? Assuming a later one does use it, can the "api" be promise based?

Comment 4

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

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

Comment 5

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

https://reviewboard.mozilla.org/r/66230/#review64734

Yes, it's certainly overengineerd for addons.

> This doesn't seem used in this patch? Assuming a later one does use it, can the "api" be promise based?

It's not used in this patch, no. Yes, it can be promise based.

Comment 6

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

https://reviewboard.mozilla.org/r/66230/#review72270

This looks good - and has tests :)

However, I suspect the addon validator might be trickier than the other engines in edge-cases I'm yet to fully think through - eg, I almost expect that if a user is experiencing bug 1285866, we will report problems that aren't actual problems with Sync but instead with the repository itself. We might still want to report *something* for that case, but almost certainly not "all addons are wrong" or similar.

I think those other validators are less problematic, so it might make sense to move the new base validator into one of those other bugs and we can take a more considered approach to the addons validator as a lower priority.

Comment 7

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

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

Comment 8

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

https://reviewboard.mozilla.org/r/66230/#review79648

This is looking good, but I'd like to better understand some of these changes.

::: services/sync/modules/engines/addons.js:545
(Diff revision 3)
>     * @param  addon
>     *         Addon instance
> +   * @param ignoreRepoCheck
> +   *         Should we skip checking the Addons repository (primarially useful
> +   *         for testing and validation, usually configured by the
> +   *         "addons.ignoreRepositoryChecking" sync pref).

IIUC, the pref here is actually services.sync.addons...

However, I don't understand why we need this new pref given AddonRepository.cacheEnabled is itself controlled by a pref. I see that the validator skips this check, but given that always passes true I don't understand the checking of the new pref above.

::: services/sync/modules/engines/addons.js:813
(Diff revision 3)
> +
> +  syncedByClient(item) {
> +    return !item.original.hidden &&
> +           !item.original.isSystem &&
> +           !(item.original.pendingOperations & AddonManager.PENDING_UNINSTALL) &&
> +           this.engine.isAddonSyncable(item.original, true);

why do we need to make this check here? I'm also not sure about the PENDING_UNINSTALL check - IIUC, when an addon that requires restart is pending uninstall we will still consider it synced by the client (as that uninstall should be synced).

::: services/sync/tests/tps/test_addon_nonrestartless_xpi.js:37
(Diff revision 3)
>    [Addons.install, [id]],
>    [Sync]
>  ]);
>  Phase("phase02", [
> -  [Addons.verify, [id], STATE_ENABLED]
> +  [Addons.verify, [id], STATE_ENABLED],
> +  [Sync]

I'd like to better understand why this additional sync is necessary. Phase 1 and 2 are both against the same profile, so IIUC the second phase is really just to ensure the state of the addon after the required restart - but I don't see why an additional sync is needed here? Ditto the other new syncs below.

::: services/sync/tests/tps/test_addon_reconciling.js:37
(Diff revision 3)
>  
>  // Now we disable in one and uninstall in the other.
>  Phase("phase03", [
>    [Sync], // Get GUID updates, potentially.
>    [Addons.setEnabled, [id], STATE_DISABLED],
> +  [Addons.skipValidation] // We don't want this profile ot sync until phase5

typo in the comment - but I don't understand the comment - that line doesn't seem to prevent a sync but instead prevents a validation. But even then, why must we skip validation here, and what will prevent the validator running in the same cases for a user?

::: services/sync/tps/extensions/tps/resource/tps.jsm:468
(Diff revision 3)
>        throw(e);
>      }
>    },
>  
>    HandleAddons: function (addons, action, state) {
> +    this.shouldValidateAddons = true;

I wonder if some of the above changes could be nuked if here we set a flag, say, this.handledAddons and only setting .shouldValidateAddons on a successful sync? IIUC, that's how the validator itself works (ie, it only validates after a successful Sync) so it makes sense TPS would do the same.)

::: services/sync/tps/extensions/tps/resource/tps.jsm:687
(Diff revision 3)
> -        }
> +        }));
> -        Logger.AssertEqual(count, 0, `Password validation error of type ${name}`);
> -      }
> -    } catch (e) {
> +      } catch (e) {
> -      // Dump the client records (should always be doable)
> -      DumpPasswords();
> +        // ignore the error, the dump string is just here to make debugging easier.
> +        clientRecordDumpStr = "<Cyclic value>";

I've been meaning to ask this for a while - how do we get into this state? I guess it is because the validator annotates the client items? If so, is there a sane way to avoid that?

Comment 9

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

https://reviewboard.mozilla.org/r/66230/#review79648

> IIUC, the pref here is actually services.sync.addons...
> 
> However, I don't understand why we need this new pref given AddonRepository.cacheEnabled is itself controlled by a pref. I see that the validator skips this check, but given that always passes true I don't understand the checking of the new pref above.

Well, I meant by sync pref that that it's under the sync branch. But yeah that's definitely unclear/confusing.

And I agree, this pref is pointless and I've removed it. I don't remember my logic for adding it in the original addon validation patch.

> why do we need to make this check here? I'm also not sure about the PENDING_UNINSTALL check - IIUC, when an addon that requires restart is pending uninstall we will still consider it synced by the client (as that uninstall should be synced).

Right, but it shouldn't show up on the server, and this function is how we indicate that.

> I'd like to better understand why this additional sync is necessary. Phase 1 and 2 are both against the same profile, so IIUC the second phase is really just to ensure the state of the addon after the required restart - but I don't see why an additional sync is needed here? Ditto the other new syncs below.

It seems worthwhile to run a validation pass after the state is changed (and it will have changed after a restart).

We need to run a sync if we're going to run the validator for two reasons: 

1. https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/addons.js#135 -- although in this case the fact that this won't be initialized is probably not going to cause errors
2. We need to ensure we can actually get the records from the server, which requires Engine.prototype.engineURL to exist, which requires storageURL to have been initialized, which in practice requires a sync, IIUC.

We could disable running validation when we havent synced or changed state, but actually checking if we've changed state sounds tricky (given that it may have changed due to having been pending a restart)

> typo in the comment - but I don't understand the comment - that line doesn't seem to prevent a sync but instead prevents a validation. But even then, why must we skip validation here, and what will prevent the validator running in the same cases for a user?

As mentioned above, if we're going to run a validation, we need to sync first (for the reasons mentioned above, but also since we've changed state since the last sync and so the server state is expected to differ from the client state), and we don't want to sync mainly because we intentionally want to leave the server and client in a different state so that, err, we can track reconciliation after they both change independentally, otherwise this test would change what it's testing (Which might not matter, but didn't feel like a good thing to do to me -- If you think a sync there would be better than disabling validation, I'm willing to take your word for it).

> I wonder if some of the above changes could be nuked if here we set a flag, say, this.handledAddons and only setting .shouldValidateAddons on a successful sync? IIUC, that's how the validator itself works (ie, it only validates after a successful Sync) so it makes sense TPS would do the same.)

As mentioned above, it seems worthwhile to validate after if the client state has changed, as it does after a reboot (which requires a sync). The only time we run TPS without syncing seems to be addons, afaict.

> I've been meaning to ask this for a while - how do we get into this state? I guess it is because the validator annotates the client items? If so, is there a sane way to avoid that?

Well, specifically this happens because we assign `original` to the properties after normalization, which we mainly do for the benefit of aboutsync/aboutsync users, or potentially any other consumer of the validation data -- in the case of duplicates, it's possible just the ID is not enough.

Making `original` into a non-enumerable property would probably avoid the need for the explicit handling of cycles, but it would hide it in aboutsync as well (unless we specifically worked around it). I also am not sure making a guarantee that the data isn't cyclic is worthwhile -- although for these collections maybe it is (I don't see a sane way to do it for bookmarks without removing all the validators metadata, which IMO is worth having for debugging purposes).

It also seems worth noting that without running the normalizeClientItem, we wouldn't have marked these up, but, addons would serialize as an empty object, since all their properties are getters stored on their prototype (ugh...).

Comment 10

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

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

Comment 11

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

https://reviewboard.mozilla.org/r/66230/#review79648

> Right, but it shouldn't show up on the server, and this function is how we indicate that.

Whoops, forgot to mention that want to check that the addon is syncable because we want "unsyncable" addons on the server to be reported as errors.

Comment 12

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

Comment on attachment 8773486 [details]
Bug 1286915 - Implement a validator for the sync addons engine

https://reviewboard.mozilla.org/r/66230/#review80534

Your explanations all make sense, thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please mark the pending issues in MozReview as resolved so that this can be autolanded.

Comment 14

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

I tried to do this, however despite clearing them all out it still says there is one open -- Some digging leads me to believe this is bug 1292371 D:

Let me know if there's anything you need me to do here -- AFAICT I can't fix this from my side.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f6a964152400
Implement a validator for the sync addons engine. r=markh

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f6a964152400