Currently bookmark validation in sync is sub-optimal, the largest issue being a number of false positives that it reports, mostly around livemarks and queries. There are also a few smaller API usability issues.

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

Comment on attachment 8753017 [details] MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh Requesting feedback because it's possible/likely this is missing some things we talked about, or other things that would be useful for aboutsync.

https://reviewboard.mozilla.org/r/52942/#review49860 At a first glance this looks good, but we should probably do the (just opened) bug 1273352 first so we can ensure there are no obvious false-positives remaining in our own personal/dev accounts.

Comment on attachment 8753017 [details] MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52942/diff/1-2/

Comment on attachment 8753017 [details] MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52942/diff/2-3/

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

Comment on attachment 8753017 [details] MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52942/diff/3-4/

This is mainly what is described in bug 1274394 comment 3, 4, and 5. I also split out structural differences from differences, since they represent more serious errors, and when displaying a summary that distinction probably becomes more important. FWIW I think we should consider what to do about the mobile root before landing this, but maybe not.

Comment on attachment 8754910 [details] MozReview Request: Bug 1273234 - Add method for summarizing sync bookmark problems generically. r?markh https://reviewboard.mozilla.org/r/54306/#review51126 This looks good, but... > FWIW I think we should consider what to do about the mobile root before landing > this, but maybe not Yeah :( And note that it's more than just the mobile root - it's all with excludeFromBackup, and I don't think we can block landing this on both fixing those bugs *and* repairing the server data - but at the same time, it seem pointless to have TPS fail if they hit them, and TBH it seems pointless recording this stuff in telemetry when we fully expect basically every single user to have that issue. So I'm wondering if we could maybe annotate that specific problem with, say, a "knownBug: true" attribute. Presumably at some point in the future we'd end up both fixing the bug and performing a "repair" of the server data, at which point we'd remove that annotation so it magically started being treated as "real". We might also find that some other types of "corruption" are similar bugs and thus we know will be very common. (I'm not too keen on the "knownBug" name, but can't think of a better one. Another alternative might be, say, a set of flags - eg, PROBLEM_TYPE_KNOWN_BUG for this kind of issue, maybe "PROBLEM_TYPE_COMMON where we don't know the specific reason but do know that many many people have it (and this one we might, say, still report in TPS but not bother reporting in telemetry until we get a handle on it), and later we might add, say, PROBLEM_TYPE_REPAIRABLE for issues we know how to fix. The new getSummary() method could then take the flags to ignore (with a default of "report everything", but TPS might say "Exclude bugs". OTOH, maybe this is getting over-generic and simple attributes starting with knownBug might be good enough and we can leave the filtering to the callers. I'm just thinking out loud here - what do you think?

Comment on attachment 8753017 [details] MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh https://reviewboard.mozilla.org/r/52942/#review51120 Looks good, but let's nail part 2 down before landing. ::: services/sync/modules/bookmark_validator.js:60 (Diff revision 4) > case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR: > itemType = 'separator'; > break; > } > > + if (treeNode.tags && !Array.isArray(treeNode.tags)) { is the array check actually needed here? If it is, I'd like to understand why as it might point at a different problem.

Comment on attachment 8754910 [details] MozReview Request: Bug 1273234 - Add method for summarizing sync bookmark problems generically. r?markh https://reviewboard.mozilla.org/r/54306/#review51168 In retrospect, I think that was a dumb idea :) I think it's fine to report these problems in telemetry (and we can see the numbers come down later) and that tps itself should just "blacklist" what it considers a "known failure" in a hacky way. So yeah, go for it!

Comment on attachment 8753017 [details] MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52942/diff/4-5/

Comment on attachment 8754910 [details] MozReview Request: Bug 1273234 - Add method for summarizing sync bookmark problems generically. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54306/diff/1-2/

https://hg.mozilla.org/integration/fx-team/rev/66845689332c https://hg.mozilla.org/integration/fx-team/rev/cf20e55e8550

https://hg.mozilla.org/mozilla-central/rev/66845689332c https://hg.mozilla.org/mozilla-central/rev/cf20e55e8550