viz: Drop temporary reference to discarded FrameSink

This CL fixes a bug where HostFrameSinkManager will not drop the temporary
reference to a FrameSink that has been invalidated from HostFrameSinkManager.

This verifies that HostFrameSinkManager knows about the FrameSinkId of
the Surface created in OnSurfaceCreated. If it doesn't then HostFrameSinkManager
asks FrameSinkManager to drop the temporary reference.

Bug: 657959
Change-Id: Ia9b8eff0885fc0b56e9d866acd41860b11a0b3ba
Reviewed-on: https://chromium-review.googlesource.com/599001Reviewed-by: Ria Jiang <riajiang@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491612}

components/viz/host/host_frame_sink_manager.cc

@@ -147,18 +147,17 @@ void HostFrameSinkManager::PerformAssignTemporaryReference(
   // Find the expected embedder for the new surface and assign the temporary
   // reference to it.
   auto iter = frame_sink_data_map_.find(surface_id.frame_sink_id());
-  if (iter != frame_sink_data_map_.end()) {
-    const FrameSinkData& data = iter->second;
-
-    // Display roots don't have temporary references to assign.
-    if (data.is_root)
-      return;
-
-    if (data.parent.has_value()) {
-      frame_sink_manager_impl_->AssignTemporaryReference(surface_id,
-                                                         data.parent.value());
-      return;
-    }
+  DCHECK(iter != frame_sink_data_map_.end());
+  const FrameSinkData& data = iter->second;
+
+  // Display roots don't have temporary references to assign.
+  if (data.is_root)
+    return;
+
+  if (data.parent.has_value()) {
+    frame_sink_manager_impl_->AssignTemporaryReference(surface_id,
+                                                       data.parent.value());
+    return;
   }
 
   // We don't have any hierarchy information for what will embed the new
@@ -169,8 +168,12 @@ void HostFrameSinkManager::PerformAssignTemporaryReference(
 void HostFrameSinkManager::OnSurfaceCreated(const SurfaceInfo& surface_info) {
   auto it = frame_sink_data_map_.find(surface_info.id().frame_sink_id());
   // If we've received a bogus or stale SurfaceId from Viz then just ignore it.
-  if (it == frame_sink_data_map_.end())
+  if (it == frame_sink_data_map_.end()) {
+    // We don't have any hierarchy information for what will embed the new
+    // surface, drop the temporary reference.
+    frame_sink_manager_->DropTemporaryReference(surface_info.id());
     return;
+  }
 
   FrameSinkData& frame_sink_data = it->second;
   if (frame_sink_data.client)

components/viz/host/host_frame_sink_manager_unittests.cc

@@ -235,6 +235,47 @@ TEST_F(HostFrameSinkManagerTest, DropTemporaryReference) {
   GetFrameSinkManagerClient()->OnSurfaceCreated(MakeSurfaceInfo(surface_id));
 }
 
+TEST_F(HostFrameSinkManagerTest, DropTemporaryReferenceForStaleClient) {
+  FakeHostFrameSinkClient client;
+  host_manager().RegisterFrameSinkId(kClientFrameSinkId, &client);
+  auto support_client =
+      CreateCompositorFrameSinkSupport(kClientFrameSinkId, false /* is_root */);
+  EXPECT_TRUE(FrameSinkDataExists(kClientFrameSinkId));
+
+  host_manager().RegisterFrameSinkId(kParentFrameSinkId, &client);
+  auto support_parent =
+      CreateCompositorFrameSinkSupport(kParentFrameSinkId, true /* is_root */);
+  EXPECT_TRUE(FrameSinkDataExists(kParentFrameSinkId));
+
+  // Register should call through to FrameSinkManagerImpl.
+  EXPECT_CALL(manager_impl(), RegisterFrameSinkHierarchy(kParentFrameSinkId,
+                                                         kClientFrameSinkId));
+  host_manager().RegisterFrameSinkHierarchy(kParentFrameSinkId,
+                                            kClientFrameSinkId);
+
+  const SurfaceId client_surface_id = MakeSurfaceId(kClientFrameSinkId, 1);
+  EXPECT_CALL(manager_impl(), DropTemporaryReference(client_surface_id))
+      .Times(0);
+  EXPECT_CALL(manager_impl(), AssignTemporaryReference(client_surface_id, _))
+      .Times(1);
+  GetFrameSinkManagerClient()->OnSurfaceCreated(
+      MakeSurfaceInfo(client_surface_id));
+  testing::Mock::VerifyAndClearExpectations(&manager_impl());
+
+  // Invaidating the client should cause the next SurfaceId to be dropped.
+  support_client.reset();
+  host_manager().InvalidateFrameSinkId(kClientFrameSinkId);
+
+  const SurfaceId client_surface_id2 = MakeSurfaceId(kClientFrameSinkId, 2);
+  EXPECT_CALL(manager_impl(), DropTemporaryReference(client_surface_id2))
+      .Times(1);
+  GetFrameSinkManagerClient()->OnSurfaceCreated(
+      MakeSurfaceInfo(client_surface_id2));
+
+  support_parent.reset();
+  host_manager().InvalidateFrameSinkId(kParentFrameSinkId);
+}
+
 TEST_F(HostFrameSinkManagerTest, DisplayRootTemporaryReference) {
   const SurfaceId surface_id = MakeSurfaceId(kParentFrameSinkId, 1);
   auto support = CreateCompositorFrameSinkSupport(surface_id.frame_sink_id(),

components/viz/service/surfaces/surface_manager.cc

@@ -194,10 +194,11 @@ void SurfaceManager::AssignTemporaryReference(const SurfaceId& surface_id,
 
 void SurfaceManager::DropTemporaryReference(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(lifetime_type_, LifetimeType::REFERENCES);
 
-  if (!HasTemporaryReference(surface_id))
+  if (lifetime_type_ != LifetimeType::REFERENCES ||
+      !HasTemporaryReference(surface_id)) {
     return;
+  }
 
   RemoveTemporaryReference(surface_id, false);
 }