[throughput] Fix a DCHECK in ReportMetrics

The crashes caused by this DCHECK happens when a begin-impl-frame
reports no damage, and then presented without frame end. This CL
fixes it.

Bug: 1043091
Change-Id: Idaef2477f7f2904fec87b202c47a3a9be6b95c42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018084Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737461}

cc/metrics/frame_sequence_tracker.cc

@@ -163,7 +163,7 @@ bool FrameSequenceMetrics::HasDataLeftForReporting() const {
          main_throughput_.frames_expected > 0;
 }
 
-void FrameSequenceMetrics::ReportMetrics(std::string debug_trace) {
+void FrameSequenceMetrics::ReportMetrics() {
   DCHECK_LE(impl_throughput_.frames_produced, impl_throughput_.frames_expected);
   DCHECK_LE(main_throughput_.frames_produced, main_throughput_.frames_expected);
   TRACE_EVENT_NESTABLE_ASYNC_END2(
@@ -176,14 +176,6 @@ void FrameSequenceMetrics::ReportMetrics(std::string debug_trace) {
       type_, ThreadType::kCompositor,
       GetIndexForMetric(FrameSequenceMetrics::ThreadType::kCompositor, type_),
       impl_throughput_);
-#if DCHECK_IS_ON()
-  if (impl_throughput_percent.has_value()) {
-    DCHECK_EQ(impl_throughput_.frames_received,
-              impl_throughput_.frames_processed)
-        << debug_trace << " " << type_;
-  }
-#endif
-
   base::Optional<int> main_throughput_percent = ThroughputData::ReportHistogram(
       type_, ThreadType::kMain,
       GetIndexForMetric(FrameSequenceMetrics::ThreadType::kMain, type_),
@@ -367,15 +359,27 @@ void FrameSequenceTrackerCollection::NotifyFramePresented(
         metrics->Merge(std::move(accumulated_metrics_[tracker->type()]));
         accumulated_metrics_.erase(tracker->type());
       }
-      if (metrics->HasEnoughDataForReporting()) {
+
 #if DCHECK_IS_ON()
+      // Handling the case like b(100)s(150)e(100)b(200)n(200), and then
+      // StopSequence() is called which put this tracker in removal_trackers_.
+      // Then P(150). In this case, frame 200 isn't processed yet, because this
+      // no damage impl frame is considered 'processed' at e(200).
+      const bool incomplete_frame_had_no_damage =
+          !tracker->compositor_frame_submitted_ &&
+          tracker->frame_had_no_compositor_damage_;
+      if (tracker->is_inside_frame_ && incomplete_frame_had_no_damage)
+        --metrics->impl_throughput().frames_received;
+
+      if (metrics->impl_throughput().frames_received !=
+          metrics->impl_throughput().frames_processed) {
         std::string output = tracker->frame_sequence_trace_.str().substr(
             tracker->ignored_trace_char_count_);
-        metrics->ReportMetrics(std::move(output));
-#else
-        metrics->ReportMetrics();
-#endif
+        NOTREACHED() << output;
       }
+#endif
+      if (metrics->HasEnoughDataForReporting())
+        metrics->ReportMetrics();
       if (metrics->HasDataLeftForReporting())
         accumulated_metrics_[tracker->type()] = std::move(metrics);
     }

cc/metrics/frame_sequence_tracker.h

@@ -110,7 +110,7 @@ class CC_EXPORT FrameSequenceMetrics {
   bool HasEnoughDataForReporting() const;
   bool HasDataLeftForReporting() const;
   // Report related metrics: throughput, checkboarding...
-  void ReportMetrics(std::string debug_trace = std::string());
+  void ReportMetrics();
 
   ThroughputData& impl_throughput() { return impl_throughput_; }
   ThroughputData& main_throughput() { return main_throughput_; }

cc/metrics/frame_sequence_tracker_unittest.cc

@@ -321,6 +321,10 @@ class FrameSequenceTrackerTest : public testing::Test {
     return tracker_->main_throughput();
   }
 
+  void SetTerminationStatus(FrameSequenceTracker::TerminationStatus status) {
+    tracker_->termination_status_ = status;
+  }
+
  protected:
   uint32_t number_of_frames_checkerboarded() const {
     return tracker_->metrics_->frames_checkerboarded();
@@ -718,7 +722,6 @@ TEST_F(FrameSequenceTrackerTest, BeginImplFrameBeforeTerminate) {
 }
 
 // b(2417)B(0,2417)E(2417)n(2417)N(2417,2417)
-
 TEST_F(FrameSequenceTrackerTest, SequenceNumberReset) {
   const char sequence[] =
       "b(6)B(0,6)n(6)e(6)Rb(1)B(0,1)N(1,1)n(1)e(1)b(2)B(1,2)n(2)e(2)";
@@ -736,6 +739,29 @@ TEST_F(FrameSequenceTrackerTest, MainThroughputWithHighLatency) {
   EXPECT_EQ(MainThroughput().frames_produced, 1u);
 }
 
+#if DCHECK_IS_ON()
+// These two tests ensures that when present a frame, the frames_received is
+// the same as frames_processed. As long as there is no crash, the condition is
+// true.
+TEST_F(FrameSequenceTrackerTest, FramesProcessedMatch1) {
+  const char sequence[] = "b(1)n(1)e(1)b(2)s(2)e(2)b(3)n(3)";
+  GenerateSequence(sequence);
+  collection_.StopSequence(FrameSequenceTrackerType::kTouchScroll);
+  SetTerminationStatus(
+      FrameSequenceTracker::TerminationStatus::kReadyForTermination);
+  GenerateSequence("P(2)");
+}
+
+TEST_F(FrameSequenceTrackerTest, FramesProcessedMatch2) {
+  const char sequence[] = "b(1)n(1)e(1)b(2)s(2)e(2)b(3)s(3)";
+  GenerateSequence(sequence);
+  collection_.StopSequence(FrameSequenceTrackerType::kTouchScroll);
+  SetTerminationStatus(
+      FrameSequenceTracker::TerminationStatus::kReadyForTermination);
+  GenerateSequence("P(2)");
+}
+#endif
+
 TEST_F(FrameSequenceTrackerTest, OffScreenMainDamage1) {
   const char sequence[] =
       "b(1)B(0,1)n(1)e(1)b(2)E(1)B(1,2)n(2)e(2)b(3)E(2)B(2,3)n(3)e(3)";