Fix sampling profiler race condition
Thread 1 is running RunSamplingThread and has just read trace into
the_trace.
Thread 2 is calling Trace::Stop and has just suspended all the
threads. At this point thread 1 is blocked on the SuspendAll.
Thread 2 goes and deletes the trace which Thread 1 still has a
pointer to, calls ResumeAll(). At this point thread 1 suspends the
threads and adds samples to the just deleted trace.
The fix is to join the thread before we delete the trace.
Bug: 18950006
Change-Id: I3090c4dac392a4e5d880c4dc8d9385aef53c7425
diff --git a/runtime/trace.cc b/runtime/trace.cc
index a1296f4..8833a85 100644
--- a/runtime/trace.cc
+++ b/runtime/trace.cc
@@ -401,9 +401,8 @@
void Trace::Stop() {
bool stop_alloc_counting = false;
- Runtime* runtime = Runtime::Current();
- runtime->GetThreadList()->SuspendAll();
- Trace* the_trace = NULL;
+ Runtime* const runtime = Runtime::Current();
+ Trace* the_trace = nullptr;
pthread_t sampling_pthread = 0U;
{
MutexLock mu(Thread::Current(), *Locks::trace_lock_);
@@ -415,19 +414,27 @@
sampling_pthread = sampling_pthread_;
}
}
- if (the_trace != NULL) {
+ // Make sure that we join before we delete the trace since we don't want to have
+ // the sampling thread access a stale pointer. This finishes since the sampling thread exits when
+ // the_trace_ is null.
+ if (sampling_pthread != 0U) {
+ CHECK_PTHREAD_CALL(pthread_join, (sampling_pthread, NULL), "sampling thread shutdown");
+ sampling_pthread_ = 0U;
+ }
+ runtime->GetThreadList()->SuspendAll();
+ if (the_trace != nullptr) {
stop_alloc_counting = (the_trace->flags_ & kTraceCountAllocs) != 0;
the_trace->FinishTracing();
if (the_trace->sampling_enabled_) {
MutexLock mu(Thread::Current(), *Locks::thread_list_lock_);
- runtime->GetThreadList()->ForEach(ClearThreadStackTraceAndClockBase, NULL);
+ runtime->GetThreadList()->ForEach(ClearThreadStackTraceAndClockBase, nullptr);
} else {
runtime->GetInstrumentation()->DisableMethodTracing();
- runtime->GetInstrumentation()->RemoveListener(the_trace,
- instrumentation::Instrumentation::kMethodEntered |
- instrumentation::Instrumentation::kMethodExited |
- instrumentation::Instrumentation::kMethodUnwind);
+ runtime->GetInstrumentation()->RemoveListener(
+ the_trace, instrumentation::Instrumentation::kMethodEntered |
+ instrumentation::Instrumentation::kMethodExited |
+ instrumentation::Instrumentation::kMethodUnwind);
}
if (the_trace->trace_file_.get() != nullptr) {
// Do not try to erase, so flush and close explicitly.
@@ -441,15 +448,9 @@
delete the_trace;
}
runtime->GetThreadList()->ResumeAll();
-
if (stop_alloc_counting) {
// Can be racy since SetStatsEnabled is not guarded by any locks.
- Runtime::Current()->SetStatsEnabled(false);
- }
-
- if (sampling_pthread != 0U) {
- CHECK_PTHREAD_CALL(pthread_join, (sampling_pthread, NULL), "sampling thread shutdown");
- sampling_pthread_ = 0U;
+ runtime->SetStatsEnabled(false);
}
}