Skip to content

GitLab

T
tangled
Commit c0439d22 authored by Balazs Engedy's avatar Balazs Engedy Committed by Commit Bot
Browse files
Options

Revert "Reland "[Sync::USS] Bookmark sync conflict resolution"" 

This reverts commit a445460b.

Reason for revert:

MSAN warning in USS/TwoClientBookmarksSyncTestIncludingUssTests.CreateSyncedBookmarks

See:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20MSan%20Tests/11051
https://chromium-swarm.appspot.com/task?id=3f17cf427082ec10&refresh=10&show_raw=1

Original change's description:
> Reland "[Sync::USS] Bookmark sync conflict resolution"
>
> This is a reland of 553ec6da
>
> Original change's description:
> > [Sync::USS] Bookmark sync conflict resolution
> >
> > This patch addresses the problem of conflict in bookmarks upon
> > receiving a remote update for a node that has local changes.
> >
> > The general polic that is implemented is that server wins except in
> > case of deletion.
> >
> > Bug: 516866
> > Change-Id: I50a85bde068dd9414e4e22bc1a33529fb3dfffa1
> > Reviewed-on: https://chromium-review.googlesource.com/1157006
> > Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
> > Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#580436}
>
> Bug: 516866
> Change-Id: I73bb31e143aa7baadd5fa8c0afd3d91554c9aa6e
> Reviewed-on: https://chromium-review.googlesource.com/1161861
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580496}

TBR=mastiz@chromium.org,mamir@chromium.org

Change-Id: I22a2a0f518fdbefe7baa3c4840884090459e62a3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 516866, 870668
Reviewed-on: https://chromium-review.googlesource.com/1161884
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: default avatarBalazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580509}
parent e36e145d
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 53 additions and 251 deletions
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
View file @ c0439d22
......@@ -1680,7 +1680,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
// Test a scenario of updating the name of the same bookmark from two clients at
// the same time.
IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
MC_BookmarkNameChangeConflict) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
......@@ -1705,7 +1705,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
// Test a scenario of updating the URL of the same bookmark from two clients at
// the same time.
IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
MC_BookmarkURLChangeConflict) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
......@@ -1732,7 +1732,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
// Test a scenario of updating the BM Folder name from two clients at the same
// time.
IN_PROC_BROWSER_TEST_P(TwoClientBookmarksSyncTestIncludingUssTests,
IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
MC_FolderNameChangeConflict) {
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
DisableVerifier();
......
components/sync_bookmarks/bookmark_remote_updates_handler.h
View file @ c0439d22
......@@ -65,7 +65,7 @@ class BookmarkRemoteUpdatesHandler {
void ProcessRemoteUpdate(const syncer::UpdateResponseData& update,
const SyncedBookmarkTracker::Entity* tracked_entity);
// Processes a remote delete of a bookmark node. |update_entity| must not be a
// Process a remote delete of a bookmark node. |update_entity| must not be a
// deletion. |tracked_entity| is the tracked entity for that server_id. It is
// passed as a dependency instead of performing a lookup inside
// ProcessRemoteDelete() to avoid wasting CPU cycles for doing another lookup
......@@ -73,15 +73,6 @@ class BookmarkRemoteUpdatesHandler {
void ProcessRemoteDelete(const syncer::EntityData& update_entity,
const SyncedBookmarkTracker::Entity* tracked_entity);
// Processes a conflict where the bookmark has been changed both locally and
// remotely. It applies the general policy the server wins expcet in the case
// of remote deletions in which local wins. |tracked_entity| is the tracked
// entity for that server_id. It is passed as a dependency instead of
// performing a lookup inside ProcessRemoteDelete() to avoid wasting CPU
// cycles for doing another lookup (this code runs on the UI thread).
void ProcessConflict(const syncer::UpdateResponseData& update,
const SyncedBookmarkTracker::Entity* tracked_entity);
// Recursively removes the entities corresponding to |node| and its children
// from |bookmark_tracker_|.
void RemoveEntityAndChildrenFromTracker(const bookmarks::BookmarkNode* node);
......
components/sync_bookmarks/synced_bookmark_tracker.cc
View file @ c0439d22
......@@ -136,7 +136,7 @@ void SyncedBookmarkTracker::Update(
auto it = sync_id_to_entities_map_.find(sync_id);
Entity* entity = it->second.get();
DCHECK(entity);
DCHECK_EQ(entity->metadata()->server_id(), sync_id);
entity->metadata()->set_server_id(sync_id);
entity->metadata()->set_server_version(server_version);
entity->metadata()->set_modification_time(
syncer::TimeToProtoTime(modification_time));
......@@ -146,14 +146,6 @@ void SyncedBookmarkTracker::Update(
// |ordered_local_tombstones_| as well if it has been locally deleted.
}
void SyncedBookmarkTracker::UpdateServerVersion(const std::string& sync_id,
int64_t server_version) {
auto it = sync_id_to_entities_map_.find(sync_id);
Entity* entity = it->second.get();
DCHECK(entity);
entity->metadata()->set_server_version(server_version);
}
void SyncedBookmarkTracker::MarkDeleted(const std::string& sync_id) {
auto it = sync_id_to_entities_map_.find(sync_id);
Entity* entity = it->second.get();
......@@ -233,7 +225,7 @@ SyncedBookmarkTracker::GetEntitiesWithLocalChanges(size_t max_entries) const {
sync_id_to_entities_map_) {
Entity* entity = pair.second.get();
if (entity->metadata()->is_deleted()) {
// Deletions are stored sorted in |ordered_local_tombstones_| and will be
// Deletion are stored sorted in |ordered_local_tombstones_| and will be
// added later.
continue;
}
......@@ -352,18 +344,6 @@ void SyncedBookmarkTracker::UpdateUponCommitResponse(
}
}
void SyncedBookmarkTracker::AckSequenceNumber(const std::string& sync_id) {
auto it = sync_id_to_entities_map_.find(sync_id);
Entity* entity =
it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
if (!entity) {
DLOG(WARNING) << "Trying to update a non existing entity.";
return;
}
entity->metadata()->set_acked_sequence_number(
entity->metadata()->sequence_number());
}
bool SyncedBookmarkTracker::IsEmpty() const {
return sync_id_to_entities_map_.empty();
}
......
components/sync_bookmarks/synced_bookmark_tracker.h
View file @ c0439d22
......@@ -106,9 +106,6 @@ class SyncedBookmarkTracker {
const sync_pb::UniquePosition& unique_position,
const sync_pb::EntitySpecifics& specifics);
// Updates the server version of an existing entry for the |sync_id|.
void UpdateServerVersion(const std::string& sync_id, int64_t server_version);
// This class maintains the order of calls to this method and the same order
// is gauaranteed when returning local changes in
// GetEntitiesWithLocalChanges() as well as in BuildBookmarkModelMetadata().
......@@ -149,11 +146,6 @@ class SyncedBookmarkTracker {
int64_t acked_sequence_number,
int64_t server_version);
// Set the value of |EntityMetadata.acked_sequence_number| in the entity with
// |sync_id| to be equal to |EntityMetadata.sequence_number| such that it is
// not returned in GetEntitiesWithLocalChanges().
void AckSequenceNumber(const std::string& sync_id);
// Whether the tracker is empty or not.
bool IsEmpty() const;
......
components/sync_bookmarks/synced_bookmark_tracker_unittest.cc
View file @ c0439d22
......@@ -150,38 +150,6 @@ TEST(SyncedBookmarkTrackerTest,
// request in a separate test probably.
}
TEST(SyncedBookmarkTrackerTest, ShouldAckSequenceNumber) {
SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
std::make_unique<sync_pb::ModelTypeState>());
const std::string kSyncId = "SYNC_ID";
const int64_t kId = 1;
const int64_t kServerVersion = 1000;
const base::Time kModificationTime(base::Time::Now() -
base::TimeDelta::FromSeconds(1));
const sync_pb::UniquePosition unique_position;
const sync_pb::EntitySpecifics specifics =
GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
bookmarks::BookmarkNode node(kId, GURL());
tracker.Add(kSyncId, &node, kServerVersion, kModificationTime,
unique_position, specifics);
// Test simple scenario of ack'ing an incrememented sequence number.
EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
tracker.IncrementSequenceNumber(kSyncId);
EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
tracker.AckSequenceNumber(kSyncId);
EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
// Test ack'ing of a mutliple times incremented sequence number.
tracker.IncrementSequenceNumber(kSyncId);
EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
tracker.IncrementSequenceNumber(kSyncId);
tracker.IncrementSequenceNumber(kSyncId);
EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
tracker.AckSequenceNumber(kSyncId);
EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
}
TEST(SyncedBookmarkTrackerTest, ShouldUpdateUponCommitResponseWithNewId) {
SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
std::make_unique<sync_pb::ModelTypeState>());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment