src: fix perfetto session reader teardown race

PerfettoSessionReader::Deleter issues a final ReadTrace() and then
Stop()s the session. Perfetto delivers the read data and the stop
notification as independent tasks on its own thread, so the stop could
win, close the uv handles and delete the reader while a
ReadTraceCallback bound to the raw pointer was still queued. That
callback then locked a destroyed mutex and signalled a closed
uv_async_t.

Only tear the reader down once the owner has released it, the session
has stopped and no read is in flight, and have both Perfetto-thread
callbacks update their flag and signal under chunks_mutex_ so the loop
thread cannot free the reader in between.

Refs: #64565

🏠 Remote-Dev: homespace

Author: MarshallOfSound

src/tracing/agent_perfetto.cc

@@ -155,6 +155,11 @@ void PerfettoSessionReader::Deleter::operator()(
   ptr->tracing_session_->FlushBlocking();
   ptr->Read();
   ptr->tracing_session_->Stop();
+  // The reader deletes itself on the tracing loop thread once the session has
+  // stopped and the final read has drained; see MaybeStartTeardown().
+  Mutex::ScopedLock lock(ptr->chunks_mutex_);
+  ptr->owner_released_ = true;
+  uv_async_send(&ptr->read_async_);
 }
 
 PerfettoSessionReader::PerfettoSessionReader(
@@ -201,12 +206,12 @@ void PerfettoSessionReader::Read() {
 
 void PerfettoSessionReader::ReadTraceCallback(
     perfetto::TracingSession::ReadTraceCallbackArgs args) {
-  // On Perfetto internal thread.
-  {
-    Mutex::ScopedLock lock(chunks_mutex_);
-    if (args.size > 0)
-      pending_chunks_.emplace_back(args.data, args.data + args.size);
-  }
+  // On Perfetto internal thread. Hold chunks_mutex_ across the flag update and
+  // the wakeup so that the loop thread cannot observe read_in_progress_ ==
+  // false and tear the reader down while this callback is still touching it.
+  Mutex::ScopedLock lock(chunks_mutex_);
+  if (args.size > 0)
+    pending_chunks_.emplace_back(args.data, args.data + args.size);
   // A single ReadTrace() cycle can yield multiple callbacks; the last one has
   // has_more == false, which clears read_in_progress_ so the next timer tick
   // can start a new read.
@@ -215,6 +220,8 @@ void PerfettoSessionReader::ReadTraceCallback(
 }
 
 void PerfettoSessionReader::SessionStopCallback() {
+  // On Perfetto internal thread; same locking rationale as ReadTraceCallback.
+  Mutex::ScopedLock lock(chunks_mutex_);
   stop_requested_ = true;
   uv_async_send(&read_async_);
 }
@@ -235,16 +242,27 @@ void PerfettoSessionReader::OnReadAsync(uv_async_t* async) {
     chunks_to_write.pop_front();
   }
 
-  if (reader->stop_requested_ && reader->handles_pending_close_ == 0) {
-    reader->writer_->Flush(true);
+  reader->MaybeStartTeardown();
+}
 
-    reader->handles_pending_close_ = 2;
-    uv_timer_stop(&reader->read_timer_);
-    uv_close(reinterpret_cast<uv_handle_t*>(&reader->read_async_),
-             OnHandleClose);
-    uv_close(reinterpret_cast<uv_handle_t*>(&reader->read_timer_),
-             OnHandleClose);
+void PerfettoSessionReader::MaybeStartTeardown() {
+  if (handles_pending_close_ != 0) return;
+  {
+    // Only tear down once the owner has let go, the session has stopped and
+    // no ReadTrace() cycle is still delivering callbacks bound to |this|.
+    // Perfetto always terminates a read with has_more == false, and both
+    // callbacks signal under chunks_mutex_, so once this condition is observed
+    // under the lock nothing on the Perfetto thread will touch |this| again.
+    Mutex::ScopedLock lock(chunks_mutex_);
+    if (!owner_released_ || !stop_requested_ || read_in_progress_) return;
   }
+
+  writer_->Flush(true);
+
+  handles_pending_close_ = 2;
+  uv_timer_stop(&read_timer_);
+  uv_close(reinterpret_cast<uv_handle_t*>(&read_async_), OnHandleClose);
+  uv_close(reinterpret_cast<uv_handle_t*>(&read_timer_), OnHandleClose);
 }
 
 // static

src/tracing/agent_perfetto.h

@@ -38,7 +38,9 @@ class TraceWriter {
 // Threading: two threads are involved.
 //   - Perfetto's internal thread invokes ReadTraceCallback. It only copies the
 //     borrowed chunk into pending_chunks_ (under chunks_mutex_) and signals
-//     read_async_; it never touches the writer or the file.
+//     read_async_; it never touches the writer or the file. The stop callback
+//     runs there too. Both signal while holding chunks_mutex_ so the loop
+//     thread can tell when no further callback will dereference the reader.
 //   - The agent's dedicated tracing loop thread runs everything else. A
 //     repeating timer (read_timer_) starts a read every kReadPeriodMs, and
 //     OnReadAsync drains pending_chunks_ into the writer there. This is why the
@@ -67,6 +69,7 @@ class PerfettoSessionReader final {
   void ReadTraceCallback(perfetto::TracingSession::ReadTraceCallbackArgs args);
   void SessionStopCallback();
   void Read();
+  void MaybeStartTeardown();
 
   static void OnReadAsync(uv_async_t* async);
   static void OnReadTimer(uv_timer_t* timer);
@@ -78,6 +81,7 @@ class PerfettoSessionReader final {
   int handles_pending_close_ = 0;
   std::atomic<bool> stop_requested_ = false;
   std::atomic<bool> read_in_progress_ = false;
+  bool owner_released_ = false;  // Guarded by chunks_mutex_.
 
   Mutex chunks_mutex_;
   std::list<std::vector<char>> pending_chunks_;