[throughput] Fix a OOM crash in FrameSequenceTracker

We are seeing OOM crash in FrameSequenceTracker::ReportSubmitFrame
and it is pointing at ignored_frame_tokens_ set being too big.

The only way this can happen is that a tracker is scheduled for
termination, but the last frame took very long time to get
its presentation feedback. During this period, there are a
lot of frames getting submitted, and these frame tokens will
be pushed into ignored_frame_tokens_ set and causing a OOM crash.

This CL fixes the problem and added a unit test.

Bug: 1070209
Change-Id: I0a9c132d8f9d45a5a89f1ff763b20d9ba12ea524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2148164
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760119}

cc/metrics/frame_sequence_tracker.cc

@@ -913,18 +913,24 @@ void FrameSequenceTracker::ReportFrameEnd(
 void FrameSequenceTracker::ReportFramePresented(
     uint32_t frame_token,
     const gfx::PresentationFeedback& feedback) {
+  // TODO(xidachen): We should early exit if |last_submitted_frame_| = 0, as it
+  // means that we are presenting the same frame_token again.
+  const bool submitted_frame_since_last_presentation = !!last_submitted_frame_;
   // !viz::FrameTokenGT(a, b) is equivalent to b >= a.
   const bool frame_token_acks_last_frame =
       !viz::FrameTokenGT(last_submitted_frame_, frame_token);
 
+  // Even if the presentation timestamp is null, we set last_submitted_frame_ to
+  // 0 such that the tracker can be terminated.
+  if (last_submitted_frame_ && frame_token_acks_last_frame)
+    last_submitted_frame_ = 0;
   // Update termination status if this is scheduled for termination, and it is
   // not waiting for any frames, or it has received the presentation-feedback
   // for the latest frame it is tracking.
   //
   // We should always wait for an impl frame to end, that is, ReportFrameEnd.
   if (termination_status_ == TerminationStatus::kScheduledForTermination &&
-      (last_submitted_frame_ == 0 || frame_token_acks_last_frame) &&
-      !is_inside_frame_) {
+      last_submitted_frame_ == 0 && !is_inside_frame_) {
     termination_status_ = TerminationStatus::kReadyForTermination;
   }
 
@@ -937,26 +943,23 @@ void FrameSequenceTracker::ReportFramePresented(
 
   TRACKER_TRACE_STREAM << "P(" << frame_token << ")";
 
-  if (ignored_frame_tokens_.contains(frame_token))
-    return;
   base::EraseIf(ignored_frame_tokens_, [frame_token](const uint32_t& token) {
     return viz::FrameTokenGT(frame_token, token);
   });
+  if (ignored_frame_tokens_.contains(frame_token))
+    return;
 
   uint32_t impl_frames_produced = 0;
   uint32_t main_frames_produced = 0;
   trace_data_.Advance(feedback.timestamp);
 
   const bool was_presented = !feedback.timestamp.is_null();
-  if (was_presented && last_submitted_frame_) {
+  if (was_presented && submitted_frame_since_last_presentation) {
     DCHECK_LT(impl_throughput().frames_produced,
               impl_throughput().frames_expected)
         << TRACKER_DCHECK_MSG;
     ++impl_throughput().frames_produced;
     ++impl_frames_produced;
-
-    if (frame_token_acks_last_frame)
-      last_submitted_frame_ = 0;
   }
 
   if (was_presented) {

cc/metrics/frame_sequence_tracker_unittest.cc

@@ -1661,27 +1661,28 @@ TEST_F(FrameSequenceTrackerTest, TrackLastImplFrame24) {
 }
 
 TEST_F(FrameSequenceTrackerTest, IgnoredFrameTokensRemovedAtPresentation1) {
-  GenerateSequence("b(5)");
+  GenerateSequence("b(5)s(1)e(5,0)P(1)");
   auto args = CreateBeginFrameArgs(/*source_id=*/1u, 1u);
   // Ack to an impl frame that doesn't exist in this tracker.
-  collection_.NotifySubmitFrame(1, /*has_missing_content=*/false,
+  collection_.NotifySubmitFrame(2, /*has_missing_content=*/false,
                                 viz::BeginFrameAck(args, true), args);
   EXPECT_EQ(IgnoredFrameTokens().size(), 1u);
-  args = CreateBeginFrameArgs(1u, 5u);
-  collection_.NotifySubmitFrame(2, false, viz::BeginFrameAck(args, true), args);
-  GenerateSequence("e(5,0)b(6)");
-  // Ack to an impl frame that doesn't exist in this tracker.
-  args = CreateBeginFrameArgs(1u, 1u);
-  collection_.NotifySubmitFrame(3, false, viz::BeginFrameAck(args, true), args);
-  args = CreateBeginFrameArgs(1u, 6u);
-  collection_.NotifySubmitFrame(4, false, viz::BeginFrameAck(args, true), args);
-  EXPECT_EQ(IgnoredFrameTokens().size(), 2u);
-  GenerateSequence("P(2)");
-  // frame_token = 2 is presented, frame_token = 3 remains in the list.
-  EXPECT_EQ(IgnoredFrameTokens().size(), 1u);
-  GenerateSequence("P(4)");
-  // Now frame_token = 4 is presented, frame_token = 3 should be removed.
-  EXPECT_TRUE(IgnoredFrameTokens().empty());
+  GenerateSequence("P(3)");
+  // Any token that is < 3 should have been removed.
+  EXPECT_EQ(IgnoredFrameTokens().size(), 0u);
+}
+
+TEST_F(FrameSequenceTrackerTest, TerminationWithNullPresentationTimeStamp) {
+  GenerateSequence("b(1)s(1)");
+  collection_.StopSequence(FrameSequenceTrackerType::kTouchScroll);
+  EXPECT_EQ(NumberOfRemovalTrackers(), 1u);
+  // Even if the presentation timestamp is null, as long as this presentation
+  // is acking the last impl frame, we consider that impl frame completed and
+  // so the tracker is ready for termination.
+  collection_.NotifyFramePresented(
+      1, {base::TimeTicks(), viz::BeginFrameArgs::DefaultInterval(), 0});
+  GenerateSequence("e(1,0)");
+  EXPECT_EQ(NumberOfRemovalTrackers(), 0u);
 }
 
 // Test the case where the frame tokens wraps around the 32-bit max value.