Brave-browser: Folder structure is not maintained on device 2

[srirambv]

Description

Folder structure is not maintained on device 2

Devices

Device 1: Ubuntu (sync chain creator)
Device 2: Windows 10 x64

Steps to Reproduce

1. Create sync chain on device 1
2. Join device 2 and add a bookmark on device 2, gets sync'd to device 1
3. Import bookmarks from other browser on device 1, wait for sometime, sync's on device 2
4. Import 5000 bookmark HTML file on device 1, wait for a really long time, some nested bookmark folder become rebellious and come on bookmark toolbar

Actual result:

Device 1:

Device 2:

Folder structure on Device 1:

Folder structure on Device 2:

Expected result:

Should maintain same folder structure

Reproduces how often:

Easy

Brave version (brave://version info)

Brave | 0.57.6 Chromium: 71.0.3578.31 (Official Build) beta (64-bit)
-- | --
Revision | c88fdf2a4ce19a713615ca4fbde7a0d0b5fe2363-refs/branch-heads/3578@{#427}
OS | Windows/Linux

Reproducible on current release:

• Does it reproduce on brave-browser dev/beta builds?
  Yes on beta build

Website problems only:

• Does the issue resolve itself when disabling Brave Shields?
• Is the issue reproducible on the latest version of Chrome?

Additional Information

Waited for about 20 mins on device 2 folder structure didn't get resolved.
cc: @AlexeyBarabash @darkdh @brave/legacy_qa

[GeetaSarvadnya]

Issue is reproducible when sync two devices (Windows 10 and Windows 8).

Windows 10 - sync creator

Windows 8

[LaurenWags]

Reproduced with macOS (device 0) and macOS (device 2) on

Brave | 0.57.6 Chromium: 71.0.3578.31 (Official Build) beta(64-bit)
-- | --
Revision | c88fdf2a4ce19a713615ca4fbde7a0d0b5fe2363-refs/branch-heads/3578@{#427}
OS | Mac OS X

Actual (from device 2):

Expected (from device 0):

[AlexeyBarabash]

Device-receiver of sync data get the bookmark before it get the folder. So the bookmarks were placed in the root Mobile bookmark node because "hideInToolbar":false . Device-receiver gets data in different portions.

Device who sends to sync:

272:22272:1126/182829.216511:INFO:CONSOLE(58)] ""send-sync-records" category_name="BOOKMARKS" records=[
// Sends folder
...
{"action":0,"bookmark":{"hideInToolbar":false,"isFolder":true,"order":"1.1.0.2.1.1.1.1.1.1.1.1.1",
"parentFolderObjectId":{"0":218,"1":216,"2":0,"3":152,"4":130,"5":40,"6":83,"7":26,"8":213,"9":208,"10":155,"11":219,"12":126,"13":69,"14":51,"15":167},
"site":{"creationTime":0,"customTitle":"700-799","favicon":"","lastAccessedTime":0,"location":"","title":"700-799"}},
"deviceId":{"0":0},"objectData":"bookmark",
"objectId":{"0":63,"1":156,"2":254,"3":78,"4":7,"5":109,"6":242,"7":174,"8":36,"9":112,"10":16,"11":79,"12":79,"13":120,"14":111,"15":43},
"syncTimestamp":1543249706276.708},
...

// Sends bookmark
{"action":0,
"bookmark":{"hideInToolbar":false,"isFolder":false,
"order":"1.1.0.2.1.1.1.1.1.1.1.1.1.13",
"parentFolderObjectId":{"0":63,"1":156,"2":254,"3":78,"4":7,"5":109,"6":242,"7":174,"8":36,"9":112,"10":16,"11":79,"12":79,"13":120,"14":111,"15":43},
"site":{"creationTime":0,
"customTitle":"711","favicon":"","lastAccessedTime":0,"location":"https://www.youtube.com/watch?v=DsstOs-K7gk",
"title":"711"}},"deviceId":{"0":0},"objectData":"bookmark",
"objectId":{"0":14,"1":52,"2":224,"3":39,"4":8,"5":175,"6":229,"7":179,"8":220,"9":25,"10":32,"11":153,"12":59,"13":93,"14":135,"15":133},
"syncTimestamp":1543249706336.472},
...
]", source: chrome-extension://nomlkjnggnifocmealianaaiobmebgil/extension/background.js (58)

Device who receives from sync

// received bookmark 
[22011:22011:1126/182957.664644:INFO:CONSOLE(263)] ""resolved-sync-records" categoryName="BOOKMARKS" records=[
...
{"action":0,
"bookmark":{"hideInToolbar":false,"isFolder":false,"order":"1.1.0.2.1.1.1.1.1.1.1.1.1.13",
"parentFolderObjectId":{"0":63,"1":156,"2":254,"3":78,"4":7,"5":109,"6":242,"7":174,"8":36,"9":112,"10":16,"11":79,"12":79,"13":120,"14":111,"15":43},
"site":{"creationTime":0,
"customTitle":"711","favicon":"","lastAccessedTime":0,
"location":"https://www.youtube.com/watch?v=DsstOs-K7gk",
"title":"711"},"prevObjectId":{}},"deviceId":{"0":0},"objectData":"bookmark",
"objectId":{"0":14,"1":52,"2":224,"3":39,"4":8,"5":175,"6":229,"7":179,"8":220,"9":25,"10":32,"11":153,"12":59,"13":93,"14":135,"15":133},
"syncTimestamp":1543 249 716 148}]"
...

// received folder 
[22011:22011:1126/183057.561026:INFO:CONSOLE(263)] ""resolved-sync-records" categoryName="BOOKMARKS" records=[
{"action":0,"bookmark":{"hideInToolbar":false,"isFolder":true,"order":"1.1.0.2.1.1.1.1.1.1.1.1.1",
"parentFolderObjectId":{"0":218,"1":216,"2":0,"3":152,"4":130,"5":40,"6":83,"7":26,"8":213,"9":208,"10":155,"11":219,"12":126,"13":69,"14":51,"15":167},
"site":{"creationTime":0,
"customTitle":"700-799","favicon":"","lastAccessedTime":0,"location":"","title":"700-799"},"prevObjectId":{}},"deviceId":{"0":0},"objectData":"bookmark","objectId":{"0":63,"1":156,"2":254,"3":78,"4":7,"5":109,"6":242,"7":174,"8":36,"9":112,"10":16,"11":79,"12":79,"13":120,"14":111,"15":43},
"syncTimestamp":1543 249 709 219},

This issue describes real steps to get the situation described in https://github.com/brave/brave-browser/issues/2104 .

[AlexeyBarabash]

The plan to fix it is:
1) When create-bookmark-item record arrives, find it's parent folder node by parentObjectId. If we cannot find the parent folder node, consider the bookmark hopes the parent will arrive soon, add to the bookmark metadata ParentObjectId
2) When create-bookmark-folder record arrives, go through all bookmarks which have ParentObjectId metadata, move bookmark in a proper folder, and remove ParentObjectId metadata.

Not quite good point - the bookmarks which are truly in the root of permanent nodes, will always have "ParentObjectId" metadata.

@darkdh , @bridiver wdyt?

[darkdh]

so where do you plan to put the bookmark temporarily until you receive the true parent?

[AlexeyBarabash]

@darkdh in the same place as now, either
Other bookmarks
Bookmarks toolbar
Mobile bookmarks

I cannot hide them because there is no way to figure out is that bookmark item the child of Bookmarks toolbar or its parent folder had not arrived yet.

Will move the bookmark if the folder will arrive. If folder will not arrive, bookmark will be still where it was placed first.

Nested folders can also have the issue.

[darkdh]

what about putting them under a new invisible[set_visible(false)] permanent node Pending Bookmarks, like what we did for Deleted Bookmarks?

[AlexeyBarabash]

@darkdh
idea is good, but if the bookmark is a child of some permanent folder, we will not receive the record of create folder for this case. So such records will forever be in Pending Bookmarks.

The code of FindParent
https://github.com/brave/brave-core/blob/master/components/brave_sync/client/bookmark_change_processor.cc#L169

Or we need to update brave-core/android/ios clients to pass the additional info about is the record the direct child of permanent folder.

[darkdh]

If the child is under those permanent nodes, why bother putting it under Pending Bookmarks?
Pending Bookmarks are only for those bookmarks who don't have parent node at the moment.
When new folder arrives, check nodes in Pending Bookmarks to see if any of those are its children.

[AlexeyBarabash]

because when the bookmark node arrives, I cannot understand is that a node, which arrived before parent, or the node which should be placed into some permanent folder.

The sign of these two cases:

• item belongs to permanent folder
• item has no parent yet
  is the same:

auto* parent_node = FindByObjectId(model, bookmark.parentFolderObjectId);
parent_node==nullptr

[AlexeyBarabash]

Pending Bookmarks would be better because it is much easier to iterate through Pending Bookmarks than all bookmarks browser have. But it is impossible to distinguish these cases of pending and is child of permanent

[darkdh]

if item belongs to permanent folder, isn't its parentFolderObjectId empty? That should be sufficient to distinguish from the case that parent hasn't arrived.

[AlexeyBarabash]

@darkdh , awesome notice, will re-check that. If parentFolderObjectId empty, will go with Pending Items

[LaurenWags]

Verified passed with

Brave | 0.58.14 Chromium: 71.0.3578.98 (Official Build) (64-bit)
-- | --
Revision | 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS | Mac OS X

• Verified all folders remained in order, nested appropriately

Verification passed on

Brave | 0.58.14 Chromium: 71.0.3578.98 (Official Build) (64-bit)
-- | --
Revision | 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS | Windows 7

The folder structure is maintained after sync

Verification passed on

Brave | 0.58.14 Chromium: 71.0.3578.98 (Official Build) (64-bit)
-- | --
Revision | 15234034d19b85dcd9a03b164ae89d04145d8368-refs/branch-heads/3578@{#897}
OS | Linux

• Verified all folders and nested folders and bookmarks retain the correct structure