Skip to content

Commit 18fa0a3

Browse files
bscliftonbsclifton
authored
Merge pull request #5212 from brave/bsc-uplift-5139-to-1.7.x
Sync duplicated object id migration fixes (uplift to 1.7.x)
2 parents 079e748 + 2498d9f commit 18fa0a3

7 files changed

+183
-70
lines changed

‎browser/profiles/brave_bookmark_model_loaded_observer.cc

+5-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "brave/browser/profiles/brave_bookmark_model_loaded_observer.h"
77

88
#include "brave/common/pref_names.h"
9+
#include "brave/components/brave_sync/features.h"
910
#include "chrome/browser/profiles/profile.h"
1011
#include "chrome/browser/sync/profile_sync_service_factory.h"
1112
#include "components/bookmarks/browser/bookmark_model.h"
@@ -44,8 +45,10 @@ void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded(
4445

4546
#if BUILDFLAG(ENABLE_BRAVE_SYNC)
4647
BraveProfileSyncServiceImpl::AddNonClonedBookmarkKeys(model);
47-
BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile_,
48-
model);
48+
BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
49+
base::FeatureList::IsEnabled(brave_sync::features::kBraveSync),
50+
profile_,
51+
model);
4952
#endif
5053

5154
BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned);

‎components/brave_sync/brave_profile_sync_service_impl.cc

+56-34
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include <algorithm>
99
#include <map>
10+
#include <set>
1011
#include <utility>
1112
#include <vector>
1213

@@ -167,19 +168,8 @@ SyncRecordPtr PrepareResolvedDevice(SyncDevice* device,
167168
return record;
168169
}
169170

170-
struct BookmarkByDateAddedComparator {
171-
bool operator()(const bookmarks::BookmarkNode* lhs,
172-
const bookmarks::BookmarkNode* rhs) const {
173-
DCHECK(lhs);
174-
DCHECK(rhs);
175-
DCHECK(!tools::IsTimeEmpty(lhs->date_added()));
176-
DCHECK(!tools::IsTimeEmpty(rhs->date_added()));
177-
return lhs->date_added() < rhs->date_added();
178-
}
179-
};
180-
using SortedNodes =
181-
std::set<const bookmarks::BookmarkNode*, BookmarkByDateAddedComparator>;
182-
using ObjectIdToNodes = std::map<std::string, SortedNodes>;
171+
using NodesSet = std::set<const bookmarks::BookmarkNode*>;
172+
using ObjectIdToNodes = std::map<std::string, NodesSet>;
183173

184174
void FillObjectsMap(const bookmarks::BookmarkNode* parent,
185175
ObjectIdToNodes* object_id_nodes) {
@@ -196,27 +186,53 @@ void FillObjectsMap(const bookmarks::BookmarkNode* parent,
196186
}
197187
}
198188

189+
void AddDeletedChildren(const BookmarkNode* node, NodesSet* deleted_nodes) {
190+
for (const auto& child : node->children()) {
191+
deleted_nodes->insert(child.get());
192+
if (node->is_folder()) {
193+
AddDeletedChildren(child.get(), deleted_nodes);
194+
}
195+
}
196+
}
197+
199198
void ClearDuplicatedNodes(ObjectIdToNodes* object_id_nodes,
200199
bookmarks::BookmarkModel* model) {
200+
size_t nodes_recreated = 0;
201+
NodesSet nodes_with_duplicates;
201202
for (ObjectIdToNodes::iterator it_object_id = object_id_nodes->begin();
202203
it_object_id != object_id_nodes->end(); ++it_object_id) {
203-
const SortedNodes& nodes = it_object_id->second;
204+
const NodesSet& nodes = it_object_id->second;
204205
if (nodes.size() > 1) {
205-
// Nodes are sorted from oldest to newest, go to the second by age
206-
SortedNodes::iterator it_nodes = nodes.begin();
207-
++it_nodes;
208-
for (; it_nodes != nodes.end(); ++it_nodes) {
209-
const bookmarks::BookmarkNode* node = *it_nodes;
210-
// Copy and delete node
211-
const auto* parent = node->parent();
212-
size_t original_index = parent->GetIndexOf(node);
213-
model->Copy(node, parent, original_index);
214-
model->Remove(node);
215-
brave_sync::AddBraveMetaInfo(parent->children()[original_index].get());
216-
}
206+
nodes_with_duplicates.insert(nodes.begin(), nodes.end());
207+
}
208+
}
209+
210+
NodesSet deleted_nodes;
211+
for (const bookmarks::BookmarkNode* node : nodes_with_duplicates) {
212+
if (deleted_nodes.find(node) != deleted_nodes.end()) {
213+
// Node already has been deleted
214+
continue;
215+
}
216+
217+
deleted_nodes.insert(node);
218+
if (node->is_folder()) {
219+
AddDeletedChildren(node, &deleted_nodes);
217220
}
221+
222+
const auto* parent = node->parent();
223+
size_t original_index = parent->GetIndexOf(node);
224+
VLOG(1) << "[BraveSync] " << __func__
225+
<< " Copying node into index=" << original_index;
226+
model->Copy(node, parent, original_index);
227+
VLOG(1) << "[BraveSync] " << __func__ << " Removing original node";
228+
model->Remove(node);
229+
nodes_recreated++;
218230
}
231+
232+
VLOG(1) << "[BraveSync] " << __func__
233+
<< " done nodes_recreated=" << nodes_recreated;
219234
}
235+
220236
} // namespace
221237

222238
BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile,
@@ -808,27 +824,33 @@ void BraveProfileSyncServiceImpl::SetPermanentNodesOrder(
808824
}
809825

810826
// static
811-
void BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
827+
bool BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
828+
bool sync_enabled,
812829
Profile* profile,
813830
BookmarkModel* model) {
831+
if (!sync_enabled) {
832+
return false;
833+
}
834+
814835
DCHECK(model);
815836
DCHECK(model->loaded());
816837

817-
bool duplicated_bookmarks_recovered =
818-
profile->GetPrefs()->GetBoolean(prefs::kDuplicatedBookmarksRecovered);
819-
if (duplicated_bookmarks_recovered) {
820-
return;
838+
int migrated_version = profile->GetPrefs()->GetInteger(
839+
prefs::kDuplicatedBookmarksMigrateVersion);
840+
841+
if (migrated_version >= 2) {
842+
return true;
821843
}
822844

823845
// Copying bookmarks through brave://bookmarks page could duplicate brave sync
824846
// metadata, which caused crash during chromium sync run
825-
// Go through nodes and re-create the oldest ones who have duplicated
826-
// object_id
847+
// Go through nodes and re-create those ones who have duplicated object_id
827848
ObjectIdToNodes object_id_nodes;
828849
FillObjectsMap(model->root_node(), &object_id_nodes);
829850
ClearDuplicatedNodes(&object_id_nodes, model);
830851

831-
profile->GetPrefs()->SetBoolean(prefs::kDuplicatedBookmarksRecovered, true);
852+
profile->GetPrefs()->SetInteger(prefs::kDuplicatedBookmarksMigrateVersion, 2);
853+
return true;
832854
}
833855

834856
std::unique_ptr<SyncRecord>

‎components/brave_sync/brave_profile_sync_service_impl.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,8 @@ class BraveProfileSyncServiceImpl
187187

188188
BraveSyncService* GetSyncService() const override;
189189

190-
static void MigrateDuplicatedBookmarksObjectIds(Profile* profile,
190+
static bool MigrateDuplicatedBookmarksObjectIds(bool sync_enabled,
191+
Profile* profile,
191192
BookmarkModel* model);
192193

193194
private:

‎components/brave_sync/brave_sync_prefs.cc

+4
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
void MigrateBraveSyncPrefs(PrefService* prefs) {
1717
prefs->ClearPref(brave_sync::prefs::kSyncPrevSeed);
18+
prefs->ClearPref(brave_sync::prefs::kDuplicatedBookmarksRecovered);
1819
}
1920

2021
namespace brave_sync {
@@ -45,6 +46,8 @@ const char kSyncRecordsToResend[] = "brave_sync_records_to_resend";
4546
const char kSyncRecordsToResendMeta[] = "brave_sync_records_to_resend_meta";
4647
const char kDuplicatedBookmarksRecovered[] =
4748
"brave_sync_duplicated_bookmarks_recovered";
49+
const char kDuplicatedBookmarksMigrateVersion[] =
50+
"brave_sync_duplicated_bookmarks_migrate_version";
4851

4952
Prefs::Prefs(PrefService* pref_service) : pref_service_(pref_service) {}
5053

@@ -75,6 +78,7 @@ void Prefs::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
7578
registry->RegisterListPref(prefs::kSyncRecordsToResend);
7679
registry->RegisterDictionaryPref(prefs::kSyncRecordsToResendMeta);
7780
registry->RegisterBooleanPref(kDuplicatedBookmarksRecovered, false);
81+
registry->RegisterIntegerPref(prefs::kDuplicatedBookmarksMigrateVersion, 0);
7882
}
7983

8084
std::string Prefs::GetSeed() const {

‎components/brave_sync/brave_sync_prefs.h

+5-1
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,13 @@ extern const char kSyncMigrateBookmarksVersion[];
7272
extern const char kSyncRecordsToResend[];
7373
// Meta info of kSyncRecordsToResend
7474
extern const char kSyncRecordsToResendMeta[];
75-
// Flag indicates we had recovered duplicated bookmarks object ids
75+
// Flag indicates we had recovered duplicated bookmarks object ids (deprecated)
7676
extern const char kDuplicatedBookmarksRecovered[];
7777

78+
// Version indicates had recovered duplicated bookmarks object ids:
79+
// 2 - we had migrated object ids
80+
extern const char kDuplicatedBookmarksMigrateVersion[];
81+
7882
class Prefs {
7983
public:
8084
explicit Prefs(PrefService* pref_service);

‎components/brave_sync/brave_sync_service_unittest.cc

+109-30
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "brave/components/brave_sync/brave_sync_service_observer.h"
1616
#include "brave/components/brave_sync/client/brave_sync_client_impl.h"
1717
#include "brave/components/brave_sync/client/client_ext_impl_data.h"
18+
#include "brave/components/brave_sync/features.h"
1819
#include "brave/components/brave_sync/jslib_const.h"
1920
#include "brave/components/brave_sync/jslib_messages.h"
2021
#include "brave/components/brave_sync/settings.h"
@@ -1632,50 +1633,128 @@ TEST_F(BraveSyncServiceTest, AddNonClonedBookmarkKeys) {
16321633
EXPECT_TRUE(meta_version.empty());
16331634
}
16341635

1636+
namespace {
1637+
1638+
void SetBraveMeta(const bookmarks::BookmarkNode* node,
1639+
const std::string& object_id,
1640+
const std::string& order,
1641+
const std::string& sync_timestamp,
1642+
const std::string& version) {
1643+
bookmarks::BookmarkNode* mutable_node = AsMutable(node);
1644+
mutable_node->SetMetaInfo("object_id", object_id);
1645+
mutable_node->SetMetaInfo("order", order);
1646+
mutable_node->SetMetaInfo("sync_timestamp", sync_timestamp);
1647+
mutable_node->SetMetaInfo("version", version);
1648+
}
1649+
1650+
void GetAllNodes(const bookmarks::BookmarkNode* parent,
1651+
std::set<const bookmarks::BookmarkNode*>* all_nodes) {
1652+
for (size_t i = 0; i < parent->children().size(); ++i) {
1653+
const bookmarks::BookmarkNode* current_child = parent->children()[i].get();
1654+
all_nodes->insert(current_child);
1655+
if (current_child->is_folder()) {
1656+
GetAllNodes(current_child, all_nodes);
1657+
}
1658+
}
1659+
}
1660+
1661+
} // namespace
1662+
16351663
TEST_F(BraveSyncServiceTest, MigrateDuplicatedBookmarksObjectIds) {
16361664
AsMutable(model()->other_node())->SetMetaInfo("order", kOtherNodeOrder);
16371665

16381666
const bookmarks::BookmarkNode* bookmark_a1 =
16391667
model()->AddURL(model()->other_node(), 0, base::ASCIIToUTF16("A1"),
16401668
GURL("https://a1.com"));
16411669

1642-
AsMutable(bookmark_a1)->SetMetaInfo("object_id", "object_id_value");
1643-
AsMutable(bookmark_a1)->SetMetaInfo("order", "255.255.255.3");
1644-
AsMutable(bookmark_a1)->SetMetaInfo("sync_timestamp", "sync_timestamp_value");
1645-
AsMutable(bookmark_a1)->SetMetaInfo("version", "version_value");
1670+
SetBraveMeta(bookmark_a1, "object_id_value", "255.255.255.3",
1671+
"sync_timestamp_value", "version_value");
16461672

16471673
model()->Copy(bookmark_a1, model()->other_node(), 1);
16481674

1649-
const bookmarks::BookmarkNode* bookmark_copy =
1650-
model()->other_node()->children().at(1).get();
1651-
1652-
std::string meta_object_id;
1653-
EXPECT_TRUE(bookmark_copy->GetMetaInfo("object_id", &meta_object_id));
1654-
EXPECT_EQ(meta_object_id, "object_id_value");
1655-
std::string meta_order;
1656-
EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_order));
1657-
EXPECT_EQ(meta_order, "255.255.255.3");
1658-
std::string meta_sync_timestamp;
1659-
EXPECT_TRUE(
1660-
bookmark_copy->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
1661-
EXPECT_EQ(meta_sync_timestamp, "sync_timestamp_value");
1662-
std::string meta_version;
1663-
EXPECT_TRUE(bookmark_copy->GetMetaInfo("version", &meta_version));
1664-
EXPECT_EQ(meta_version, "version_value");
1675+
model()->Copy(bookmark_a1, model()->other_node(), 2);
1676+
1677+
const bookmarks::BookmarkNode* folder_f1 =
1678+
model()->AddFolder(model()->other_node(), 3, base::ASCIIToUTF16("F1"));
1679+
SetBraveMeta(folder_f1, "object_id_value", "255.255.255.5",
1680+
"sync_timestamp_value", "version_value");
1681+
1682+
const bookmarks::BookmarkNode* bookmark_b1 = model()->AddURL(
1683+
folder_f1, 0, base::ASCIIToUTF16("B1"), GURL("https://b1.com"));
1684+
SetBraveMeta(bookmark_b1, "object_id_value", "255.255.255.5.1",
1685+
"sync_timestamp_value", "version_value");
1686+
1687+
model()->Copy(folder_f1, model()->other_node(), 4);
1688+
model()->Copy(folder_f1, model()->other_node(), 5);
1689+
model()->Move(model()->other_node()->children()[5].get(), folder_f1, 0);
1690+
1691+
std::set<const bookmarks::BookmarkNode*> all_nodes;
1692+
GetAllNodes(model()->other_node(), &all_nodes);
1693+
for (const bookmarks::BookmarkNode* node : all_nodes) {
1694+
// Verify fields after copying
1695+
std::string meta_object_id;
1696+
EXPECT_TRUE(node->GetMetaInfo("object_id", &meta_object_id));
1697+
EXPECT_EQ(meta_object_id, "object_id_value");
1698+
std::string meta_sync_timestamp;
1699+
EXPECT_TRUE(node->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
1700+
EXPECT_EQ(meta_sync_timestamp, "sync_timestamp_value");
1701+
std::string meta_version;
1702+
EXPECT_TRUE(node->GetMetaInfo("version", &meta_version));
1703+
EXPECT_EQ(meta_version, "version_value");
1704+
1705+
// Simulate all bookmarks don`t have added time, as a worse case,
1706+
// but happened on live profile
1707+
AsMutable(node)->set_date_added(base::Time());
1708+
}
16651709

16661710
sync_service()->AddNonClonedBookmarkKeys(model());
16671711

16681712
// Do the migration
1669-
BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile(),
1670-
model());
1671-
bookmark_copy = model()->other_node()->children().at(1).get();
1713+
bool result =
1714+
BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
1715+
true,
1716+
profile(),
1717+
model());
1718+
EXPECT_TRUE(result);
1719+
1720+
// All the bookmarks after migration must not have sync meta info
1721+
all_nodes.clear();
1722+
GetAllNodes(model()->other_node(), &all_nodes);
1723+
for (const bookmarks::BookmarkNode* bookmark : all_nodes) {
1724+
std::string migrated_object_id;
1725+
std::string migrated_order;
1726+
std::string migrated_sync_timestamp;
1727+
std::string migrated_version;
1728+
EXPECT_FALSE(bookmark->GetMetaInfo("object_id", &migrated_object_id));
1729+
EXPECT_FALSE(bookmark->GetMetaInfo("order", &migrated_order));
1730+
EXPECT_FALSE(
1731+
bookmark->GetMetaInfo("sync_timestamp", &migrated_sync_timestamp));
1732+
EXPECT_FALSE(bookmark->GetMetaInfo("version", &migrated_version));
1733+
}
1734+
}
16721735

1673-
std::string meta_migrated_object_id;
1674-
EXPECT_TRUE(
1675-
bookmark_copy->GetMetaInfo("object_id", &meta_migrated_object_id));
1676-
EXPECT_NE(meta_migrated_object_id, "object_id_value");
1736+
TEST_F(BraveSyncServiceTest, SyncDisabledMigrateDuplicatedBookmarksObjectIds) {
1737+
AsMutable(model()->other_node())->SetMetaInfo("order", kOtherNodeOrder);
16771738

1678-
std::string meta_migrated_order;
1679-
EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_migrated_order));
1680-
EXPECT_NE(meta_migrated_order, "255.255.255.3");
1739+
const bookmarks::BookmarkNode* bookmark_a1 =
1740+
model()->AddURL(model()->other_node(), 0, base::ASCIIToUTF16("A1"),
1741+
GURL("https://a1.com"));
1742+
1743+
SetBraveMeta(bookmark_a1, "object_id_value", "255.255.255.3",
1744+
"sync_timestamp_value", "version_value");
1745+
1746+
model()->Copy(bookmark_a1, model()->other_node(), 1);
1747+
1748+
model()->Copy(bookmark_a1, model()->other_node(), 2);
1749+
1750+
// Do the migration
1751+
bool result =
1752+
BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
1753+
false,
1754+
profile(),
1755+
model());
1756+
1757+
// Should return false, because sync is disable
1758+
// Migration will be a no-op
1759+
EXPECT_FALSE(result);
16811760
}

‎components/brave_sync/syncer_helper.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,8 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) {
103103
DCHECK(!sync_timestamp.empty());
104104
// Set other_node to have same sync_timestamp as least added child
105105
if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE) {
106-
tools::AsMutable(node->parent())->SetMetaInfo("sync_timestamp",
107-
sync_timestamp);
106+
tools::AsMutable(node->parent())
107+
->SetMetaInfo("sync_timestamp", sync_timestamp);
108108
}
109109
}
110110

0 commit comments

Comments
 (0)