ResetIncrState before all other track_event packets
While adding support for timestamp delta encoding, we realised that
first packet coming from track_event data source could be something
different from `ResetIncrementalState`. Which can cause problems.
For example, see line 87 in https://www.diffchecker.com/gKUL1x4J
As we are dropping the explicit `timestamp_clock_id` field from each
packet and utilising it's value from `trace_packet_defaults`, it's very
important to ensure that a packet which sets correct default values in
`trace_packet_defaults` comes before any other packets.
Bug: 224246750
Change-Id: I7790742052367c0a5828727857ec04a3b3b6bb0c
diff --git a/include/perfetto/tracing/internal/track_event_data_source.h b/include/perfetto/tracing/internal/track_event_data_source.h
index 2361352..633f3e2 100644
--- a/include/perfetto/tracing/internal/track_event_data_source.h
+++ b/include/perfetto/tracing/internal/track_event_data_source.h
@@ -361,7 +361,7 @@
TrackRegistry::Get()->UpdateTrack(track, desc.SerializeAsString());
Base::template Trace([&](typename Base::TraceContext ctx) {
TrackEventInternal::WriteTrackDescriptor(
- track, ctx.tls_inst_->trace_writer.get());
+ track, ctx.tls_inst_->trace_writer.get(), ctx.GetIncrementalState());
});
}
@@ -453,11 +453,8 @@
// Make sure incremental state is valid.
TraceWriterBase* trace_writer = ctx.tls_inst_->trace_writer.get();
TrackEventIncrementalState* incr_state = ctx.GetIncrementalState();
- if (incr_state->was_cleared) {
- incr_state->was_cleared = false;
- TrackEventInternal::ResetIncrementalState(trace_writer,
- trace_timestamp);
- }
+ TrackEventInternal::ResetIncrementalStateIfRequired(
+ trace_writer, incr_state, trace_timestamp);
// Write the track descriptor before any event on the track.
if (track) {
@@ -516,7 +513,7 @@
TrackRegistry::Get()->UpdateTrack(track, std::move(callback));
Base::template Trace([&](typename Base::TraceContext ctx) {
TrackEventInternal::WriteTrackDescriptor(
- track, ctx.tls_inst_->trace_writer.get());
+ track, ctx.tls_inst_->trace_writer.get(), ctx.GetIncrementalState());
});
}
diff --git a/include/perfetto/tracing/internal/track_event_internal.h b/include/perfetto/tracing/internal/track_event_internal.h
index 5bb6ec2..538274c 100644
--- a/include/perfetto/tracing/internal/track_event_internal.h
+++ b/include/perfetto/tracing/internal/track_event_internal.h
@@ -160,8 +160,15 @@
perfetto::protos::pbzero::TrackEvent::Type,
TraceTimestamp timestamp = {GetClockId(), GetTimeNs()});
- static void ResetIncrementalState(TraceWriterBase*, TraceTimestamp);
-
+ static void ResetIncrementalStateIfRequired(
+ TraceWriterBase* trace_writer,
+ TrackEventIncrementalState* incr_state,
+ TraceTimestamp timestamp) {
+ if (incr_state->was_cleared) {
+ incr_state->was_cleared = false;
+ ResetIncrementalState(trace_writer, incr_state, timestamp);
+ }
+ }
// TODO(altimin): Remove this method once Chrome uses
// EventContext::AddDebugAnnotation directly.
template <typename T>
@@ -184,15 +191,18 @@
auto it_and_inserted = incr_state->seen_tracks.insert(track.uuid);
if (PERFETTO_LIKELY(!it_and_inserted.second))
return;
- WriteTrackDescriptor(track, trace_writer);
+ WriteTrackDescriptor(track, trace_writer, incr_state);
}
// Unconditionally write a track descriptor into the trace.
template <typename TrackType>
static void WriteTrackDescriptor(const TrackType& track,
- TraceWriterBase* trace_writer) {
- TrackRegistry::Get()->SerializeTrack(
- track, NewTracePacket(trace_writer, {GetClockId(), GetTimeNs()}));
+ TraceWriterBase* trace_writer,
+ TrackEventIncrementalState* incr_state) {
+ TraceTimestamp ts = {GetClockId(), GetTimeNs()};
+ ResetIncrementalStateIfRequired(trace_writer, incr_state, ts);
+ TrackRegistry::Get()->SerializeTrack(track,
+ NewTracePacket(trace_writer, ts));
}
// Get the current time in nanoseconds in the trace clock timebase.
@@ -214,6 +224,10 @@
static const Track kDefaultTrack;
private:
+ static void ResetIncrementalState(TraceWriterBase*,
+ TrackEventIncrementalState* incr_state,
+ TraceTimestamp);
+
static protozero::MessageHandle<protos::pbzero::TracePacket> NewTracePacket(
TraceWriterBase*,
TraceTimestamp,
diff --git a/src/tracing/internal/track_event_internal.cc b/src/tracing/internal/track_event_internal.cc
index 3717fb2..c439c54 100644
--- a/src/tracing/internal/track_event_internal.cc
+++ b/src/tracing/internal/track_event_internal.cc
@@ -308,8 +308,10 @@
}
// static
-void TrackEventInternal::ResetIncrementalState(TraceWriterBase* trace_writer,
- TraceTimestamp timestamp) {
+void TrackEventInternal::ResetIncrementalState(
+ TraceWriterBase* trace_writer,
+ TrackEventIncrementalState* incr_state,
+ TraceTimestamp timestamp) {
auto default_track = ThreadTrack::Current();
{
// Mark any incremental state before this point invalid. Also set up
@@ -329,8 +331,8 @@
// trace points won't explicitly reference it. We also write the process
// descriptor from every thread that writes trace events to ensure it gets
// emitted at least once.
- WriteTrackDescriptor(default_track, trace_writer);
- WriteTrackDescriptor(ProcessTrack::Current(), trace_writer);
+ WriteTrackDescriptor(default_track, trace_writer, incr_state);
+ WriteTrackDescriptor(ProcessTrack::Current(), trace_writer, incr_state);
}
// static
diff --git a/src/tracing/test/api_integrationtest.cc b/src/tracing/test/api_integrationtest.cc
index 26d8a03..0a4d461 100644
--- a/src/tracing/test/api_integrationtest.cc
+++ b/src/tracing/test/api_integrationtest.cc
@@ -1406,7 +1406,7 @@
if (packet.has_track_descriptor()) {
if (packet.trusted_packet_sequence_id() == main_thread_sequence) {
descs.push_back(packet.track_descriptor());
- } else {
+ } else if (packet.track_descriptor().has_thread()) {
thread_descs.push_back(packet.track_descriptor());
}
}
@@ -1431,9 +1431,8 @@
EXPECT_EQ("goodbye.exe", descs[2].name());
// The child thread records only its own thread descriptor (twice, since it
- // was mutated). The child thread also emits another copy of the process
- // descriptor.
- EXPECT_EQ(3u, thread_descs.size());
+ // was mutated).
+ ASSERT_EQ(2u, thread_descs.size());
EXPECT_EQ("TestThread", thread_descs[0].name());
EXPECT_NE(0, thread_descs[0].thread().pid());
EXPECT_NE(0, thread_descs[0].thread().tid());
@@ -1478,7 +1477,8 @@
continue;
if (packet.has_track_descriptor()) {
auto td = packet.track_descriptor();
- EXPECT_TRUE(td.has_process());
+ if (!td.has_process())
+ continue;
EXPECT_NE(0, td.process().pid());
EXPECT_TRUE(td.has_chrome_process());
EXPECT_EQ("testing.exe", td.process().process_name());
