Initialize test UrlValidityChecker in InstantService and simplify Url…

…ValidityChecker 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 <kristipark@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599377}

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

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<SearchProviderObserver> search_provider_observer_;
 
   // Test UrlValidityChecker used for testing.
-  UrlValidityChecker* url_checker_for_testing_;
+  UrlValidityChecker* url_checker_for_testing_ = nullptr;
 
   PrefChangeRegistrar pref_change_registrar_;
 

chrome/browser/search/url_validity_checker_factory.cc

@@ -6,21 +6,22 @@
 
 #include <memory>
 
+#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<UrlValidityCheckerFactory>::DestructorAtExit
-      instance = LAZY_INSTANCE_INITIALIZER;
-  return &(instance.Get().url_validity_checker_);
+  static base::NoDestructor<UrlValidityCheckerImpl> 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;

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<UrlValidityCheckerFactory>;
+  friend class base::NoDestructor<UrlValidityCheckerFactory>;
 
   UrlValidityCheckerFactory();
   ~UrlValidityCheckerFactory();
 
-  // The only instance that exists.
-  UrlValidityCheckerImpl url_validity_checker_;
-
   DISALLOW_COPY_AND_ASSIGN(UrlValidityCheckerFactory);
 };
 

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<void(bool valid, const base::TimeDelta& duration)>;
+      base::OnceCallback<void(bool valid, base::TimeDelta duration)>;
 
   virtual ~UrlValidityChecker() = default;
 

components/search/url_validity_checker_impl.cc

@@ -41,9 +41,9 @@ struct UrlValidityCheckerImpl::PendingRequest {
 };
 
 UrlValidityCheckerImpl::UrlValidityCheckerImpl(
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-    : url_loader_factory_(url_loader_factory),
-      clock_(base::DefaultTickClock::GetInstance()) {}
+    scoped_refptr<network::SharedURLLoaderFactory> 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<PendingRequest>::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();
-}

components/search/url_validity_checker_impl.h

@@ -30,19 +30,15 @@ class SharedURLLoaderFactory;
 
 class UrlValidityCheckerImpl : public UrlValidityChecker {
  public:
-  explicit UrlValidityCheckerImpl(
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
+  UrlValidityCheckerImpl(
+      scoped_refptr<network::SharedURLLoaderFactory> 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<PendingRequest>::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<PendingRequest> pending_requests_;
   scoped_refptr<network::SharedURLLoaderFactory> 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_;
 

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,18 +26,15 @@ class UrlValidityCheckerImplTest : public testing::Test {
         test_shared_loader_factory_(
             base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
                 &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 {}
 
   void SetUp() override {
     test_shared_loader_factory_ =
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             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<network::SharedURLLoaderFactory> 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);