Fix frame rejection and eviction when parent sequence number is reset

When embed token is reset, we should not enforce that sequence numbers
monotonically increase. Also, any call to EvictSurface should only
affect surfaces with the same embed token as the evicted id.

Bug: 931801
Change-Id: I3cc1ecc80197b02fe0a3c5cffed4988cfd8e5d0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1570247Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#651735}

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -118,10 +118,13 @@ void CompositorFrameSinkSupport::OnSurfaceActivated(Surface* surface) {
   if (!last_activated_surface_id_.is_valid() ||
       local_surface_id > last_activated_local_surface_id) {
     if (last_activated_surface_id_.is_valid()) {
-      CHECK_GE(local_surface_id.parent_sequence_number(),
-               last_activated_local_surface_id.parent_sequence_number());
-      CHECK_GE(local_surface_id.child_sequence_number(),
-               last_activated_local_surface_id.child_sequence_number());
+      if (last_activated_local_surface_id.embed_token() ==
+          local_surface_id.embed_token()) {
+        DCHECK_GE(local_surface_id.parent_sequence_number(),
+                  last_activated_local_surface_id.parent_sequence_number());
+        DCHECK_GE(local_surface_id.child_sequence_number(),
+                  last_activated_local_surface_id.child_sequence_number());
+      }
 
       Surface* prev_surface =
           surface_manager_->GetSurfaceForId(last_activated_surface_id_);
@@ -237,21 +240,18 @@ CompositorFrameSinkSupport::TakeCopyOutputRequests(
 }
 
 void CompositorFrameSinkSupport::EvictSurface(const LocalSurfaceId& id) {
-  DCHECK_GE(id.parent_sequence_number(), last_evicted_parent_sequence_number_);
-  last_evicted_parent_sequence_number_ = id.parent_sequence_number();
+  DCHECK(id.embed_token() != last_evicted_local_surface_id_.embed_token() ||
+         id.parent_sequence_number() >=
+             last_evicted_local_surface_id_.parent_sequence_number());
+  last_evicted_local_surface_id_ = id;
   surface_manager_->DropTemporaryReference(SurfaceId(frame_sink_id_, id));
   MaybeEvictSurfaces();
 }
 
 void CompositorFrameSinkSupport::MaybeEvictSurfaces() {
-  if (last_activated_surface_id_.is_valid() &&
-      last_activated_surface_id_.local_surface_id().parent_sequence_number() <=
-          last_evicted_parent_sequence_number_) {
+  if (IsEvicted(last_activated_surface_id_.local_surface_id()))
     EvictLastActiveSurface();
-  }
-  if (last_created_surface_id_.is_valid() &&
-      last_created_surface_id_.local_surface_id().parent_sequence_number() <=
-          last_evicted_parent_sequence_number_) {
+  if (IsEvicted(last_created_surface_id_.local_surface_id())) {
     surface_manager_->MarkSurfaceForDestruction(last_created_surface_id_);
     last_created_surface_id_ = SurfaceId();
   }
@@ -460,7 +460,9 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrameInternal(
          child_initiated_synchronization_event);
 
     DCHECK(surface_info.is_valid());
-    if (!monotonically_increasing_id) {
+    if (local_surface_id.embed_token() ==
+            last_created_local_surface_id.embed_token() &&
+        !monotonically_increasing_id) {
       TRACE_EVENT_INSTANT0("viz", "LocalSurfaceId decreased",
                            TRACE_EVENT_SCOPE_THREAD);
       return SubmitResult::SURFACE_ID_DECREASED;
@@ -476,8 +478,7 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrameInternal(
 
     // Don't recreate a surface that was previously evicted. Drop the
     // CompositorFrame and return all its resources.
-    if (local_surface_id.parent_sequence_number() <=
-        last_evicted_parent_sequence_number_) {
+    if (IsEvicted(local_surface_id)) {
       TRACE_EVENT_INSTANT0("viz", "Submit rejected to evicted surface",
                            TRACE_EVENT_SCOPE_THREAD);
       return SubmitResult::ACCEPTED;
@@ -801,4 +802,12 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
   return (frame_time - last_frame_time_) >= throttled_rate;
 }
 
+bool CompositorFrameSinkSupport::IsEvicted(
+    const LocalSurfaceId& local_surface_id) const {
+  return local_surface_id.embed_token() ==
+             last_evicted_local_surface_id_.embed_token() &&
+         local_surface_id.parent_sequence_number() <=
+             last_evicted_local_surface_id_.parent_sequence_number();
+}
+
 }  // namespace viz

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -231,6 +231,8 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   void EvictLastActiveSurface();
   bool ShouldSendBeginFrame(base::TimeTicks timestamp);
 
+  bool IsEvicted(const LocalSurfaceId& local_surface_id) const;
+
   mojom::CompositorFrameSinkClient* const client_;
 
   FrameSinkManagerImpl* const frame_sink_manager_;
@@ -311,7 +313,7 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   uint32_t trace_sequence_ = 0;
 
   PresentationFeedbackMap presentation_feedbacks_;
-  uint32_t last_evicted_parent_sequence_number_ = 0;
+  LocalSurfaceId last_evicted_local_surface_id_;
 
   base::TimeTicks last_frame_time_;
 

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -1213,4 +1213,52 @@ TEST_F(CompositorFrameSinkSupportTest,
   EXPECT_EQ(SubmitResult::SURFACE_OWNED_BY_ANOTHER_CLIENT, result);
 }
 
+// This test verifies that the parent sequence number of the submitted
+// CompositorFrames can decrease as long as the embed token changes as well.
+TEST_F(CompositorFrameSinkSupportTest, SubmitAfterReparenting) {
+  LocalSurfaceId local_surface_id1(2, base::UnguessableToken::Create());
+  LocalSurfaceId local_surface_id2(1, base::UnguessableToken::Create());
+
+  ASSERT_NE(local_surface_id1.embed_token(), local_surface_id2.embed_token());
+
+  CompositorFrame frame =
+      CompositorFrameBuilder().AddDefaultRenderPass().Build();
+  SubmitResult result = support_->MaybeSubmitCompositorFrame(
+      local_surface_id1, std::move(frame), base::nullopt, 0,
+      mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
+  EXPECT_EQ(SubmitResult::ACCEPTED, result);
+
+  frame = CompositorFrameBuilder().AddDefaultRenderPass().Build();
+  result = support_->MaybeSubmitCompositorFrame(
+      local_surface_id2, std::move(frame), base::nullopt, 0,
+      mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
+
+  // Even though |local_surface_id2| has a smaller parent sequence number than
+  // |local_surface_id1|, the submit should still succeed because it has a
+  // different embed token.
+  EXPECT_EQ(SubmitResult::ACCEPTED, result);
+}
+
+// This test verifies that surfaces created with a new embed token are not
+// compared against the evicted parent sequence number of the previous embed
+// token.
+TEST_F(CompositorFrameSinkSupportTest, EvictThenReparent) {
+  LocalSurfaceId local_surface_id1(2, base::UnguessableToken::Create());
+  LocalSurfaceId local_surface_id2(1, base::UnguessableToken::Create());
+
+  ASSERT_NE(local_surface_id1.embed_token(), local_surface_id2.embed_token());
+
+  support_->EvictSurface(local_surface_id1);
+  CompositorFrame frame =
+      CompositorFrameBuilder().AddDefaultRenderPass().Build();
+  support_->SubmitCompositorFrame(local_surface_id2, std::move(frame));
+  manager_.surface_manager()->GarbageCollectSurfaces();
+
+  // Even though |local_surface_id2| has a smaller parent sequence number than
+  // |local_surface_id1|, it should not be evicted because it has a different
+  // embed token.
+  EXPECT_TRUE(
+      GetSurfaceForId(SurfaceId(support_->frame_sink_id(), local_surface_id2)));
+}
+
 }  // namespace viz