Suppress handler thread from appearing in MinidumpProcessor's ProcessState

(#65).  r=bryner
 - Interface change: (ProcessState).crash_thread is now requesting_thread and
   will be populated for non-crash dumps.  If the requesting thread cannot
   be determined, requesting_thread is set to -1.

http://groups.google.com/group/airbag-dev/browse_thread/thread/c422ec481a2db440


git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@62 4c0a9323-5329-0410-9bdc-e9ce6186880e

src/processor/minidump.cc

@@ -661,7 +661,8 @@ const u_int8_t* MinidumpMemoryRegion::GetMemory() {
 
 
 u_int64_t MinidumpMemoryRegion::GetBase() {
-  return valid_ ? descriptor_->start_of_memory_range : (u_int64_t)-1;
+  return valid_ ?
+      descriptor_->start_of_memory_range : static_cast<u_int64_t>(-1);
 }
 
 
@@ -829,8 +830,12 @@ MinidumpContext* MinidumpThread::GetContext() {
 }
 
 
-u_int32_t MinidumpThread::GetThreadID() {
-  return valid_ ? thread_.thread_id : (u_int32_t)-1;
+bool MinidumpThread::GetThreadID(u_int32_t *thread_id) const {
+  if (!thread_id || !valid_)
+    return false;
+
+  *thread_id = thread_.thread_id;
+  return true;
 }
 
 
@@ -928,7 +933,10 @@ bool MinidumpThreadList::Read(u_int32_t expected_size) {
     if (!thread->Read())
       return false;
 
-    u_int32_t thread_id = thread->GetThreadID();
+    u_int32_t thread_id;
+    if (!thread->GetThreadID(&thread_id))
+      return false;
+
     if (GetThreadByID(thread_id)) {
       // Another thread with this ID is already in the list.  Data error.
       return false;
@@ -1710,8 +1718,12 @@ bool MinidumpException::Read(u_int32_t expected_size) {
 }
 
 
-u_int32_t MinidumpException::GetThreadID() {
-  return valid_ ? exception_.thread_id : 0;
+bool MinidumpException::GetThreadID(u_int32_t *thread_id) const {
+  if (!thread_id || !valid_)
+    return false;
+
+  *thread_id = exception_.thread_id;
+  return true;
 }
 
 
@@ -2008,6 +2020,85 @@ void MinidumpMiscInfo::Print() {
     printf("  processor_current_idle_state = 0x%x\n",
            misc_info_.processor_current_idle_state);
   }
+  printf("\n");
+}
+
+
+//
+// MinidumpAirbagInfo
+//
+
+
+MinidumpAirbagInfo::MinidumpAirbagInfo(Minidump* minidump)
+    : MinidumpStream(minidump),
+      airbag_info_() {
+}
+
+
+bool MinidumpAirbagInfo::Read(u_int32_t expected_size) {
+  valid_ = false;
+
+  if (expected_size != sizeof(airbag_info_))
+    return false;
+
+  if (!minidump_->ReadBytes(&airbag_info_, sizeof(airbag_info_)))
+    return false;
+
+  if (minidump_->swap()) {
+    Swap(&airbag_info_.validity);
+    Swap(&airbag_info_.dump_thread_id);
+    Swap(&airbag_info_.requesting_thread_id);
+  }
+
+  valid_ = true;
+  return true;
+}
+
+
+bool MinidumpAirbagInfo::GetDumpThreadID(u_int32_t *thread_id) const {
+  if (!thread_id || !valid_ ||
+      !(airbag_info_.validity & MD_AIRBAG_INFO_VALID_DUMP_THREAD_ID)) {
+    return false;
+  }
+
+  *thread_id = airbag_info_.dump_thread_id;
+  return true;
+}
+
+
+bool MinidumpAirbagInfo::GetRequestingThreadID(u_int32_t *thread_id)
+    const {
+  if (!thread_id || !valid_ ||
+      !(airbag_info_.validity & MD_AIRBAG_INFO_VALID_REQUESTING_THREAD_ID)) {
+    return false;
+  }
+
+  *thread_id = airbag_info_.requesting_thread_id;
+  return true;
+}
+
+
+void MinidumpAirbagInfo::Print() {
+  if (!valid_)
+    return;
+
+  printf("MDRawAirbagInfo\n");
+  printf("  validity             = 0x%x\n", airbag_info_.validity);
+
+  if (airbag_info_.validity & MD_AIRBAG_INFO_VALID_DUMP_THREAD_ID) {
+    printf("  dump_thread_id       = 0x%x\n", airbag_info_.dump_thread_id);
+  } else {
+    printf("  dump_thread_id       = (invalid)\n");
+  }
+
+  if (airbag_info_.validity & MD_AIRBAG_INFO_VALID_DUMP_THREAD_ID) {
+    printf("  requesting_thread_id = 0x%x\n",
+           airbag_info_.requesting_thread_id);
+  } else {
+    printf("  requesting_thread_id = (invalid)\n");
+  }
+
+  printf("\n");
 }
 
 
@@ -2135,7 +2226,8 @@ bool Minidump::Read() {
       case MD_MEMORY_LIST_STREAM:
       case MD_EXCEPTION_STREAM:
       case MD_SYSTEM_INFO_STREAM:
-      case MD_MISC_INFO_STREAM: {
+      case MD_MISC_INFO_STREAM:
+      case MD_AIRBAG_INFO_STREAM: {
         if (stream_map->find(stream_type) != stream_map->end()) {
           // Another stream with this type was already found.  A minidump
           // file should contain at most one of each of these stream types.
@@ -2196,6 +2288,12 @@ MinidumpMiscInfo* Minidump::GetMiscInfo() {
 }
 
 
+MinidumpAirbagInfo* Minidump::GetAirbagInfo() {
+  MinidumpAirbagInfo* airbag_info;
+  return GetStream(&airbag_info);
+}
+
+
 void Minidump::Print() {
   if (!valid_)
     return;
@@ -2235,7 +2333,7 @@ void Minidump::Print() {
        ++iterator) {
     u_int32_t stream_type = iterator->first;
     MinidumpStreamInfo info = iterator->second;
-    printf("  stream type %2d at index %d\n", stream_type, info.stream_index);
+    printf("  stream type 0x%x at index %d\n", stream_type, info.stream_index);
   }
   printf("\n");
 }

src/processor/minidump_dump.cc

@@ -32,81 +32,83 @@
 //
 // Author: Mark Mentovai
 
-#include <stdlib.h>
-#include <stdio.h>
-
-#include <string>
+#include <cstdio>
 
 #include "google_airbag/processor/minidump.h"
 
-
-using std::string;
 using namespace google_airbag;
 
-
-int main(int argc, char** argv) {
+int main(int argc, char **argv) {
   if (argc != 2) {
     fprintf(stderr, "usage: %s <file>\n", argv[0]);
-    exit(1);
+    return 1;
   }
 
   Minidump minidump(argv[1]);
   if (!minidump.Read()) {
     printf("minidump.Read() failed\n");
-    exit(1);
+    return 1;
   }
   minidump.Print();
 
-  int error = 0;
+  int errors = 0;
 
-  MinidumpThreadList* threadList = minidump.GetThreadList();
-  if (!threadList) {
-    error |= 1 << 2;
+  MinidumpThreadList *thread_list = minidump.GetThreadList();
+  if (!thread_list) {
+    ++errors;
     printf("minidump.GetThreadList() failed\n");
   } else {
-    threadList->Print();
+    thread_list->Print();
   }
 
-  MinidumpModuleList* moduleList = minidump.GetModuleList();
-  if (!moduleList) {
-    error |= 1 << 3;
+  MinidumpModuleList *module_list = minidump.GetModuleList();
+  if (!module_list) {
+    ++errors;
     printf("minidump.GetModuleList() failed\n");
   } else {
-    moduleList->Print();
+    module_list->Print();
   }
 
-  MinidumpMemoryList* memoryList = minidump.GetMemoryList();
-  if (!memoryList) {
-    error |= 1 << 4;
+  MinidumpMemoryList *memory_list = minidump.GetMemoryList();
+  if (!memory_list) {
+    ++errors;
     printf("minidump.GetMemoryList() failed\n");
   } else {
-    memoryList->Print();
+    memory_list->Print();
   }
 
-  MinidumpException* exception = minidump.GetException();
+  MinidumpException *exception = minidump.GetException();
   if (!exception) {
-    error |= 1 << 5;
+    // Exception info is optional, so don't treat this as an error.
     printf("minidump.GetException() failed\n");
   } else {
     exception->Print();
   }
 
-  MinidumpSystemInfo* systemInfo = minidump.GetSystemInfo();
-  if (!systemInfo) {
-    error |= 1 << 6;
+  MinidumpSystemInfo *system_info = minidump.GetSystemInfo();
+  if (!system_info) {
+    ++errors;
     printf("minidump.GetSystemInfo() failed\n");
   } else {
-    systemInfo->Print();
+    system_info->Print();
   }
 
-  MinidumpMiscInfo* miscInfo = minidump.GetMiscInfo();
-  if (!miscInfo) {
-    error |= 1 << 7;
+  MinidumpMiscInfo *misc_info = minidump.GetMiscInfo();
+  if (!misc_info) {
+    ++errors;
     printf("minidump.GetMiscInfo() failed\n");
   } else {
-    miscInfo->Print();
+    misc_info->Print();
+  }
+
+  MinidumpAirbagInfo *airbag_info = minidump.GetAirbagInfo();
+  if (!airbag_info) {
+    // Airbag info is optional, so don't treat this as an error.
+    printf("minidump.GetAirbagInfo() failed\n");
+  } else {
+    airbag_info->Print();
   }
 
   // Use return instead of exit to allow destructors to run.
-  return(error);
+  return errors == 0 ? 0 : 1;
 }

src/processor/minidump_dump_test

@@ -30,7 +30,7 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 testdata_dir=$srcdir/src/processor/testdata
-./src/processor/minidump_dump $testdata_dir/minidump1.dmp | \
- tr -s '\015' '\012' | \
- diff -u $testdata_dir/minidump1.out -
+./src/processor/minidump_dump $testdata_dir/minidump2.dmp | \
+ tr -d '\015' | \
+ diff -u $testdata_dir/minidump2.dump.out -
 exit $?

src/processor/minidump_processor.cc

@@ -54,11 +54,22 @@ ProcessState* MinidumpProcessor::Process(const string &minidump_file) {
   process_state->cpu_ = GetCPUInfo(&dump, &process_state->cpu_info_);
   process_state->os_ = GetOSInfo(&dump, &process_state->os_version_);
 
-  u_int32_t exception_thread_id = 0;
+  u_int32_t dump_thread_id = 0;
+  bool has_dump_thread = false;
+  u_int32_t requesting_thread_id = 0;
+  bool has_requesting_thread = false;
+
+  MinidumpAirbagInfo *airbag_info = dump.GetAirbagInfo();
+  if (airbag_info) {
+    has_dump_thread = airbag_info->GetDumpThreadID(&dump_thread_id);
+    has_requesting_thread =
+        airbag_info->GetRequestingThreadID(&requesting_thread_id);
+  }
+
   MinidumpException *exception = dump.GetException();
   if (exception) {
     process_state->crashed_ = true;
-    exception_thread_id = exception->GetThreadID();
+    has_requesting_thread = exception->GetThreadID(&requesting_thread_id);
 
     process_state->crash_reason_ = GetCrashReason(
         &dump, &process_state->crash_address_);
@@ -69,7 +80,7 @@ ProcessState* MinidumpProcessor::Process(const string &minidump_file) {
     return NULL;
   }
 
-  bool found_crash_thread = false;
+  bool found_requesting_thread = false;
   unsigned int thread_count = threads->thread_count();
   for (unsigned int thread_index = 0;
        thread_index < thread_count;
@@ -79,24 +90,46 @@ ProcessState* MinidumpProcessor::Process(const string &minidump_file) {
       return NULL;
     }
 
+    u_int32_t thread_id;
+    if (!thread->GetThreadID(&thread_id)) {
+      return NULL;
+    }
+
+    // If this thread is the thread that produced the minidump, don't process
+    // it.  Because of the problems associated with a thread producing a
+    // dump of itself (when both its context and its stack are in flux),
+    // processing that stack wouldn't provide much useful data.
+    if (has_dump_thread && thread_id == dump_thread_id) {
+      continue;
+    }
+
     MinidumpContext *context = thread->GetContext();
 
-    if (process_state->crashed_ &&
-        thread->GetThreadID() == exception_thread_id) {
-      if (found_crash_thread) {
-        // There can't be more than one crash thread.
+    if (has_requesting_thread && thread_id == requesting_thread_id) {
+      if (found_requesting_thread) {
+        // There can't be more than one requesting thread.
         return NULL;
       }
 
-      // Use the exception record's context for the crashed thread, instead
-      // of the thread's own context.  For the crashed thread, the thread's
-      // own context is the state inside the exception handler.  Using it
-      // would not result in the expected stack trace from the time of the
-      // crash.
-      context = exception->GetContext();
+      // Use processed_state->threads_.size() instead of thread_index.
+      // thread_index points to the thread index in the minidump, which
+      // might be greater than the thread index in the threads vector if
+      // any of the minidump's threads are skipped and not placed into the
+      // processed threads vector.  The thread vector's current size will
+      // be the index of the current thread when it's pushed into the
+      // vector.
+      process_state->requesting_thread_ = process_state->threads_.size();
 
-      process_state->crash_thread_ = thread_index;
-      found_crash_thread = true;
+      found_requesting_thread = true;
+
+      if (process_state->crashed_) {
+        // Use the exception record's context for the crashed thread, instead
+        // of the thread's own context.  For the crashed thread, the thread's
+        // own context is the state inside the exception handler.  Using it
+        // would not result in the expected stack trace from the time of the
+        // crash.
+        context = exception->GetContext();
+      }
     }
 
     MinidumpMemoryRegion *thread_memory = thread->GetMemory();
@@ -121,8 +154,8 @@ ProcessState* MinidumpProcessor::Process(const string &minidump_file) {
     process_state->threads_.push_back(stack.release());
   }
 
-  // If the process crashed, there must be a crash thread.
-  if (process_state->crashed_ && !found_crash_thread) {
+  // If a requesting thread was indicated, it must be present.
+  if (has_requesting_thread && !found_requesting_thread) {
     return NULL;
   }
 

src/processor/minidump_processor_unittest.cc

@@ -66,7 +66,7 @@ string TestSymbolSupplier::GetSymbolFile(MinidumpModule *module) {
     // reached by a SimpleSymbolSupplier.
     return string(getenv("srcdir") ? getenv("srcdir") : ".") +
       "/src/processor/testdata/symbols/"
-      "test_app.pdb/63FE4780728D49379B9D7BB6460CB42A1/test_app.sym";
+      "test_app.pdb/8DDB7E9A365748938D6EB08B1DCA31AA1/test_app.sym";
   }
 
   return "";
@@ -91,9 +91,9 @@ static bool RunTests() {
   ASSERT_EQ(state->os_version(), "5.1.2600 Service Pack 2");
   ASSERT_TRUE(state->crashed());
   ASSERT_EQ(state->crash_reason(), "EXCEPTION_ACCESS_VIOLATION");
-  ASSERT_EQ(state->crash_address(), 0);
+  ASSERT_EQ(state->crash_address(), 0x45);
   ASSERT_EQ(state->threads()->size(), 1);
-  ASSERT_EQ(state->crash_thread(), 0);
+  ASSERT_EQ(state->requesting_thread(), 0);
   CallStack *stack = state->threads()->at(0);
   ASSERT_TRUE(stack);
   ASSERT_EQ(stack->frames()->size(), 4);
@@ -102,13 +102,13 @@ static bool RunTests() {
   ASSERT_EQ(stack->frames()->at(0)->module_name, "c:\\test_app.exe");
   ASSERT_EQ(stack->frames()->at(0)->function_name, "CrashFunction()");
   ASSERT_EQ(stack->frames()->at(0)->source_file_name, "c:\\test_app.cc");
-  ASSERT_EQ(stack->frames()->at(0)->source_line, 65);
+  ASSERT_EQ(stack->frames()->at(0)->source_line, 51);
 
   ASSERT_EQ(stack->frames()->at(1)->module_base, 0x400000);
   ASSERT_EQ(stack->frames()->at(1)->module_name, "c:\\test_app.exe");
   ASSERT_EQ(stack->frames()->at(1)->function_name, "main");
   ASSERT_EQ(stack->frames()->at(1)->source_file_name, "c:\\test_app.cc");
-  ASSERT_EQ(stack->frames()->at(1)->source_line, 70);
+  ASSERT_EQ(stack->frames()->at(1)->source_line, 56);
 
   // This comes from the CRT
   ASSERT_EQ(stack->frames()->at(2)->module_base, 0x400000);

src/processor/minidump_stackwalk.cc

@@ -193,19 +193,21 @@ static bool PrintMinidumpProcess(const string &minidump_file,
     printf("No crash\n");
   }
 
-  // If there's a crash thread, print it first.
-  int crash_thread = -1;
-  if (process_state->crashed()) {
-    crash_thread = process_state->crash_thread();
+  // If the thread that requested the dump is known, print it first.
+  int requesting_thread = process_state->requesting_thread(); 
+  if (requesting_thread != -1) {
     printf("\n");
-    printf("Thread %d (crashed)\n", crash_thread);
-    PrintStack(process_state->threads()->at(crash_thread), cpu);
+    printf("Thread %d (%s)\n",
+          requesting_thread,
+          process_state->crashed() ? "crashed" :
+                                     "requested dump, did not crash");
+    PrintStack(process_state->threads()->at(requesting_thread), cpu);
   }
 
   // Print all of the threads in the dump.
   int thread_count = process_state->threads()->size();
   for (int thread_index = 0; thread_index < thread_count; ++thread_index) {
-    if (thread_index != crash_thread) {
+    if (thread_index != requesting_thread) {
       // Don't print the crash thread again, it was already printed.
       printf("\n");
       printf("Thread %d\n", thread_index);

src/processor/testdata/minidump2.stackwalk.out

@@ -4,16 +4,16 @@ CPU: x86
      GenuineIntel family 6 model 13 stepping 8
 
 Crash reason:  EXCEPTION_ACCESS_VIOLATION
-Crash address: 0x0
+Crash address: 0x45
 
 Thread 0 (crashed)
- 0  test_app.exe!CrashFunction() [test_app.cc : 65 + 0x3]
-    eip = 0x0040102e   esp = 0x0012ff3c   ebp = 0x0012ff40   ebx = 0x7c80abc1
-    esi = 0x00000002   edi = 0x00000a28   eax = 0x00000000   ecx = 0x00000001
-    edx = 0x0041c888   efl = 0x00010286
- 1  test_app.exe!main [test_app.cc : 70 + 0x4]
-    eip = 0x0040107f   esp = 0x0012ff48   ebp = 0x0012ff70
+ 0  test_app.exe!CrashFunction() [test_app.cc : 51 + 0x3]
+    eip = 0x0040208e   esp = 0x0012feec   ebp = 0x0012fef0   ebx = 0x7c80abc1
+    esi = 0x00000002   edi = 0x00000a28   eax = 0x00000045   ecx = 0x0012fefc
+    edx = 0x7c90eb94   efl = 0x00010246
+ 1  test_app.exe!main [test_app.cc : 56 + 0x4]
+    eip = 0x004020df   esp = 0x0012fef8   ebp = 0x0012ff70
  2  test_app.exe!__tmainCRTStartup [crt0.c : 318 + 0x11]
-    eip = 0x0040153c   esp = 0x0012ff78   ebp = 0x0012ffc0   ebx = 0x7c80abc1
+    eip = 0x0040395c   esp = 0x0012ff78   ebp = 0x0012ffc0
  3  kernel32.dll!BaseProcessStart + 0x22
     eip = 0x7c816fd7   esp = 0x0012ffc8   ebp = 0x0012fff0

src/processor/testdata/test_app.cc

@@ -28,47 +28,32 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 // This file is used to generate minidump2.dmp and minidump2.sym.
-// cl /Zi /Fetest_app.exe test_app.cc dbghelp.lib
+// cl /Zi test_app.cc /Fetest_app.exe /I airbag/src \
+//   airbag/src/client/windows/releasestaticcrt/exception_handler.lib \
+//   ole32.lib
 // Then run test_app to generate a dump, and dump_syms to create the .sym file.
 
-#include <windows.h>
-#include <dbghelp.h>
+#include <cstdio>
 
-static LONG HandleException(EXCEPTION_POINTERS *exinfo) {
-  HANDLE dump_file = CreateFile("dump.dmp",
-                                GENERIC_WRITE,
-                                FILE_SHARE_WRITE,
-                                NULL,
-                                CREATE_ALWAYS,
-                                FILE_ATTRIBUTE_NORMAL,
-                                NULL);
+#include "client/windows/handler/exception_handler.h"
 
-  MINIDUMP_EXCEPTION_INFORMATION except_info;
-  except_info.ThreadId = GetCurrentThreadId();
-  except_info.ExceptionPointers = exinfo;
-  except_info.ClientPointers = false;
-  
-  MiniDumpWriteDump(GetCurrentProcess(),
-                    GetCurrentProcessId(),
-                    dump_file,
-                    MiniDumpNormal,
-                    &except_info,
-                    NULL,
-                    NULL);
-
-  CloseHandle(dump_file);
-  return EXCEPTION_EXECUTE_HANDLER;
+void callback(const std::wstring &id, void *context, bool succeeded) {
+  if (succeeded) {
+    printf("dump guid is %ws\n", id.c_str());
+  } else {
+    printf("dump failed\n");
+  }
+  exit(1);
 }
 
 void CrashFunction() {
-  int *i = NULL;
+  int *i = reinterpret_cast<int*>(0x45);
   *i = 5;  // crash!
 }
 
-int main(int argc, char *argv[]) {
-  __try {
-    CrashFunction();
-  } __except(HandleException(GetExceptionInformation())) {
-  }
+int main(int argc, char **argv) {
+  google_airbag::ExceptionHandler eh(L".", callback, NULL, true);
+  CrashFunction();
+  printf("did not crash?\n");
   return 0;
 }