RELAND Surface sync: Late arriving CF should not activate if dependent CF dropped

A late arriving CompositorFrame should not activate if the dependent CF has
been dropped. This can happen if a CompositorFrame is marked for destruction
and then pulled out of the destruction queue ("surface resurrection").

This CL ensures that when a surface is reset, a SurfaceDiscarded notification
is sent to the SurfaceDependencyTracker followed by SurfaceCreated to mimic
creating a new surface.

Bug: 796602
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I20d1e96526db6464f9a095d59ea9633f81ddd615
TBR: samans@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/848633
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526692}

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -1386,6 +1386,53 @@ TEST_F(SurfaceSynchronizationTest, MultiLevelLateArrivingDependency) {
   EXPECT_FALSE(child_surface1()->has_deadline());
 }
 
+// This test verifies that a late arriving CompositorFrame does not activate
+// if its dependent CompositorFrame has been dropped in the intervening time.
+// This test also verifies that there is no crash when the dependent
+// CompositorFrame is evicted and replaced by a new pending CompositorFrame.
+TEST_F(SurfaceSynchronizationTest, MissingActiveFrameWithLateDependencies) {
+  const SurfaceId display_id = MakeSurfaceId(kDisplayFrameSink, 1);
+  const SurfaceId parent_id1 = MakeSurfaceId(kParentFrameSink, 1);
+  const SurfaceId parent_id2 = MakeSurfaceId(kParentFrameSink, 2);
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
+
+  display_support().SubmitCompositorFrame(
+      display_id.local_surface_id(),
+      MakeCompositorFrame({parent_id1}, empty_surface_ids(),
+                          std::vector<TransferableResource>()));
+
+  EXPECT_TRUE(display_surface()->HasPendingFrame());
+  EXPECT_FALSE(display_surface()->HasActiveFrame());
+  EXPECT_TRUE(display_surface()->has_deadline());
+
+  // Advance BeginFrames to trigger a deadline. This activates the
+  // CompositorFrame submitted above.
+  for (int i = 0; i < 3; ++i) {
+    SendNextBeginFrame();
+    EXPECT_TRUE(display_surface()->has_deadline());
+  }
+  SendNextBeginFrame();
+  EXPECT_FALSE(display_surface()->has_deadline());
+  EXPECT_FALSE(display_surface()->HasPendingFrame());
+  EXPECT_TRUE(display_surface()->HasActiveFrame());
+
+  display_support().EvictCurrentSurface();
+  display_support().SubmitCompositorFrame(
+      display_id.local_surface_id(),
+      MakeCompositorFrame({parent_id2}, empty_surface_ids(),
+                          std::vector<TransferableResource>()));
+
+  // A late arriving CompositorFrame will not activate immediately if the
+  // dependent CompositorFrame has been evicted.
+  parent_support().SubmitCompositorFrame(
+      parent_id1.local_surface_id(),
+      MakeCompositorFrame({child_id1}, empty_surface_ids(),
+                          std::vector<TransferableResource>()));
+  EXPECT_TRUE(parent_surface()->has_deadline());
+  EXPECT_TRUE(parent_surface()->HasPendingFrame());
+  EXPECT_FALSE(parent_surface()->HasActiveFrame());
+}
+
 // This test verifies that CompositorFrames submitted to a surface referenced
 // by a parent CompositorFrame as a fallback will be rejected and ACK'ed
 // immediately.

components/viz/service/surfaces/surface_manager.cc

@@ -56,7 +56,7 @@ SurfaceManager::SurfaceManager(LifetimeType lifetime_type,
     reference_factory_ = new StubSurfaceReferenceFactory();
     temporary_reference_timer_.Start(
         FROM_HERE, base::TimeDelta::FromSeconds(10), this,
-        &SurfaceManager::MarkOldTemporaryReference);
+        &SurfaceManager::MarkOldTemporaryReferences);
     // TODO(kylechar): After collecting UMA stats on the number of old temporary
     // references, we may want to turn the timer off when there are no temporary
     // references to avoid waking the thread unnecessarily.
@@ -67,9 +67,24 @@ SurfaceManager::SurfaceManager(LifetimeType lifetime_type,
 }
 
 SurfaceManager::~SurfaceManager() {
+  // Garbage collect surfaces here to avoid calling back into
+  // SurfaceDependencyTracker as members of SurfaceManager are being
+  // destroyed.
+  temporary_references_.clear();
+  temporary_reference_ranges_.clear();
+  // Create a copy of the children set as RemoveSurfaceReferenceImpl below will
+  // mutate that set.
+  base::flat_set<SurfaceId> children(
+      GetSurfacesReferencedByParent(root_surface_id_));
+  for (const auto& child : children)
+    RemoveSurfaceReferenceImpl(root_surface_id_, child);
+
+  GarbageCollectSurfaces();
+
   // All SurfaceClients and their surfaces are supposed to be
   // destroyed before SurfaceManager.
-  DCHECK_EQ(surfaces_to_destroy_.size(), surface_map_.size());
+  DCHECK(surface_map_.empty());
+  DCHECK(surfaces_to_destroy_.empty());
 }
 
 #if DCHECK_IS_ON()
@@ -126,7 +141,10 @@ Surface* SurfaceManager::CreateSurface(
 
   DCHECK(IsMarkedForDestruction(surface_info.id()));
   surfaces_to_destroy_.erase(surface_info.id());
+  SurfaceDiscarded(surface);
   surface->Reset(surface_client);
+  for (auto& observer : observer_list_)
+    observer.OnSurfaceCreated(surface_info.id());
   return surface;
 }
 
@@ -517,7 +535,7 @@ Surface* SurfaceManager::GetLatestInFlightSurface(
   return fallback_surface;
 }
 
-void SurfaceManager::MarkOldTemporaryReference() {
+void SurfaceManager::MarkOldTemporaryReferences() {
   if (temporary_references_.empty()) {
     UMA_HISTOGRAM_EXACT_LINEAR(kUmaNumOldTemporaryReferences, 0, kUmaStatMax);
     return;

components/viz/service/surfaces/surface_manager.h

@@ -273,7 +273,7 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   void RemoveTemporaryReference(const SurfaceId& surface_id, bool remove_range);
 
   // Marks old temporary references for logging and deletion.
-  void MarkOldTemporaryReference();
+  void MarkOldTemporaryReferences();
 
   // Removes the surface from the surface map and destroys it.
   void DestroySurfaceInternal(const SurfaceId& surface_id);