1309774 - The bookmark validator should ignore or otherwise special-case local items that Sync will ignore

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

Sync now is far more strict about what items it will upload from the client to the server. However, the bookmark validator reports some of these items as "appearing on the client but missing from the server". We need to be a little smarter here - there's no point reporting this as a problem when Sync goes out of its way to ensure they don't end up on the server.

I don't think we should simply ignore them, but instead should treat them as "server unexpected" if they do appear on the server, but be silent about them if they don't appear on the server.

See bug 1309255 for an example of this.

Comment 1

[Lina Butler [:lina]]

From triage: let's add a validation version number to the Sync ping, too.

Comment 2

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

Attachment: Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server.

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

Comment 3

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

Attachment: Bug 1309774 - Part 2. Add version number to validation data in sync ping.

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

Comment 4

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

Note that this doesn't validate the client-side part, and only tells the server to ignore them (e.g. I still have a million duplicated left pane roots, but it's not clear that these are doing any harm).

A cool side effect is that with this patch, I only have a single validation error: a non-structual difference for the mobile root's title: "mobile" (server) vs "Mobile Bookmarks" (client), which would be fixed by 1311782.

Comment 5

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

Comment on attachment 8803068 [details]
Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server.

https://reviewboard.mozilla.org/r/87290/#review87998

LGTM, thanks!

::: services/sync/modules/bookmark_validator.js:148
(Diff revision 1)
>      }
>      return result;
>    }
>  }
>  
> +// Defined lazily to avoid initializing PlacesUtils.bookmarks too soon.

Even though this patch just moved this list, I think we should arrange to use getChangeRootIds() in bookmarks.js somehow - maybe ask Kit to see if he thinks it should be in PlacesSyncUtils, otherwise we'll just expose it directly from bookmarks.js

Comment 6

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

Comment on attachment 8803069 [details]
Bug 1309774 - Part 2. Add version number to validation data in sync ping.

https://reviewboard.mozilla.org/r/87292/#review88192

::: services/sync/modules/telemetry.js:176
(Diff revision 1)
>    recordValidation(validationResult) {
>      if (this.validation) {
>        log.error(`Multiple validations occurred for engine ${this.name}!`);
>        return;
>      }
> -    let { problems, duration, recordCount } = validationResult;
> +    let { problems, version, duration, recordCount } = validationResult;

Kit put me on to some syntax I hadn't seen before, meaning this could be written as:

`let { problems, version = 0, duration, recordCount: checked = 0 } = validationResult;`

or something like that :) Not saying you need to do that here, but thought I'd share :)

Comment 7

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

Comment on attachment 8803068 [details]
Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server.

https://reviewboard.mozilla.org/r/87290/#review87998

> Even though this patch just moved this list, I think we should arrange to use getChangeRootIds() in bookmarks.js somehow - maybe ask Kit to see if he thinks it should be in PlacesSyncUtils, otherwise we'll just expose it directly from bookmarks.js

Took me an embarassing amount of time to realize that this doesn't work since getChangeRootIds isn't guids or sync guids, it's bookmark IDs (e.g. integers).

Comment 8

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

Comment on attachment 8803068 [details]
Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server.

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

Comment 9

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

Comment on attachment 8803069 [details]
Bug 1309774 - Part 2. Add version number to validation data in sync ping.

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

Comment 10

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

Comment on attachment 8803068 [details]
Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server.

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

Comment 11

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

Comment on attachment 8803069 [details]
Bug 1309774 - Part 2. Add version number to validation data in sync ping.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64001171c0c7
Part 1. Sync bookmark validator should only expect to see children of roots on server. r=markh
https://hg.mozilla.org/integration/autoland/rev/637a0c2c0608
Part 2. Add version number to validation data in sync ping. r=markh

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/64001171c0c7
https://hg.mozilla.org/mozilla-central/rev/637a0c2c0608