Check if parent FrameSinkId is registered.

Add a check that the parent FrameSinkId is registered before creating an
OffscreeCanvasSurfaceImpl, which is really a generic embedded surface
and not just used for offscreen canvas. The IPC channel that controls
the destruction of the parent CompositorFrameSink, and subsequent
invalidation of parent FrameSinkId, is different than the IPC channel
used to create embedded surfaces. We can't rely on ordering of messages
between them. It's possible the parent FrameSinkId is invalidated before
the browser gets the request to create an embedded surface.

If this state is detected then just drop the request for the embedded
surface. This will avoid triggering the DCHECK that the parent
FrameSinkId is registered and avoid doing work for something that is
about to be destroyed.

The experiment to enable UseSurfaceLayerForVideo, which uses
OffscreenCanvasSurfaceImpl, had tests running into this problem and
hitting a DCHECK in HostFrameSinkManager::RegisterFrameSinkHierarchy().

Bug: 830003, 829306
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I4e8cb26548444ecbf941a87a984b21aa4c846580
Reviewed-on: https://chromium-review.googlesource.com/1014330
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551489}

components/viz/host/host_frame_sink_manager.cc

@@ -181,9 +181,16 @@ void HostFrameSinkManager::OnFrameTokenChanged(const FrameSinkId& frame_sink_id,
     data.client->OnFrameTokenChanged(frame_token);
 }
 
-void HostFrameSinkManager::RegisterFrameSinkHierarchy(
+bool HostFrameSinkManager::RegisterFrameSinkHierarchy(
     const FrameSinkId& parent_frame_sink_id,
     const FrameSinkId& child_frame_sink_id) {
+  auto iter = frame_sink_data_map_.find(parent_frame_sink_id);
+  // |parent_frame_sink_id| isn't registered so it can't embed anything.
+  if (iter == frame_sink_data_map_.end() ||
+      !iter->second.IsFrameSinkRegistered()) {
+    return false;
+  }
+
   // Register and store the parent.
   frame_sink_manager_->RegisterFrameSinkHierarchy(parent_frame_sink_id,
                                                   child_frame_sink_id);
@@ -193,10 +200,11 @@ void HostFrameSinkManager::RegisterFrameSinkHierarchy(
   DCHECK(!base::ContainsValue(child_data.parents, parent_frame_sink_id));
   child_data.parents.push_back(parent_frame_sink_id);
 
-  FrameSinkData& parent_data = frame_sink_data_map_[parent_frame_sink_id];
-  DCHECK(parent_data.IsFrameSinkRegistered());
+  FrameSinkData& parent_data = iter->second;
   DCHECK(!base::ContainsValue(parent_data.children, child_frame_sink_id));
   parent_data.children.push_back(child_frame_sink_id);
+
+  return true;
 }
 
 void HostFrameSinkManager::UnregisterFrameSinkHierarchy(

components/viz/host/host_frame_sink_manager.h

@@ -114,9 +114,15 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
                                  mojom::CompositorFrameSinkRequest request,
                                  mojom::CompositorFrameSinkClientPtr client);
 
-  // Registers frame sink hierarchy. Both parent and child FrameSinkIds must be
-  // registered before calling. A frame sink can have multiple parents.
-  void RegisterFrameSinkHierarchy(const FrameSinkId& parent_frame_sink_id,
+  // Registers FrameSink hierarchy. It's expected that the parent will embed
+  // the child. If |parent_frame_sink_id| is registered then it will be added as
+  // a parent of |child_frame_sink_id| and the function will return true. If
+  // |parent_frame_sink_id| is not registered then the function will return
+  // false.
+  //
+  // |child_frame_sink_id| must be registered before calling. A frame sink
+  // can have multiple parents.
+  bool RegisterFrameSinkHierarchy(const FrameSinkId& parent_frame_sink_id,
                                   const FrameSinkId& child_frame_sink_id);
 
   // Unregisters FrameSink hierarchy. Client must have registered frame sink

content/browser/renderer_host/offscreen_canvas_provider_impl_unittest.cc

@@ -39,11 +39,12 @@ constexpr viz::FrameSinkId kFrameSinkParent(kRendererClientId, 1);
 constexpr viz::FrameSinkId kFrameSinkA(kRendererClientId, 3);
 constexpr viz::FrameSinkId kFrameSinkB(kRendererClientId, 4);
 
-// Creates a closure that sets |error_variable| true when run.
-base::OnceClosure ConnectionErrorClosure(bool* error_variable) {
-  DCHECK(error_variable);
-  return base::BindOnce([](bool* error_variable) { *error_variable = true; },
-                        error_variable);
+// Runs RunLoop until |endpoint| encounters a connection error.
+template <class T>
+void WaitForConnectionError(T* endpoint) {
+  base::RunLoop run_loop;
+  endpoint->set_connection_error_handler(run_loop.QuitClosure());
+  run_loop.Run();
 }
 
 // Stub OffscreenCanvasSurfaceClient that stores the latest SurfaceInfo.
@@ -57,7 +58,8 @@ class StubOffscreenCanvasSurfaceClient
     blink::mojom::OffscreenCanvasSurfaceClientPtr client;
     binding_.Bind(mojo::MakeRequest(&client));
     binding_.set_connection_error_handler(
-        ConnectionErrorClosure(&connection_error_));
+        base::BindOnce([](bool* error_variable) { *error_variable = true; },
+                       &connection_error_));
     return client;
   }
 
@@ -242,16 +244,27 @@ TEST_F(OffscreenCanvasProviderImplTest, ClientConnectionWrongOrder) {
       kFrameSinkA, compositor_frame_sink_client.BindInterfacePtr(),
       mojo::MakeRequest(&compositor_frame_sink));
 
-  // Observe connection errors on |compositor_frame_sink|.
-  bool connection_error = false;
-  compositor_frame_sink.set_connection_error_handler(
-      ConnectionErrorClosure(&connection_error));
+  // The request will fail and trigger a connection error.
+  WaitForConnectionError(&compositor_frame_sink);
+}
 
-  RunUntilIdle();
+// Check that trying to create an OffscreenCanvasSurfaceImpl when the parent
+// FrameSinkId has already been invalidated fails.
+TEST_F(OffscreenCanvasProviderImplTest, ParentNotRegistered) {
+  StubOffscreenCanvasSurfaceClient surface_client;
+  provider()->CreateOffscreenCanvasSurface(kFrameSinkA, kFrameSinkB,
+                                           surface_client.GetInterfacePtr());
 
-  // The connection for |compositor_frame_sink| will have failed and triggered a
-  // connection error.
-  EXPECT_TRUE(connection_error);
+  viz::mojom::CompositorFrameSinkPtr compositor_frame_sink;
+  viz::MockCompositorFrameSinkClient compositor_frame_sink_client;
+  // The embedder, kFrameSinkA, has already been invalidated and isn't
+  // registered at this point. This request should fail.
+  provider()->CreateCompositorFrameSink(
+      kFrameSinkB, compositor_frame_sink_client.BindInterfacePtr(),
+      mojo::MakeRequest(&compositor_frame_sink));
+
+  // The request will fail and trigger a connection error.
+  WaitForConnectionError(&compositor_frame_sink);
 }
 
 // Check that trying to create an OffscreenCanvasSurfaceImpl with a client id

content/browser/renderer_host/offscreen_canvas_surface_impl.cc

@@ -46,11 +46,18 @@ void OffscreenCanvasSurfaceImpl::CreateCompositorFrameSink(
     return;
   }
 
+  // The request to create an embedded surface and the lifetime of the parent
+  // are controlled by different IPC channels. It's possible the parent
+  // FrameSinkId has been invalidated by the time this request has arrived. In
+  // that case, drop the request since there is no embedder.
+  if (!host_frame_sink_manager_->RegisterFrameSinkHierarchy(
+          parent_frame_sink_id_, frame_sink_id_)) {
+    return;
+  }
+
   host_frame_sink_manager_->CreateCompositorFrameSink(
       frame_sink_id_, std::move(request), std::move(client));
 
-  host_frame_sink_manager_->RegisterFrameSinkHierarchy(parent_frame_sink_id_,
-                                                       frame_sink_id_);
   has_created_compositor_frame_sink_ = true;
 }