Commit bc486700 authored by Tommy Li's avatar Tommy Li Committed by Chromium LUCI CQ

[search] Update RepeatableQueriesService to tolerate nullptr DSE

TemplateURLService::GetDefaultSearchProvider() can sometimes return
nullptr.

This condition occurs more frequently on startup, before the
TemplateURLService has loaded the sqlite3 keyword table.

This condition is made more frequent (temporarily) since my recent
patch:
https://chromium.googlesource.com/chromium/src/+/ecae40df17fd8eaea5e45ad62eae8e5c906e3aa7

This CL fixes a crash by making RepeatableQueriesService check whether
the DSE is nullptr and gracefully early exiting in that condition.

Bug: 1163453
Change-Id: I2ac2b9ca205ed7b3ecfa81cdf425b246ea2f1d8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2613387
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: default avatarMoe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840932}
parent 63cdc438
......@@ -184,8 +184,12 @@ void RepeatableQueriesService::DeleteQueryWithDestinationURL(const GURL& url) {
}
// Delete all the Google search URLs for the given query from history.
history_service_->DeleteMatchingURLsForKeyword(
template_url_service_->GetDefaultSearchProvider()->id(), it->query);
const TemplateURL* default_provider =
template_url_service_->GetDefaultSearchProvider();
if (default_provider) {
history_service_->DeleteMatchingURLsForKeyword(default_provider->id(),
it->query);
}
// Make sure the query is not suggested again.
MarkQueryAsDeleted(it->query);
......@@ -227,10 +231,12 @@ void RepeatableQueriesService::SigninStatusChanged() {
}
GURL RepeatableQueriesService::GetQueryDestinationURL(
const base::string16& query) {
const base::string16& query,
const TemplateURL* search_provider) {
DCHECK(search_provider);
TemplateURLRef::SearchTermsArgs search_terms_args(query);
const TemplateURLRef& search_url_ref =
template_url_service_->GetDefaultSearchProvider()->url_ref();
const TemplateURLRef& search_url_ref = search_provider->url_ref();
const SearchTermsData& search_terms_data =
template_url_service_->search_terms_data();
DCHECK(search_url_ref.SupportsReplacement(search_terms_data));
......@@ -242,6 +248,8 @@ GURL RepeatableQueriesService::GetQueryDeletionURL(
const std::string& deletion_url) {
const auto* default_provider =
template_url_service_->GetDefaultSearchProvider();
if (!default_provider)
return GURL();
const SearchTermsData& search_terms_data =
template_url_service_->search_terms_data();
GURL request_url = default_provider->GenerateSearchURL(search_terms_data);
......@@ -345,6 +353,11 @@ void RepeatableQueriesService::RepeatableQueriesResponseLoaded(
void RepeatableQueriesService::RepeatableQueriesParsed(
data_decoder::DataDecoder::ValueOrError result) {
const TemplateURL* default_provider =
template_url_service_->GetDefaultSearchProvider();
if (!default_provider)
return;
repeatable_queries_.clear();
std::vector<RepeatableQuery> queries;
......@@ -352,7 +365,8 @@ void RepeatableQueriesService::RepeatableQueriesParsed(
for (auto& query : queries) {
if (IsQueryDeleted(query.query))
continue;
query.destination_url = GetQueryDestinationURL(query.query);
query.destination_url =
GetQueryDestinationURL(query.query, default_provider);
repeatable_queries_.push_back(query);
if (repeatable_queries_.size() >= kMaxQueries)
break;
......@@ -363,6 +377,11 @@ void RepeatableQueriesService::RepeatableQueriesParsed(
}
void RepeatableQueriesService::GetRepeatableQueriesFromURLDatabase() {
const TemplateURL* default_provider =
template_url_service_->GetDefaultSearchProvider();
if (!default_provider)
return;
repeatable_queries_.clear();
// Fail if the in-memory URLDatabase is not available.
......@@ -392,7 +411,7 @@ void RepeatableQueriesService::GetRepeatableQueriesFromURLDatabase() {
if (IsQueryDeleted(repeatable_query.query))
continue;
repeatable_query.destination_url =
GetQueryDestinationURL(repeatable_query.query);
GetQueryDestinationURL(repeatable_query.query, default_provider);
repeatable_queries_.push_back(repeatable_query);
if (repeatable_queries_.size() >= kMaxQueries)
break;
......@@ -444,6 +463,9 @@ void RepeatableQueriesService::DeleteRepeatableQueryFromServer(
})");
GURL request_url = GetQueryDeletionURL(deletion_url);
if (!request_url.is_valid())
return;
auto deletion_request = std::make_unique<network::ResourceRequest>();
variations::AppendVariationsHeaderUnknownSignedIn(
request_url, variations::InIncognito::kNo, deletion_request.get());
......
......@@ -19,6 +19,7 @@
class SearchProviderObserver;
class TemplateURLService;
class TemplateURL;
namespace base {
class SequencedTaskRunner;
......@@ -119,8 +120,10 @@ class RepeatableQueriesService : public KeyedService {
// Called when the signin status changes.
void SigninStatusChanged();
// Returns the server destination URL for |query|.
GURL GetQueryDestinationURL(const base::string16& query);
// Returns the server destination URL for |query| with |search_provider|.
// |search_provider| may not be nullptr.
GURL GetQueryDestinationURL(const base::string16& query,
const TemplateURL* search_provider);
// Returns the resolved deletion URL for the given relative deletion URL.
GURL GetQueryDeletionURL(const std::string& deletion_url);
......
......@@ -159,8 +159,10 @@ class TestRepeatableQueriesService : public RepeatableQueriesService {
RepeatableQueriesService::FlushForTesting(std::move(flushed));
}
GURL GetQueryDestinationURL(const base::string16& query) {
return RepeatableQueriesService::GetQueryDestinationURL(query);
GURL GetQueryDestinationURL(const base::string16& query,
const TemplateURL* search_provider) {
return RepeatableQueriesService::GetQueryDestinationURL(query,
search_provider);
}
GURL GetQueryDeletionURL(const std::string& deletion_url) {
......@@ -252,7 +254,8 @@ class RepeatableQueriesServiceTest : public ::testing::Test,
void SignOut() { identity_env_->SetCookieAccounts({}); }
GURL GetQueryDestinationURL(const std::string& query) {
return service_->GetQueryDestinationURL(base::ASCIIToUTF16(query));
return service_->GetQueryDestinationURL(base::ASCIIToUTF16(query),
default_search_provider());
}
void RefreshAndMaybeWaitForService() {
......
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