Closed Bug 1377183 Opened 4 years ago Closed 4 years ago

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


(Firefox :: Sync, enhancement, P2)




Firefox 56
Tracking Status
firefox56 --- fixed


(Reporter: lina, Assigned: lina)




(1 file)

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

* 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:,259

* This comment needs to be updated: 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).
We believe this will report client cycles when there aren't any.
Priority: -- → P2
Summary: Audit `_followQueries` to ensure it's doing what we think it's doing → Audit `_followQueries` to determine if it's as broken as we think it is
Kit to determine an action plan.
Assignee: nobody → kit
Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

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.
Attachment #8884116 - Flags: review?(tchiovoloni) → review+
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.
Attachment #8884116 - Flags: review+ → review?(tchiovoloni)
Attachment #8884116 - Flags: review?(tchiovoloni)
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.
Attachment #8884116 - Flags: review?(tchiovoloni)
Comment on attachment 8884116 [details]
Bug 1377183 - Manually follow folder queries when validating bookmarks.

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).
Attachment #8884116 - Flags: review?(tchiovoloni) → review+
Pushed by
Manually follow folder queries when validating bookmarks. r=tcsc
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1380835
You need to log in before you can comment on or make changes to this bug.