perfetto: TrackEvent exporter: Skip only relevant fields if incomplete

Some TracePackets (e.g. service-emitted or metadata ones) don't use
incremental state, so shouldn't be skipped even if the
incremental_state_cleared flag wasn't yet seen.

Bug: 928738
Change-Id: Iadc649e5460b00fb4af5a8f30c03ed9b17aace9c
TBR: oysteine@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1541163Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644892}

services/tracing/perfetto/track_event_json_exporter.cc

@@ -63,10 +63,6 @@ void TrackEventJSONExporter::ProcessPackets(
       current_state_.incomplete = true;
     }
 
-    if (current_state_.incomplete) {
-      continue;
-    }
-
     // Now we process the data from the packet. First by getting the interned
     // strings out and processed.
     if (packet.has_interned_data()) {
@@ -85,10 +81,9 @@ void TrackEventJSONExporter::ProcessPackets(
     } else if (packet.has_trace_stats()) {
       SetTraceStatsMetadata(packet.trace_stats());
     } else {
+      // If none of the above matched, this packet was emitted by the service
+      // and has no equivalent in the old trace format. We thus ignore it.
     }
-    // If none of the above matched this packet was emitted by the service or
-    // some other producer. And as a result has no equivalent in the old trace
-    // format so we ignore it.
   }
 }
 
@@ -154,6 +149,12 @@ base::Optional<int64_t> TrackEventJSONExporter::ComputeThreadTimeUs(
 void TrackEventJSONExporter::HandleInternedData(
     const ChromeTracePacket& packet) {
   DCHECK(packet.has_interned_data());
+
+  // InternedData is only emitted on sequences with incremental state.
+  if (current_state_.incomplete) {
+    return;
+  }
+
   const auto& data = packet.interned_data();
   // Even if the interned data was reset we should not change the values in the
   // interned data.
@@ -189,6 +190,11 @@ void TrackEventJSONExporter::HandleProcessDescriptor(
   // Save the current state we need for future packets.
   current_state_.pid = process.pid();
 
+  // ProcessDescriptor is only emitted on sequences with incremental state.
+  if (current_state_.incomplete) {
+    return;
+  }
+
   // If we aren't outputting traceEvents then we don't need to look at the
   // metadata that might need to be emitted.
   if (!ShouldOutputTraceEvents()) {
@@ -259,6 +265,12 @@ void TrackEventJSONExporter::HandleProcessDescriptor(
 void TrackEventJSONExporter::HandleThreadDescriptor(
     const ChromeTracePacket& packet) {
   DCHECK(packet.has_thread_descriptor());
+
+  // ThreadDescriptor is only emitted on sequences with incremental state.
+  if (current_state_.incomplete) {
+    return;
+  }
+
   const auto& thread = packet.thread_descriptor();
   // Save the current state we need for future packets.
   current_state_.pid = thread.pid();
@@ -334,6 +346,11 @@ void TrackEventJSONExporter::HandleChromeEvents(
 void TrackEventJSONExporter::HandleTrackEvent(const ChromeTracePacket& packet) {
   DCHECK(packet.has_track_event());
 
+  // TrackEvents need incremental state.
+  if (current_state_.incomplete) {
+    return;
+  }
+
   // If we aren't outputting traceEvents nothing in a TrackEvent currently will
   // be needed so just return early.
   if (!ShouldOutputTraceEvents()) {

services/tracing/perfetto/track_event_json_exporter_unittest.cc

@@ -1582,7 +1582,6 @@ TEST_F(TrackEventJsonExporterTest, TestMetadata) {
   std::vector<perfetto::protos::TracePacket> trace_packet_protos;
 
   trace_packet_protos.emplace_back();
-  trace_packet_protos.back().set_incremental_state_cleared(true);
   {
     auto* new_metadata =
         trace_packet_protos.back().mutable_chrome_events()->add_metadata();
@@ -1630,7 +1629,6 @@ TEST_F(TrackEventJsonExporterTest, TestLegacySystemFtrace) {
   trace_packet_protos.emplace_back();
   trace_packet_protos.back().mutable_chrome_events()->add_legacy_ftrace_output(
       ftrace);
-  trace_packet_protos.back().set_incremental_state_cleared(true);
   FinalizePackets(trace_packet_protos);
 
   auto* sys_trace = parsed_trace_data()->FindKey("systemTraceEvents");
@@ -1642,7 +1640,6 @@ TEST_F(TrackEventJsonExporterTest, TestLegacySystemTraceEvents) {
   std::vector<perfetto::protos::TracePacket> trace_packet_protos;
 
   trace_packet_protos.emplace_back();
-  trace_packet_protos.back().set_incremental_state_cleared(true);
   auto* json_trace = trace_packet_protos.back()
                          .mutable_chrome_events()
                          ->add_legacy_json_trace();
@@ -1669,7 +1666,6 @@ TEST_F(TrackEventJsonExporterTest, TestLegacyUserTrace) {
   std::vector<perfetto::protos::TracePacket> trace_packet_protos;
 
   trace_packet_protos.emplace_back();
-  trace_packet_protos.back().set_incremental_state_cleared(true);
   auto* json_trace = trace_packet_protos.back()
                          .mutable_chrome_events()
                          ->add_legacy_json_trace();
@@ -1699,7 +1695,6 @@ TEST_F(TrackEventJsonExporterTest, TestTraceStats) {
   std::vector<perfetto::protos::TracePacket> trace_packet_protos;
 
   trace_packet_protos.emplace_back();
-  trace_packet_protos.back().set_incremental_state_cleared(true);
   // We are just checking that we correctly called the function provided by the
   // base class. See json_trace_exporter_unittest for a more complete test.
   trace_packet_protos.back().mutable_trace_stats();
@@ -1757,9 +1752,16 @@ TEST_F(TrackEventJsonExporterTest, ComplexLongSequenceWithDroppedPackets) {
   track_event->set_thread_time_delta_us(3);
   *track_event->mutable_legacy_event() =
       CreateLegacyEvent(/* name_iid = */ 2, kLegacyFlags, kLegacyPhase);
+
   // This event will be dropped.
   trace_packet_protos.push_back(trace_packet_protos.back());
   trace_packet_protos.back().set_previous_packet_dropped(true);
+
+  // Add a TraceStats packet, which shouldn't be dropped even though the
+  // incremental state was not cleared yet.
+  trace_packet_protos.emplace_back();
+  trace_packet_protos.back().mutable_trace_stats();
+
   // Reset the state.
   AddThreadDescriptorPacket(
       /* sort_index = */ base::nullopt, ThreadDescriptor::THREAD_UNSPECIFIED,
@@ -1823,6 +1825,10 @@ TEST_F(TrackEventJsonExporterTest, ComplexLongSequenceWithDroppedPackets) {
                     &events));
   EXPECT_EQ(kReferenceTimeUs + 4, events[0]->timestamp);
   EXPECT_EQ(kReferenceThreadTimeUs + 3, events[0]->thread_timestamp);
+
+  // We only verify that the TraceStats packet was not dropped.
+  EXPECT_THAT(unparsed_trace_data_,
+              testing::HasSubstr("\"perfetto_trace_stats\""));
 }
 }  // namespace
 }  // namespace tracing