perfetto: Reset event sink between sessions on non-flushing threads.

Addresses a bug where we end up tracing to the buffer of a previous
tracing session.

Bug: 929080
Change-Id: I4b4f21a994eddf66b6714b43185c3f78a2ea1532
Reviewed-on: https://chromium-review.googlesource.com/c/1456006
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629902}

services/tracing/public/cpp/perfetto/trace_event_data_source.cc

@@ -130,8 +130,10 @@ class TraceEventDataSource::ThreadLocalEventSink {
  public:
   ThreadLocalEventSink(
       std::unique_ptr<perfetto::StartupTraceWriter> trace_writer,
+      perfetto::BufferID target_buffer,
       bool thread_will_flush)
       : trace_writer_(std::move(trace_writer)),
+        target_buffer_(target_buffer),
         thread_will_flush_(thread_will_flush) {
 #if DCHECK_IS_ON()
     static std::atomic<int32_t> id_counter(1);
@@ -432,8 +434,11 @@ class TraceEventDataSource::ThreadLocalEventSink {
     trace_writer_->Flush();
   }
 
+  perfetto::BufferID target_buffer() const { return target_buffer_; }
+
  private:
   std::unique_ptr<perfetto::StartupTraceWriter> trace_writer_;
+  perfetto::BufferID target_buffer_;
   const bool thread_will_flush_;
   ChromeEventBundleHandle event_bundle_;
   perfetto::TraceWriter::TracePacketHandle trace_packet_handle_;
@@ -504,7 +509,8 @@ void TraceEventDataSource::StartTracing(
 
     DCHECK(!producer_client_);
     producer_client_ = producer_client;
-    target_buffer_ = data_source_config.target_buffer();
+    target_buffer_.store(data_source_config.target_buffer(),
+                         std::memory_order_relaxed);
     // Reduce lock contention by binding the registry without holding the lock.
     unbound_writer_registry = std::move(startup_writer_registry_);
   }
@@ -552,7 +558,7 @@ void TraceEventDataSource::StopTracing(
     base::AutoLock lock(lock_);
     DCHECK(producer_client_);
     producer_client_ = nullptr;
-    target_buffer_ = 0;
+    target_buffer_.store(0, std::memory_order_relaxed);
   }
 
   if (was_enabled) {
@@ -629,12 +635,13 @@ TraceEventDataSource::CreateThreadLocalEventSink(bool thread_will_flush) {
   if (startup_writer_registry_) {
     return new ThreadLocalEventSink(
         startup_writer_registry_->CreateUnboundTraceWriter(),
-        thread_will_flush);
+        target_buffer_.load(std::memory_order_relaxed), thread_will_flush);
   } else if (producer_client_) {
     return new ThreadLocalEventSink(
         std::make_unique<perfetto::StartupTraceWriter>(
-            producer_client_->CreateTraceWriter(target_buffer_)),
-        thread_will_flush);
+            producer_client_->CreateTraceWriter(
+                target_buffer_.load(std::memory_order_relaxed))),
+        target_buffer_.load(std::memory_order_relaxed), thread_will_flush);
   } else {
     return nullptr;
   }
@@ -648,6 +655,24 @@ void TraceEventDataSource::OnAddTraceEvent(
   auto* thread_local_event_sink =
       static_cast<ThreadLocalEventSink*>(ThreadLocalEventSinkSlot()->Get());
 
+  // Make sure the sink was reset since the last tracing session. Normally, it
+  // is reset on Flush after the session is disabled. However, it may not have
+  // been reset if the current thread doesn't support flushing. In that case, we
+  // need to check here that it writes to the right buffer.
+  //
+  // Because we want to avoid locking for each event, we access |target_buffer_|
+  // racily. It's OK if we don't see it change to the new buffer immediately. In
+  // that case, the first few trace events may get lost, but we will eventually
+  // notice that we are writing to the wrong buffer once the change to
+  // |target_buffer_| has propagated, and reset the sink. Note we will still
+  // acquire the |lock_| to safely recreate the sink in
+  // CreateThreadLocalEventSink().
+  if (!thread_will_flush && thread_local_event_sink &&
+      GetInstance()->target_buffer_.load(std::memory_order_relaxed) !=
+          thread_local_event_sink->target_buffer()) {
+    thread_local_event_sink = nullptr;
+  }
+
   if (!thread_local_event_sink) {
     thread_local_event_sink =
         GetInstance()->CreateThreadLocalEventSink(thread_will_flush);

services/tracing/public/cpp/perfetto/trace_event_data_source.h

@@ -121,7 +121,11 @@ class COMPONENT_EXPORT(TRACING_CPP) TraceEventDataSource
   base::OnceClosure stop_complete_callback_;
 
   base::Lock lock_;  // Protects subsequent members.
-  uint32_t target_buffer_ = 0;
+  // Regular accesses to |target_buffer_| in conjunction with other fields are
+  // protected by |lock_|, thus no memory order guarantees are required when
+  // writing or reading it in those places. Note that OnAddTraceEvent()
+  // accesses its atomic value racily without holding |lock_|.
+  std::atomic<uint32_t> target_buffer_{0};
   ProducerClient* producer_client_ = nullptr;
   // We own the registry during startup, but transfer its ownership to the
   // ProducerClient once the perfetto service is available. Only set if