1377183 - Audit `_followQueries` to determine if it's as broken as we think it is

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler (ex-Mozilla)]

We use `_followQueries` to find the concrete records for `place:` queries. A few things about that function stand out:

* It relies on `recordMap` actually matching what's in Places. In theory, that's OK; we assume there's no cycle if the query record doesn't exist. (It's hard to determine this without a real tree, since we don't sync enough information to map numeric folder IDs in `place:` URLs to their GUIDs. The common exceptions are https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/toolkit/components/places/nsNavHistoryQuery.cpp#160-165).

* We accidentally called `PlacesUtils.getFolderContents` with the wrong argument (bug 1119282), which might also explain the surprisingly high incidence of `clientCycles` problems in our validation data.

* We loop over the query's siblings to find `queryNode`, but it's not clear to me how these two lines are different, and if the loop can be removed: https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/services/sync/modules/bookmark_validator.js#239,259

* This comment needs to be updated: https://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/services/sync/modules/bookmark_validator.js#252-253 Tag queries have children; `queryNodeParent.childCount` doesn't appear to throw for them. (And `root.hasChildren === false` for left pane queries and "Recent Tags", even though they're `folder=` queries and appear to have children in the UI).

Comment 1

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

We believe this will report client cycles when there aren't any.

Comment 2

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

Kit to determine an action plan.

Comment 3

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1377183 - Manually follow folder queries when validating bookmarks.

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

Comment 4

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

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

https://reviewboard.mozilla.org/r/155046/#review160818

This is much simpler and saner code than before!

I'm a little concerned about the loss of generality, but given that it probably didn't actually work before... Not that concerned.

::: services/sync/modules/bookmark_validator.js:259
(Diff revision 1)
>        }
> -      // Might be worth trying to parse the place: query instead so that this
> -      // works "automatically" with things like aboutsync.
> -      let id;
> -      try {
> -        id = await PlacesUtils.promiseItemId(guid);
> +      let params = new URLSearchParams(entry.bmkUri.slice(QUERY_PROTOCOL.length));
> +      entry.concreteItems = [];
> +      let folderIds = params.getAll("folder");
> +      for (let folderId of folderIds) {
> +        if (!(folderId in QUERY_FOLDER_NAME_TO_ROOT_GUID)) {

Can you handle the `folder=$numericId` case also? I have two of these in my bookmarks and it seems plausible that they could be involved in cycles.

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

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

Comment 6

[Lina Butler (ex-Mozilla)]

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

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

Comment 7

[Lina Butler (ex-Mozilla)]

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

I decided not to follow numeric IDs at first, because that requires a live tree, and because non-tag queries that Sync doesn't rewrite are likely to be broken. However, it occurs to me that the folder ID on one device might point to a *different* folder on another device and create a cycle. Ugh.

Anyway, I updated the patch to incorporate your suggestion, but it's a pretty substantial rework from the first cut. Please take another look.

Comment 8

[Lina Butler (ex-Mozilla)]

I just realized "requires a live tree" isn't true. About Sync exports trees with `includeItemIds: true`. That, along with the GUID, gives us enough info to follow queries without needing our real local tree to match an export.

Comment 9

[Lina Butler (ex-Mozilla)]

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

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

Comment 10

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

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

https://reviewboard.mozilla.org/r/155046/#review161384

Looks good with issue addressed.

::: services/sync/modules/bookmark_validator.js:270
(Diff revisions 1 - 4)
>  
>    async createClientRecordsFromTree(clientTree) {
>      // Iterate over the treeNode, converting it to something more similar to what
>      // the server stores.
>      let records = [];
> -    let recordsByGuid = new Map();
> +    let recordsByQueryId = new Map();

I think a comment explaining what is stored in this map would be good.

::: services/sync/modules/bookmark_validator.js:330
(Diff revisions 1 - 4)
>        treeNode.type = itemType;
>        treeNode.pos = treeNode.index;
>        treeNode.bmkUri = treeNode.uri;
>        records.push(treeNode);
> -      // We want to use the "real" guid here.
> -      recordsByGuid.set(treeNode.guid, treeNode);
> +      if (localId) {
> +        let queryId = treeNode.guid in ROOT_GUID_TO_QUERY_FOLDER_NAME ?

I think we need to store a mapping for both the mnemonic and the real ID in the case that it's in the lookup table.

E.g. You could have a query for `UNFILED_BOOKMARKS` that is just to it's id, and not to the name `UNFILED_BOOKMARKS`. (Even if the places code prevents it, sync could have added the cycle by syncing a query from another computer).

Comment 11

[Lina Butler (ex-Mozilla)]

Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155046/diff/4-5/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2449e1c92db6
Manually follow folder queries when validating bookmarks. r=tcsc

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2449e1c92db6