Add AddChildWindowToBrowser to DisplayClient mojo interface

On Windows, we've been seeing hangs on account of the browser process waiting for the GPU process when the compositor frame sink is destroyed. The hangs occur because the GPU process is busy with work. Since browser process waits happen on the main browser process thread, the window becomes frosted/ghosted if no input is processed after some time. We need to remove these waits. In current code, failure to wait will cause the browser process window handle (HWND) to be destroyed out from under the GPU process. In DComp modes, the GPU process passes the window handle to OS APIs and also sends it back to the browser process as part of the GpuHost::SetChildSurface mojo call. These calls would use an invalid window handle if the browser process destroys the window handle out from under the GPU process. To avoid the hang and keep the GPU process from using invalid handles, this CL moves SetChildSurface from GpuHost to DisplayClient and renames it to ParentChildSurfaceToBrowser. This new function does not require the browser process HWND, only the child (GPU) process HWND. Since there is a 1:1 relationship between the DisplayClient and compositor frame sink, invalid HWNDs should no longer need to be passed to the browser process. DisplayClient gets disconnected when the browser process destroys the compositor frame sink. A follow on change will remove code in the GPU process that uses the browser HWND. Once that is complete, we can remove the wait in the browser process. Bug: 1372474 Change-Id: Ibf9cf4f3acf237525d6c4fb94197eab18b826753 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984022 Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Reviewed-by: Kyle Charbonneau <kylechar@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Commit-Queue: Rafael Cintron <rafael.cintron@microsoft.com> Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org> Reviewed-by: Matthew Denton <mpdenton@chromium.org> Cr-Commit-Position: refs/heads/main@{#1069385}
Denger-Network · Nov 9, 2022 · c8b5379 · c8b5379
1 parent e036dbf
commit c8b5379
Show file tree

Hide file tree

Showing 41 changed files with 129 additions and 95 deletions.
diff --git a/android_webview/browser/gfx/hardware_renderer_viz.cc b/android_webview/browser/gfx/hardware_renderer_viz.cc
@@ -99,6 +99,8 @@ class HardwareRendererViz::OnViz : public viz::DisplayClient {
   void DisplayDidReceiveCALayerParams(
       const gfx::CALayerParams& ca_layer_params) override {}
   void DisplayDidCompleteSwapWithSize(const gfx::Size& pixel_size) override {}
+  void DisplayAddChildWindowToBrowser(
+      gpu::SurfaceHandle child_window) override {}
   void SetWideColorEnabled(bool enabled) override {}
   void SetPreferredFrameInterval(base::TimeDelta interval) override {}
   base::TimeDelta GetPreferredFrameIntervalForFrameSinkId(

diff --git a/cc/test/fake_output_surface_client.h b/cc/test/fake_output_surface_client.h
@@ -25,6 +25,7 @@ class FakeOutputSurfaceClient : public viz::OutputSurfaceClient {
       const gfx::PresentationFeedback& feedback) override {}
   void DidReceiveReleasedOverlays(
       const std::vector<gpu::Mailbox>& released_overlays) override {}
+  void AddChildWindowToBrowser(gpu::SurfaceHandle child_window) override {}
 
   int swap_count() { return swap_count_; }
 

diff --git a/cc/test/test_layer_tree_frame_sink.cc b/cc/test/test_layer_tree_frame_sink.cc
@@ -304,6 +304,9 @@ void TestLayerTreeFrameSink::DisplayDidReceiveCALayerParams(
 void TestLayerTreeFrameSink::DisplayDidCompleteSwapWithSize(
     const gfx::Size& pixel_Size) {}
 
+void TestLayerTreeFrameSink::DisplayAddChildWindowToBrowser(
+    gpu::SurfaceHandle child_window) {}
+
 void TestLayerTreeFrameSink::OnNeedsBeginFrames(bool needs_begin_frames) {
   support_->SetNeedsBeginFrame(needs_begin_frames);
 }

diff --git a/cc/test/test_layer_tree_frame_sink.h b/cc/test/test_layer_tree_frame_sink.h
@@ -120,6 +120,7 @@ class TestLayerTreeFrameSink : public LayerTreeFrameSink,
   void DisplayDidReceiveCALayerParams(
       const gfx::CALayerParams& ca_layer_params) override;
   void DisplayDidCompleteSwapWithSize(const gfx::Size& pixel_size) override;
+  void DisplayAddChildWindowToBrowser(gpu::SurfaceHandle child_window) override;
   void SetWideColorEnabled(bool enabled) override {}
   void SetPreferredFrameInterval(base::TimeDelta interval) override {}
   base::TimeDelta GetPreferredFrameIntervalForFrameSinkId(

diff --git a/components/viz/host/gpu_host_impl.cc b/components/viz/host/gpu_host_impl.cc
@@ -618,11 +618,11 @@ void GpuHostImpl::DidUpdateDXGIInfo(gfx::mojom::DXGIInfoPtr dxgi_info) {
   delegate_->DidUpdateDXGIInfo(std::move(dxgi_info));
 }
 
-void GpuHostImpl::SetChildSurface(gpu::SurfaceHandle parent,
-                                  gpu::SurfaceHandle child) {
+void GpuHostImpl::AddChildWindow(gpu::SurfaceHandle parent_window,
+                                 gpu::SurfaceHandle child_window) {
   if (pid_ != base::kNullProcessId) {
     gfx::RenderingWindowManager::GetInstance()->RegisterChild(
-        parent, child, /*expected_child_process_id=*/pid_);
+        parent_window, child_window, /*expected_child_process_id=*/pid_);
   }
 }
 #endif  // BUILDFLAG(IS_WIN)

diff --git a/components/viz/host/gpu_host_impl.h b/components/viz/host/gpu_host_impl.h
@@ -212,6 +212,8 @@ class VIZ_HOST_EXPORT GpuHostImpl : public mojom::GpuHost
 
 #if BUILDFLAG(IS_WIN)
   mojom::InfoCollectionGpuService* info_collection_gpu_service();
+  void AddChildWindow(gpu::SurfaceHandle parent_window,
+                      gpu::SurfaceHandle child_window);
 #endif
 
  private:
@@ -258,8 +260,6 @@ class VIZ_HOST_EXPORT GpuHostImpl : public mojom::GpuHost
 #if BUILDFLAG(IS_WIN)
   void DidUpdateOverlayInfo(const gpu::OverlayInfo& overlay_info) override;
   void DidUpdateDXGIInfo(gfx::mojom::DXGIInfoPtr dxgi_info) override;
-  void SetChildSurface(gpu::SurfaceHandle parent,
-                       gpu::SurfaceHandle child) override;
 #endif
   void GetIsolationKey(int32_t client_id,
                        const blink::WebGPUExecutionContextToken& token,

diff --git a/components/viz/host/host_display_client.cc b/components/viz/host/host_display_client.cc
@@ -13,6 +13,7 @@
 
 #if BUILDFLAG(IS_WIN)
 #include <windows.h>
+#include <utility>
 
 #include "components/viz/common/display/use_layered_window.h"
 #include "components/viz/host/layered_window_updater_impl.h"
@@ -57,6 +58,10 @@ void HostDisplayClient::CreateLayeredWindowUpdater(
   layered_window_updater_ =
       std::make_unique<LayeredWindowUpdaterImpl>(widget_, std::move(receiver));
 }
+void HostDisplayClient::AddChildWindowToBrowser(
+    gpu::SurfaceHandle child_window) {
+  NOTREACHED();
+}
 #endif
 
 // TODO(crbug.com/1052397): Revisit the macro expression once build flag switch

diff --git a/components/viz/host/host_display_client.h b/components/viz/host/host_display_client.h
@@ -34,6 +34,11 @@ class VIZ_HOST_EXPORT HostDisplayClient : public mojom::DisplayClient {
   mojo::PendingRemote<mojom::DisplayClient> GetBoundRemote(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
+ protected:
+#if BUILDFLAG(IS_APPLE) || BUILDFLAG(IS_WIN)
+  gfx::AcceleratedWidget widget() const { return widget_; }
+#endif
+
  private:
   // mojom::DisplayClient implementation:
 #if BUILDFLAG(IS_APPLE)
@@ -44,6 +49,7 @@ class VIZ_HOST_EXPORT HostDisplayClient : public mojom::DisplayClient {
 #if BUILDFLAG(IS_WIN)
   void CreateLayeredWindowUpdater(
       mojo::PendingReceiver<mojom::LayeredWindowUpdater> receiver) override;
+  void AddChildWindowToBrowser(gpu::SurfaceHandle child_window) override;
 #endif
 
 // TODO(crbug.com/1052397): Revisit the macro expression once build flag switch

diff --git a/components/viz/service/display/display.cc b/components/viz/service/display/display.cc
@@ -1095,6 +1095,12 @@ void Display::DidReceiveReleasedOverlays(
     renderer_->DidReceiveReleasedOverlays(released_overlays);
 }
 
+void Display::AddChildWindowToBrowser(gpu::SurfaceHandle child_window) {
+  if (client_) {
+    client_->DisplayAddChildWindowToBrowser(child_window);
+  }
+}
+
 void Display::SetNeedsRedrawRect(const gfx::Rect& damage_rect) {
   aggregator_->SetFullDamageForSurface(current_surface_id_);
   damage_tracker_->SetRootSurfaceDamaged();

diff --git a/components/viz/service/display/display.h b/components/viz/service/display/display.h
@@ -167,6 +167,7 @@ class VIZ_SERVICE_EXPORT Display : public DisplaySchedulerClient,
       const gfx::PresentationFeedback& feedback) override;
   void DidReceiveReleasedOverlays(
       const std::vector<gpu::Mailbox>& released_overlays) override;
+  void AddChildWindowToBrowser(gpu::SurfaceHandle child_window) override;
 
   // LatestLocalSurfaceIdLookupDelegate implementation.
   LocalSurfaceId GetSurfaceAtAggregation(

diff --git a/components/viz/service/display/display_client.h b/components/viz/service/display/display_client.h
@@ -6,6 +6,7 @@
 #define COMPONENTS_VIZ_SERVICE_DISPLAY_DISPLAY_CLIENT_H_
 
 #include "components/viz/common/quads/aggregated_render_pass.h"
+#include "gpu/ipc/common/surface_handle.h"
 #include "services/viz/public/mojom/compositing/compositor_frame_sink.mojom.h"
 
 namespace gfx {
@@ -28,6 +29,8 @@ class DisplayClient {
   virtual void DisplayDidReceiveCALayerParams(
       const gfx::CALayerParams& ca_layer_params) = 0;
   virtual void DisplayDidCompleteSwapWithSize(const gfx::Size& pixel_size) = 0;
+  virtual void DisplayAddChildWindowToBrowser(
+      gpu::SurfaceHandle child_window) = 0;
   virtual void SetWideColorEnabled(bool enabled) = 0;
   virtual void SetPreferredFrameInterval(base::TimeDelta interval) = 0;
   virtual base::TimeDelta GetPreferredFrameIntervalForFrameSinkId(

diff --git a/components/viz/service/display/display_unittest.cc b/components/viz/service/display/display_unittest.cc
@@ -124,6 +124,8 @@ class StubDisplayClient : public DisplayClient {
   void DisplayDidReceiveCALayerParams(
       const gfx::CALayerParams& ca_layer_params) override {}
   void DisplayDidCompleteSwapWithSize(const gfx::Size& pixel_size) override {}
+  void DisplayAddChildWindowToBrowser(
+      gpu::SurfaceHandle child_window) override {}
   void SetWideColorEnabled(bool enabled) override {}
   void SetPreferredFrameInterval(base::TimeDelta interval) override {}
   base::TimeDelta GetPreferredFrameIntervalForFrameSinkId(

diff --git a/components/viz/service/display/output_surface_client.h b/components/viz/service/display/output_surface_client.h
@@ -12,6 +12,7 @@
 #include "components/viz/common/resources/returned_resource.h"
 #include "components/viz/service/viz_service_export.h"
 #include "gpu/command_buffer/common/mailbox.h"
+#include "gpu/ipc/common/surface_handle.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/gpu_fence_handle.h"
 #include "ui/latency/latency_info.h"
@@ -52,6 +53,10 @@ class VIZ_SERVICE_EXPORT OutputSurfaceClient {
   virtual void DidReceiveReleasedOverlays(
       const std::vector<gpu::Mailbox>& released_overlays) = 0;
 
+  // Sends the created child window to the browser process so that it can be
+  // parented to the browser process window.
+  virtual void AddChildWindowToBrowser(gpu::SurfaceHandle child_window) = 0;
+
  protected:
   virtual ~OutputSurfaceClient() {}
 };

diff --git a/components/viz/service/display/renderer_perftest.cc b/components/viz/service/display/renderer_perftest.cc
@@ -86,6 +86,8 @@ class WaitForSwapDisplayClient : public DisplayClient {
     DCHECK(loop_);
     loop_->Quit();
   }
+  void DisplayAddChildWindowToBrowser(
+      gpu::SurfaceHandle child_window) override {}
   void SetWideColorEnabled(bool enabled) override {}
   void SetPreferredFrameInterval(base::TimeDelta interval) override {}
   base::TimeDelta GetPreferredFrameIntervalForFrameSinkId(

diff --git a/components/viz/service/display_embedder/skia_output_surface_dependency.h b/components/viz/service/display_embedder/skia_output_surface_dependency.h
@@ -12,7 +12,6 @@
 #include "base/callback_helpers.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
-#include "build/build_config.h"
 #include "components/viz/service/display/display_compositor_memory_and_task_controller.h"
 #include "components/viz/service/viz_service_export.h"
 #include "gpu/command_buffer/common/constants.h"
@@ -100,12 +99,6 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceDependency {
   // called only from GPU Thread.
   virtual void ScheduleDelayedGPUTaskFromGPUThread(base::OnceClosure task) = 0;
 
-#if BUILDFLAG(IS_WIN)
-  virtual void DidCreateAcceleratedSurfaceChildWindow(
-      gpu::SurfaceHandle parent_window,
-      gpu::SurfaceHandle child_window) = 0;
-#endif
-
   virtual void DidLoseContext(gpu::error::ContextLostReason reason,
                               const GURL& active_url) = 0;
 

diff --git a/components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc b/components/viz/service/display_embedder/skia_output_surface_dependency_impl.cc
@@ -144,14 +144,6 @@ void SkiaOutputSurfaceDependencyImpl::ScheduleDelayedGPUTaskFromGPUThread(
       FROM_HERE, std::move(task), kDelayForDelayedWork);
 }
 
-#if BUILDFLAG(IS_WIN)
-void SkiaOutputSurfaceDependencyImpl::DidCreateAcceleratedSurfaceChildWindow(
-    gpu::SurfaceHandle parent_window,
-    gpu::SurfaceHandle child_window) {
-  gpu_service_impl_->SendCreatedChildWindow(parent_window, child_window);
-}
-#endif
-
 void SkiaOutputSurfaceDependencyImpl::DidLoseContext(
     gpu::error::ContextLostReason reason,
     const GURL& active_url) {

diff --git a/components/viz/service/display_embedder/skia_output_surface_dependency_impl.h b/components/viz/service/display_embedder/skia_output_surface_dependency_impl.h
@@ -9,7 +9,6 @@
 
 #include "base/callback_helpers.h"
 #include "base/memory/raw_ptr.h"
-#include "build/build_config.h"
 #include "components/viz/service/display_embedder/skia_output_surface_dependency.h"
 
 namespace base {
@@ -55,13 +54,6 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceDependencyImpl
   scoped_refptr<base::TaskRunner> GetClientTaskRunner() override;
   void ScheduleGrContextCleanup() override;
   void ScheduleDelayedGPUTaskFromGPUThread(base::OnceClosure task) override;
-
-#if BUILDFLAG(IS_WIN)
-  void DidCreateAcceleratedSurfaceChildWindow(
-      gpu::SurfaceHandle parent_window,
-      gpu::SurfaceHandle child_window) override;
-#endif
-
   void DidLoseContext(gpu::error::ContextLostReason reason,
                       const GURL& active_url) override;
 

diff --git a/components/viz/service/display_embedder/skia_output_surface_impl.cc b/components/viz/service/display_embedder/skia_output_surface_impl.cc
@@ -872,13 +872,16 @@ void SkiaOutputSurfaceImpl::InitializeOnGpuThread(
       base::BindOnce(&SkiaOutputSurfaceImpl::ContextLost, weak_ptr_);
   auto schedule_gpu_task = base::BindRepeating(
       &SkiaOutputSurfaceImpl::ScheduleOrRetainGpuTask, weak_ptr_);
+  auto add_child_window_to_browser_callback = base::BindRepeating(
+      &SkiaOutputSurfaceImpl::AddChildWindowToBrowser, weak_ptr_);
 
   impl_on_gpu_ = SkiaOutputSurfaceImplOnGpu::Create(
       dependency_, renderer_settings_, gpu_task_scheduler_->GetSequenceId(),
       display_compositor_controller_->controller_on_gpu(),
       std::move(did_swap_buffer_complete_callback),
       std::move(buffer_presented_callback), std::move(context_lost_callback),
-      std::move(schedule_gpu_task), std::move(vsync_callback_runner));
+      std::move(schedule_gpu_task), std::move(vsync_callback_runner),
+      std::move(add_child_window_to_browser_callback));
   if (!impl_on_gpu_) {
     *result = false;
   } else {
@@ -1044,6 +1047,13 @@ void SkiaOutputSurfaceImpl::BufferPresented(
   }
 }
 
+void SkiaOutputSurfaceImpl::AddChildWindowToBrowser(
+    gpu::SurfaceHandle child_window) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(client_);
+  client_->AddChildWindowToBrowser(child_window);
+}
+
 void SkiaOutputSurfaceImpl::OnGpuVSync(base::TimeTicks timebase,
                                        base::TimeDelta interval) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

diff --git a/components/viz/service/display_embedder/skia_output_surface_impl.h b/components/viz/service/display_embedder/skia_output_surface_impl.h
@@ -197,6 +197,7 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
                               const gfx::Size& pixel_size,
                               gfx::GpuFenceHandle release_fence);
   void BufferPresented(const gfx::PresentationFeedback& feedback);
+  void AddChildWindowToBrowser(gpu::SurfaceHandle child_window);
 
   // Provided as a callback for the GPU thread.
   void OnGpuVSync(base::TimeTicks timebase, base::TimeDelta interval);

diff --git a/components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc b/components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc
@@ -240,7 +240,8 @@ std::unique_ptr<SkiaOutputSurfaceImplOnGpu> SkiaOutputSurfaceImplOnGpu::Create(
     BufferPresentedCallback buffer_presented_callback,
     ContextLostCallback context_lost_callback,
     ScheduleGpuTaskCallback schedule_gpu_task,
-    GpuVSyncCallback gpu_vsync_callback) {
+    GpuVSyncCallback gpu_vsync_callback,
+    AddChildWindowToBrowserCallback add_child_window_to_browser_callback) {
   TRACE_EVENT0("viz", "SkiaOutputSurfaceImplOnGpu::Create");
 
   auto context_state = deps->GetSharedContextState();
@@ -262,7 +263,8 @@ std::unique_ptr<SkiaOutputSurfaceImplOnGpu> SkiaOutputSurfaceImplOnGpu::Create(
       context_state->feature_info(), renderer_settings, sequence_id,
       shared_gpu_deps, std::move(did_swap_buffer_complete_callback),
       std::move(buffer_presented_callback), std::move(context_lost_callback),
-      std::move(schedule_gpu_task), std::move(gpu_vsync_callback));
+      std::move(schedule_gpu_task), std::move(gpu_vsync_callback),
+      std::move(add_child_window_to_browser_callback));
   if (!impl_on_gpu->Initialize())
     return nullptr;
 
@@ -280,7 +282,8 @@ SkiaOutputSurfaceImplOnGpu::SkiaOutputSurfaceImplOnGpu(
     BufferPresentedCallback buffer_presented_callback,
     ContextLostCallback context_lost_callback,
     ScheduleGpuTaskCallback schedule_gpu_task,
-    GpuVSyncCallback gpu_vsync_callback)
+    GpuVSyncCallback gpu_vsync_callback,
+    AddChildWindowToBrowserCallback add_child_window_to_browser_callback)
     : dependency_(std::move(deps)),
       shared_gpu_deps_(shared_gpu_deps),
       feature_info_(std::move(feature_info)),
@@ -303,6 +306,8 @@ SkiaOutputSurfaceImplOnGpu::SkiaOutputSurfaceImplOnGpu(
       context_lost_callback_(std::move(context_lost_callback)),
       schedule_gpu_task_(std::move(schedule_gpu_task)),
       gpu_vsync_callback_(std::move(gpu_vsync_callback)),
+      add_child_window_to_browser_callback_(
+          std::move(add_child_window_to_browser_callback)),
       gpu_preferences_(dependency_->GetGpuPreferences()),
       async_read_result_lock_(base::MakeRefCounted<AsyncReadResultLock>()) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
@@ -1824,10 +1829,9 @@ bool SkiaOutputSurfaceImplOnGpu::InitializeForVulkan() {
     return false;
 
 #if BUILDFLAG(IS_WIN)
-  gpu::SurfaceHandle child_surface = output_device->GetChildSurfaceHandle();
-  if (child_surface != gpu::kNullSurfaceHandle) {
-    DidCreateAcceleratedSurfaceChildWindow(dependency_->GetSurfaceHandle(),
-                                           child_surface);
+  gpu::SurfaceHandle child_window = output_device->GetChildSurfaceHandle();
+  if (child_window != gpu::kNullSurfaceHandle) {
+    AddChildWindowToBrowser(child_window);
   }
 #endif  // BUILDFLAG(IS_WIN)
   output_device_ = std::move(output_device);
@@ -1863,10 +1867,9 @@ bool SkiaOutputSurfaceImplOnGpu::InitializeForDawn() {
             dawn_context_provider_, dependency_->GetSurfaceHandle(),
             gfx::SurfaceOrigin::kTopLeft, shared_gpu_deps_->memory_tracker(),
             GetDidSwapBuffersCompleteCallback());
-    const gpu::SurfaceHandle child_surface_handle =
+    const gpu::SurfaceHandle child_window_handle =
         output_device->GetChildSurfaceHandle();
-    DidCreateAcceleratedSurfaceChildWindow(dependency_->GetSurfaceHandle(),
-                                           child_surface_handle);
+    AddChildWindowToBrowser(child_window_handle);
     output_device_ = std::move(output_device);
 #else
     NOTREACHED();
@@ -2058,11 +2061,10 @@ bool SkiaOutputSurfaceImplOnGpu::IsDisplayedAsOverlay() {
 }
 
 #if BUILDFLAG(IS_WIN)
-void SkiaOutputSurfaceImplOnGpu::DidCreateAcceleratedSurfaceChildWindow(
-    gpu::SurfaceHandle parent_window,
+void SkiaOutputSurfaceImplOnGpu::AddChildWindowToBrowser(
     gpu::SurfaceHandle child_window) {
-  dependency_->DidCreateAcceleratedSurfaceChildWindow(parent_window,
-                                                      child_window);
+  PostTaskToClientThread(
+      base::BindOnce(add_child_window_to_browser_callback_, child_window));
 }
 #endif