4 files changed, 40 insertions, 17 deletions

diff --git a/libbacktrace/BacktraceCurrent.cpp b/libbacktrace/BacktraceCurrent.cpp
index 95cd4d1..2714d93 100644
--- a/libbacktrace/BacktraceCurrent.cpp
+++ b/libbacktrace/BacktraceCurrent.cpp
@@ -96,7 +96,7 @@ static pthread_mutex_t g_sigaction_mutex = PTHREAD_MUTEX_INITIALIZER;
 static void SignalHandler(int, siginfo_t*, void* sigcontext) {
   ThreadEntry* entry = ThreadEntry::Get(getpid(), gettid(), false);
   if (!entry) {
-    BACK_LOGW("Unable to find pid %d tid %d information", getpid(), gettid());
+    BACK_LOGE("pid %d, tid %d entry not found", getpid(), gettid());
     return;
   }
 
@@ -109,9 +109,14 @@ static void SignalHandler(int, siginfo_t*, void* sigcontext) {
   // the thread run ahead causing problems.
   // The number indicates that we are waiting for the second Wake() call
   // overall which is made by the thread requesting an unwind.
-  entry->Wait(2);
-
-  ThreadEntry::Remove(entry);
+  if (entry->Wait(2)) {
+    // Do not remove the entry here because that can result in a deadlock
+    // if the code cannot properly send a signal to the thread under test.
+    entry->Wake();
+  } else {
+    // At this point, it is possible that entry has been freed, so just exit.
+    BACK_LOGE("Timed out waiting for unwind thread to indicate it completed.");
+  }
 }
 
 bool BacktraceCurrent::UnwindThread(size_t num_ignore_frames) {
@@ -128,17 +133,15 @@ bool BacktraceCurrent::UnwindThread(size_t num_ignore_frames) {
   act.sa_flags = SA_RESTART | SA_SIGINFO | SA_ONSTACK;
   sigemptyset(&act.sa_mask);
   if (sigaction(THREAD_SIGNAL, &act, &oldact) != 0) {
-    BACK_LOGW("sigaction failed %s", strerror(errno));
-    entry->Unlock();
+    BACK_LOGE("sigaction failed: %s", strerror(errno));
     ThreadEntry::Remove(entry);
     pthread_mutex_unlock(&g_sigaction_mutex);
     return false;
   }
 
   if (tgkill(Pid(), Tid(), THREAD_SIGNAL) != 0) {
-    BACK_LOGW("tgkill %d failed: %s", Tid(), strerror(errno));
+    BACK_LOGE("tgkill %d failed: %s", Tid(), strerror(errno));
     sigaction(THREAD_SIGNAL, &oldact, nullptr);
-    entry->Unlock();
     ThreadEntry::Remove(entry);
     pthread_mutex_unlock(&g_sigaction_mutex);
     return false;
@@ -146,17 +149,30 @@ bool BacktraceCurrent::UnwindThread(size_t num_ignore_frames) {
 
   // Wait for the thread to get the ucontext. The number indicates
   // that we are waiting for the first Wake() call made by the thread.
-  entry->Wait(1);
+  bool wait_completed = entry->Wait(1);
 
   // After the thread has received the signal, allow other unwinders to
   // continue.
   sigaction(THREAD_SIGNAL, &oldact, nullptr);
   pthread_mutex_unlock(&g_sigaction_mutex);
 
-  bool unwind_done = UnwindFromContext(num_ignore_frames, entry->GetUcontext());
+  bool unwind_done = false;
+  if (wait_completed) {
+    unwind_done = UnwindFromContext(num_ignore_frames, entry->GetUcontext());
 
-  // Tell the signal handler to exit and release the entry.
-  entry->Wake();
+    // Tell the signal handler to exit and release the entry.
+    entry->Wake();
+
+    // Wait for the thread to indicate it is done with the ThreadEntry.
+    if (!entry->Wait(3)) {
+      // Send a warning, but do not mark as a failure to unwind.
+      BACK_LOGW("Timed out waiting for signal handler to indicate it finished.");
+    }
+  } else {
+    BACK_LOGE("Timed out waiting for signal handler to get ucontext data.");
+  }
+
+  ThreadEntry::Remove(entry);
 
   return unwind_done;
 }
diff --git a/libbacktrace/BacktraceLog.h b/libbacktrace/BacktraceLog.h
index 1632ec2..5c39f1c 100644
--- a/libbacktrace/BacktraceLog.h
+++ b/libbacktrace/BacktraceLog.h
@@ -25,4 +25,7 @@
 #define BACK_LOGW(format, ...) \
   ALOGW("%s: " format, __PRETTY_FUNCTION__, ##__VA_ARGS__)
 
+#define BACK_LOGE(format, ...) \
+  ALOGE("%s: " format, __PRETTY_FUNCTION__, ##__VA_ARGS__)
+
 #endif // _LIBBACKTRACE_BACKTRACE_LOG_H
diff --git a/libbacktrace/ThreadEntry.cpp b/libbacktrace/ThreadEntry.cpp
index e8b60c8..084c1aa 100644
--- a/libbacktrace/ThreadEntry.cpp
+++ b/libbacktrace/ThreadEntry.cpp
@@ -69,7 +69,7 @@ ThreadEntry* ThreadEntry::Get(pid_t pid, pid_t tid, bool create) {
 }
 
 void ThreadEntry::Remove(ThreadEntry* entry) {
-  pthread_mutex_unlock(&entry->mutex_);
+  entry->Unlock();
 
   pthread_mutex_lock(&ThreadEntry::list_mutex_);
   if (--entry->ref_count_ == 0) {
@@ -96,20 +96,24 @@ ThreadEntry::~ThreadEntry() {
   pthread_cond_destroy(&wait_cond_);
 }
 
-void ThreadEntry::Wait(int value) {
+bool ThreadEntry::Wait(int value) {
   timespec ts;
   clock_gettime(CLOCK_MONOTONIC, &ts);
-  ts.tv_sec += 10;
+  ts.tv_sec += 5;
 
+  bool wait_completed = true;
   pthread_mutex_lock(&wait_mutex_);
   while (wait_value_ != value) {
     int ret = pthread_cond_timedwait(&wait_cond_, &wait_mutex_, &ts);
     if (ret != 0) {
-      BACK_LOGW("pthread_cond_timedwait failed: %s", strerror(ret));
+      BACK_LOGW("pthread_cond_timedwait for value %d failed: %s", value, strerror(ret));
+      wait_completed = false;
       break;
     }
   }
   pthread_mutex_unlock(&wait_mutex_);
+
+  return wait_completed;
 }
 
 void ThreadEntry::Wake() {
diff --git a/libbacktrace/ThreadEntry.h b/libbacktrace/ThreadEntry.h
index 94becf2..11924a3 100644
--- a/libbacktrace/ThreadEntry.h
+++ b/libbacktrace/ThreadEntry.h
@@ -29,7 +29,7 @@ public:
 
   void Wake();
 
-  void Wait(int);
+  bool Wait(int);
 
   void CopyUcontextFromSigcontext(void*);