Closed Bug 1309774 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: markh, Assigned: tcsc)

Details

Attachments

(2 files)

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.
From triage: let's add a validation version number to the Sync ping, too.
Assignee: nobody → tchiovoloni
Priority: -- → P1
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 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
Attachment #8803068 - Flags: review?(markh) → review+
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 :)
Attachment #8803069 - Flags: review?(markh) → review+
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).
Pushed by tchiovoloni@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/64001171c0c7
https://hg.mozilla.org/mozilla-central/rev/637a0c2c0608
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.