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

[srirambv]

Description

Folder structure is not maintained when a device is readded to sycn chain. Follow up to #2133

Steps to Reproduce

1. Create sync chain on bc beta (0.58.11)
2. Import bookmarks html file on bc, check the folder structure
3. Add Android device to chain
4. Remove bc from sync chain via Android device
5. Add bc back using secret code
6. Wait for sync to progress
7. Bookmarks which was already on device gets messed up on both bc and Android

Actual result:

Bookmarks file used
brave_sorted_1000.zip

Expected result:

Should not mess up folder structure after device is added back to the sync chain

Reproduces how often:

Easy

Brave version (brave://version info)

1.0.71 (Sync1) on Android
0.58.11 Beta on Linux

Reproducible on current release:

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

Website problems only:

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

Additional Information

cc: @darkdh @AlexeyBarabash @brave/legacy_qa

[AlexeyBarabash]

With debug master build got a failed DCHECK

[729:729:1211/200637.659433:INFO:brave_sync_service_impl.cc(641)] fetched 5 devices
[729:729:1211/200637.660255:INFO:CONSOLE(264)] ""resolved-sync-records" categoryName="PREFERENCES" records=[
{"action":0,"deviceId":{"0":0},"objectId":{"0":240,"1":25,"2":62,"3":67,"4":66,"5":66,"6":189,"7":115,"8":43,"9":16,"10":9,"11":150,"12":121,"13":184,"14":176,"15":233},
"device":{"name":"BC-A"},"objectData":"device","syncTimestamp":1544550955791},{"action":0,"deviceId":{"0":1},"objectId":{"0":150,"1":18,"2":151,"3":195,"4":55,"5":14,"6":223,"7":78,"8":173,"9":231,"10":65,"11":30,"12":84,"13":196,"14":170,"15":183},"device":{"name":"mi4c"},"objectData":"device","syncTimestamp":1544551370138},
{"action":2,"deviceId":{"0":0},"objectId":{"0":240,"1":25,"2":62,"3":67,"4":66,"5":66,"6":189,"7":115,"8":43,"9":16,"10":9,"11":150,"12":121,"13":184,"14":176,"15":233},
"device":{"name":"BC-A"},"objectData":"device","syncTimestamp":1544551486655},{"action":2,"deviceId":{"0":0},"objectId":{"0":240,"1":25,"2":62,"3":67,"4":66,"5":66,"6":189,"7":115,"8":43,"9":16,"10":9,"11":150,"12":121,"13":184,"14":176,"15":233},"device":{"name":"BC-A"},"objectData":"device","syncTimestamp":1544551502652},
{"action":0,"deviceId":{"0":2},"objectId":{"0":217,"1":190,"2":209,"3":105,"4":107,"5":241,"6":132,"7":181,"8":213,"9":13,"10":9,"11":52,"12":41,"13":177,"14":215,"15":221},
"device":{"name":"BC-B2"},"objectData":"device","syncTimestamp":1544551596865}]", source: chrome-extension://nomlkjnggnifocmealianaaiobmebgil/extension/background.js (264)
[729:729:1211/200637.662045:FATAL:sync_devices.cc(175)] Check failed: existing_it != std::end(devices_). 
#0 0x7feaad00970d base::debug::StackTrace::StackTrace()
#1 0x7feaacd11b1a base::debug::StackTrace::StackTrace()
#2 0x7feaacd818fb logging::LogMessage::~LogMessage()
#3 0x55c6fca85328 brave_sync::SyncDevices::Merge()
#4 0x55c6fca94975 brave_sync::BraveSyncServiceImpl::OnResolvedPreferences()
#5 0x55c6fca93f71 brave_sync::BraveSyncServiceImpl::OnResolvedSyncRecords()
#6 0x55c6fd30ad98 extensions::api::BraveSyncResolvedSyncRecordsFunction::Run()

[AlexeyBarabash]

With Android 1.0.71(sync2) and brave-core

Brave	0.60.1 Chromium: 71.0.3578.80 (Developer Build) (64-bit)
Revision	2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}

I don't see the original issue, but I see folders are duplicated:

This happened because when brave-core was removed from sync, its sync-related data was cleared, included the object ids for bookmarks.

[rebron]

@darkh @AlexeyBarabash Any more status on this issue? Can you guys ping @srirambv? About to release 58.x and we want this issue addressed.

[darkdh]

@srirambv testes this issue without the fix of #2133
We don't have folder hierarchy out of order anymore, it's been fixed in brave/brave-core#982
The only thing we need to deal with is duplicate root folder after removing and re-adding the device

[darkdh]

In order to solve this we have to preserve meta info like "object_id", "order", "sync_timestamp", "last_send_time" and "last_updated_time". so that we can connect to existing sync chain

1. on device a and device b

    A -
      |-A1
      |-A2
   

2. unlink device b and delete A2 on device a
3. When device b connect to device a, it will be updated as

     A -
       |-A1
   

--
but it will cause huge impact when unlinked device b connecting to new sync chain with device c

  C -
    |- C1

device c will never get old data from device b, although device b will have

 A -          C-
   |-A1        |-C1
   |-A2

so I propose we keep duplicate entries behavior which means when a device disconnect from sync chain, all its data will be new no matter which sync chain it connects to later

[darkdh]

@AlexeyBarabash @bbondy ^^^

[AlexeyBarabash]

@darkdh
Maybe the option for the case when connecting to the a new chain, then do actual reset of fields "object_id", "order", "sync_timestamp", "last_send_time" and "last_updated_time". We can distinguish cases reconnect to old chain and connect to new chain by compare seed . When we are reconnecting to the old chain, we have prev_seed==new_seed and when we are connecting to new chain, it is the opposite prev_seed!=new_seed.
So need to preserve the old seed when doing reset.

[cezaraugusto]

moving to 0.59 per last sync meeting

[srirambv]

Verification passed on

Brave	0.59.14 Chromium: 72.0.3626.28 (Official Build) beta(64-bit)
Revision	997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS	Linux

• Verified re-adding bc to the sync chain doesn't mess up the existing bookmark folder structure

Verified passed with

Brave	0.59.18 Chromium: 72.0.3626.28 (Official Build) beta(64-bit)
Revision	997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS	Mac OS X

• Verified STR in description
• Verified test plan from Prevent duplicate nodes from reconnect and refine reset to make sure deleted record is sent brave-core#1135

Verified passed with

Brave	0.59.20 Chromium: 72.0.3626.28 (Official Build) beta (64-bit)
Revision	997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS	Windows 7

• used test plan from Prevent duplicate nodes from reconnect and refine reset to make sure deleted record is sent brave-core#1135