Avoid Duplicate Surface Destruction in GarbageCollectSurfaces

Destroying surfaces has the potential to add other surfaces to
the |surfaces_to_destroy_| set. This can cause problems when
a surface is pending destruction (already pulled out of
|surfaces_to_destroy_|) and is then re-added to the list,
leading to a double deletion.

This change makes deletion robust against this pattern, in
order to have a merge-safe fix.

A future change will make surface destruction
idempotent, so we don't need to worry about these cases.

Bug: 934674
Change-Id: Ie08505ef37d4a6e2ca48462372e3e09d3d49bc58
Reviewed-on: https://chromium-review.googlesource.com/c/1484354
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634988}

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -3171,4 +3171,46 @@ TEST_F(SurfaceSynchronizationTest, ChildNotThrottledWhenParentBlocked2) {
   EXPECT_FALSE(child_surface6->HasActiveFrame());
 }
 
+// Tests that in cases where a pending-deletion surface (surface A) is
+// activated during anothother surface (surface B)'s deletion, we don't attempt
+// to delete surface A twice.
+TEST_F(SurfaceSynchronizationTest, SurfaceActivationDuringDeletion) {
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1, 1);
+  // Child-initiated synchronization event:
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 1, 2);
+
+  // Submit a CompositorFrame to |child_id1|.
+  child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Child 1 should not yet be active.
+  Surface* child_surface1 = GetSurfaceForId(child_id1);
+  ASSERT_NE(nullptr, child_surface1);
+  EXPECT_FALSE(child_surface1->HasPendingFrame());
+  EXPECT_TRUE(child_surface1->HasActiveFrame());
+
+  // Submit a CompositorFrame to |child_id2|.
+  child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Child 2 should not yet be active.
+  Surface* child_surface2 = GetSurfaceForId(child_id2);
+  ASSERT_NE(nullptr, child_surface2);
+  EXPECT_TRUE(child_surface2->HasPendingFrame());
+  EXPECT_FALSE(child_surface2->HasActiveFrame());
+
+  // Evict |child_id2|. both surfaces should be marked for deletion.
+  child_support1().EvictSurface(child_id1.local_surface_id());
+  EXPECT_TRUE(IsMarkedForDestruction(child_id1));
+  EXPECT_TRUE(IsMarkedForDestruction(child_id2));
+
+  // Garbage collect to delete the above surfaces. This will activate
+  // |child_id2|, which will cause it to attempt re-deletion.
+  ExpireAllTemporaryReferencesAndGarbageCollect();
+
+  // Neither should still be marked for deletion.
+  EXPECT_FALSE(IsMarkedForDestruction(child_id1));
+  EXPECT_FALSE(IsMarkedForDestruction(child_id2));
+}
+
 }  // namespace viz

components/viz/service/surfaces/surface_manager.cc

@@ -206,6 +206,17 @@ void SurfaceManager::GarbageCollectSurfaces() {
   // ~Surface() draw callback could modify |surfaces_to_destroy_|.
   for (const SurfaceId& surface_id : surfaces_to_delete)
     DestroySurfaceInternal(surface_id);
+
+  // Run another pass over surfaces_to_delete, all of which have just been
+  // deleted, making sure they are not present in |surfaces_to_destroy_|. This
+  // is necessary as ~Surface may re-add already-in-destruction surfaces to the
+  // set and we need to avoid double-deletion.
+  // TODO(ericrk): Removing surfaces both here and above allows for
+  // GarbageCollectSurfaces re-entrancy, which is exercised in tests and is
+  // hard to prove can't happen in the wild. Evaluate whether we should allow
+  // re-entrancy, and if not just remove here.
+  for (const SurfaceId& surface_id : surfaces_to_delete)
+    surfaces_to_destroy_.erase(surface_id);
 }
 
 const base::flat_set<SurfaceId>& SurfaceManager::GetSurfacesReferencedByParent(