Client cycles are not reported as errors by the bookmark validator

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [data-integrity])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
In https://bugzilla.mozilla.org/attachment.cgi?id=8783756 the validator should report cycle, but it won't, since the cycle is on the client and not on the server.

This should really just be a matter of running the cycle detection function on the client root, although it's possible a little finagling would be needed to handle things named differently between the two.

(More generally, we should probably be doing more thorough validation of the client state -- at the moment its very limited, but that can probably be done in other bugs)
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8784968 [details]
Bug 1297400 - Check for client cycles in the client tree in the bookmark validator

https://reviewboard.mozilla.org/r/74298/#review72262

Thanks! If this is actually reporting a real client cycle on that attachment, then (a) ship it! and (b) I'd expect places should do what it can to avoid this happening, so maybe we should have another bug on file to prevent places from allowing Sync to do that in the first place!
Attachment #8784968 - Flags: review?(markh) → review+
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [data-integrity]
(Assignee)

Comment 3

2 years ago
Hm, well, it does for contrived cases (which don't come from places -- although I'll try harder to cause this), and doesn't for the example of the bookmarks from bug 1297234. But I'm not sure the JSON data output by aboutsync can contain cycles -- I'm fairly certain it can't, AFAICT it would crash when `JSON.stringify`ing it.
ISTM a good start might be to simply see if it is possible to cause places functions to create such a cycle in the first place - IIUC, we are just speculating there was a client cycle in attachment 8783756 [details] rather than being certain?
(Assignee)

Comment 5

2 years ago
So, assuming that but 1297234 (and others) are cycles that, err, go through queries (along the lines of bug 1301235), it's not really clear what we should do. You can do this via the UI, and it is certainly a problem, but it's not necessarialy a problem with sync, and it's unclear that the validator should report it...

But I'll see if I can get a patch that *does* report it working in the mean time.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8784968 [details]
Bug 1297400 - Check for client cycles in the client tree in the bookmark validator

Unmarking you from r+ since it's barely the same patch as before.

I'm still unsure about a couple things, but I'll mark them as comments.
Attachment #8784968 - Flags: review+ → review?(markh)
(Assignee)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8784968 [details]
Bug 1297400 - Check for client cycles in the client tree in the bookmark validator

https://reviewboard.mozilla.org/r/74298/#review76712

Unlike the previous patch, if I create a bookmark cycle on the client (going through a query), this detects it.

I'm still a little iffy on this, because:

1. it actually syncs fine, and
2. you can create it through the UI (even though doing so breaks the UI afterwards, as described in bug 1301235).

Additionally, the approach here wouldn't work with aboutsync if it's using a non-local provider for bookmark data (The only thing I could imagine working there is parsing the place: URI and figuring out what it refers to. That or including this information with the export.

That said, there seems to be reason to believe sync is creating this in some cases, so despite that, it seems worth including in validation.

::: services/sync/modules/bookmark_validator.js:125
(Diff revision 2)
>  
>  class BookmarkValidator {
>  
> +  _followQueries(recordMap) {
> +    for (let [guid, entry] of recordMap) {
> +      if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith("place:"))) {

It's not clear to me why we sometimes the annotation is missing, leading to entry.type === "bookmark" on something with a "place:..." uri, when it absolutely is a query, and behaves as such in the UI.

::: services/sync/modules/bookmark_validator.js:532
(Diff revision 2)
>          let cycleStart = currentPath.lastIndexOf(node);
>          let cyclePath = currentPath.slice(cycleStart).map(n => n.id);
>          cycles.push(cyclePath);
>          return;
>        } else if (seenEver.has(node)) {
> -        // This is a problem, but we catch it earlier (multipleParents)
> +        // If we're checking the server, this is a problem, but it should already be reported.

It should be impossible for multipleParents to legitamitely occur on the client, due to the way places implements it's tree structure (e.g. just a parent id).

Comment 9

2 years ago
mozreview-review
Comment on attachment 8784968 [details]
Bug 1297400 - Check for client cycles in the client tree in the bookmark validator

https://reviewboard.mozilla.org/r/74298/#review77180

This looks great, thanks.

::: services/sync/modules/bookmark_validator.js:139
(Diff revision 2)
> +      queryNodeParent = queryNodeParent.root;
> +      let queryNode = null;
> +      let numSiblings = 0;
> +      let containerWasOpen = queryNodeParent.containerOpen;
> +      try {
> +        queryNodeParent.containerOpen = true;

I think this should be before the try - if this fails for some obscure reason we don't want to enter the finally.
Attachment #8784968 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cfc2be1d3b51
Check for client cycles in the client tree in the bookmark validator r=markh
Keywords: checkin-needed

Comment 12

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