[Tab Group Sync] Restore hidden originating tab groups
If a shared tab group is removed, its originating saved tab group is
restored after this CL. This is required in case of sharing failure on
a remote device (while on the current device the group is already
transitioned). To avoid deletion of the originating saved tab group by
GC, this CL restores the saved tab group.
In case a remote shared group was actually removed, a corresponding
originating saved tab group should also be removed on the same device,
so restoring it should be safe as it will be eventually deleted.
Bug: 403172514
Change-Id: I6784b2a3aab9f507d8107083d2b0219156d9de07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6402793
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/main@{#1440153}
diff --git a/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc b/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc
index c0189f19..ade83c19 100644
--- a/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_shared_tab_group_data_sync_test.cc
@@ -27,6 +27,7 @@
#include "components/saved_tab_groups/public/utils.h"
#include "components/saved_tab_groups/test_support/saved_tab_group_test_utils.h"
#include "components/signin/public/identity_manager/identity_test_utils.h"
+#include "components/sync/base/client_tag_hash.h"
#include "components/sync/base/collaboration_id.h"
#include "components/sync/base/data_type.h"
#include "components/sync/protocol/saved_tab_group_specifics.pb.h"
@@ -53,6 +54,7 @@
using tab_groups::HasSavedGroupMetadata;
using testing::Contains;
using testing::ElementsAre;
+using testing::Optional;
using testing::SizeIs;
using testing::UnorderedElementsAre;
@@ -182,6 +184,17 @@
return GetClient(0)->GetGaiaIdForDefaultTestAccount();
}
+ sync_pb::SyncEntity::CollaborationMetadata MakeCollaborationMetadata(
+ const std::string& collaboration_id) {
+ sync_pb::SyncEntity::CollaborationMetadata collaboration_metadata;
+ collaboration_metadata.set_collaboration_id(collaboration_id);
+ collaboration_metadata.mutable_creation_attribution()
+ ->set_obfuscated_gaia_id(GetGaiaId().ToString());
+ collaboration_metadata.mutable_last_update_attribution()
+ ->set_obfuscated_gaia_id(GetGaiaId().ToString());
+ return collaboration_metadata;
+ }
+
void AddSpecificsToFakeServer(
sync_pb::SharedTabGroupDataSpecifics shared_specifics,
const std::string& collaboration_id) {
@@ -191,12 +204,6 @@
sync_pb::EntitySpecifics entity_specifics;
*entity_specifics.mutable_shared_tab_group_data() =
std::move(shared_specifics);
- sync_pb::SyncEntity::CollaborationMetadata collaboration_metadata;
- collaboration_metadata.set_collaboration_id(collaboration_id);
- collaboration_metadata.mutable_creation_attribution()
- ->set_obfuscated_gaia_id(GetGaiaId().ToString());
- collaboration_metadata.mutable_last_update_attribution()
- ->set_obfuscated_gaia_id(GetGaiaId().ToString());
GetFakeServer()->InjectEntity(
syncer::PersistentUniqueClientEntity::
CreateFromSharedSpecificsForTesting(
@@ -204,7 +211,7 @@
GetClientTag(entity_specifics.shared_tab_group_data(),
collaboration_id),
entity_specifics, /*creation_time=*/0, /*last_modified_time=*/0,
- collaboration_metadata));
+ MakeCollaborationMetadata(collaboration_id)));
}
void AddSavedSpecificsToFakeServer(
@@ -240,6 +247,23 @@
TabGroupSyncService::TabGroupSharingCallback());
}
+ void InjectTombstoneToFakeServer(
+ const sync_pb::SharedTabGroupDataSpecifics& shared_group_specifics,
+ const CollaborationId& collaboration_id) {
+ const syncer::ClientTagHash shared_group_client_tag_hash =
+ syncer::ClientTagHash::FromUnhashed(
+ syncer::SHARED_TAB_GROUP_DATA,
+ GetClientTag(shared_group_specifics, collaboration_id.value()));
+
+ GetFakeServer()->InjectEntity(
+ syncer::PersistentTombstoneEntity::CreateNewShared(
+ syncer::LoopbackServerEntity::CreateId(
+ syncer::SHARED_TAB_GROUP_DATA,
+ shared_group_client_tag_hash.value()),
+ shared_group_client_tag_hash.value(),
+ MakeCollaborationMetadata(collaboration_id.value())));
+ }
+
// Returns the only saved tab group specifics from the fake server. The group
// must exist and be the only one.
sync_pb::SavedTabGroupSpecifics GetOnlySavedTabGroupSpecificsFromServer() {
@@ -616,6 +640,76 @@
HasTabMetadata("Saved tab 2", "http://google.com/saved_2")));
}
+// This test covers the following scenario for the device #2:
+// 1. User shares a saved tab group from device #1.
+// 2. Shared tab group is committed to the server.
+// 3. Shared tab group is received by device #2, the originating saved tab group
+// is transitioned to shared and marked as hidden.
+// 4. Sharing fails on device #1 and the shared tab group is deleted (uploading
+// tombstones).
+// 5. Device #2 receives the tombstones and applies the deletion of the shared
+// tab group. The originating saved tab group should be restored.
+IN_PROC_BROWSER_TEST_F(SingleClientSharedTabGroupDataSyncTest,
+ ShouldRestoreOriginatingSavedGroupOnShareFailure) {
+ const GURL kUrl = embedded_test_server()->GetURL(kDefaultURLPath);
+ const CollaborationId kCollaborationId("collaboration");
+
+ ASSERT_TRUE(SetupClients());
+ RegisterCollaboration(kCollaborationId);
+
+ // Create both shared and oritinating saved tab groups remotely.
+ const base::Uuid kOriginatingSavedGroupGuid = base::Uuid::GenerateRandomV4();
+ const base::Uuid kSharedGroupGuid = base::Uuid::GenerateRandomV4();
+
+ const sync_pb::SharedTabGroupDataSpecifics shared_group_specifics =
+ MakeSharedTabGroupSpecifics(
+ kSharedGroupGuid,
+ /*originating_saved_group_guid=*/kOriginatingSavedGroupGuid, "title",
+ sync_pb::SharedTabGroup::CYAN);
+ AddSpecificsToFakeServer(shared_group_specifics, kCollaborationId.value());
+ AddSpecificsToFakeServer(MakeSharedTabGroupTabSpecifics(
+ /*guid=*/base::Uuid::GenerateRandomV4(),
+ kSharedGroupGuid, kDefaultTabTitle, kUrl),
+ kCollaborationId.value());
+ AddSavedSpecificsToFakeServer(MakeSavedTabGroupSpecifics(
+ kOriginatingSavedGroupGuid, "title",
+ sync_pb::SavedTabGroup::SAVED_TAB_GROUP_COLOR_BLUE));
+ AddSavedSpecificsToFakeServer(MakeSavedTabGroupTabSpecifics(
+ /*guid=*/base::Uuid::GenerateRandomV4(), kOriginatingSavedGroupGuid,
+ kDefaultTabTitle, kUrl));
+
+ // The initial merge should result in a shared tab group with a hidden
+ // originating saved tab group.
+ ASSERT_TRUE(SetupSync());
+
+ ASSERT_TRUE(
+ SavedTabOrGroupExistsChecker(GetTabGroupSyncService(), kSharedGroupGuid)
+ .Wait());
+
+ // Only shared tab group is available from GetAllGroups().
+ ASSERT_THAT(GetTabGroupSyncService()->GetAllGroups(),
+ ElementsAre(HasSharedGroupMetadata(
+ "title", TabGroupColorId::kCyan, kCollaborationId.value())));
+
+ // The originating saved tab group is hidden but still available.
+ ASSERT_THAT(
+ GetTabGroupSyncService()->GetGroup(kOriginatingSavedGroupGuid),
+ Optional(HasSavedGroupMetadata(u"title", TabGroupColorId::kBlue)));
+
+ // Simulate a failure of the sharing operation on the remote client which
+ // resulted in a tombstone of the shared tab group.
+ InjectTombstoneToFakeServer(shared_group_specifics, kCollaborationId);
+
+ ASSERT_TRUE(SavedTabOrGroupDoesNotExistChecker(GetTabGroupSyncService(),
+ kSharedGroupGuid)
+ .Wait());
+
+ // The originating saved tab group should be restored and available.
+ EXPECT_THAT(
+ GetTabGroupSyncService()->GetAllGroups(),
+ ElementsAre(HasSavedGroupMetadata(u"title", TabGroupColorId::kBlue)));
+}
+
IN_PROC_BROWSER_TEST_F(SingleClientSharedTabGroupDataSyncTest,
ShouldFailDataTypeForCrossCollaborationUpdates) {
ASSERT_TRUE(SetupSync());
diff --git a/components/saved_tab_groups/internal/saved_tab_group_model.cc b/components/saved_tab_groups/internal/saved_tab_group_model.cc
index 7fb6956..755c540 100644
--- a/components/saved_tab_groups/internal/saved_tab_group_model.cc
+++ b/components/saved_tab_groups/internal/saved_tab_group_model.cc
@@ -856,6 +856,17 @@
}
}
+void SavedTabGroupModel::RestoreHiddenGroupFromSync(
+ const base::Uuid& group_id) {
+ SavedTabGroup* group = GetMutableGroup(group_id);
+ CHECK(group);
+ group->SetIsHidden(false);
+ for (SavedTabGroupModelObserver& observer : observers_) {
+ observer.SavedTabGroupUpdatedFromSync(group->saved_guid(),
+ /*tab_guid=*/std::nullopt);
+ }
+}
+
void SavedTabGroupModel::OnSyncBridgeUpdateTypeChanged(
SyncBridgeUpdateType sync_bridge_update_type) {
for (SavedTabGroupModelObserver& observer : observers_) {
diff --git a/components/saved_tab_groups/internal/saved_tab_group_model.h b/components/saved_tab_groups/internal/saved_tab_group_model.h
index 8dd9b39..a060bdcd 100644
--- a/components/saved_tab_groups/internal/saved_tab_group_model.h
+++ b/components/saved_tab_groups/internal/saved_tab_group_model.h
@@ -239,6 +239,9 @@
// Marks that a tab group is hidden and should not be shown to users.
void SetGroupHidden(const base::Uuid& group_id);
+ // Restores the hidden state of a tab group.
+ void RestoreHiddenGroupFromSync(const base::Uuid& group_id);
+
// Called to notify of the sync bridge state changes, e.g. whether initial
// merge or disable sync are in progress. Invoked only for shared tab group
// bridge.
diff --git a/components/saved_tab_groups/internal/tab_group_sync_service_impl.cc b/components/saved_tab_groups/internal/tab_group_sync_service_impl.cc
index b7b9ca2..862d6dc 100644
--- a/components/saved_tab_groups/internal/tab_group_sync_service_impl.cc
+++ b/components/saved_tab_groups/internal/tab_group_sync_service_impl.cc
@@ -1316,6 +1316,21 @@
return std::get<1>(entry) == removed_group.saved_guid();
});
+ if (removed_group.GetOriginatingTabGroupGuid().has_value()) {
+ const SavedTabGroup* originating_group =
+ model_->Get(removed_group.GetOriginatingTabGroupGuid().value());
+ if (originating_group && originating_group->is_hidden()) {
+ // It's possible that the originating saved tab group still exists when
+ // the shared tab group is removed. This can happen if the sharing
+ // operation failed on the remote client. In this case, restore the
+ // originating saved tab group.
+ // In case the shared tab group was unshared on the remote client, the
+ // originating saved tab group will be removed by the client as well, so
+ // it will be deleted from the model eventually.
+ model_->RestoreHiddenGroupFromSync(originating_group->saved_guid());
+ }
+ }
+
if (is_initialized_) {
for (auto& observer : observers_) {
observer.OnTabGroupRemoved(removed_group.saved_guid(), source);
diff --git a/components/saved_tab_groups/internal/tab_group_sync_service_unittest.cc b/components/saved_tab_groups/internal/tab_group_sync_service_unittest.cc
index 8cca2a4..e829d6f 100644
--- a/components/saved_tab_groups/internal/tab_group_sync_service_unittest.cc
+++ b/components/saved_tab_groups/internal/tab_group_sync_service_unittest.cc
@@ -878,6 +878,34 @@
ASSERT_TRUE(model_->Contains(shared_group.saved_guid()));
}
+TEST_F(TabGroupSyncServiceTest,
+ RestoreHiddenOriginatingSavedGroupOnRemoteSharingFailure) {
+ // Simulate a remote transition of `group_1_` to a shared tab group.
+ ASSERT_THAT(tab_group_sync_service_->GetAllGroups(),
+ Contains(HasGuid(group_1_.saved_guid())));
+
+ SavedTabGroup shared_group =
+ group_1_.CloneAsSharedTabGroup(CollaborationId(kCollaborationId));
+ shared_group.MarkTransitionedToShared();
+ ASSERT_FALSE(shared_group.saved_tabs().empty());
+ model_->AddedFromSync(shared_group);
+ WaitForPostedTasks();
+
+ // Only `shared_group` should be available in the service.
+ ASSERT_THAT(tab_group_sync_service_->GetAllGroups(),
+ Contains(HasGuid(shared_group.saved_guid())));
+ ASSERT_THAT(tab_group_sync_service_->GetAllGroups(),
+ Not(Contains(HasGuid(group_1_.saved_guid()))));
+
+ // Simulate a remote deletion of `shared_group`.
+ model_->RemovedFromSync(shared_group.saved_guid());
+ WaitForPostedTasks();
+
+ // The originating saved tab group should be restored and available.
+ EXPECT_THAT(tab_group_sync_service_->GetAllGroups(),
+ Contains(HasGuid(group_1_.saved_guid())));
+}
+
TEST_F(TabGroupSyncServiceTest, NavigateTab) {
base::HistogramTester histogram_tester;
auto local_tab_id_2 = test::GenerateRandomTabID();
diff --git a/components/sync/engine/loopback_server/persistent_tombstone_entity.cc b/components/sync/engine/loopback_server/persistent_tombstone_entity.cc
index dc52034..b147aca 100644
--- a/components/sync/engine/loopback_server/persistent_tombstone_entity.cc
+++ b/components/sync/engine/loopback_server/persistent_tombstone_entity.cc
@@ -22,14 +22,24 @@
std::unique_ptr<LoopbackServerEntity>
PersistentTombstoneEntity::CreateFromEntity(const sync_pb::SyncEntity& entity) {
return CreateNewInternal(entity.id_string(), entity.version(),
- entity.client_tag_hash());
+ entity.client_tag_hash(), entity.collaboration());
}
// static
std::unique_ptr<LoopbackServerEntity> PersistentTombstoneEntity::CreateNew(
const std::string& id,
const std::string& client_tag_hash) {
- return CreateNewInternal(id, 0, client_tag_hash);
+ return CreateNewInternal(id, 0, client_tag_hash,
+ sync_pb::SyncEntity::CollaborationMetadata());
+}
+
+// static
+std::unique_ptr<LoopbackServerEntity>
+PersistentTombstoneEntity::CreateNewShared(
+ const std::string& id,
+ const std::string& client_tag_hash,
+ const sync_pb::SyncEntity::CollaborationMetadata& collaboration_metadata) {
+ return CreateNewInternal(id, 0, client_tag_hash, collaboration_metadata);
}
// static
@@ -37,24 +47,27 @@
PersistentTombstoneEntity::CreateNewInternal(
const std::string& id,
int64_t version,
- const std::string& client_tag_hash) {
+ const std::string& client_tag_hash,
+ const sync_pb::SyncEntity::CollaborationMetadata& collaboration_metadata) {
const DataType data_type = LoopbackServerEntity::GetDataTypeFromId(id);
if (data_type == syncer::UNSPECIFIED) {
DLOG(WARNING) << "Invalid ID was given: " << id;
return nullptr;
}
- return base::WrapUnique(
- new PersistentTombstoneEntity(id, version, data_type, client_tag_hash));
+ return base::WrapUnique(new PersistentTombstoneEntity(
+ id, version, data_type, client_tag_hash, collaboration_metadata));
}
PersistentTombstoneEntity::PersistentTombstoneEntity(
const string& id,
int64_t version,
const DataType& data_type,
- const std::string& client_tag_hash)
+ const std::string& client_tag_hash,
+ const sync_pb::SyncEntity::CollaborationMetadata& collaboration_metadata)
: LoopbackServerEntity(id, data_type, version, string()),
- client_tag_hash_(client_tag_hash) {
+ client_tag_hash_(client_tag_hash),
+ collaboration_metadata_(collaboration_metadata) {
sync_pb::EntitySpecifics specifics;
AddDefaultFieldValue(data_type, &specifics);
SetSpecifics(specifics);
@@ -74,6 +87,9 @@
if (!client_tag_hash_.empty()) {
proto->set_client_tag_hash(client_tag_hash_);
}
+ if (collaboration_metadata_.has_collaboration_id()) {
+ *proto->mutable_collaboration() = collaboration_metadata_;
+ }
}
bool PersistentTombstoneEntity::IsDeleted() const {
diff --git a/components/sync/engine/loopback_server/persistent_tombstone_entity.h b/components/sync/engine/loopback_server/persistent_tombstone_entity.h
index f89379c9..2b37bc3 100644
--- a/components/sync/engine/loopback_server/persistent_tombstone_entity.h
+++ b/components/sync/engine/loopback_server/persistent_tombstone_entity.h
@@ -10,6 +10,7 @@
#include "components/sync/base/data_type.h"
#include "components/sync/engine/loopback_server/loopback_server_entity.h"
+#include "components/sync/protocol/sync_entity.pb.h"
namespace sync_pb {
class SyncEntity;
@@ -31,6 +32,11 @@
const std::string& id,
const std::string& client_tag_hash);
+ static std::unique_ptr<LoopbackServerEntity> CreateNewShared(
+ const std::string& id,
+ const std::string& client_tag_hash,
+ const sync_pb::SyncEntity::CollaborationMetadata& collaboration_metadata);
+
// LoopbackServerEntity implementation.
bool RequiresParentId() const override;
std::string GetParentId() const override;
@@ -43,15 +49,21 @@
static std::unique_ptr<LoopbackServerEntity> CreateNewInternal(
const std::string& id,
int64_t version,
- const std::string& client_tag_hash);
+ const std::string& client_tag_hash,
+ const sync_pb::SyncEntity::CollaborationMetadata& collaboration_metadata);
- PersistentTombstoneEntity(const std::string& id,
- int64_t version,
- const syncer::DataType& data_type,
- const std::string& client_tag_hash);
+ PersistentTombstoneEntity(
+ const std::string& id,
+ int64_t version,
+ const syncer::DataType& data_type,
+ const std::string& client_tag_hash,
+ const sync_pb::SyncEntity::CollaborationMetadata& collaboration_metadata);
// The tag hash for this entity.
const std::string client_tag_hash_;
+
+ // Collaboration metadata for this entity for shared data types.
+ const sync_pb::SyncEntity::CollaborationMetadata collaboration_metadata_;
};
} // namespace syncer
