Surface synchronization: Assign temporary ref owner sooner

This CL plumbs an OnSurfaceCreated over from Surface to SurfaceManager
to FrameSinkManagerImpl to HostFrameSinkManager so that the host can
assign an owner to the temporary reference. Previously we waited until
activation to assign an owner to the temporary reference but that had
the effect of taking longer for a newer fallback surface to be picked
by SurfaceManager.

This CL also simplifies GetLatestInFlightSurface by removing the parent
FrameSinkId parameter. Instead, the parent FrameSinkId is extracted from
the surface hierarchy already stored in SurfaceManager.

Bug: 672962
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I012f5feda5e3172978cc3686694362e4d85f5832
Reviewed-on: https://chromium-review.googlesource.com/820162Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523589}

components/viz/common/surfaces/frame_sink_id.cc

@@ -9,7 +9,7 @@
 namespace viz {
 
 std::string FrameSinkId::ToString() const {
-  return base::StringPrintf("FrameSinkId(%d, %d)", client_id_, sink_id_);
+  return base::StringPrintf("FrameSinkId(%u, %u)", client_id_, sink_id_);
 }
 
 std::ostream& operator<<(std::ostream& out, const FrameSinkId& frame_sink_id) {

components/viz/host/host_frame_sink_manager.cc

@@ -310,12 +310,13 @@ void HostFrameSinkManager::RegisterAfterConnectionLoss() {
   }
 }
 
+void HostFrameSinkManager::OnSurfaceCreated(const SurfaceId& surface_id) {
+  if (assign_temporary_references_)
+    PerformAssignTemporaryReference(surface_id);
+}
+
 void HostFrameSinkManager::OnFirstSurfaceActivation(
     const SurfaceInfo& surface_info) {
-  // TODO(kylechar): This needs to happen when the surface is created, not when
-  // it first activates.
-  if (assign_temporary_references_)
-    PerformAssignTemporaryReference(surface_info.id());
 
   auto it = frame_sink_data_map_.find(surface_info.id().frame_sink_id());
 

components/viz/host/host_frame_sink_manager.h

@@ -194,6 +194,7 @@ class VIZ_HOST_EXPORT HostFrameSinkManager
   void RegisterAfterConnectionLoss();
 
   // mojom::FrameSinkManagerClient:
+  void OnSurfaceCreated(const SurfaceId& surface_id) override;
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override;
   void OnClientConnectionClosed(const FrameSinkId& frame_sink_id) override;
   void OnAggregatedHitTestRegionListUpdated(

components/viz/host/host_frame_sink_manager_unittest.cc

@@ -45,11 +45,6 @@ SurfaceId MakeSurfaceId(const FrameSinkId& frame_sink_id, uint32_t parent_id) {
       LocalSurfaceId(parent_id, base::UnguessableToken::Deserialize(0, 1u)));
 }
 
-// Makes a SurfaceInfo with a default device_scale_factor and size.
-SurfaceInfo MakeSurfaceInfo(const SurfaceId& surface_id) {
-  return SurfaceInfo(surface_id, 1.f, gfx::Size(1, 1));
-}
-
 // A mock implementation of mojom::FrameSinkManager.
 class MockFrameSinkManagerImpl : public FrameSinkManagerImpl {
  public:
@@ -333,11 +328,10 @@ TEST_F(HostFrameSinkManagerLocalTest, AssignTemporaryReference) {
   host().RegisterFrameSinkHierarchy(kFrameSinkParent1,
                                     surface_id.frame_sink_id());
 
-  // When HostFrameSinkManager gets OnFirstSurfaceActivation() it should assign
+  // When HostFrameSinkManager gets OnSurfaceCreated() it should assign
   // the temporary reference to the registered parent |kFrameSinkParent1|.
   EXPECT_CALL(impl(), AssignTemporaryReference(surface_id, kFrameSinkParent1));
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(surface_id);
 }
 
 // Verify that we drop temporary reference to a surface that doesn't have any
@@ -350,11 +344,10 @@ TEST_F(HostFrameSinkManagerLocalTest, DropTemporaryReference) {
   auto support = CreateCompositorFrameSinkSupport(surface_id.frame_sink_id(),
                                                   false /* is_root */);
 
-  // When HostFrameSinkManager gets OnFirstSurfaceActivation() it should find no
+  // When HostFrameSinkManager gets OnSurfaceCreated() it should find no
   // registered parent and drop the temporary reference.
   EXPECT_CALL(impl(), DropTemporaryReference(surface_id));
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(surface_id);
 }
 
 // Verify that we drop the temporary reference to a new surface if the frame
@@ -383,8 +376,7 @@ TEST_F(HostFrameSinkManagerLocalTest, DropTemporaryReferenceForStaleClient) {
   EXPECT_CALL(impl(),
               AssignTemporaryReference(client_surface_id, kFrameSinkParent1))
       .Times(1);
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(client_surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(client_surface_id);
   testing::Mock::VerifyAndClearExpectations(&impl());
 
   // Invaidating the child should cause the temporary reference to the next
@@ -395,8 +387,7 @@ TEST_F(HostFrameSinkManagerLocalTest, DropTemporaryReferenceForStaleClient) {
   const SurfaceId client_surface_id2 = MakeSurfaceId(kFrameSinkChild1, 2);
   EXPECT_CALL(impl(), DropTemporaryReference(client_surface_id2)).Times(1);
   EXPECT_CALL(impl(), AssignTemporaryReference(client_surface_id2, _)).Times(0);
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(client_surface_id2));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(client_surface_id2);
 
   support_parent.reset();
   host().InvalidateFrameSinkId(kFrameSinkParent1);
@@ -437,8 +428,7 @@ TEST_F(HostFrameSinkManagerLocalTest, HierarchyMultipleParents) {
   // reference.
   const SurfaceId surface_id = MakeSurfaceId(id_child, 1);
   EXPECT_CALL(impl(), AssignTemporaryReference(surface_id, id_parent1));
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(surface_id);
   testing::Mock::VerifyAndClearExpectations(&impl());
 
   // Unregistering hierarchy with multiple parents should also work.
@@ -476,8 +466,7 @@ TEST_F(HostFrameSinkManagerLocalTest,
   EXPECT_CALL(impl(),
               AssignTemporaryReference(client_surface_id, kFrameSinkParent1))
       .Times(1);
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(client_surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(client_surface_id);
   testing::Mock::VerifyAndClearExpectations(&impl());
 
   // Invaidating the parent should cause the next SurfaceId to be dropped
@@ -488,8 +477,7 @@ TEST_F(HostFrameSinkManagerLocalTest,
   const SurfaceId client_surface_id2 = MakeSurfaceId(kFrameSinkChild1, 2);
   EXPECT_CALL(impl(), DropTemporaryReference(client_surface_id2)).Times(1);
   EXPECT_CALL(impl(), AssignTemporaryReference(client_surface_id2, _)).Times(0);
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(client_surface_id2));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(client_surface_id2);
 
   support_child.reset();
   host().InvalidateFrameSinkId(kFrameSinkChild1);
@@ -503,12 +491,11 @@ TEST_F(HostFrameSinkManagerLocalTest, DisplayRootTemporaryReference) {
   auto support = CreateCompositorFrameSinkSupport(surface_id.frame_sink_id(),
                                                   true /* is_root */);
 
-  // When HostFrameSinkManager gets OnFirstSurfaceActivation() it should do
+  // When HostFrameSinkManager gets OnSurfaceCreated() it should do
   // nothing since |kFrameSinkParent1| is a display root.
   EXPECT_CALL(impl(), DropTemporaryReference(surface_id)).Times(0);
   EXPECT_CALL(impl(), AssignTemporaryReference(surface_id, _)).Times(0);
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(surface_id);
 }
 
 // Test the creation and desctruction of HitTestQuery, which is stored in
@@ -645,10 +632,9 @@ TEST_F(HostFrameSinkManagerRemoteTest, AssignTemporaryReference) {
   host().RegisterFrameSinkHierarchy(kFrameSinkParent1,
                                     surface_id.frame_sink_id());
 
-  // When HostFrameSinkManager gets OnFirstSurfaceActivation() it should assign
+  // When HostFrameSinkManager gets OnSuraceCreated() it should assign
   // the temporary reference to the registered parent |kFrameSinkParent1|.
-  GetFrameSinkManagerClient()->OnFirstSurfaceActivation(
-      MakeSurfaceInfo(surface_id));
+  GetFrameSinkManagerClient()->OnSurfaceCreated(surface_id);
 
   base::RunLoop run_loop;
   EXPECT_CALL(impl(), AssignTemporaryReference(surface_id, kFrameSinkParent1))

components/viz/service/display/display_scheduler.cc

@@ -295,6 +295,8 @@ void DisplayScheduler::OnBeginFrameSourcePausedChanged(bool paused) {
     NOTIMPLEMENTED();
 }
 
+void DisplayScheduler::OnSurfaceCreated(const SurfaceId& surface_id) {}
+
 void DisplayScheduler::OnFirstSurfaceActivation(
     const SurfaceInfo& surface_info) {}
 

components/viz/service/display/display_scheduler.h

@@ -69,6 +69,7 @@ class VIZ_SERVICE_EXPORT DisplayScheduler : public BeginFrameObserverBase,
   void OnBeginFrameSourcePausedChanged(bool paused) override;
 
   // SurfaceObserver implementation.
+  void OnSurfaceCreated(const SurfaceId& surface_id) override;
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override;
   void OnSurfaceActivated(const SurfaceId& surface_id) override;
   void OnSurfaceDestroyed(const SurfaceId& surface_id) override;

components/viz/service/display/surface_aggregator.cc

@@ -187,7 +187,6 @@ void SurfaceAggregator::UnrefResources(
 void SurfaceAggregator::HandleSurfaceQuad(
     const SurfaceDrawQuad* surface_quad,
     float parent_device_scale_factor,
-    const FrameSinkId& parent_frame_sink_id,
     const gfx::Transform& target_transform,
     const ClipData& clip_rect,
     RenderPass* dest_pass,
@@ -216,8 +215,7 @@ void SurfaceAggregator::HandleSurfaceQuad(
   }
 
   Surface* fallback_surface = manager_->GetLatestInFlightSurface(
-      parent_frame_sink_id, primary_surface_id,
-      *surface_quad->fallback_surface_id);
+      primary_surface_id, *surface_quad->fallback_surface_id);
 
   // If the fallback is specified and missing then that's an error. Report the
   // error to console, and log the UMA.
@@ -366,7 +364,6 @@ void SurfaceAggregator::EmitSurfaceContent(
         dest_pass->transform_to_root_target);
 
     CopyQuadsToPass(source.quad_list, source.shared_quad_state_list,
-                    surface->surface_id().frame_sink_id(),
                     surface->GetActiveFrame().device_scale_factor(),
                     child_to_parent_map, gfx::Transform(), ClipData(),
                     copy_pass.get(), surface_id);
@@ -421,7 +418,6 @@ void SurfaceAggregator::EmitSurfaceContent(
         CalculateClipRect(clip_rect, surface_quad_clip_rect, target_transform);
 
     CopyQuadsToPass(quads, last_pass.shared_quad_state_list,
-                    surface->surface_id().frame_sink_id(),
                     surface->GetActiveFrame().device_scale_factor(),
                     child_to_parent_map, surface_transform, quads_clip,
                     dest_pass, surface_id);
@@ -633,7 +629,6 @@ SharedQuadState* SurfaceAggregator::CopyAndScaleSharedQuadState(
 void SurfaceAggregator::CopyQuadsToPass(
     const QuadList& source_quad_list,
     const SharedQuadStateList& source_shared_quad_state_list,
-    const FrameSinkId& parent_frame_sink_id,
     float parent_device_scale_factor,
     const std::unordered_map<ResourceId, ResourceId>& child_to_parent_map,
     const gfx::Transform& target_transform,
@@ -677,8 +672,8 @@ void SurfaceAggregator::CopyQuadsToPass(
         continue;
 
       HandleSurfaceQuad(surface_quad, parent_device_scale_factor,
-                        parent_frame_sink_id, target_transform, clip_rect,
-                        dest_pass, ignore_undamaged, &damage_rect_in_quad_space,
+                        target_transform, clip_rect, dest_pass,
+                        ignore_undamaged, &damage_rect_in_quad_space,
                         &damage_rect_in_quad_space_valid);
     } else {
       if (quad->shared_quad_state != last_copied_source_shared_quad_state) {
@@ -784,7 +779,6 @@ void SurfaceAggregator::CopyPasses(const CompositorFrame& frame,
         source.has_damage_from_contributing_content, source.generate_mipmap);
 
     CopyQuadsToPass(source.quad_list, source.shared_quad_state_list,
-                    surface->surface_id().frame_sink_id(),
                     frame.device_scale_factor(), child_to_parent_map,
                     gfx::Transform(), ClipData(), copy_pass.get(),
                     surface->surface_id());
@@ -1002,8 +996,7 @@ gfx::Rect SurfaceAggregator::PrewalkTree(Surface* surface,
         // TODO(fsamuel): Consider caching this value somewhere so that
         // HandleSurfaceQuad doesn't need to call it again.
         Surface* fallback_surface = manager_->GetLatestInFlightSurface(
-            surface->surface_id().frame_sink_id(), surface_info.primary_id,
-            *surface_info.fallback_id);
+            surface_info.primary_id, *surface_info.fallback_id);
         if (fallback_surface && fallback_surface->HasActiveFrame())
           child_surface = fallback_surface;
       }

components/viz/service/display/surface_aggregator.h

@@ -109,7 +109,6 @@ class VIZ_SERVICE_EXPORT SurfaceAggregator {
 
   void HandleSurfaceQuad(const SurfaceDrawQuad* surface_quad,
                          float parent_device_scale_factor,
-                         const FrameSinkId& parent_frame_sink_id,
                          const gfx::Transform& target_transform,
                          const ClipData& clip_rect,
                          RenderPass* dest_pass,
@@ -164,7 +163,6 @@ class VIZ_SERVICE_EXPORT SurfaceAggregator {
   void CopyQuadsToPass(
       const QuadList& source_quad_list,
       const SharedQuadStateList& source_shared_quad_state_list,
-      const FrameSinkId& parent_frame_sink_id,
       float parent_device_scale_factor,
       const std::unordered_map<ResourceId, ResourceId>& resource_to_child_map,
       const gfx::Transform& target_transform,

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -69,16 +69,18 @@ class FakeFrameSinkManagerClient : public mojom::FrameSinkManagerClient {
   }
 
   // mojom::FrameSinkManagerClient:
-  void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {
-    auto iter = temporary_references_to_assign_.find(surface_info.id());
+  void OnSurfaceCreated(const SurfaceId& surface_id) override {
+    auto iter = temporary_references_to_assign_.find(surface_id);
     if (iter == temporary_references_to_assign_.end()) {
-      manager_->DropTemporaryReference(surface_info.id());
+      manager_->DropTemporaryReference(surface_id);
       return;
     }
 
-    manager_->AssignTemporaryReference(surface_info.id(), iter->second);
+    manager_->AssignTemporaryReference(surface_id, iter->second);
     temporary_references_to_assign_.erase(iter);
   }
+
+  void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
   void OnClientConnectionClosed(const FrameSinkId& frame_sink_id) override {}
   void OnAggregatedHitTestRegionListUpdated(
       const FrameSinkId& frame_sink_id,

components/viz/service/frame_sinks/frame_sink_manager_impl.cc

@@ -348,16 +348,18 @@ bool FrameSinkManagerImpl::ChildContains(
   return false;
 }
 
+void FrameSinkManagerImpl::OnSurfaceCreated(const SurfaceId& surface_id) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  if (client_)
+    client_->OnSurfaceCreated(surface_id);
+}
+
 void FrameSinkManagerImpl::OnFirstSurfaceActivation(
     const SurfaceInfo& surface_info) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_GT(surface_info.device_scale_factor(), 0.0f);
 
-  // TODO(kylechar): |client_| will try to find an owner for the temporary
-  // reference to the new surface. With surface synchronization this might not
-  // be necessary, because a surface reference might already exist and no
-  // temporary reference was created. It could be useful to let |client_| know
-  // if it should find an owner.
   if (client_)
     client_->OnFirstSurfaceActivation(surface_info);
 }

components/viz/service/frame_sinks/frame_sink_manager_impl.h

@@ -112,6 +112,7 @@ class VIZ_SERVICE_EXPORT FrameSinkManagerImpl : public SurfaceObserver,
   const HitTestManager* hit_test_manager() { return &hit_test_manager_; }
 
   // SurfaceObserver implementation.
+  void OnSurfaceCreated(const SurfaceId& surface_id) override;
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override;
   void OnSurfaceActivated(const SurfaceId& surface_id) override;
   bool OnSurfaceDamaged(const SurfaceId& surface_id,

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -140,11 +140,10 @@ class SurfaceSynchronizationTest : public testing::Test {
         surface_id);
   }
 
-  Surface* GetLatestInFlightSurface(const FrameSinkId& parent,
-                                    const SurfaceId& primary_surface_id,
+  Surface* GetLatestInFlightSurface(const SurfaceId& primary_surface_id,
                                     const SurfaceId& fallback_surface_id) {
     return frame_sink_manager().surface_manager()->GetLatestInFlightSurface(
-        parent, primary_surface_id, fallback_surface_id);
+        primary_surface_id, fallback_surface_id);
   }
 
   FakeExternalBeginFrameSource* begin_frame_source() {
@@ -1850,8 +1849,7 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
   EXPECT_TRUE(HasTemporaryReference(child_id1));
   EXPECT_THAT(GetChildReferences(parent_id), IsEmpty());
   EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(parent_id.frame_sink_id(), child_id2,
-                                     child_id1));
+            GetLatestInFlightSurface(child_id2, child_id1));
 
   parent_support().SubmitCompositorFrame(
       parent_id.local_surface_id(),
@@ -1868,8 +1866,7 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
   EXPECT_FALSE(HasTemporaryReference(child_id1));
   EXPECT_THAT(GetChildReferences(parent_id), UnorderedElementsAre(child_id1));
   EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(parent_id.frame_sink_id(), child_id2,
-                                     child_id1));
+            GetLatestInFlightSurface(child_id2, child_id1));
 
   // Submit a child CompositorFrame to a new SurfaceId and verify that
   // GetLatestInFlightSurface returns the right surface.
@@ -1884,22 +1881,19 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
   // GetLatestInFlightSurface will not return child_id2's surface because it
   // does not yet have an owner.
   EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(parent_id.frame_sink_id(), child_id2,
-                                     child_id1));
+            GetLatestInFlightSurface(child_id2, child_id1));
 
   // Now that the owner of |child_id2| is known, GetLatestInFlightSurface will
   // return it as a possible fallback.
   frame_sink_manager().surface_manager()->AssignTemporaryReference(
       child_id2, parent_id.frame_sink_id());
   EXPECT_EQ(GetSurfaceForId(child_id2),
-            GetLatestInFlightSurface(parent_id.frame_sink_id(), child_id2,
-                                     child_id1));
+            GetLatestInFlightSurface(child_id2, child_id1));
 
   // If the primary surface is old, then we shouldn't return an in-flight
   // surface that is newer than the primary.
   EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(parent_id.frame_sink_id(), child_id1,
-                                     child_id1));
+            GetLatestInFlightSurface(child_id1, child_id1));
 }
 
 // This test verifies that GetLatestInFlightSurface will return nullptr
@@ -1928,8 +1922,7 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithBogusFallback) {
   // If the fallback surface doesn't exist, then GetLatestInFlightSurface should
   // always return nullptr.
   const SurfaceId bogus_child_id = MakeSurfaceId(kChildFrameSink1, 10);
-  EXPECT_EQ(nullptr, GetLatestInFlightSurface(parent_id.frame_sink_id(),
-                                              child_id1, bogus_child_id));
+  EXPECT_EQ(nullptr, GetLatestInFlightSurface(child_id1, bogus_child_id));
 }
 
 // This test verifies that GetLatestInFlightSurface will return the fallback
@@ -1962,8 +1955,7 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceDifferentFrameSinkIds) {
   child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
                                          MakeDefaultCompositorFrame());
   EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(parent_id.frame_sink_id(), child_id2,
-                                     child_id1));
+            GetLatestInFlightSurface(child_id2, child_id1));
 }
 
 // This test verifies that if a child submits a LocalSurfaceId newer that the

components/viz/service/frame_sinks/video_detector.h

@@ -71,6 +71,7 @@ class VIZ_SERVICE_EXPORT VideoDetector : public SurfaceObserver {
   void OnVideoActivityEnded();
 
   // SurfaceObserver implementation.
+  void OnSurfaceCreated(const SurfaceId& surface_id) override {}
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
   void OnSurfaceActivated(const SurfaceId& surface_id) override {}
   void OnSurfaceDestroyed(const SurfaceId& surface_id) override {}

components/viz/service/hit_test/hit_test_manager.h

@@ -37,6 +37,7 @@ class VIZ_SERVICE_EXPORT HitTestManager : public SurfaceObserver {
       const SurfaceId& surface_id) const;
 
   // SurfaceObserver:
+  void OnSurfaceCreated(const SurfaceId& surface_id) override {}
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override {}
   void OnSurfaceActivated(const SurfaceId& surface_id) override;
   void OnSurfaceDestroyed(const SurfaceId& surface_id) override {}

components/viz/service/surfaces/surface.cc

@@ -81,11 +81,8 @@ void Surface::UnrefResources(const std::vector<ReturnedResource>& resources) {
 }
 
 void Surface::RejectCompositorFramesToFallbackSurfaces() {
-  std::vector<FrameSinkId> frame_sink_ids_for_dependencies;
-  for (const SurfaceId& surface_id :
-       GetPendingFrame().metadata.activation_dependencies) {
-    frame_sink_ids_for_dependencies.push_back(surface_id.frame_sink_id());
-  }
+  const std::vector<SurfaceId>& activation_dependencies =
+      GetPendingFrame().metadata.activation_dependencies;
 
   for (const SurfaceId& surface_id :
        GetPendingFrame().metadata.referenced_surfaces) {
@@ -93,19 +90,23 @@ void Surface::RejectCompositorFramesToFallbackSurfaces() {
     // ID in |activation_dependencies| with the same frame sink ID is said to
     // be a fallback surface that can be used in place of the primary surface
     // if the deadline passes before the dependency becomes available.
-    auto it = std::find(frame_sink_ids_for_dependencies.begin(),
-                        frame_sink_ids_for_dependencies.end(),
-                        surface_id.frame_sink_id());
-    bool is_fallback_surface = it != frame_sink_ids_for_dependencies.end();
+    auto it = std::find_if(
+        activation_dependencies.begin(), activation_dependencies.end(),
+        [surface_id](const SurfaceId& dependency) {
+          return dependency.frame_sink_id() == surface_id.frame_sink_id();
+        });
+    bool is_fallback_surface = it != activation_dependencies.end();
     if (!is_fallback_surface)
       continue;
 
-    Surface* surface = surface_manager_->GetSurfaceForId(surface_id);
+    Surface* fallback_surface =
+        surface_manager_->GetLatestInFlightSurface(*it, surface_id);
+
     // A misbehaving client may report a non-existent surface ID as a
     // |referenced_surface|. In that case, |surface| would be nullptr, and
     // there is nothing to do here.
-    if (surface)
-      surface->Close();
+    if (fallback_surface)
+      fallback_surface->Close();
   }
 }
 

components/viz/service/surfaces/surface_manager.cc

@@ -112,6 +112,8 @@ Surface* SurfaceManager::CreateSurface(
       // real reference is received, is added to prevent this from happening.
       AddTemporaryReference(surface_info.id());
     }
+    for (auto& observer : observer_list_)
+      observer.OnSurfaceCreated(surface_info.id());
     return surface_map_[surface_info.id()].get();
   }
 
@@ -464,7 +466,6 @@ void SurfaceManager::RemoveTemporaryReference(const SurfaceId& surface_id,
 }
 
 Surface* SurfaceManager::GetLatestInFlightSurface(
-    const FrameSinkId& parent,
     const SurfaceId& primary_surface_id,
     const SurfaceId& fallback_surface_id) {
   if (!using_surface_references())
@@ -488,6 +489,9 @@ Surface* SurfaceManager::GetLatestInFlightSurface(
   if (it == temporary_reference_ranges_.end())
     return fallback_surface;
 
+  const base::flat_set<SurfaceId>& fallback_parents =
+      GetSurfacesThatReferenceChild(fallback_surface_id);
+
   const std::vector<LocalSurfaceId>& temp_surfaces = it->second;
   for (const LocalSurfaceId& local_surface_id : base::Reversed(temp_surfaces)) {
     // The in-flight surface cannot be newer than the primary surface ID.
@@ -496,9 +500,12 @@ Surface* SurfaceManager::GetLatestInFlightSurface(
       continue;
     }
 
-    // |parent| must own this temporary reference.
     SurfaceId surface_id(fallback_surface_id.frame_sink_id(), local_surface_id);
-    if (parent != temporary_references_[surface_id].owner)
+    base::Optional<FrameSinkId> owner = temporary_references_[surface_id].owner;
+    // Determine if the owner of this temporary reference matches one of the
+    // parents of the fallback surface. Typically a surface has a single parent
+    // so this operation should be cheap.
+    if (!IsOwnerAmongFallbackParents(fallback_parents, owner))
       continue;
 
     Surface* surface = GetSurfaceForId(surface_id);
@@ -656,6 +663,18 @@ bool SurfaceManager::IsMarkedForDestruction(const SurfaceId& surface_id) {
   return surfaces_to_destroy_.count(surface_id) != 0;
 }
 
+bool SurfaceManager::IsOwnerAmongFallbackParents(
+    const base::flat_set<SurfaceId>& fallback_parents,
+    const base::Optional<FrameSinkId>& owner) const {
+  if (!owner)
+    return false;
+
+  return std::find_if(fallback_parents.begin(), fallback_parents.end(),
+                      [owner](const SurfaceId& parent) {
+                        return parent.frame_sink_id() == owner;
+                      }) != fallback_parents.end();
+}
+
 void SurfaceManager::SurfaceWillBeDrawn(Surface* surface) {
   for (auto& observer : observer_list_)
     observer.OnSurfaceWillBeDrawn(surface);

components/viz/service/surfaces/surface_manager.h

@@ -199,13 +199,12 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
 
   // Returns the most recent surface associated with the |fallback_surface_id|'s
   // FrameSinkId that was created prior to the current primary surface and
-  // verified by the viz host to be owned by |parent|. If the FrameSinkId of the
-  // |primary_surface_id| does not match the |fallback_surface_id|'s then this
-  // method will always return the fallback surface because we cannot guarantee
-  // the latest in flight surface from the fallback frame sink is older than the
-  // primary surface.
-  Surface* GetLatestInFlightSurface(const FrameSinkId& parent,
-                                    const SurfaceId& primary_surface_id,
+  // verified by the viz host to be owned by the fallback surface's parent. If
+  // the FrameSinkId of the |primary_surface_id| does not match the
+  // |fallback_surface_id|'s then this method will always return the fallback
+  // surface because we cannot guarantee the latest in flight surface from the
+  // fallback frame sink is older than the primary surface.
+  Surface* GetLatestInFlightSurface(const SurfaceId& primary_surface_id,
                                     const SurfaceId& fallback_surface_id);
 
   // Called by SurfaceAggregator notifying us that it will use |surface| in the
@@ -288,6 +287,12 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   // Returns true if |surface_id| is in the garbage collector's queue.
   bool IsMarkedForDestruction(const SurfaceId& surface_id);
 
+  // Determines if the provided |owner| FrameSinkId matches the FrameSinkId of
+  // a surface in the set of |fallback_parents|.
+  bool IsOwnerAmongFallbackParents(
+      const base::flat_set<SurfaceId>& fallback_parents,
+      const base::Optional<FrameSinkId>& owner) const;
+
   // Use reference or sequence based lifetime management.
   LifetimeType lifetime_type_;
 

components/viz/service/surfaces/surface_observer.h

@@ -15,6 +15,9 @@ struct BeginFrameArgs;
 
 class SurfaceObserver {
  public:
+  // Called when a Surface with a new SurfaceId is created.
+  virtual void OnSurfaceCreated(const SurfaceId& surface_id) = 0;
+
   // Called when a CompositorFrame with a new SurfaceId activates for the first
   // time.
   virtual void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) = 0;

components/viz/test/fake_surface_observer.h

@@ -36,6 +36,7 @@ class FakeSurfaceObserver : public SurfaceObserver {
 
  private:
   // SurfaceObserver implementation:
+  void OnSurfaceCreated(const SurfaceId& surface_id) override {}
   bool OnSurfaceDamaged(const SurfaceId& surface_id,
                         const BeginFrameAck& ack) override;
   void OnFirstSurfaceActivation(const SurfaceInfo& surface_info) override;

services/viz/privileged/interfaces/compositing/frame_sink_manager.mojom

@@ -97,6 +97,9 @@ interface FrameSinkManager {
 // compositor. The frame sink manager host is either the browser process in
 // Chrome or the window server process.
 interface FrameSinkManagerClient {
+  // Called by the frame sink manager when a new Surface is created.
+  OnSurfaceCreated(SurfaceId surface_id);
+
   // Called by the frame sink manager when a CompositorFrame with a new
   // SurfaceId activates for the first time.
   OnFirstSurfaceActivation(SurfaceInfo surface_info);