Closed Bug 1273234 Opened 4 years ago Closed 4 years ago

Improve sync's client/server bookmark validation code

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
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.
Attachment #8753017 - Flags: feedback?(markh)
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/
Attachment #8753017 - Flags: feedback?(markh)
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/
Attachment #8753017 - Attachment description: MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. f?markh → MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator.
Blocks: 1274394
Review commit: https://reviewboard.mozilla.org/r/54306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54306/
Attachment #8753017 - Attachment description: MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. → MozReview Request: Bug 1273234 - Reduce the number of false positives reported by sync's bookmark_validator. r?markh
Attachment #8754910 - Flags: review?(markh)
Attachment #8753017 - Flags: review?(markh)
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.
Attachment #8754910 - Flags: review?(markh)
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?
Attachment #8753017 - Flags: review?(markh) → review+
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!
Attachment #8754910 - Flags: review+
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/mozilla-central/rev/66845689332c
https://hg.mozilla.org/mozilla-central/rev/cf20e55e8550
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.