1297400 - Client cycles are not reported as errors by the bookmark validator

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

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 1

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

Attachment: Bug 1297400 - Check for client cycles in the client tree in the bookmark validator

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

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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!

Comment 3

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

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.

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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?

Comment 5

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

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 6

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74298/diff/1-2/

Comment 7

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

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.

Comment 8

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

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

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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.

Comment 10

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74298/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfc2be1d3b51
Check for client cycles in the client tree in the bookmark validator r=markh

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cfc2be1d3b51