1273234 - Improve sync's client/server bookmark validation code

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

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.

Comment 1

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

Attachment: MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh

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

Comment 2

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

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.

Comment 3

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

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 4

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

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 5

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

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/

Comment 6

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

Attachment: MozReview Request: Bug 1273234 - Add method for summarizing sync bookmark problems generically. r?markh

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

Comment 7

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

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/

Comment 8

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

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 9

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

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 10

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

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 11

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

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 12

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

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 13

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

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/

Comment 14

[Pulsebot]

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

Comment 15

[Carsten Book [:Tomcat]]

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