1297234 - Circular references in my bookmark tree when using Sync

Status: NEW

(Firefox :: Sync, defect, P3)

Description

[Chris Karlof [:ckarlof]]

Attachment: ckarlof export.json

I have some circular references in my bookmark tree that I feel almost certainly related to using Sync. I've attached a screenshot and an anonymized export of my bookmarks.

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: screenshot.jpg

Comment 2

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

Attachment appears corrupt somehow.

Comment 3

[Richard Newman [:rnewman]]

See also Bug 812348 Comment 26.

Comment 4

[Richard Newman [:rnewman]]

Attachment 8446874 [details] is a trip.

Comment 5

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

(In reply to Mark Hammond [:markh] from comment #2)
> Attachment appears corrupt somehow.

Apparently a firefox and/or bugzilla bug :( https://bug1297234.bmoattachments.org/attachment.cgi?id=8783756 loads fine in curl

(I'll flip the type of the attachment back for now so I can report a different bug, once I find my yak razor...)

Comment 6

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

These don't appear as circular references - eg, in unfiled I can find a record with id=212, guid="9DdcFuS4gEj7", "place:folder=TOOLBAR" - so it *looks* like the toolbar, but is a copy of it, not the actual toolbar record. Interestingly it also has an annotation {"name":"sync/parent","value":"fHXiZB_2TTYD"}], which implies Sync treated this record as an orphan at one point. The server records show that GUID as deleted.

I'll need to ponder this some more...

Comment 7

[Chris Karlof [:ckarlof]]

Attachment: ckarlof export.json

Comment 8

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

STR to get the places root and the left-pane folders on the server - on Aurora only - haven't tried nightly but I'm hopeful Kit's work there might prevent this.

* start sync in a new profile, create a new fxa account, sync.
* press ctrl+b - this creates left-pane items.
* bookmarks engine sees the notification, but the anno doesn't exist yet.
* sync

A second profile then "correctly" syncs these left-folder items under the places root - but they remain invisible in that profile, and independent of the left-folder items in that second profile (ie, if you do those same steps on that second profile, you end up with multiple copies of these "phantom" left-pane items.

I still can't come up with a scenario that creates these circular-looking items though. I was hoping to find code that tried to remove these "phantom" items, thinking that this might be enough to trigger it - a feature of the data in this attachment is that parents of these strange items are marked as deleted, leading me to believe Sync reparented them.

Comment 9

[Chris Karlof [:ckarlof]]

Mark, I wonder if the full STR would require Android syncing in some way. I know that Android has some rules to put stuff in Other Bookmarks when it can't find the parent.

Comment 10

[Lina Butler [:lina]]

Ooh, that's a very good point! We do the same on Desktop, but I wonder what Android does when it sees the left pane root. (Given that Android doesn't use Places). If it sees the children and reparents them under "Other Bookmarks", then sees the left pane root and ignores it...that could explain how the queries end up under Other Bookmarks.

(I have a strong suspicion they're queries, considering that `place:folder=TOOLBAR`, `place:transition=7&sort=4` [Downloads], `place:folder=UNFILED_BOOKMARKS` [Other Bookmarks] appear several times in the export. So the tree isn't circular...but it looks circular because the UI is rendering those queries as folders).

Comment 11

[Richard Newman [:rnewman]]

See also Bug 641617, Bug 824502, Bug 641074 Comment 31.

I don't remember seeing place:folder=TOOLBAR last time we touched query handling, so perhaps some of those bugs are no longer relevant.

But regardless, yes, Android will add orphans to unsorted and leave them there:

    if (needsReparenting != 0) {
      Logger.error(LOG_TAG, "Finish called but " + needsReparenting +
                            " bookmark(s) have been placed in unsorted bookmarks and not been reparented.");

(`prepareRecord` is the bit that modifies the record we store.)

The synchronizer is expecting to see the parent record at some point before it next does an upload -- i.e., before the end of a successful sync. In this case it never will, and it'll move the incoming query items into Unsorted Bookmarks, and will update the server accordingly. Desktop will then process those incoming records. Presumably there's a GUID collision, and so Sync will move the original queries into Unsorted Bookmarks. Places will then recreate them, but the damage is already done.

Comment 12

[Richard Newman [:rnewman]]

There is probably a repro sequence that happens on desktop alone, because I'm sure we've seen endless regress prior to Fennec syncing -- desktop reparents under Unsorted too, it just doesn't upload unless you then touched Unsorted. Any approach that gets one of these query records onto the server with an unknown parent GUID would cause this.

Comment 13

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

Apart from comment 8, the "left pane" code in places will, in some cases, re-create the left-pane items, including deleting any items it believes were the "old" left-pane items. This shouldn't normally happen, but when it does (via me instrumenting the code so it thinks it needs rebuild when it normally wouldn't), we can see the deletions of these items, and the creation of the replacement items being synced. While I can't actually reproduce the exact problem here, this certainly (and obviously) causes a number of problems.

The above is all on Firefox 50 - and the good news is that none of this abuse causes problems with 52 - the current tracker does the right thing, refuses to Sync these items and the validator continues to report everything is perfect.

I'm not sure it is worth spending any more time trying to reproduce this issue on older Firefox versions given we know the code has changed so much and we can't reproduce any weirdness or corruption on trunk - so unassigning and resetting priority so it shows back up in our triage list for discussion.

Comment 14

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

So yeah, "circular references" is complicated.

1) There's an existing bug in places that allows you to create "pseudo circular references". For example, you can copy "bookmarks toolbar" and paste it into "bookmarks menu", then copy "bookmarks menu" and paste it in "bookmarks toolbar". Sadly, it is easy to accidentally create this via an accidental drag-drop when the bookmarks sidebar is open. Using the UI, this then looks exactly like circular references - if you expand "bookmarks toolbar" you will see "bookmarks menu". If you expand that, you will see another "bookmarks toolbar" - inside which is another "bookmarks menu". You can continue to expand these infinitely until the UI breaks.

However, these *aren't* real circular references - the record with guid="toolbar" has a child with a URL of "place:folder=BOOKMARKS_MENU&excludeItems=1&expandQueries=0" - but that child itself has no children. Similarly, the record with guid="menu" has a child with a URL of "place:folder=TOOLBAR&excludeItems=1&expandQueries=0" - but these queries need to be executed for it to become circular and we don't execute those queries for Sync.

In the above situation, the validator reports no problems, and there's exactly 2 records causing this problem. For example, this problem can be reproduced with less than 10 records total. Note also that Fennec completely ignores this situation and doesn't show these circular references - it seems to ignore all queries and only shows actual bookmarks. I strongly suspect iOS is the same.

2) In Chris's problem, it *appears* the problem above also exists. I suspect that this was caused by an accidental drag-drop when the bookmarks folder is open. However, this seems to have additional problems - for example, there are many items with a URL of "place:folder=BOOKMARKS_MENU" - note the missing query params from the ones shown above - which is exactly how left-pane items are created. Many of these have deleted items as parent. I believe these are "left pane" items that have ended up in the "Other Bookmarks" folder, placed there because the parent of the left-pane items are marked as deleted. Some of the items have "places" as the parent.

Specifically:
* There are 5 bookmarks with place:folder=BOOKMARKS_MENU
* There are 5 bookmarks with place:folder=TOOLBAR
* There are 5 bookmarks with place:folder=UNFILED_BOOKMARKS

For all the above, the parent is either deleted, or the parent chain up to the "places" root is intact. None of those items have the special GUID for these items.

* There is 1 bookmark with place:folder=BOOKMARKS_MENU&folder=UNFILED_BOOKMARKS&folder=TOOLBAR&queryType=1&sort=12&maxResults=10&excludeQueries=1. I'm not sure what this is.

In terms of "repair":
* For (1) there's not a whole lot we should do IMO. It is a bug in places, but best I can tell, doesn't impact Sync (ie, doesn't cause Sync to create an infinite number of records, even though the tree structure appears of infinite depth in the UI.)

* For all records on the server, where the "path" to the places root is intact, but *does not* go through the roots we care about, we could either delete them from the server or simply have our validator ignore them. The latter seems simpler, and it may even be possible to arrange for iOS to ignore them too. I'm not sure if desktop actually ignores such incoming records yet - I thought we had a bug on file for that, but can't find it now.

* I suspect that all remaining items which look like a "left pane root" (ie, "place:folder=*" and with no query string) could be deleted locally and from the server - many of these will be picked up by the above, but ones with deleted intermediate parents will not.

Comment 15

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

(In reply to Mark Hammond [:markh] from comment #14)
> In the above situation, the validator reports no problems, and there's
> exactly 2 records causing this problem. For example, this problem can be
> reproduced with less than 10 records total. Note also that Fennec completely
> ignores this situation and doesn't show these circular references - it seems
> to ignore all queries and only shows actual bookmarks. I strongly suspect
> iOS is the same.

The validator should actually be reporting these as of bug 1297400 (and I believe the PR for displaying it in aboutsync was merged).

This theoretically means we can know how common this issue is via the sync ping.

(That said, it doesn't know how to follow queries if they don't exist locally, so it won't report anything for Chris's bookmarks -- short of making the validator parse the query uri, I don't see an alternative here)

Comment 16

[Lina Butler [:lina]]

Downgrading to P3 while we figure out a strategy for removing these records from the server.