Inject a fake ConversionManager into ConversionHostTest.*

This change fixes flakes found in the ConversionHostTest suite. These
flakes are currently caused by a race between storage initialization and
test profile deletion. If the test profile is deleted prior to the
storage initialization task finishing, the task can hang in the middle
due to so undefined behavior of operating on the db causing the test to
timeout.

These tests do not need a real ConversionManager and only require
a valid pointer. This change allows ConversionHostTest to specify a
TestConversionManager which is injected via new layer of abstraction,
the ConversionManagerProvider. It also switches ConversionManager to an
impl pattern rather than marking all methods virtual.

This test manager does not need to
initialize a storage layer, fixing the timeout issue.

This is also a pre-requisite for https://crrev.com/c/2012883, which
requires a mock manager to fully test ConversionHost functionality.

Bug: 1051334
Change-Id: I376382c2d859e7c2ee8512227d40657082348310
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135061
Commit-Queue: John Delaney <johnidel@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756810}

content/browser/BUILD.gn

@@ -682,8 +682,9 @@ jumbo_source_set("browser") {
     "content_service_delegate_impl.h",
     "conversions/conversion_host.cc",
     "conversions/conversion_host.h",
-    "conversions/conversion_manager.cc",
     "conversions/conversion_manager.h",
+    "conversions/conversion_manager_impl.cc",
+    "conversions/conversion_manager_impl.h",
     "conversions/conversion_policy.cc",
     "conversions/conversion_policy.h",
     "conversions/conversion_report.cc",

content/browser/conversions/conversion_host.cc

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "content/browser/conversions/conversion_manager.h"
+#include "content/browser/conversions/conversion_manager_impl.h"
 #include "content/browser/conversions/conversion_policy.h"
 #include "content/browser/conversions/storable_conversion.h"
 #include "content/browser/storage_partition_impl.h"
@@ -19,8 +20,43 @@
 
 namespace content {
 
-ConversionHost::ConversionHost(WebContents* contents)
-    : web_contents_(contents), receiver_(contents, this) {}
+namespace {
+
+// Provides access to the manager owned by the default storage partition.
+class ConversionManagerProviderImpl : public ConversionManager::Provider {
+ public:
+  ConversionManagerProviderImpl() = default;
+  ConversionManagerProviderImpl(const ConversionManagerProviderImpl& other) =
+      delete;
+  ConversionManagerProviderImpl& operator=(
+      const ConversionManagerProviderImpl& other) = delete;
+  ~ConversionManagerProviderImpl() override = default;
+
+  // ConversionManagerProvider:
+  ConversionManager* GetManager(WebContents* web_contents) const override {
+    return static_cast<StoragePartitionImpl*>(
+               BrowserContext::GetDefaultStoragePartition(
+                   web_contents->GetBrowserContext()))
+        ->GetConversionManager();
+  }
+};
+
+}  // namespace
+
+// static
+std::unique_ptr<ConversionHost> ConversionHost::CreateForTesting(
+    WebContents* web_contents,
+    std::unique_ptr<ConversionManager::Provider> conversion_manager_provider) {
+  auto host = std::make_unique<ConversionHost>(web_contents);
+  host->conversion_manager_provider_ = std::move(conversion_manager_provider);
+  return host;
+}
+
+ConversionHost::ConversionHost(WebContents* web_contents)
+    : conversion_manager_provider_(
+          std::make_unique<ConversionManagerProviderImpl>()),
+      web_contents_(web_contents),
+      receiver_(web_contents, this) {}
 
 ConversionHost::~ConversionHost() = default;
 
@@ -28,10 +64,6 @@ ConversionHost::~ConversionHost() = default;
 // page-load to a reasonable number.
 void ConversionHost::RegisterConversion(
     blink::mojom::ConversionPtr conversion) {
-  // If there is no conversion manager available, ignore any conversion
-  // registrations.
-  if (!GetManager())
-    return;
   content::RenderFrameHost* render_frame_host =
       receiver_.GetCurrentTargetFrame();
 
@@ -42,6 +74,13 @@ void ConversionHost::RegisterConversion(
     return;
   }
 
+  // If there is no conversion manager available, ignore any conversion
+  // registrations.
+  ConversionManager* conversion_manager =
+      conversion_manager_provider_->GetManager(web_contents_);
+  if (!conversion_manager)
+    return;
+
   // Only allow conversion registration on secure pages with a secure conversion
   // redirects.
   if (!network::IsOriginPotentiallyTrustworthy(
@@ -54,12 +93,12 @@ void ConversionHost::RegisterConversion(
   }
 
   StorableConversion storable_conversion(
-      GetManager()->GetConversionPolicy().GetSanitizedConversionData(
+      conversion_manager->GetConversionPolicy().GetSanitizedConversionData(
           conversion->conversion_data),
       render_frame_host->GetLastCommittedOrigin(),
       conversion->reporting_origin);
 
-  GetManager()->HandleConversion(storable_conversion);
+  conversion_manager->HandleConversion(storable_conversion);
 }
 
 void ConversionHost::SetCurrentTargetFrameForTesting(
@@ -67,11 +106,4 @@ void ConversionHost::SetCurrentTargetFrameForTesting(
   receiver_.SetCurrentTargetFrameForTesting(render_frame_host);
 }
 
-ConversionManager* ConversionHost::GetManager() {
-  return static_cast<StoragePartitionImpl*>(
-             BrowserContext::GetDefaultStoragePartition(
-                 web_contents_->GetBrowserContext()))
-      ->GetConversionManager();
-}
-
 }  // namespace content

content/browser/conversions/conversion_host.h

@@ -6,12 +6,13 @@
 #define CONTENT_BROWSER_CONVERSIONS_CONVERSION_HOST_H_
 
 #include "base/gtest_prod_util.h"
+#include "content/browser/conversions/conversion_manager.h"
+#include "content/common/content_export.h"
 #include "content/public/browser/web_contents_receiver_set.h"
 #include "third_party/blink/public/mojom/conversions/conversions.mojom.h"
 
 namespace content {
 
-class ConversionManager;
 class RenderFrameHost;
 class WebContents;
 
@@ -20,6 +21,10 @@ class WebContents;
 // bound to lifetime of the WebContents.
 class CONTENT_EXPORT ConversionHost : public blink::mojom::ConversionHost {
  public:
+  static std::unique_ptr<ConversionHost> CreateForTesting(
+      WebContents* web_contents,
+      std::unique_ptr<ConversionManager::Provider> conversion_manager_provider);
+
   explicit ConversionHost(WebContents* web_contents);
   ConversionHost(const ConversionHost& other) = delete;
   ConversionHost& operator=(const ConversionHost& other) = delete;
@@ -39,8 +44,9 @@ class CONTENT_EXPORT ConversionHost : public blink::mojom::ConversionHost {
   // Sets the target frame on |receiver_|.
   void SetCurrentTargetFrameForTesting(RenderFrameHost* render_frame_host);
 
-  // Gets the manager for this web contents. Can be null.
-  ConversionManager* GetManager();
+  // Gives access to a ConversionManager implementation to forward impressions
+  // and conversion registrations to.
+  std::unique_ptr<ConversionManager::Provider> conversion_manager_provider_;
 
   WebContents* web_contents_;
 

content/browser/conversions/conversion_host_unittest.cc

@@ -7,10 +7,12 @@
 #include <memory>
 
 #include "base/test/scoped_feature_list.h"
+#include "content/browser/conversions/conversion_manager.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/common/content_features.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/test/fake_mojo_message_dispatch_context.h"
+#include "content/test/navigation_simulator_impl.h"
 #include "content/test/test_render_frame_host.h"
 #include "content/test/test_web_contents.h"
 #include "mojo/public/cpp/test_support/test_utils.h"
@@ -20,17 +22,57 @@
 
 namespace content {
 
-class ConversionHostTest : public RenderViewHostTestHarness {
+class TestConversionManager : public ConversionManager {
+ public:
+  TestConversionManager() = default;
+  ~TestConversionManager() override = default;
+
+  void HandleConversion(const StorableConversion& impression) override {
+    num_conversions_++;
+  }
+
+  const ConversionPolicy& GetConversionPolicy() const override {
+    return policy;
+  }
+
+  size_t num_impressions() const { return num_impressions_; }
+
+  void Reset() {
+    num_impressions_ = 0u;
+    num_conversions_ = 0u;
+  }
+
+ private:
+  ConversionPolicy policy;
+  size_t num_impressions_ = 0u;
+  size_t num_conversions_ = 0u;
+};
+
+class TestManagerProvider : public ConversionManager::Provider {
  public:
-  ConversionHostTest() {
-    feature_list_.InitAndEnableFeature(features::kConversionMeasurement);
+  explicit TestManagerProvider(ConversionManager* manager)
+      : manager_(manager) {}
+  ~TestManagerProvider() override = default;
+
+  ConversionManager* GetManager(WebContents* web_contents) const override {
+    return manager_;
   }
 
+ private:
+  ConversionManager* manager_ = nullptr;
+};
+
+class ConversionHostTest : public RenderViewHostTestHarness {
+ public:
+  ConversionHostTest() = default;
+
   void SetUp() override {
     RenderViewHostTestHarness::SetUp();
     static_cast<WebContentsImpl*>(web_contents())
         ->RemoveReceiverSetForTesting(blink::mojom::ConversionHost::Name_);
-    conversion_host_ = std::make_unique<ConversionHost>(web_contents());
+
+    conversion_host_ = ConversionHost::CreateForTesting(
+        web_contents(), std::make_unique<TestManagerProvider>(&test_manager_));
     contents()->GetMainFrame()->InitializeRenderFrameIfNeeded();
   }
 
@@ -40,8 +82,10 @@ class ConversionHostTest : public RenderViewHostTestHarness {
 
   ConversionHost* conversion_host() { return conversion_host_.get(); }
 
+ protected:
+  TestConversionManager test_manager_;
+
  private:
-  base::test::ScopedFeatureList feature_list_;
   std::unique_ptr<ConversionHost> conversion_host_;
 };
 

content/browser/conversions/conversion_manager.h

@@ -5,63 +5,41 @@
 #ifndef CONTENT_BROWSER_CONVERSIONS_CONVERSION_MANAGER_H_
 #define CONTENT_BROWSER_CONVERSIONS_CONVERSION_MANAGER_H_
 
-#include <memory>
-
-#include "base/files/file_path.h"
-#include "base/macros.h"
-#include "base/memory/scoped_refptr.h"
-#include "base/memory/weak_ptr.h"
-#include "base/sequenced_task_runner.h"
 #include "content/browser/conversions/conversion_policy.h"
-#include "content/browser/conversions/conversion_storage.h"
 #include "content/browser/conversions/storable_conversion.h"
-
-namespace base {
-class Clock;
-}  // namespace base
+#include "content/browser/conversions/storable_impression.h"
+#include "content/common/content_export.h"
 
 namespace content {
 
-class ConversionStorage;
+class WebContents;
 
-// UI thread class that manages the lifetime of the underlying conversion
-// storage. Owned by the storage partition.
-class ConversionManager {
+// Interface that mediates data flow between the network, storage layer, and
+// blink.
+class CONTENT_EXPORT ConversionManager {
  public:
-  // |storage_task_runner| should run with base::TaskPriority::BEST_EFFORT.
-  ConversionManager(
-      const base::FilePath& user_data_directory,
-      scoped_refptr<base::SequencedTaskRunner> storage_task_runner);
-  ConversionManager(const ConversionManager& other) = delete;
-  ConversionManager& operator=(const ConversionManager& other) = delete;
-  ~ConversionManager();
+  // Provides access to a ConversionManager implementation. This layer of
+  // abstraction is to allow tests to mock out the ConversionManager without
+  // injecting a manager explicitly.
+  class Provider {
+   public:
+    virtual ~Provider() = default;
+
+    // Gets the ConversionManager that should be used for handling conversions
+    // that occur in the given |web_contents|. Returns nullptr if conversion
+    // measurement is not enabled in the given |web_contents|, e.g. when the
+    // browser context is off the record.
+    virtual ConversionManager* GetManager(WebContents* web_contents) const = 0;
+  };
+  virtual ~ConversionManager() = default;
 
   // Process a newly registered conversion. Will create and log any new
   // conversion reports to storage.
-  void HandleConversion(const StorableConversion& conversion);
-
-  const ConversionPolicy& GetConversionPolicy() const;
-
- private:
-  void OnInitCompleted(bool success);
-
-  // Task runner used to perform operations on |storage_|. Runs with
-  // base::TaskPriority::BEST_EFFORT.
-  scoped_refptr<base::SequencedTaskRunner> storage_task_runner_;
-
-  base::Clock* clock_;
-
-  // ConversionStorage instance which is scoped to lifetime of
-  // |storage_task_runner_|. |storage_| should be accessed by calling
-  // base::PostTask with |storage_task_runner_|, and should not be accessed
-  // directly.
-  std::unique_ptr<ConversionStorage, base::OnTaskRunnerDeleter> storage_;
-
-  // Policy used for controlling API configurations such as reporting and
-  // attribution models. Unique ptr so it can be overridden for testing.
-  std::unique_ptr<ConversionPolicy> conversion_policy_;
+  virtual void HandleConversion(const StorableConversion& conversion) = 0;
 
-  base::WeakPtrFactory<ConversionManager> weak_factory_;
+  // Returns the ConversionPolicy that is used to control API policies such
+  // as noise.
+  virtual const ConversionPolicy& GetConversionPolicy() const = 0;
 };
 
 }  // namespace content

content/browser/conversions/conversion_manager.cc → content/browser/conversions/conversion_manager_impl.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/conversions/conversion_manager.h"
+#include "content/browser/conversions/conversion_manager_impl.h"
 
 #include <memory>
 
@@ -14,7 +14,7 @@
 
 namespace content {
 
-ConversionManager::ConversionManager(
+ConversionManagerImpl::ConversionManagerImpl(
     const base::FilePath& user_data_directory,
     scoped_refptr<base::SequencedTaskRunner> task_runner)
     : storage_task_runner_(std::move(task_runner)),
@@ -32,13 +32,14 @@ ConversionManager::ConversionManager(
       storage_task_runner_.get(), FROM_HERE,
       base::BindOnce(&ConversionStorage::Initialize,
                      base::Unretained(storage_.get())),
-      base::BindOnce(&ConversionManager::OnInitCompleted,
+      base::BindOnce(&ConversionManagerImpl::OnInitCompleted,
                      weak_factory_.GetWeakPtr()));
 }
 
-ConversionManager::~ConversionManager() = default;
+ConversionManagerImpl::~ConversionManagerImpl() = default;
 
-void ConversionManager::HandleConversion(const StorableConversion& conversion) {
+void ConversionManagerImpl::HandleConversion(
+    const StorableConversion& conversion) {
   if (!storage_)
     return;
 
@@ -54,11 +55,11 @@ void ConversionManager::HandleConversion(const StorableConversion& conversion) {
           base::Unretained(storage_.get()), conversion));
 }
 
-const ConversionPolicy& ConversionManager::GetConversionPolicy() const {
+const ConversionPolicy& ConversionManagerImpl::GetConversionPolicy() const {
   return *conversion_policy_;
 }
 
-void ConversionManager::OnInitCompleted(bool success) {
+void ConversionManagerImpl::OnInitCompleted(bool success) {
   if (!success)
     storage_.reset();
 }

content/browser/conversions/conversion_manager_impl.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_CONVERSIONS_CONVERSION_MANAGER_IMPL_H_
+#define CONTENT_BROWSER_CONVERSIONS_CONVERSION_MANAGER_IMPL_H_
+
+#include <memory>
+
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
+#include "content/browser/conversions/conversion_manager.h"
+#include "content/browser/conversions/conversion_policy.h"
+#include "content/browser/conversions/conversion_storage.h"
+
+namespace base {
+class Clock;
+}  // namespace base
+
+namespace content {
+
+// UI thread class that manages the lifetime of the underlying conversion
+// storage. Owned by the storage partition.
+class ConversionManagerImpl : public ConversionManager {
+ public:
+  // |storage_task_runner| should run with base::TaskPriority::BEST_EFFORT.
+  ConversionManagerImpl(
+      const base::FilePath& user_data_directory,
+      scoped_refptr<base::SequencedTaskRunner> storage_task_runner);
+  ConversionManagerImpl(const ConversionManagerImpl& other) = delete;
+  ConversionManagerImpl& operator=(const ConversionManagerImpl& other) = delete;
+  ~ConversionManagerImpl() override;
+
+  // ConversionManager:
+  void HandleConversion(const StorableConversion& conversion) override;
+  const ConversionPolicy& GetConversionPolicy() const override;
+
+ private:
+  void OnInitCompleted(bool success);
+
+  // Task runner used to perform operations on |storage_|. Runs with
+  // base::TaskPriority::BEST_EFFORT.
+  scoped_refptr<base::SequencedTaskRunner> storage_task_runner_;
+
+  base::Clock* clock_;
+
+  // ConversionStorage instance which is scoped to lifetime of
+  // |storage_task_runner_|. |storage_| should be accessed by calling
+  // base::PostTask with |storage_task_runner_|, and should not be accessed
+  // directly. |storage_| can be null if the storage initialization did not
+  // succeed.
+  //
+  // TODO(https://crbug.com/1066920): This should use base::SequenceBound to
+  // avoid having to call PostTask manually, as well as use base::Unretained on
+  // |storage_|.
+  std::unique_ptr<ConversionStorage, base::OnTaskRunnerDeleter> storage_;
+
+  // Policy used for controlling API configurations such as reporting and
+  // attribution models. Unique ptr so it can be overridden for testing.
+  std::unique_ptr<ConversionPolicy> conversion_policy_;
+
+  base::WeakPtrFactory<ConversionManagerImpl> weak_factory_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_CONVERSIONS_CONVERSION_MANAGER_IMPL_H_

content/browser/storage_partition_impl.cc

@@ -44,7 +44,7 @@
 #include "content/browser/browsing_data/storage_partition_code_cache_data_remover.h"
 #include "content/browser/code_cache/generated_code_cache.h"
 #include "content/browser/code_cache/generated_code_cache_context.h"
-#include "content/browser/conversions/conversion_manager.h"
+#include "content/browser/conversions/conversion_manager_impl.h"
 #include "content/browser/cookie_store/cookie_store_context.h"
 #include "content/browser/devtools/devtools_instrumentation.h"
 #include "content/browser/devtools/devtools_url_loader_interceptor.h"
@@ -1546,7 +1546,7 @@ void StoragePartitionImpl::Initialize() {
   // The Conversion Measurement API is not available in Incognito mode.
   if (!is_in_memory_ &&
       base::FeatureList::IsEnabled(features::kConversionMeasurement)) {
-    conversion_manager_ = std::make_unique<ConversionManager>(
+    conversion_manager_ = std::make_unique<ConversionManagerImpl>(
         path, base::ThreadPool::CreateSequencedTaskRunner(
                   {base::MayBlock(), base::TaskPriority::BEST_EFFORT}));
   }
@@ -1830,7 +1830,7 @@ StoragePartitionImpl::GetNativeFileSystemManager() {
   return native_file_system_manager_.get();
 }
 
-ConversionManager* StoragePartitionImpl::GetConversionManager() {
+ConversionManagerImpl* StoragePartitionImpl::GetConversionManager() {
   DCHECK(initialized_);
   return conversion_manager_.get();
 }

content/browser/storage_partition_impl.h

@@ -65,7 +65,7 @@ class ProtoDatabaseProvider;
 namespace content {
 
 class BackgroundFetchContext;
-class ConversionManager;
+class ConversionManagerImpl;
 class CookieStoreContext;
 class BlobRegistryWrapper;
 class PrefetchURLLoaderService;
@@ -196,7 +196,7 @@ class CONTENT_EXPORT StoragePartitionImpl
   CookieStoreContext* GetCookieStoreContext();
   NativeFileSystemManagerImpl* GetNativeFileSystemManager();
   QuotaContext* GetQuotaContext();
-  ConversionManager* GetConversionManager();
+  ConversionManagerImpl* GetConversionManager();
   NativeIOContext* GetNativeIOContext();
 
   // blink::mojom::DomStorage interface.
@@ -495,7 +495,7 @@ class CONTENT_EXPORT StoragePartitionImpl
   std::unique_ptr<leveldb_proto::ProtoDatabaseProvider>
       proto_database_provider_;
   scoped_refptr<ContentIndexContextImpl> content_index_context_;
-  std::unique_ptr<ConversionManager> conversion_manager_;
+  std::unique_ptr<ConversionManagerImpl> conversion_manager_;
   std::unique_ptr<NativeIOContext> native_io_context_;
 
   // ReceiverSet for DomStorage, using the