From: csagan5 <32685696+csagan5@users.noreply.github.com> Date: Sun, 6 Mar 2022 18:55:58 +0100 Subject: OpenSearch: miscellaneous Fix upstream bug with recently added engines prematurely discarded because they have no last-visit timestamp Fix upstream bug with visited engines visit time not updated Allow using search engine URLs with non-empty paths Add verbose logging License: GPL-3.0-only - https://spdx.org/licenses/GPL-3.0-only.html --- .../settings/SearchEngineAdapter.java | 4 +- .../search_engine_tab_helper.cc | 36 ++++++++++++---- .../renderer/chrome_render_frame_observer.cc | 2 + .../search_engines/template_url_fetcher.cc | 41 ++++++++++++++----- .../search_engines/template_url_fetcher.h | 2 +- .../search_engines/template_url_parser.cc | 2 +- .../search_engines/template_url_service.h | 8 ++-- 7 files changed, 70 insertions(+), 25 deletions(-) diff --git a/chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/SearchEngineAdapter.java b/chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/SearchEngineAdapter.java --- a/chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/SearchEngineAdapter.java +++ b/chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/SearchEngineAdapter.java @@ -320,7 +320,9 @@ public class SearchEngineAdapter extends BaseAdapter } if (shouldShowRecentSearchEngines && recentEngineNum < MAX_RECENT_ENGINE_NUM - && templateUrl.getLastVisitedTime() > displayTime) { + // newly added search engines have never been visited + && (templateUrl.getLastVisitedTime() == 0 || + templateUrl.getLastVisitedTime() > displayTime)) { recentEngineNum++; } else { iterator.remove(); diff --git a/chrome/browser/ui/search_engines/search_engine_tab_helper.cc b/chrome/browser/ui/search_engines/search_engine_tab_helper.cc --- a/chrome/browser/ui/search_engines/search_engine_tab_helper.cc +++ b/chrome/browser/ui/search_engines/search_engine_tab_helper.cc @@ -6,6 +6,7 @@ #include +#include "base/logging.h" #include "chrome/browser/favicon/favicon_utils.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/search_engines/template_url_fetcher_factory.h" @@ -48,6 +49,7 @@ void SearchEngineTabHelper::BindOpenSearchDescriptionDocumentHandler( receiver) { // Bind only for outermost main frames. if (rfh->GetParentOrOuterDocument()) { + LOG(INFO) << "OpenSearch: not on main frame"; return; } @@ -78,6 +80,7 @@ std::u16string SearchEngineTabHelper::GenerateKeywordFromNavigationEntry( // Don't autogenerate keywords for pages that are the result of form // submissions. if (IsFormSubmit(entry)) { + LOG(INFO) << "OpenSearch: cannot generate keyword for a form submission for entry " << entry->GetURL(); return std::u16string(); } @@ -87,6 +90,7 @@ std::u16string SearchEngineTabHelper::GenerateKeywordFromNavigationEntry( if (!url.is_valid()) { url = entry->GetURL(); if (!url.is_valid()) { + LOG(INFO) << "OpenSearch: user-typed/entry URL are invalid for entry " << entry->GetURL(); return std::u16string(); } } @@ -96,10 +100,10 @@ std::u16string SearchEngineTabHelper::GenerateKeywordFromNavigationEntry( // b) have a path. // // If we relax the path constraint, we need to be sure to sanitize the path - // elements and update AutocompletePopup to look for keywords using the path. + // elements and update TemplateURL to look for keywords using the path. // See http://b/issue?id=863583. - if (!(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme)) || - (url.GetPath().length() > 1)) { + if (!(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme))) { + LOG(INFO) << "OpenSearch: invalid scheme for entry " << entry->GetURL(); return std::u16string(); } @@ -128,14 +132,15 @@ void SearchEngineTabHelper::PageHasOpenSearchDescriptionDocument( // http://b/issue?id=863583. For that reason, this doesn't check and allow // urls referring to osdd urls with same schemes. if (!osdd_url.is_valid() || !osdd_url.SchemeIsHTTPOrHTTPS()) { + LOG(INFO) << "OpenSearch: not a valid OSDD URL " << osdd_url; return; } Profile* profile = Profile::FromBrowserContext(web_contents()->GetBrowserContext()); if (page_url != web_contents()->GetLastCommittedURL() || - !TemplateURLFetcherFactory::GetForProfile(profile) || - profile->IsOffTheRecord()) { + !TemplateURLFetcherFactory::GetForProfile(profile)) { + LOG(INFO) << "OpenSearch: page URL mismatch on page " << page_url; return; } @@ -149,6 +154,7 @@ void SearchEngineTabHelper::PageHasOpenSearchDescriptionDocument( --index; } if (!entry || IsFormSubmit(entry)) { + LOG(INFO) << "OpenSearch: cannot find form submission for entry " << entry->GetURL(); return; } @@ -159,11 +165,18 @@ void SearchEngineTabHelper::PageHasOpenSearchDescriptionDocument( return; } + std::u16string page_keyword = TemplateURL::GenerateKeyword(page_url); + if (page_keyword != keyword) { + LOG(INFO) << "OpenSearch: keyword mismatch for entry " << entry->GetURL(); + return; + } + auto* frame = web_contents()->GetPrimaryMainFrame(); mojo::Remote url_loader_factory; frame->CreateNetworkServiceDefaultFactory( url_loader_factory.BindNewPipeAndPassReceiver()); + bool is_off_the_record = profile->IsOffTheRecord(); // Download the OpenSearch description document. If this is successful, a // new keyword will be created when done. #if BUILDFLAG(IS_ANDROID) @@ -172,6 +185,7 @@ void SearchEngineTabHelper::PageHasOpenSearchDescriptionDocument( } #endif TemplateURLFetcherFactory::GetForProfile(profile)->ScheduleDownload( + is_off_the_record, keyword, osdd_url, entry->GetFavicon().url, frame->GetLastCommittedOrigin(), url_loader_factory.get(), frame->GetRoutingID(), @@ -216,12 +230,19 @@ void SearchEngineTabHelper::GenerateKeywordIfNecessary( return; } - std::u16string keyword(GenerateKeywordFromNavigationEntry( - controller.GetEntryAtIndex(last_index - 1))); + NavigationEntry* entry = controller.GetEntryAtIndex(last_index - 1); + std::u16string keyword(GenerateKeywordFromNavigationEntry(entry)); if (keyword.empty()) { return; } + GURL url = handle->GetSearchableFormURL(); + std::u16string page_keyword = TemplateURL::GenerateKeyword(url); + if (page_keyword != keyword) { + LOG(INFO) << "OpenSearch: GenerateKeywordIfNecessary(): keyword mismatch for entry " << entry->GetURL(); + return; + } + TemplateURLService* url_service = TemplateURLServiceFactory::GetForProfile(profile); if (!url_service) { @@ -233,7 +254,6 @@ void SearchEngineTabHelper::GenerateKeywordIfNecessary( return; } - GURL url = handle->GetSearchableFormURL(); if (!url_service->CanAddAutogeneratedKeyword(keyword, url)) { return; } diff --git a/chrome/renderer/chrome_render_frame_observer.cc b/chrome/renderer/chrome_render_frame_observer.cc --- a/chrome/renderer/chrome_render_frame_observer.cc +++ b/chrome/renderer/chrome_render_frame_observer.cc @@ -15,6 +15,7 @@ #include "base/command_line.h" #include "base/functional/bind.h" +#include "base/logging.h" #include "base/no_destructor.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" @@ -273,6 +274,7 @@ void ChromeRenderFrameObserver::DidFinishLoad() { GURL osdd_url = frame->GetDocument().OpenSearchDescriptionURL(); if (!osdd_url.is_empty()) { + DLOG(INFO) << "OpenSearch: found OSDD URL: " << osdd_url; mojo::Remote osdd_handler; render_frame()->GetBrowserInterfaceBroker().GetInterface( diff --git a/components/search_engines/template_url_fetcher.cc b/components/search_engines/template_url_fetcher.cc --- a/components/search_engines/template_url_fetcher.cc +++ b/components/search_engines/template_url_fetcher.cc @@ -73,7 +73,8 @@ class TemplateURLFetcher::RequestDelegate { const url::Origin& initiator, network::mojom::URLLoaderFactory* url_loader_factory, int render_frame_id, - int32_t request_id); + int32_t request_id, + bool is_off_the_record); RequestDelegate(const RequestDelegate&) = delete; RequestDelegate& operator=(const RequestDelegate&) = delete; @@ -99,6 +100,7 @@ class TemplateURLFetcher::RequestDelegate { std::u16string keyword_; const GURL osdd_url_; const GURL favicon_url_; + bool is_off_the_record_; base::CallbackListSubscription template_url_subscription_; @@ -113,11 +115,13 @@ TemplateURLFetcher::RequestDelegate::RequestDelegate( const url::Origin& initiator, network::mojom::URLLoaderFactory* url_loader_factory, int render_frame_id, - int32_t request_id) + int32_t request_id, + bool is_off_the_record) : fetcher_(fetcher), keyword_(keyword), osdd_url_(osdd_url), - favicon_url_(favicon_url) { + favicon_url_(favicon_url), + is_off_the_record_(is_off_the_record) { TemplateURLService* model = fetcher_->template_url_service_; DCHECK(model); // TemplateURLFetcher::ScheduleDownload verifies this. @@ -231,7 +235,9 @@ void TemplateURLFetcher::RequestDelegate::AddSearchProvider() { // omnibox until they are activated. data.is_active = TemplateURLData::ActiveStatus::kUnspecified; - model->Add(std::make_unique(data)); + if (!is_off_the_record_) { + model->Add(std::make_unique(data)); + } fetcher_->RequestCompleted(this); // WARNING: RequestCompleted deletes us. @@ -246,6 +252,7 @@ TemplateURLFetcher::~TemplateURLFetcher() { } void TemplateURLFetcher::ScheduleDownload( + bool is_off_the_record, const std::u16string& keyword, const GURL& osdd_url, const GURL& favicon_url, @@ -263,21 +270,35 @@ void TemplateURLFetcher::ScheduleDownload( return; } - const TemplateURL* template_url = + TemplateURL* template_url = template_url_service_->GetTemplateURLForKeyword(keyword); - if (template_url && (!template_url->safe_for_autoreplace() || - template_url->originating_url() == osdd_url)) - return; + if (template_url) { + if (!template_url->safe_for_autoreplace()) { + LOG(INFO) << "OpenSearch: OSDD URL not safe for autoreplace: " << osdd_url; + return; + } + if (template_url->originating_url() == osdd_url) { + // Either there is a user created TemplateURL for this keyword, or the + // keyword has the same OSDD url and we've parsed it. + DLOG(INFO) << "OpenSearch: OSDD URL was already parsed: " << osdd_url; + // always update the visit timestamp + template_url_service_->UpdateTemplateURLVisitTime(template_url); + return; + } + } // Make sure we aren't already downloading this request. for (const auto& request : requests_) { - if ((request->url() == osdd_url) || (request->keyword() == keyword)) + if ((request->url() == osdd_url) || (request->keyword() == keyword)) { + LOG(INFO) << "OpenSearch: already downloading OSDD URL: " << osdd_url; return; + } } + LOG(INFO) << "OpenSearch: getting " << osdd_url; requests_.push_back(std::make_unique( this, keyword, osdd_url, favicon_url, initiator, url_loader_factory, - render_frame_id, request_id)); + render_frame_id, request_id, is_off_the_record)); } void TemplateURLFetcher::RequestCompleted(RequestDelegate* request) { diff --git a/components/search_engines/template_url_fetcher.h b/components/search_engines/template_url_fetcher.h --- a/components/search_engines/template_url_fetcher.h +++ b/components/search_engines/template_url_fetcher.h @@ -48,7 +48,7 @@ class TemplateURLFetcher : public KeyedService { // TemplateURL in the model for |keyword|, or we're already downloading an // OSDD for this keyword, no download is started. // - void ScheduleDownload(const std::u16string& keyword, + void ScheduleDownload(bool is_off_the_record, const std::u16string& keyword, const GURL& osdd_url, const GURL& favicon_url, const url::Origin& initiator, diff --git a/components/search_engines/template_url_parser.cc b/components/search_engines/template_url_parser.cc --- a/components/search_engines/template_url_parser.cc +++ b/components/search_engines/template_url_parser.cc @@ -176,7 +176,7 @@ void SafeTemplateURLParser::OnXmlParseComplete( std::move(callback_).Run([&]() -> std::unique_ptr { ASSIGN_OR_RETURN(const base::Value root, std::move(value_or_error), [](std::string error) -> std::unique_ptr { - DLOG(ERROR) + LOG(ERROR) << "Failed to parse XML: " << std::move(error); return nullptr; }); diff --git a/components/search_engines/template_url_service.h b/components/search_engines/template_url_service.h --- a/components/search_engines/template_url_service.h +++ b/components/search_engines/template_url_service.h @@ -409,7 +409,10 @@ class TemplateURLService final : public WebDataServiceConsumer, void UpdateProviderFavicons(const GURL& potential_search_url, const GURL& favicon_url); - // Return true if the given |url| can be made the default. This returns false + // Updates the last_visited time of |url| to the current time. + void UpdateTemplateURLVisitTime(TemplateURL* url); + + // Return true if the given |url| can be made the default. This returns false // regardless of |url| if the default search provider is managed by policy or // controlled by an extension. bool CanMakeDefault(const TemplateURL* url) const; @@ -823,9 +826,6 @@ class TemplateURLService final : public WebDataServiceConsumer, // SetKeywordSearchTermsForURL is invoked. void UpdateKeywordSearchTermsForURL(const URLVisitedDetails& details); - // Updates the last_visited time of |url| to the current time. - void UpdateTemplateURLVisitTime(TemplateURL* url); - // If necessary, generates a visit for the site http:// + t_url.keyword(). void AddTabToSearchVisit(const TemplateURL& t_url); --