Folder structure is not maintained on device 2 #2133

srirambv · 2018-11-14T03:38:12Z

Description

Folder structure is not maintained on device 2

Devices

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

Steps to Reproduce

Create sync chain on device 1
Join device 2 and add a bookmark on device 2, gets sync'd to device 1
Import bookmarks from other browser on device 1, wait for sometime, sync's on device 2
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 · 2018-11-14T12:26:50Z

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

Windows 10 - sync creator

Windows 8

LaurenWags · 2018-11-15T14:24:46Z

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 · 2018-11-26T19:59:11Z

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 #2104 .

AlexeyBarabash · 2018-11-26T20:58:51Z

The plan to fix it is:

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
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 · 2018-11-26T21:15:35Z

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

AlexeyBarabash · 2018-11-26T21:31:01Z

@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 · 2018-11-26T21:44:26Z

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

AlexeyBarabash · 2018-11-26T21:57:15Z

@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 · 2018-11-26T22:08:23Z

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 · 2018-11-26T22:49:32Z

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 · 2018-11-26T23:05:26Z

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 · 2018-11-26T23:09:23Z

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 · 2018-11-26T23:40:02Z

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

LaurenWags · 2018-12-18T15:51:50Z

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

srirambv added bug release/blocking feature/sync QA/Yes release-notes/include QA/Test-Plan-Specified labels Nov 14, 2018

srirambv added this to the 0.57.x - Beta milestone Nov 14, 2018

AlexeyBarabash self-assigned this Nov 26, 2018

AlexeyBarabash mentioned this issue Nov 26, 2018

Brave sync be ready to receive bookmark before folder #2104

Closed

bbondy removed this from the 0.57.x - Beta milestone Nov 27, 2018

bbondy added this to the 1.x Backlog milestone Dec 2, 2018

This was referenced Dec 5, 2018

Sync folder structure not kept brave/brave-core#982

Merged

Bookmarks orders sometimes are sent equal for different bookmarks items #2367

Closed

srirambv mentioned this issue Dec 11, 2018

folders/bookmarks are duplicate when a device is re-added to sync chain #2443

Closed

AlexeyBarabash closed this as completed in brave/brave-core#982 Dec 11, 2018

AlexeyBarabash modified the milestones: 1.x Backlog, 0.58.x - Beta Dec 12, 2018

rebron mentioned this issue Dec 12, 2018

Removed folders re-synced only on toolbar (no folders structure), when two devices are still in sync #2458

Closed

LaurenWags added the QA Pass-macOS label Dec 18, 2018

btlechowski added the QA Pass-Win64 label Dec 19, 2018

This was referenced Dec 19, 2018

Release notes for 0.58.x (Release Channel) #2633

Closed

Manual test run on Linux for 0.58.x - Release #2648

Closed

kjozwiak added release-notes/exclude and removed release-notes/include labels Dec 19, 2018

LaurenWags mentioned this issue Dec 20, 2018

[QA] 0.58.x sync issues that still need to be verified (Linux only) #2673

Closed

srirambv added the QA Pass-Linux label Jan 4, 2019

kjozwiak mentioned this issue Jan 31, 2019

Enabling Sync by default #3180

Closed

srirambv mentioned this issue Feb 11, 2019

Sync Bookmark - Sort by name by default #3297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Folder structure is not maintained on device 2 #2133

Folder structure is not maintained on device 2 #2133

srirambv commented Nov 14, 2018

GeetaSarvadnya commented Nov 14, 2018

LaurenWags commented Nov 15, 2018 •

edited

Loading

AlexeyBarabash commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018 •

edited

Loading

AlexeyBarabash commented Nov 26, 2018

LaurenWags commented Dec 18, 2018 •

edited by srirambv

Loading

Folder structure is not maintained on device 2 #2133

Folder structure is not maintained on device 2 #2133

Comments

srirambv commented Nov 14, 2018

Description

Devices

Steps to Reproduce

Actual result:

Expected result:

Reproduces how often:

Brave version (brave://version info)

Reproducible on current release:

Website problems only:

Additional Information

GeetaSarvadnya commented Nov 14, 2018

LaurenWags commented Nov 15, 2018 • edited Loading

AlexeyBarabash commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

AlexeyBarabash commented Nov 26, 2018

darkdh commented Nov 26, 2018 • edited Loading

AlexeyBarabash commented Nov 26, 2018

LaurenWags commented Dec 18, 2018 • edited by srirambv Loading

LaurenWags commented Nov 15, 2018 •

edited

Loading

darkdh commented Nov 26, 2018 •

edited

Loading

LaurenWags commented Dec 18, 2018 •

edited by srirambv

Loading