heapprofd: Fix crash when tracing service disconnects
When the tracing service (traced) disconnects (because it crashes or
other reasons), if heapprofd has an active client, it will crash,
because:
* HeapprofdProducer::Restart() is executed.
* HeapprofdProducer destructor destroys its unwinding_workers_
* UnwindingWorker destroys its members. In order:
* thread_task_runner_ is destroyed, so that the thread is stopped.
* (If there's one connected client) client_data_ is destroyed.
* ClientData includes a UnixSocket, which calls PostTask on
thread_task_runner_.
* thread_task_runner_ was destroyed earlier.
This patch makes fixes the problem by destroying client_data_ (on
thread_task_runner_) before destroying the thread_task_runner_ itself.
Bug: 233338564
Bug: 155759761
Change-Id: Id1874abbff8f7a7b702542a943a8de3f3a52277a
(cherry picked from commit abc9d3ea914ba24e4219fef68a67aca65b8c23a5)
Merged-In: Id1874abbff8f7a7b702542a943a8de3f3a52277a
diff --git a/src/profiling/memory/unwinding.cc b/src/profiling/memory/unwinding.cc
index d500e0b..e52e8f0 100644
--- a/src/profiling/memory/unwinding.cc
+++ b/src/profiling/memory/unwinding.cc
@@ -198,6 +198,29 @@
return true;
}
+UnwindingWorker::~UnwindingWorker() {
+ if (thread_task_runner_.get() == nullptr) {
+ return;
+ }
+ std::mutex mutex;
+ std::condition_variable cv;
+
+ std::unique_lock<std::mutex> lock(mutex);
+ bool done = false;
+ thread_task_runner_.PostTask([&mutex, &cv, &done, this] {
+ for (auto& it : client_data_) {
+ auto& client_data = it.second;
+ client_data.sock->Shutdown(false);
+ }
+ client_data_.clear();
+
+ std::lock_guard<std::mutex> inner_lock(mutex);
+ done = true;
+ cv.notify_one();
+ });
+ cv.wait(lock, [&done] { return done; });
+}
+
void UnwindingWorker::OnDisconnect(base::UnixSocket* self) {
pid_t peer_pid = self->peer_pid_linux();
auto it = client_data_.find(peer_pid);
diff --git a/src/profiling/memory/unwinding.h b/src/profiling/memory/unwinding.h
index 6b5bcdb..9bc41c3 100644
--- a/src/profiling/memory/unwinding.h
+++ b/src/profiling/memory/unwinding.h
@@ -85,6 +85,9 @@
: delegate_(delegate),
thread_task_runner_(std::move(thread_task_runner)) {}
+ ~UnwindingWorker() override;
+ UnwindingWorker(UnwindingWorker&&) = default;
+
// Public API safe to call from other threads.
void PostDisconnectSocket(pid_t pid);
void PostHandoffSocket(HandoffData);
@@ -138,19 +141,11 @@
std::map<pid_t, ClientData> client_data_;
Delegate* delegate_;
- // Task runner with a dedicated thread. Keep last as instances this class are
- // currently (incorrectly) being destroyed on the main thread, instead of the
- // task thread. By destroying this task runner first, we ensure that the
- // UnwindingWorker is not active while the rest of its state is being
- // destroyed. Additionally this ensures that the destructing thread sees a
- // consistent view of the memory due to the ThreadTaskRunner's destructor
- // joining a thread.
- //
- // Additionally, keep the destructor defaulted, as its body would still race
- // against an active task thread.
- //
- // TODO(rsavitski): make the task thread own the object's lifetime (likely by
- // refactoring base::ThreadTaskRunner).
+ // Task runner with a dedicated thread. Keep last. By destroying this task
+ // runner first, we ensure that the UnwindingWorker is not active while the
+ // rest of its state is being destroyed. Additionally this ensures that the
+ // destructing thread sees a consistent view of the memory due to the
+ // ThreadTaskRunner's destructor joining a thread.
base::ThreadTaskRunner thread_task_runner_;
};
