Fix gpu process dependency injection plumbing

When running the GPU process, we use dependency injection to customize
its behaviour through the ExternalDependencies struct. VizMainImpl's
constructor takes an ExternalDependencies parameter and std::moves it to
a member field. Some of the code, however, continued to reference the
parameter instead of the field. This lead to the code assuming that some
dependencies were stubbed out.

This CL updates VizMainImpl's constructor to consistently check the
member field for the injected values and adds tests to check that some
injected values end up in use.

Bug: 1040583
Change-Id: If1c7922f4c7286c2a73afd33572b287ad29f64ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1992701Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Tom McKee <tommckee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733385}

components/viz/service/BUILD.gn

@@ -456,6 +456,7 @@ viz_source_set("unit_tests") {
     "frame_sinks/video_detector_unittest.cc",
     "gl/gpu_service_impl_unittest.cc",
     "hit_test/hit_test_aggregator_unittest.cc",
+    "main/viz_main_impl_unittest.cc",
     "surfaces/referenced_surface_tracker_unittest.cc",
     "surfaces/surface_unittest.cc",
   ]
@@ -478,6 +479,7 @@ viz_source_set("unit_tests") {
     "//components/viz/client",
     "//components/viz/common",
     "//components/viz/host",
+    "//components/viz/service/main:main",
     "//components/viz/test:test_support",
     "//gpu/command_buffer/client",
     "//gpu/command_buffer/client:gles2_implementation",

components/viz/service/main/viz_main_impl.cc

@@ -11,7 +11,7 @@
 #include "base/feature_list.h"
 #include "base/message_loop/message_pump_type.h"
 #include "base/power_monitor/power_monitor.h"
-#include "base/power_monitor/power_monitor_device_source.h"
+#include "base/power_monitor/power_monitor_source.h"
 #include "base/single_thread_task_runner.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "build/build_config.h"
@@ -68,16 +68,16 @@ VizMainImpl::VizMainImpl(Delegate* delegate,
   // TODO(crbug.com/609317): Remove this when Mus Window Server and GPU are
   // split into separate processes. Until then this is necessary to be able to
   // run Mushrome (chrome with mus) with Mus running in the browser process.
-  if (!base::PowerMonitor::IsInitialized()) {
+  if (dependencies_.power_monitor_source) {
     base::PowerMonitor::Initialize(
-        std::make_unique<base::PowerMonitorDeviceSource>());
+        std::move(dependencies_.power_monitor_source));
   }
 
   if (!dependencies_.io_thread_task_runner)
     io_thread_ = CreateAndStartIOThread();
 
-  if (dependencies.viz_compositor_thread_runner) {
-    viz_compositor_thread_runner_ = dependencies.viz_compositor_thread_runner;
+  if (dependencies_.viz_compositor_thread_runner) {
+    viz_compositor_thread_runner_ = dependencies_.viz_compositor_thread_runner;
   } else {
     viz_compositor_thread_runner_impl_ =
         std::make_unique<VizCompositorThreadRunnerImpl>();
@@ -88,11 +88,11 @@ VizMainImpl::VizMainImpl(Delegate* delegate,
         viz_compositor_thread_runner_->task_runner());
   }
 
-  if (!gpu_init_->gpu_info().in_process_gpu && dependencies.ukm_recorder) {
+  if (!gpu_init_->gpu_info().in_process_gpu && dependencies_.ukm_recorder) {
     // NOTE: If the GPU is running in the browser process, we can use the
     // browser's UKMRecorder.
-    ukm_recorder_ = std::move(dependencies.ukm_recorder);
-    ukm::DelegatingUkmRecorder::Get()->AddDelegate(ukm_recorder_->GetWeakPtr());
+    ukm::DelegatingUkmRecorder::Get()->AddDelegate(
+        dependencies_.ukm_recorder->GetWeakPtr());
   }
 
   gpu_service_ = std::make_unique<GpuServiceImpl>(
@@ -122,8 +122,9 @@ VizMainImpl::~VizMainImpl() {
   viz_compositor_thread_runner_ = nullptr;
   viz_compositor_thread_runner_impl_.reset();
 
-  if (ukm_recorder_)
-    ukm::DelegatingUkmRecorder::Get()->RemoveDelegate(ukm_recorder_.get());
+  if (dependencies_.ukm_recorder)
+    ukm::DelegatingUkmRecorder::Get()->RemoveDelegate(
+        dependencies_.ukm_recorder.get());
 }
 
 void VizMainImpl::SetLogMessagesForHost(LogMessages log_messages) {

components/viz/service/main/viz_main_impl.h

@@ -5,7 +5,9 @@
 #ifndef COMPONENTS_VIZ_SERVICE_MAIN_VIZ_MAIN_IMPL_H_
 #define COMPONENTS_VIZ_SERVICE_MAIN_VIZ_MAIN_IMPL_H_
 
+#include <memory>
 #include <string>
+#include <vector>
 
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread.h"
@@ -22,6 +24,10 @@
 #include "services/viz/privileged/mojom/viz_main.mojom.h"
 #include "ui/gfx/font_render_params.h"
 
+namespace base {
+class PowerMonitorSource;
+}
+
 namespace gpu {
 class GpuInit;
 class SyncPointManager;
@@ -63,6 +69,23 @@ class VizMainImpl : public mojom::VizMain {
 
     ExternalDependencies& operator=(ExternalDependencies&& other);
 
+    // Note that, due to the design of |base::PowerMonitor|, it is inherently
+    // racy to decide to initialize or not based on a call to
+    // |base::PowerMonitor::IsInitialized|. This makes it difficult for
+    // VizMainImpl to know whether to call initialize or not as the correct
+    // choice depends on the context in which VizMainImpl will be used.
+    //
+    // To work around this, calling code must understand whether VizMainImpl
+    // should initialize |base::PowerMonitor| or not and can then use the
+    // |power_monitor_source| to signal its intent.
+    //
+    // If |power_monitor_source| is null, |PowerMonitor::Initialize| will not
+    // be called. If |power_monitor_source| is non-null, it will be std::move'd
+    // in to a call of |PowerMonitor::Initialize|.
+    //
+    // We use a |PowerMonitorSource| here instead of a boolean flag so that
+    // tests can use mocks and fakes for testing.
+    mutable std::unique_ptr<base::PowerMonitorSource> power_monitor_source;
     gpu::SyncPointManager* sync_point_manager = nullptr;
     gpu::SharedImageManager* shared_image_manager = nullptr;
     base::WaitableEvent* shutdown_event = nullptr;
@@ -152,7 +175,6 @@ class VizMainImpl : public mojom::VizMain {
 
   const scoped_refptr<base::SingleThreadTaskRunner> gpu_thread_task_runner_;
 
-  std::unique_ptr<ukm::MojoUkmRecorder> ukm_recorder_;
   mojo::AssociatedReceiver<mojom::VizMain> receiver_{this};
 
   std::unique_ptr<discardable_memory::ClientDiscardableSharedMemoryManager>

components/viz/service/main/viz_main_impl_unittest.cc

+// 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.
+
+#include "components/viz/service/main/viz_main_impl.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/power_monitor/power_monitor.h"
+#include "base/power_monitor/power_monitor_source.h"
+#include "base/test/task_environment.h"
+#include "gpu/config/gpu_info.h"
+#include "gpu/ipc/service/gpu_init.h"
+#include "services/metrics/public/cpp/mojo_ukm_recorder.h"
+#include "services/metrics/public/cpp/ukm_entry_builder.h"
+#include "services/metrics/public/cpp/ukm_recorder.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace viz {
+
+// This mock lets us listen for delegate messages generated by a |VizMainImpl|.
+// We use this to check that Viz's task runner is the task runner specified by
+// dependency injection.
+class MockDelegate : public VizMainImpl::Delegate {
+ public:
+  MOCK_METHOD1(PostCompositorThreadCreated,
+               void(base::SingleThreadTaskRunner*));
+  MOCK_METHOD0(OnInitializationFailed, void());
+  MOCK_METHOD1(OnGpuServiceConnection, void(GpuServiceImpl*));
+  MOCK_METHOD0(QuitMainMessageLoop, void());
+};
+
+// This mock lets us listen for UKM events to be added. We use this to verify
+// that the dependency-injected UKM recorder actually gets used.
+class MockUkmRecorder : public ukm::MojoUkmRecorder {
+ public:
+  MockUkmRecorder()
+      : ukm::MojoUkmRecorder(
+            mojo::PendingRemote<ukm::mojom::UkmRecorderInterface>()) {}
+
+  MOCK_METHOD1(AddEntry, void(ukm::mojom::UkmEntryPtr));
+};
+
+class MockVizCompositorThreadRunner : public VizCompositorThreadRunner {
+ public:
+  explicit MockVizCompositorThreadRunner(
+      base::SingleThreadTaskRunner* task_runner)
+      : VizCompositorThreadRunner(), task_runner_(task_runner) {}
+
+  base::SingleThreadTaskRunner* task_runner() override { return task_runner_; }
+  MOCK_METHOD1(CreateFrameSinkManager, void(mojom::FrameSinkManagerParamsPtr));
+  MOCK_METHOD3(CreateFrameSinkManager,
+               void(mojom::FrameSinkManagerParamsPtr,
+                    gpu::CommandBufferTaskExecutor*,
+                    GpuServiceImpl*));
+#if BUILDFLAG(USE_VIZ_DEVTOOLS)
+  MOCK_METHOD1(CreateVizDevTools, void(mojom::VizDevToolsParamsPtr));
+#endif
+  MOCK_METHOD1(CleanupForShutdown, void(base::OnceClosure));
+
+ private:
+  base::SingleThreadTaskRunner* const task_runner_;
+};
+
+class MockPowerMonitorSource : public base::PowerMonitorSource {
+ public:
+  explicit MockPowerMonitorSource(bool* leak_guard)
+      : base::PowerMonitorSource(), leak_guard_(leak_guard) {
+    *leak_guard_ = true;
+  }
+
+  ~MockPowerMonitorSource() override { *leak_guard_ = false; }
+
+  bool IsOnBatteryPowerImpl() override { return false; }
+
+ private:
+  // An external flag to signal as to whether or not this object is still
+  // alive.
+  bool* leak_guard_;
+};
+
+TEST(VizMainImplTest, OopVizDependencyInjection) {
+  VizMainImpl::ExternalDependencies external_deps;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      base::ThreadTaskRunnerHandle::Get();
+
+  // |VizMainImpl| is supposed to use the |UkmRecorder| injected through
+  // |ExternalDependencies|.
+  std::unique_ptr<MockUkmRecorder> mock_ukm_recorder =
+      std::make_unique<MockUkmRecorder>();
+  EXPECT_CALL(*mock_ukm_recorder, AddEntry);
+  external_deps.ukm_recorder = std::move(mock_ukm_recorder);
+
+  // |VizMainImpl| is supposed to use the task runner injected through
+  // |ExternalDependencies|. We can check which task runner |VizMainImpl| will
+  // use by looking for the task runner reported to the delegate.
+  MockDelegate mock_delegate;
+  EXPECT_CALL(mock_delegate, PostCompositorThreadCreated(task_runner.get()));
+  MockVizCompositorThreadRunner mock_runner(task_runner.get());
+  external_deps.viz_compositor_thread_runner = &mock_runner;
+
+  bool mock_source_is_alive = false;
+  external_deps.power_monitor_source =
+      std::make_unique<MockPowerMonitorSource>(&mock_source_is_alive);
+  ASSERT_TRUE(mock_source_is_alive);
+
+  auto gpu_init = std::make_unique<gpu::GpuInit>();
+  // Need to force GpuInit to request an OOP viz; if |GpuInit| stops owning the
+  // |GPUInfo|, this const_cast may break.
+  const_cast<gpu::GPUInfo&>(gpu_init->gpu_info()).in_process_gpu = false;
+
+  VizMainImpl impl(&mock_delegate, std::move(external_deps),
+                   std::move(gpu_init));
+
+  // Generate a UKM event so that the MockRecorder sees it. We use the global
+  // recorder from |ukm::UkmRecorder::Get()| because that's the recorder that
+  // client code is expected to use. That the correct recorder sees the entry
+  // is handled with the EXPECT_CALL of |mock_ukm_recorder|.
+  ukm::UkmRecorder* recorder = ukm::UkmRecorder::Get();
+  ukm::UkmEntryBuilder builder(ukm::SourceId(42), "Event.ScrollUpdate.Touch");
+  builder.SetMetric("TimeToScrollUpdateSwapBegin", 17);
+  builder.Record(recorder);
+
+  // Need to shutdown the |PowerMonitor| infrastructure.
+  EXPECT_TRUE(base::PowerMonitor::IsInitialized());
+  base::PowerMonitor::ShutdownForTesting();
+  // Double-check that we're not leaking the MockPowerMonitorSource
+  // instance.
+  ASSERT_FALSE(mock_source_is_alive);
+}
+
+}  // namespace viz

content/gpu/gpu_child_thread.cc

@@ -13,6 +13,8 @@
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/memory/weak_ptr.h"
+#include "base/power_monitor/power_monitor.h"
+#include "base/power_monitor/power_monitor_device_source.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/thread_checker.h"
@@ -70,6 +72,10 @@ ChildThreadImpl::Options GetOptions() {
 
 viz::VizMainImpl::ExternalDependencies CreateVizMainDependencies() {
   viz::VizMainImpl::ExternalDependencies deps;
+  if (!base::PowerMonitor::IsInitialized()) {
+    deps.power_monitor_source =
+        std::make_unique<base::PowerMonitorDeviceSource>();
+  }
   if (GetContentClient()->gpu()) {
     deps.sync_point_manager = GetContentClient()->gpu()->GetSyncPointManager();
     deps.shared_image_manager =