From 8c7eeb6df92a1d9d33cd68f60bfe51ecaaeeb723 Mon Sep 17 00:00:00 2001 From: Kristi Park Date: Fri, 12 Oct 2018 22:16:52 +0000 Subject: [PATCH] Initialize test UrlValidityChecker in InstantService and simplify UrlValidityChecker testing Fix crashes caused by |url_checker_for_testing_| not being initialized properly. Additionally, construct UrlValidityChecker with NoDestructor instead of a global LazyInstance. We prefer to leak singletons to avoid unnecessary work at shutdown. Also simplify UrlValidityChecker by passing TickClock, which provides cleaner timeout and duration testing. Bug: 894742 Change-Id: I468f76e07dc6f551a8515d8416d0c67f5f2035a1 Reviewed-on: https://chromium-review.googlesource.com/c/1277959 Commit-Queue: Kristi Park Reviewed-by: Mathieu Perreault Cr-Commit-Position: refs/heads/master@{#599377} --- chrome/browser/search/instant_service.cc | 4 ++-- chrome/browser/search/instant_service.h | 4 ++-- .../search/url_validity_checker_factory.cc | 17 ++++++++-------- .../search/url_validity_checker_factory.h | 9 +++------ components/search/url_validity_checker.h | 2 +- .../search/url_validity_checker_impl.cc | 20 ++++++++----------- components/search/url_validity_checker_impl.h | 16 +++------------ .../url_validity_checker_impl_unittest.cc | 20 +++++-------------- 8 files changed, 33 insertions(+), 59 deletions(-) diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc index 176e21be690e3..ba99c9929c2fd 100644 --- a/chrome/browser/search/instant_service.cc +++ b/chrome/browser/search/instant_service.cc @@ -485,7 +485,7 @@ void InstantService::OnDoesUrlResolveComplete( const GURL& url, chrome::mojom::EmbeddedSearch::DoesUrlResolveCallback callback, bool resolves, - const base::TimeDelta& duration) { + base::TimeDelta duration) { bool timeout = false; if (!resolves) { // Internally update the default "https" scheme to "http" if UI dialog has @@ -819,7 +819,7 @@ void InstantService::FallbackToDefaultThemeInfo() { UrlValidityChecker* InstantService::GetUrlValidityChecker() { if (url_checker_for_testing_ != nullptr) return url_checker_for_testing_; - return UrlValidityCheckerFactory::GetInstance(); + return UrlValidityCheckerFactory::GetUrlValidityChecker(); } // static diff --git a/chrome/browser/search/instant_service.h b/chrome/browser/search/instant_service.h index 40632389d1a1a..a033e79f588a1 100644 --- a/chrome/browser/search/instant_service.h +++ b/chrome/browser/search/instant_service.h @@ -165,7 +165,7 @@ class InstantService : public KeyedService, const GURL& url, chrome::mojom::EmbeddedSearch::DoesUrlResolveCallback callback, bool resolves, - const base::TimeDelta& duration); + base::TimeDelta duration); // content::NotificationObserver: void Observe(int type, @@ -231,7 +231,7 @@ class InstantService : public KeyedService, std::unique_ptr search_provider_observer_; // Test UrlValidityChecker used for testing. - UrlValidityChecker* url_checker_for_testing_; + UrlValidityChecker* url_checker_for_testing_ = nullptr; PrefChangeRegistrar pref_change_registrar_; diff --git a/chrome/browser/search/url_validity_checker_factory.cc b/chrome/browser/search/url_validity_checker_factory.cc index ae29f5e418c4f..51f818e648de9 100644 --- a/chrome/browser/search/url_validity_checker_factory.cc +++ b/chrome/browser/search/url_validity_checker_factory.cc @@ -6,21 +6,22 @@ #include +#include "base/time/default_tick_clock.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/net/system_network_context_manager.h" #include "content/public/browser/browser_thread.h" #include "services/network/public/cpp/shared_url_loader_factory.h" // static -UrlValidityChecker* UrlValidityCheckerFactory::GetInstance() { +UrlValidityChecker* UrlValidityCheckerFactory::GetUrlValidityChecker() { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - static base::LazyInstance::DestructorAtExit - instance = LAZY_INSTANCE_INITIALIZER; - return &(instance.Get().url_validity_checker_); + static base::NoDestructor checker( + g_browser_process->system_network_context_manager() + ->GetSharedURLLoaderFactory(), + base::DefaultTickClock::GetInstance()); + return checker.get(); } -UrlValidityCheckerFactory::UrlValidityCheckerFactory() - : url_validity_checker_(g_browser_process->system_network_context_manager() - ->GetSharedURLLoaderFactory()) {} +UrlValidityCheckerFactory::UrlValidityCheckerFactory() = default; -UrlValidityCheckerFactory::~UrlValidityCheckerFactory() {} +UrlValidityCheckerFactory::~UrlValidityCheckerFactory() = default; diff --git a/chrome/browser/search/url_validity_checker_factory.h b/chrome/browser/search/url_validity_checker_factory.h index 1ebd79d0c7060..0606a0b539d16 100644 --- a/chrome/browser/search/url_validity_checker_factory.h +++ b/chrome/browser/search/url_validity_checker_factory.h @@ -5,25 +5,22 @@ #ifndef CHROME_BROWSER_SEARCH_URL_VALIDITY_CHECKER_FACTORY_H_ #define CHROME_BROWSER_SEARCH_URL_VALIDITY_CHECKER_FACTORY_H_ -#include "base/lazy_instance.h" #include "base/macros.h" +#include "base/no_destructor.h" #include "components/search/url_validity_checker_impl.h" // Singleton that owns a single UrlValidityCheckerImpl instance. Should only be // called from the UI thread. class UrlValidityCheckerFactory { public: - static UrlValidityChecker* GetInstance(); + static UrlValidityChecker* GetUrlValidityChecker(); private: - friend struct base::LazyInstanceTraitsBase; + friend class base::NoDestructor; UrlValidityCheckerFactory(); ~UrlValidityCheckerFactory(); - // The only instance that exists. - UrlValidityCheckerImpl url_validity_checker_; - DISALLOW_COPY_AND_ASSIGN(UrlValidityCheckerFactory); }; diff --git a/components/search/url_validity_checker.h b/components/search/url_validity_checker.h index 8ac60fa59a402..7560e81034a43 100644 --- a/components/search/url_validity_checker.h +++ b/components/search/url_validity_checker.h @@ -15,7 +15,7 @@ class UrlValidityChecker { // The callback invoked when the request completes. Returns true if the // response was |valid| and the request |duration|. using UrlValidityCheckerCallback = - base::OnceCallback; + base::OnceCallback; virtual ~UrlValidityChecker() = default; diff --git a/components/search/url_validity_checker_impl.cc b/components/search/url_validity_checker_impl.cc index a19061e36d57f..697eb5e8efbc0 100644 --- a/components/search/url_validity_checker_impl.cc +++ b/components/search/url_validity_checker_impl.cc @@ -41,9 +41,9 @@ struct UrlValidityCheckerImpl::PendingRequest { }; UrlValidityCheckerImpl::UrlValidityCheckerImpl( - scoped_refptr url_loader_factory) - : url_loader_factory_(url_loader_factory), - clock_(base::DefaultTickClock::GetInstance()) {} + scoped_refptr url_loader_factory, + const base::TickClock* tick_clock) + : url_loader_factory_(std::move(url_loader_factory)), clock_(tick_clock) {} UrlValidityCheckerImpl::~UrlValidityCheckerImpl() = default; @@ -56,8 +56,9 @@ void UrlValidityCheckerImpl::DoesUrlResolve( resource_request->method = "HEAD"; resource_request->allow_credentials = false; - auto request_iter = pending_requests_.emplace( - pending_requests_.begin(), url, NowTicks(), clock_, std::move(callback)); + auto request_iter = pending_requests_.emplace(pending_requests_.begin(), url, + clock_->NowTicks(), clock_, + std::move(callback)); request_iter->loader = network::SimpleURLLoader::Create( std::move(resource_request), traffic_annotation); // Don't follow redirects to prevent leaking URL data to HTTP sites. @@ -94,13 +95,8 @@ void UrlValidityCheckerImpl::OnSimpleLoaderComplete( void UrlValidityCheckerImpl::OnSimpleLoaderHandler( std::list::iterator request_iter, bool valid) { - base::TimeDelta elapsed_time = NowTicks() - request_iter->time_created; + base::TimeDelta elapsed_time = + clock_->NowTicks() - request_iter->time_created; std::move(request_iter->callback).Run(valid, elapsed_time); pending_requests_.erase(request_iter); } - -base::TimeTicks UrlValidityCheckerImpl::NowTicks() const { - if (!time_ticks_for_testing_.is_null()) - return time_ticks_for_testing_; - return base::TimeTicks::Now(); -} diff --git a/components/search/url_validity_checker_impl.h b/components/search/url_validity_checker_impl.h index e3a0257999f61..d4996e5c1fd31 100644 --- a/components/search/url_validity_checker_impl.h +++ b/components/search/url_validity_checker_impl.h @@ -30,19 +30,15 @@ class SharedURLLoaderFactory; class UrlValidityCheckerImpl : public UrlValidityChecker { public: - explicit UrlValidityCheckerImpl( - scoped_refptr url_loader_factory); + UrlValidityCheckerImpl( + scoped_refptr url_loader_factory, + const base::TickClock* tick_clock); ~UrlValidityCheckerImpl() override; void DoesUrlResolve(const GURL& url, net::NetworkTrafficAnnotationTag traffic_annotation, UrlValidityCheckerCallback callback) override; - // Used for testing. - void SetTimeTicksForTesting(const base::TimeTicks& time_ticks) { - time_ticks_for_testing_ = time_ticks; - } - private: struct PendingRequest; @@ -62,17 +58,11 @@ class UrlValidityCheckerImpl : public UrlValidityChecker { void OnSimpleLoaderHandler(std::list::iterator request_iter, bool valid); - // Returns base::TimeTicks::Now() or the test TimeTicks if not null. - base::TimeTicks NowTicks() const; - // Stores any ongoing network requests. Once a request is completed, it is // deleted from the list. std::list pending_requests_; scoped_refptr url_loader_factory_; - // Test time ticks used for testing. - base::TimeTicks time_ticks_for_testing_; - // Non-owned pointer to TickClock. Used for request timeouts. const base::TickClock* const clock_; diff --git a/components/search/url_validity_checker_impl_unittest.cc b/components/search/url_validity_checker_impl_unittest.cc index 6a7d200256c19..b300abf74a04e 100644 --- a/components/search/url_validity_checker_impl_unittest.cc +++ b/components/search/url_validity_checker_impl_unittest.cc @@ -10,7 +10,6 @@ #include "base/test/bind_test_util.h" #include "base/test/mock_callback.h" #include "base/test/scoped_task_environment.h" -#include "base/test/simple_test_tick_clock.h" #include "net/traffic_annotation/network_traffic_annotation_test_helper.h" #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h" #include "services/network/test/test_url_loader_factory.h" @@ -27,10 +26,8 @@ class UrlValidityCheckerImplTest : public testing::Test { test_shared_loader_factory_( base::MakeRefCounted( &test_url_loader_factory_)), - url_checker_(test_shared_loader_factory_) { - // Start |clock_| at non-zero. - clock_.Advance(base::TimeDelta::FromSeconds(1)); - } + url_checker_(test_shared_loader_factory_, + scoped_task_environment_.GetMockTickClock()) {} ~UrlValidityCheckerImplTest() override {} @@ -38,7 +35,6 @@ class UrlValidityCheckerImplTest : public testing::Test { test_shared_loader_factory_ = base::MakeRefCounted( url_loader_factory()); - url_checker()->SetTimeTicksForTesting(clock_.NowTicks()); } UrlValidityCheckerImpl* url_checker() { return &url_checker_; } @@ -47,18 +43,12 @@ class UrlValidityCheckerImplTest : public testing::Test { return &test_url_loader_factory_; } - void AdvanceClock(const base::TimeDelta& delta) { - clock_.Advance(delta); - url_checker()->SetTimeTicksForTesting(clock_.NowTicks()); - } - base::test::ScopedTaskEnvironment scoped_task_environment_; private: network::TestURLLoaderFactory test_url_loader_factory_; scoped_refptr test_shared_loader_factory_; - base::SimpleTestTickClock clock_; UrlValidityCheckerImpl url_checker_; DISALLOW_COPY_AND_ASSIGN(UrlValidityCheckerImplTest); @@ -75,7 +65,7 @@ TEST_F(UrlValidityCheckerImplTest, DoesUrlResolve_OnSuccess) { "HTTP/1.1 200 OK\nContent-type: text/html\n\n"); url_loader_factory()->SetInterceptor( base::BindLambdaForTesting([&](const network::ResourceRequest& request) { - AdvanceClock(expected_duration); + scoped_task_environment_.FastForwardBy(expected_duration); url_loader_factory()->AddResponse( request.url, response, std::string(), network::URLLoaderCompletionStatus(net::OK)); @@ -107,7 +97,7 @@ TEST_F(UrlValidityCheckerImplTest, DoesUrlResolve_OnFailure) { url_loader_factory()->SetInterceptor( base::BindLambdaForTesting([&](const network::ResourceRequest& request) { - AdvanceClock(expected_duration); + scoped_task_environment_.FastForwardBy(expected_duration); url_loader_factory()->AddResponse( request.url, network::ResourceResponseHead(), std::string(), network::URLLoaderCompletionStatus(net::ERR_FAILED)); @@ -134,7 +124,7 @@ TEST_F(UrlValidityCheckerImplTest, DoesUrlResolve_OnRedirect) { {redirect_info, network::ResourceResponseHead()}}; url_loader_factory()->SetInterceptor( base::BindLambdaForTesting([&](const network::ResourceRequest& request) { - AdvanceClock(expected_duration); + scoped_task_environment_.FastForwardBy(expected_duration); url_loader_factory()->AddResponse( request.url, network::ResourceResponseHead(), std::string(), network::URLLoaderCompletionStatus(), redirects);