Use sync no merge

[AlexeyBarabash]

Submitter Checklist:

Fixes brave/brave-browser#6636

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Android
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that these changes pass automated tests (unit, browser, security tests) on
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[AlexeyBarabash]

This is marked as draft because I found at least one STR which does not work well.

1. Create profile A with bookmarks A1.com, A2.com

2. Create profile B with bookmarks B1.com, B2.com

3. Enable sync on device A

4. Connect device B to the sync chain with code-phrase

5. An device A create a folder FolderA and bookmark a.com

6. Wait while FolderA and a.com appear on device B

7. On device B move bookmark a.com into FolderA

8. On device A this change will never be reflected, but expected

For now, from what I have seen, deviceB does not sends UPDATE + new parentObjectId record.

[AlexeyBarabash]

STR from above work fine on master branch.
I cannot reproduce STR on master branch of brave-core

[AlexeyBarabash]

Here is what happens inside when the issue above happens.

There are two important points.

I. When bookmark is applied to model, it is placed to the correct position in BookmarkChangeProcessor::ApplyChangesFromSyncModel . Also here after we moved bookmark, we have to update chromium syncer node for the new positon to make bookmark node in bookmark model match bookmark sync node for the position data. Here is the stack illustrating this:

[17850:17850:1122/173144.212771:ERROR:model_neutral_mutable_entry.cc(173)] [BraveSync] PutIsUnsynced INSERT unsynced GetNonUniqueName()=AA.com
#3 0x5626be1a90c4 syncer::syncable::ModelNeutralMutableEntry::PutIsUnsynced()
#4 0x5626be1df45e syncer::syncable::MarkForSyncing()
#5 0x5626bf827299 syncer::WriteNode::MarkForSyncing()
#6 0x5626bf829bfa syncer::WriteNode::SetPosition()
#7 0x5626bf9d36bc sync_bookmarks::BookmarkChangeProcessor::PlaceSyncNode()
#8 0x5626bf9d342b sync_bookmarks::BookmarkChangeProcessor::MoveSyncNode()
#9 0x5626bf9d81d2 sync_bookmarks::BookmarkChangeProcessor::ApplyChangesFromSyncModel()
#10 0x5626be2a883c syncer::SyncBackendRegistrar::OnChangesApplied()

This causes bookmark resending on next sync send cycle for each moved bookmark which just had been applied from the cloud. Chromium does not have such behaviour because Chromium does not need to update bookmark sync node after moving bookmark node in model.

II. The actual situation for the STR above.

The problem: deviceB does not send record AA.com with a new parentObjectId.

Steps from deviceB:

1. get-existing-objects AA.com "parentFolderObjectId":{} - OK
2. resolve-sync-records ...
3. resolved-sync-records AA.com "parentFolderObjectId":{} - OK
4. AA.com is applied to model
5. AA.com is marked as UNSYNCED as in pt.I above:

[17850:17850:1122/173144.212771:ERROR:model_neutral_mutable_entry.cc(173)] [BraveSync] PutIsUnsynced INSERT unsynced GetNonUniqueName()=AA.com
#3 0x5626be1a90c4 syncer::syncable::ModelNeutralMutableEntry::PutIsUnsynced()
#4 0x5626be1df45e syncer::syncable::MarkForSyncing()
#5 0x5626bf827299 syncer::WriteNode::MarkForSyncing()
#6 0x5626bf829bfa syncer::WriteNode::SetPosition()
#7 0x5626bf9d36bc sync_bookmarks::BookmarkChangeProcessor::PlaceSyncNode()
#8 0x5626bf9d342b sync_bookmarks::BookmarkChangeProcessor::MoveSyncNode()
#9 0x5626bf9d81d2 sync_bookmarks::BookmarkChangeProcessor::ApplyChangesFromSyncModel()
#10 0x5626be2a883c syncer::SyncBackendRegistrar::OnChangesApplied()

from this point the mess begins

6. preparing sync entity to be sent into Brave cloud

[BraveSync] ConvertCommitsToBraveRecords bm_specifics.title()=AA.com

7. send-sync-records AA.com "parentFolderObjectId":{}

8. AA.com is marked synced, because it passed ProcessCommitResponse

[17850:18355:1122/173151.829373:ERROR:model_neutral_mutable_entry.cc(181)] [BraveSync] PutIsUnsynced ERASE unsynced GetNonUniqueName()=AA.com
#3 0x5626be1a9371 syncer::syncable::ModelNeutralMutableEntry::PutIsUnsynced()
#4 0x5626be23681e syncer::commit_util::(anonymous namespace)::ProcessSuccessfulCommitResponse()
#5 0x5626be236386 syncer::commit_util::ProcessSingleCommitResponse()
#6 0x5626be234376 syncer::DirectoryCommitContribution::ProcessCommitResponse()
#7 0x5626be27bffc syncer::Commit::PostAndProcessResponse()
#8 0x5626be261146 syncer::Syncer::BuildAndPostCommits()
#9 0x5626be2608da syncer::Syncer::NormalSyncShare()
#10 0x5626be24ea42 syncer::SyncSchedulerImpl::DoNudgeSyncCycleJob()

9. We are moving bookmark AA.com into folderA and bookmark sync node is marked as UNSYNCED

[17850:17850:1122/173217.842695:ERROR:model_neutral_mutable_entry.cc(173)] [BraveSync] PutIsUnsynced INSERT unsynced GetNonUniqueName()=AA.com
#3 0x5626be1a90c4 syncer::syncable::ModelNeutralMutableEntry::PutIsUnsynced()
#4 0x5626be1df45e syncer::syncable::MarkForSyncing()
#5 0x5626be23bb28 syncer::syncable::UpdateEntryWithEncryption()
#6 0x5626bf827c62 syncer::WriteNode::SetEntitySpecifics()
#7 0x5626bf827df7 syncer::WriteNode::SetBookmarkSpecifics()
#8 0x5626bf9d4929 sync_bookmarks::BookmarkChangeProcessor::SetSyncNodeMetaInfo()
#9 0x5626bf9d67c9 sync_bookmarks::BookmarkChangeProcessor::BookmarkNodeMoved()
#10 0x5626be0a7084 bookmarks::BookmarkModel::Move()
#11 0x5626c017a474 chrome::DropBookmarks()

10. Happens next scheduled get-existing-objects and we get AA.com from pt.7
    get-existing-objects AA.com "parentFolderObjectId":{}

11. resolve-sync-records we send pair [{server_record, local_record}] here
    so we send
    resolve-sync-records [{AA.com, "parentFolderObjectId":{}}, {AA.com, "parentFolderObjectId":{NEW_PARENT}}]

12. sync lib does resolve to {AA.com, "parentFolderObjectId":{NEW_PARENT}} , because timestampt is later

13. The main point ConflictResolver marks bookmark sync entity of AA.com which has a NEW_PARENT as synced, so it is excluded from the next sending to Brave AWS:

[17850:18355:1122/173221.990694:ERROR:get_updates_processor.cc(60)] [BraveSync] AddBookmarkSpecifics cloud sync title=AA.com
[17850:17850:1122/173222.001605:ERROR:model_neutral_mutable_entry.cc(181)] [BraveSync] PutIsUnsynced ERASE unsynced GetNonUniqueName()=AA.com
#3 0x5626be1a9371 syncer::syncable::ModelNeutralMutableEntry::PutIsUnsynced()
#4 0x5626be22bf85 syncer::conflict_util::IgnoreLocalChanges()
#5 0x5626be22ae0f syncer::ConflictResolver::ProcessSimpleConflict()
#6 0x5626be22b598 syncer::ConflictResolver::ResolveConflicts()
#7 0x5626be228faa syncer::DirectoryUpdateHandler::ApplyUpdatesImpl()

This happened because incoming update is equal to pending changes.

[AlexeyBarabash]

I am going to introduce into brave/sync#353 the 3rd state like resolve to none if remote record is equal to local record to see , will it help for STR above. Seems it should.

[AlexeyBarabash]

Attempt to intruduce resolve equal records to [] didn't help because we have to
resolve-sync-records [{AA.com, "parentFolderObjectId":{}}, {AA.com, "parentFolderObjectId":{NEW_PARENT}}]
which are different and all goes the same way.

The solution could be to avoid mark chromium sync nodes as UNSYNCED after moving bookmark.
We do move in BookmarkChangeProcessor::ApplyChangesFromSyncModel and this will be removed soon. Maybe the issue will gone after switching to USSBookmarks as discussed with @darkdh .

[AlexeyBarabash]

Closing as moving to sync v2