Improve sync's client/server bookmark validation code

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Sync
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

Trunk
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8753017 [details]
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/
(Assignee)

Comment 2

2 years ago
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)

Updated

2 years ago
Depends on: 1273352
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.
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1274394
(Assignee)

Comment 6

2 years ago
Created attachment 8754910 [details]
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/
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)
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
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.

Updated

2 years ago
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?

Updated

2 years ago
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+
(Assignee)

Comment 12

2 years ago
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/
(Assignee)

Comment 13

2 years ago
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/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/66845689332c
https://hg.mozilla.org/mozilla-central/rev/cf20e55e8550
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.