Fix surface resurrection when CompositorFrameSink recreated

A surface may stay alive after its CompositorFrameSink is thrown away.
This can happen if the surface is referenced by a parent compositor.

A new child CompositorFrameSink may be created (after context lost)
and it may reuse the same surface before it is garbage collected.
In this case, the surface refers to an invalid SurfaceClient
(CompositorFrameSinkSuppport), and crashes. This CL fixes the issue.

Bug: 791172
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I7814bf40b706c52d755fdd69accce1deee9862a0
Reviewed-on: https://chromium-review.googlesource.com/818126Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523199}

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -111,6 +111,12 @@ class SurfaceSynchronizationTest : public testing::Test {
     return child_support2().GetCurrentSurfaceForTesting();
   }
 
+  void CreateFrameSink(const FrameSinkId& frame_sink_id, bool is_root) {
+    supports_[frame_sink_id] = CompositorFrameSinkSupport::Create(
+        &support_client_, &frame_sink_manager_, frame_sink_id, is_root,
+        kNeedsSyncPoints);
+  }
+
   void DestroyFrameSink(const FrameSinkId& frame_sink_id) {
     auto it = supports_.find(frame_sink_id);
     if (it == supports_.end())
@@ -1029,6 +1035,50 @@ TEST_F(SurfaceSynchronizationTest, SurfaceResurrection) {
   EXPECT_EQ(child_id, surface_observer().last_created_surface_id());
 }
 
+// Verifies that if a surface is marked destroyed and a new frame arrives after
+// a CompositorFrameSink is destroyed and recreated then it will be recovered.
+TEST_F(SurfaceSynchronizationTest, SurfaceResurrectionAfterDestruction) {
+  const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
+  const SurfaceId child_id = MakeSurfaceId(kChildFrameSink1, 3);
+
+  // Create the child surface by submitting a frame to it.
+  EXPECT_EQ(nullptr, GetSurfaceForId(child_id));
+  child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that the child surface is created.
+  Surface* surface = GetSurfaceForId(child_id);
+  EXPECT_NE(nullptr, surface);
+
+  // Add a reference from the parent to the child.
+  parent_support().SubmitCompositorFrame(
+      parent_id.local_surface_id(),
+      MakeCompositorFrame({child_id}, {child_id},
+                          std::vector<TransferableResource>()));
+
+  // Attempt to destroy the child surface. The surface must still exist since
+  // the parent needs it but it will be marked as destroyed.
+  child_support1().EvictCurrentSurface();
+  surface = GetSurfaceForId(child_id);
+  EXPECT_NE(nullptr, surface);
+  EXPECT_TRUE(IsMarkedForDestruction(child_id));
+
+  // Child submits another frame to the same local surface id that is marked
+  // destroyed.
+  surface_observer().Reset();
+  DestroyFrameSink(child_id.frame_sink_id());
+  CreateFrameSink(child_id.frame_sink_id(), false);
+  child_support1().SubmitCompositorFrame(child_id.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that the surface that was marked destroyed is recovered and is being
+  // used again.
+  Surface* surface2 = GetSurfaceForId(child_id);
+  EXPECT_EQ(surface, surface2);
+  EXPECT_FALSE(IsMarkedForDestruction(child_id));
+  EXPECT_EQ(surface2->client().get(), &child_support1());
+  EXPECT_EQ(child_id, surface_observer().last_created_surface_id());
+}
 // Verifies that if a LocalSurfaceId belonged to a surface that doesn't
 // exist anymore, it can still be reused for new surfaces.
 TEST_F(SurfaceSynchronizationTest, LocalSurfaceIdIsReusable) {

components/viz/service/surfaces/surface.cc

@@ -41,8 +41,11 @@ Surface::~Surface() {
   deadline_.Cancel();
 }
 
-void Surface::ResetSeenFirstFrameActivation() {
+void Surface::Reset(base::WeakPtr<SurfaceClient> client) {
   seen_first_frame_activation_ = false;
+  pending_frame_data_.reset();
+  active_frame_data_.reset();
+  surface_client_ = client;
 }
 
 bool Surface::InheritActivationDeadlineFrom(

components/viz/service/surfaces/surface.h

@@ -84,10 +84,10 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
           bool needs_sync_tokens);
   ~Surface();
 
-  // Clears the |seen_first_frame_activation_| bit causing a
-  // FirstSurfaceActivation to be triggered on the next CompositorFrame
-  // activation.
-  void ResetSeenFirstFrameActivation();
+  // Clears the pending and active frame data as well as the
+  // |seen_first_frame_activation_| bit causing a FirstSurfaceActivation to be
+  // triggered on the next CompositorFrame activation.
+  void Reset(base::WeakPtr<SurfaceClient> client);
 
   const SurfaceId& surface_id() const { return surface_info_.id(); }
   const SurfaceId& previous_frame_surface_id() const {

components/viz/service/surfaces/surface_manager.cc

@@ -123,7 +123,7 @@ Surface* SurfaceManager::CreateSurface(
 
   DCHECK(IsMarkedForDestruction(surface_info.id()));
   surfaces_to_destroy_.erase(surface_info.id());
-  surface->ResetSeenFirstFrameActivation();
+  surface->Reset(surface_client);
   return surface;
 }
 

components/viz/service/surfaces/surface_unittest.cc

@@ -80,11 +80,6 @@ TEST(SurfaceTest, PresentationCallback) {
     EXPECT_CALL(client, DidDiscardCompositorFrame(4)).Times(1);
     support->SubmitCompositorFrame(local_surface_id, std::move(frame));
   }
-
-  // The frame with token 2 will be discarded when the surface is destroyed.
-  EXPECT_CALL(client, DidDiscardCompositorFrame(2)).Times(1);
-  support->EvictCurrentSurface();
-  frame_sink_manager.surface_manager()->GarbageCollectSurfaces();
 }
 
 TEST(SurfaceTest, SurfaceLifetime) {