Skip to content

Commit

Permalink
[search_engines] Stop making duplicate engines in ProcessSyncChanges()
Browse files Browse the repository at this point in the history 
This CL simplifies ProcessSyncChanges() to no longer attempt to uniquify
keywords for engines.

We can do that now because the preceding CL made Add() and Update()
tolerant of engines with keyword conflicts:
https://chromium-review.googlesource.com/c/chromium/src/+/2548160

The long explanation is here:
go/chrome-search-engines-2020-improvements

Bug: 1022775
Change-Id: If99d4d12510625b9474e4abfcb235c202f4911cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558911
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832023}
  • Loading branch information
Tommy Li authored and Chromium LUCI CQ committed Nov 30, 2020
1 parent 319669d commit 81a1da0
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 79 deletions.
94 changes: 47 additions & 47 deletions chrome/browser/search_engines/template_url_service_sync_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -970,11 +970,12 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesNoConflicts) {
EXPECT_TRUE(model()->GetTemplateURLForGUID("key4"));
}

TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsSyncWins) {
TEST_F(TemplateURLServiceSyncTest,
ProcessChangesWithDuplicateKeywordsSyncWins) {
MergeAndExpectNotify(CreateInitialSyncData(), 1);

// Process different types of changes, with conflicts. Note that all this data
// has a newer timestamp, so Sync will win in these scenarios.
// Process different types of changes, with duplicate keywords. Note that all
// this data has a newer timestamp, so Sync will win in these scenarios.
syncer::SyncChangeList changes;
changes.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_ADD,
CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://new.com", "aaa")));
Expand All @@ -983,75 +984,74 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsSyncWins) {
ProcessAndExpectNotify(changes, 1);

// Add one, update one, so we're up to 4.
EXPECT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
// Sync is always newer here, so it should always win. We should create
// SyncChanges for the changes to the local entities, since they're synced
// too.
EXPECT_EQ(2U, processor()->change_list_size());
ASSERT_TRUE(processor()->contains_guid("key2"));
EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
processor()->change_for_guid("key2").change_type());
ASSERT_TRUE(processor()->contains_guid("key3"));
EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
processor()->change_for_guid("key3").change_type());
ASSERT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());

// aaa conflicts with key2 and wins, forcing key2's keyword to update.
// aaa duplicates the keyword of key2 and wins. key2 still has its keyword,
// but is shadowed by aaa.
EXPECT_TRUE(model()->GetTemplateURLForGUID("aaa"));
EXPECT_EQ(model()->GetTemplateURLForGUID("aaa"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key2")));
EXPECT_TRUE(model()->GetTemplateURLForGUID("key2"));
EXPECT_EQ(model()->GetTemplateURLForGUID("key2"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key2.com")));
// key1 update conflicts with key3 and wins, forcing key3's keyword to update.
TemplateURL* key2_turl = model()->GetTemplateURLForGUID("key2");
ASSERT_TRUE(key2_turl);
ASSERT_EQ(ASCIIToUTF16("key2"), key2_turl->keyword());
// key1 update duplicates the keyword of key3 and wins. key3 still has its
// keyword but is shadowed by key1 now.
EXPECT_TRUE(model()->GetTemplateURLForGUID("key1"));
EXPECT_EQ(model()->GetTemplateURLForGUID("key1"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key3")));
EXPECT_TRUE(model()->GetTemplateURLForGUID("key3"));
EXPECT_EQ(model()->GetTemplateURLForGUID("key3"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key3.com")));
TemplateURL* key3_turl = model()->GetTemplateURLForGUID("key3");
ASSERT_TRUE(key3_turl);
EXPECT_EQ(ASCIIToUTF16("key3"), key3_turl->keyword());

// Sync is always newer here, so it should always win. But we DO NOT create
// new sync updates in response to processing sync changes. That could cause
// an infinite loop. Instead, on next startup, we will merge changes anyways.
EXPECT_EQ(0U, processor()->change_list_size());
}

TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsLocalWins) {
TEST_F(TemplateURLServiceSyncTest,
ProcessChangesWithDuplicateKeywordsLocalWins) {
MergeAndExpectNotify(CreateInitialSyncData(), 1);

// Process different types of changes, with conflicts. Note that all this data
// has an older timestamp, so the local data will win in these scenarios.
// Process different types of changes, with duplicate keywords. Note that all
// this data has an older timestamp, so the local data will win in this case.
syncer::SyncChangeList changes;
changes.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_ADD,
CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://new.com", "aaa",
10)));
// Update the keyword of engine with GUID "key1" to "key3", which will
// duplicate the keyword of engine with GUID "key3".
changes.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_UPDATE,
CreateTestTemplateURL(ASCIIToUTF16("key3"), "http://key3.com", "key1",
10)));
ProcessAndExpectNotify(changes, 1);

// Add one, update one, so we're up to 4.
EXPECT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
// Local data wins twice so two updates are pushed up to Sync.
EXPECT_EQ(2U, processor()->change_list_size());

// aaa conflicts with key2 and loses, forcing it's keyword to update.
EXPECT_TRUE(model()->GetTemplateURLForGUID("aaa"));
EXPECT_EQ(model()->GetTemplateURLForGUID("aaa"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("new.com")));
EXPECT_TRUE(model()->GetTemplateURLForGUID("key2"));
EXPECT_EQ(model()->GetTemplateURLForGUID("key2"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key2")));
// key1 update conflicts with key3 and loses, forcing key1's keyword to
// update.
EXPECT_TRUE(model()->GetTemplateURLForGUID("key1"));
EXPECT_EQ(model()->GetTemplateURLForGUID("key1"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key3.com")));
ASSERT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());

// aaa duplicates the keyword of key2 and loses. It still exists, and kept its
// keyword as "key2", but it's NOT best TemplateURL for "key2".
TemplateURL* aaa_turl = model()->GetTemplateURLForGUID("aaa");
ASSERT_TRUE(aaa_turl);
EXPECT_EQ(ASCIIToUTF16("key2"), aaa_turl->keyword());

TemplateURL* key2_turl = model()->GetTemplateURLForGUID("key2");
ASSERT_TRUE(key2_turl);
EXPECT_NE(aaa_turl, key2_turl);
EXPECT_EQ(key2_turl, model()->GetTemplateURLForKeyword(ASCIIToUTF16("key2")));

// key1 update duplicates the keyword of key3 and loses. It updates its
// keyword to "key3", but is NOT the best TemplateURL for "key3".
TemplateURL* key1_turl = model()->GetTemplateURLForGUID("key1");
ASSERT_TRUE(key1_turl);
EXPECT_EQ(ASCIIToUTF16("key3"), key1_turl->keyword());
EXPECT_TRUE(model()->GetTemplateURLForGUID("key3"));
EXPECT_EQ(model()->GetTemplateURLForGUID("key3"),
model()->GetTemplateURLForKeyword(ASCIIToUTF16("key3")));

ASSERT_TRUE(processor()->contains_guid("aaa"));
EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
processor()->change_for_guid("aaa").change_type());
ASSERT_TRUE(processor()->contains_guid("key1"));
EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
processor()->change_for_guid("key1").change_type());
// Local data wins twice, but we specifically DO NOT push updates to Sync
// in response to processing sync updates. That can cause an infinite loop.
EXPECT_EQ(0U, processor()->change_list_size());
}

TEST_F(TemplateURLServiceSyncTest, ProcessTemplateURLChange) {
Expand Down
85 changes: 53 additions & 32 deletions components/search_engines/template_url_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ enum SearchTemplateURLEvent {
SYNC_DELETE_SUCCESS = 0,
SYNC_DELETE_FAIL_NONEXISTENT_ENGINE = 1,
SYNC_DELETE_FAIL_DEFAULT_SEARCH_PROVIDER = 2,
SYNC_ADD_SUCCESS = 3,
SYNC_ADD_CONVERTED_TO_UPDATE = 4,
SYNC_ADD_FAIL_OTHER_ERROR = 5,
SYNC_UPDATE_SUCCESS = 6,
SYNC_UPDATE_CONVERTED_TO_ADD = 7,
SEARCH_TEMPLATE_URL_EVENT_MAX,
};

Expand Down Expand Up @@ -1041,48 +1046,64 @@ base::Optional<syncer::ModelError> TemplateURLService::ProcessSyncChanges(
continue;
}

if ((iter->change_type() == syncer::SyncChange::ACTION_ADD &&
existing_turl) ||
(iter->change_type() == syncer::SyncChange::ACTION_UPDATE &&
!existing_turl)) {
// Can't ADD an already-existing engine, and can't UPDATE a non-existent
// engine. Early exit here to avoid ResolvingSyncKeywordConflict().
error = sync_error_factory_->CreateAndUploadError(FROM_HERE, error_msg);
continue;
}
// Because TemplateURLService sometimes ignores remote Sync changes which
// we cannot cleanly apply, we need to handle ADD and UPDATE together.
// Ignore what the other Sync layers THINK the change type is. Instead:
// If we have an existing engine, treat as an update.
DCHECK(iter->change_type() == syncer::SyncChange::ACTION_ADD ||
iter->change_type() == syncer::SyncChange::ACTION_UPDATE);

// Explicitly don't check for conflicts against extension keywords; in this
// case the functions which modify the keyword map know how to handle the
// conflicts.
// TODO(mpcomplete): If we allow editing extension keywords, then those will
// need to undergo conflict resolution.
TemplateURL* existing_keyword_turl =
FindNonExtensionTemplateURLForKeyword(turl->keyword());
const bool has_conflict =
existing_keyword_turl && (existing_keyword_turl != existing_turl);
if (has_conflict) {
// Resolve any conflicts with other entries so we can safely update the
// keyword.
ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
&new_changes);
}
if (!existing_turl) {
if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE) {
// This can happen if we have silently deleted a replaceable engine due
// to keyword conflict, and Sync server sends us an UPDATE to it.
UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName,
SYNC_UPDATE_CONVERTED_TO_ADD,
SEARCH_TEMPLATE_URL_EVENT_MAX);
}

if (iter->change_type() == syncer::SyncChange::ACTION_ADD) {
base::AutoReset<DefaultSearchChangeOrigin> change_origin(
&dsp_change_origin_, DSP_CHANGE_SYNC_ADD);
// Force the local ID to kInvalidTemplateURLID so we can add it.
TemplateURLData data(turl->data());
data.id = kInvalidTemplateURLID;
auto added_ptr = std::make_unique<TemplateURL>(data);
TemplateURL* added = added_ptr.get();
if (Add(std::move(added_ptr)))

TemplateURL* added = Add(std::make_unique<TemplateURL>(data));
if (added) {
MaybeUpdateDSEViaPrefs(added);
continue;
}

DCHECK_EQ(syncer::SyncChange::ACTION_UPDATE, iter->change_type());
if (Update(existing_turl, *turl))
UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName,
SYNC_ADD_SUCCESS,
SEARCH_TEMPLATE_URL_EVENT_MAX);
} else {
// Currently, in practice, this means that we tried to add a replaceable
// duplicate that was worse than our existing entry, but the API doesn't
// promise that, so we just log a generic SYNC_ADD_FAIL_OTHER_ERROR.
UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName,
SYNC_ADD_FAIL_OTHER_ERROR,
SEARCH_TEMPLATE_URL_EVENT_MAX);
}
} else {
if (iter->change_type() == syncer::SyncChange::ACTION_ADD) {
// This can happen if we have ignored a DELETE request in the past to
// avoid deleting the default search provider, and later on, Sync tries
// to re-ADD something it thinks it has deleted.
UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName,
SYNC_ADD_CONVERTED_TO_UPDATE,
SEARCH_TEMPLATE_URL_EVENT_MAX);
}

// Since we've already found |existing_turl| by GUID, this Update() should
// always return true, but we still don't want to crash if it fails.
DCHECK(existing_turl);
bool update_success = Update(existing_turl, *turl);
DCHECK(update_success);

MaybeUpdateDSEViaPrefs(existing_turl);
UMA_HISTOGRAM_ENUMERATION(kSearchTemplateURLEventsHistogramName,
SYNC_UPDATE_SUCCESS,
SEARCH_TEMPLATE_URL_EVENT_MAX);
}
}

// If something went wrong, we want to prematurely exit to avoid pushing
Expand Down
5 changes: 5 additions & 0 deletions tools/metrics/histograms/enums.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65925,6 +65925,11 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
<int value="0" label="SYNC_DELETE_SUCCESS"/>
<int value="1" label="SYNC_DELETE_FAIL_NONEXISTENT_ENGINE"/>
<int value="2" label="SYNC_DELETE_FAIL_DEFAULT_SEARCH_PROVIDER"/>
<int value="3" label="SYNC_ADD_SUCCESS"/>
<int value="4" label="SYNC_ADD_CONVERTED_TO_UPDATE"/>
<int value="5" label="SYNC_ADD_FAIL_OTHER_ERROR"/>
<int value="6" label="SYNC_UPDATE_SUCCESS"/>
<int value="7" label="SYNC_UPDATE_CONVERTED_TO_ADD"/>
</enum>

<enum name="SearchWidgetUseInfo">
Expand Down

0 comments on commit 81a1da0

Please sign in to comment.