Allow exception handler callbacks more flexibility (#81). r=bryner

 - Provide an optional filter callback that gets triggered before attempting
   to write a dump, to give client code a chance to refuse handling early
   in the process.
 - Allow exceptions that are unhandled by Airbag (due to filter callback or
   dump callback return value, or failure to write a dump) to be passed to the
   previous handler or to the system.
 - In order to pass exceptions unhandled by the topmost Airbag handler to
   lower handlers, fix up the stacking of ExceptionHandler objects, and give
   each ExceptionHandler object its own thread (like the Mac implementation)
   to avoid deadlock.
 - Provide a dump_path argument to callbacks, as requested by developers and
   already implemented in the Mac handler.
 - Avoid calling c_str in exception handler code (#90).

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


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

1 files changed, 177 insertions, 57 deletions

diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc
index 4df0cef3..10d5ec08 100644
--- a/src/client/windows/handler/exception_handler.cc
+++ b/src/client/windows/handler/exception_handler.cc
@@ -29,6 +29,7 @@
 
 #include <ObjBase.h>
 
+#include <cassert>
 #include <cstdio>
 
 #include "common/windows/string_utils-inl.h"
@@ -39,49 +40,83 @@
 
 namespace google_airbag {
 
-ExceptionHandler *ExceptionHandler::current_handler_ = NULL;
-HANDLE ExceptionHandler::handler_thread_ = NULL;
-CRITICAL_SECTION ExceptionHandler::handler_critical_section_;
-HANDLE ExceptionHandler::handler_start_semaphore_ = NULL;
-HANDLE ExceptionHandler::handler_finish_semaphore_ = NULL;
-ExceptionHandler *ExceptionHandler::requesting_handler_ = NULL;
-DWORD ExceptionHandler::requesting_thread_id_ = 0;
-EXCEPTION_POINTERS *ExceptionHandler::exception_info_ = NULL;
-bool ExceptionHandler::handler_return_value_ = false;
+static const int kExceptionHandlerThreadInitialStackSize = 64 * 1024;
+
+vector<ExceptionHandler *> *ExceptionHandler::handler_stack_ = NULL;
+LONG ExceptionHandler::handler_stack_index_ = 0;
+CRITICAL_SECTION ExceptionHandler::handler_stack_critical_section_;
+bool ExceptionHandler::handler_stack_critical_section_initialized_ = false;
 
 ExceptionHandler::ExceptionHandler(const wstring &dump_path,
+                                   FilterCallback filter,
                                    MinidumpCallback callback,
                                    void *callback_context,
                                    bool install_handler)
-    : callback_(callback), callback_context_(callback_context),
-      dump_path_(dump_path), dbghelp_module_(NULL),
-      minidump_write_dump_(NULL), previous_handler_(current_handler_),
-      previous_filter_(NULL) {
-  if (!handler_thread_) {
-    // The first time an ExceptionHandler is created, set up the handler
-    // thread and the synchronization primitives.
-    InitializeCriticalSection(&handler_critical_section_);
-    handler_start_semaphore_ = CreateSemaphore(NULL, 0, 1, NULL);
-    handler_finish_semaphore_ = CreateSemaphore(NULL, 0, 1, NULL);
-
-    DWORD thread_id;
-    handler_thread_ = CreateThread(NULL,         // lpThreadAttributes
-                                   64 * 1024,    // dwStackSize
-                                   ExceptionHandlerThreadMain,
-                                   NULL,         // lpParameter
-                                   0,            // dwCreationFlags
-                                   &thread_id);
-  }
+    : filter_(filter),
+      callback_(callback),
+      callback_context_(callback_context),
+      dump_path_(),
+      next_minidump_id_(),
+      next_minidump_path_(),
+      dump_path_c_(),
+      next_minidump_id_c_(NULL),
+      next_minidump_path_c_(NULL),
+      dbghelp_module_(NULL),
+      minidump_write_dump_(NULL),
+      installed_handler_(install_handler),
+      previous_filter_(NULL),
+      handler_thread_(0),
+      handler_critical_section_(),
+      handler_start_semaphore_(NULL),
+      handler_finish_semaphore_(NULL),
+      requesting_thread_id_(0),
+      exception_info_(NULL),
+      handler_return_value_(false) {
+  // set_dump_path calls UpdateNextID.  This sets up all of the path and id
+  // strings, and their equivalent c_str pointers.
+  set_dump_path(dump_path);
+
+  // Set synchronization primitives and the handler thread.  Each
+  // ExceptionHandler object gets its own handler thread, even if
+  // install_handler is false, because that's the only way to reliably
+  // guarantee sufficient stack space in an exception, and the only way to
+  // get a snapshot of the requesting thread's context outside of an
+  // exception.
+  InitializeCriticalSection(&handler_critical_section_);
+  handler_start_semaphore_ = CreateSemaphore(NULL, 0, 1, NULL);
+  handler_finish_semaphore_ = CreateSemaphore(NULL, 0, 1, NULL);
+
+  DWORD thread_id;
+  handler_thread_ = CreateThread(NULL,         // lpThreadAttributes
+                                 kExceptionHandlerThreadInitialStackSize,
+                                 ExceptionHandlerThreadMain,
+                                 this,         // lpParameter
+                                 0,            // dwCreationFlags
+                                 &thread_id);
 
-  UpdateNextID();
   dbghelp_module_ = LoadLibrary(L"dbghelp.dll");
   if (dbghelp_module_) {
     minidump_write_dump_ = reinterpret_cast<MiniDumpWriteDump_type>(
         GetProcAddress(dbghelp_module_, "MiniDumpWriteDump"));
   }
+
   if (install_handler) {
+    if (!handler_stack_critical_section_initialized_) {
+      InitializeCriticalSection(&handler_stack_critical_section_);
+      handler_stack_critical_section_initialized_ = true;
+    }
+
+    EnterCriticalSection(&handler_stack_critical_section_);
+
+    // The first time an ExceptionHandler that installs a handler is
+    // created, set up the handler stack.
+    if (!handler_stack_) {
+      handler_stack_ = new vector<ExceptionHandler *>();
+    }
+    handler_stack_->push_back(this);
     previous_filter_ = SetUnhandledExceptionFilter(HandleException);
-    current_handler_ = this;
+
+    LeaveCriticalSection(&handler_stack_critical_section_);
   }
 }
 
@@ -89,35 +124,58 @@ ExceptionHandler::~ExceptionHandler() {
   if (dbghelp_module_) {
     FreeLibrary(dbghelp_module_);
   }
-  if (current_handler_ == this) {
+
+  if (installed_handler_) {
+    EnterCriticalSection(&handler_stack_critical_section_);
+
     SetUnhandledExceptionFilter(previous_filter_);
-    current_handler_ = previous_handler_;
-  }
+    if (handler_stack_->back() == this) {
+      handler_stack_->pop_back();
+    } else {
+      // TODO(mmentovai): use advapi32!ReportEvent to log the warning to the
+      // system's application event log.
+      fprintf(stderr, "warning: removing Airbag handler out of order\n");
+      for (vector<ExceptionHandler *>::iterator iterator =
+               handler_stack_->begin();
+           iterator != handler_stack_->end();
+           ++iterator) {
+        if (*iterator == this) {
+          handler_stack_->erase(iterator);
+        }
+      }
+    }
+
+    if (handler_stack_->empty()) {
+      // When destroying the last ExceptionHandler that installed a handler,
+      // clean up the handler stack.
+      delete handler_stack_;
+      handler_stack_ = NULL;
+    }
 
-  if (previous_handler_ == NULL) {
-    // When destroying the last ExceptionHandler, clean up the handler thread
-    // and synchronization primitives.
-    TerminateThread(handler_thread_, 1);
-    handler_thread_ = NULL;
-    DeleteCriticalSection(&handler_critical_section_);
-    CloseHandle(handler_start_semaphore_);
-    handler_start_semaphore_ = NULL;
-    CloseHandle(handler_finish_semaphore_);
-    handler_finish_semaphore_ = NULL;
+    LeaveCriticalSection(&handler_stack_critical_section_);
   }
+
+  // Clean up the handler thread and synchronization primitives.
+  TerminateThread(handler_thread_, 1);
+  DeleteCriticalSection(&handler_critical_section_);
+  CloseHandle(handler_start_semaphore_);
+  CloseHandle(handler_finish_semaphore_);
 }
 
 // static
 DWORD ExceptionHandler::ExceptionHandlerThreadMain(void *lpParameter) {
+  ExceptionHandler *self = reinterpret_cast<ExceptionHandler *>(lpParameter);
+  assert(self);
+
   while (true) {
-    if (WaitForSingleObject(handler_start_semaphore_, INFINITE) ==
+    if (WaitForSingleObject(self->handler_start_semaphore_, INFINITE) ==
         WAIT_OBJECT_0) {
       // Perform the requested action.
-      handler_return_value_ = requesting_handler_->WriteMinidumpWithException(
-          requesting_thread_id_, exception_info_);
+      self->handler_return_value_ = self->WriteMinidumpWithException(
+          self->requesting_thread_id_, self->exception_info_);
 
       // Allow the requesting thread to proceed.
-      ReleaseSemaphore(handler_finish_semaphore_, 1, NULL);
+      ReleaseSemaphore(self->handler_finish_semaphore_, 1, NULL);
     }
   }
 
@@ -128,15 +186,66 @@ DWORD ExceptionHandler::ExceptionHandlerThreadMain(void *lpParameter) {
 
 // static
 LONG ExceptionHandler::HandleException(EXCEPTION_POINTERS *exinfo) {
-  return current_handler_->WriteMinidumpOnHandlerThread(exinfo) ?
-      EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH;
+  // Increment handler_stack_index_ so that if another Airbag handler is
+  // registered using this same HandleException function, and it needs to be
+  // called while this handler is running (either becaause this handler
+  // declines to handle the exception, or an exception occurs during
+  // handling), HandleException will find the appropriate ExceptionHandler
+  // object in handler_stack_ to deliver the exception to.
+  //
+  // Because handler_stack_ is addressed in reverse (as |size - index|),
+  // preincrementing handler_stack_index_ avoids needing to subtract 1 from
+  // the argument to |at|.
+  //
+  // The index is maintained instead of popping elements off of the handler
+  // stack and pushing them at the end of this method.  This avoids ruining
+  // the order of elements in the stack in the event that some other thread
+  // decides to manipulate the handler stack (such as creating a new
+  // ExceptionHandler object) while an exception is being handled.
+  EnterCriticalSection(&handler_stack_critical_section_);
+  ExceptionHandler *current_handler =
+      handler_stack_->at(handler_stack_->size() - ++handler_stack_index_);
+  LeaveCriticalSection(&handler_stack_critical_section_);
+
+  // In case another exception occurs while this handler is doing its thing,
+  // it should be delivered to the previous filter.
+  LPTOP_LEVEL_EXCEPTION_FILTER previous = current_handler->previous_filter_;
+  LPTOP_LEVEL_EXCEPTION_FILTER restore = SetUnhandledExceptionFilter(previous);
+
+  LONG action;
+  if (current_handler->WriteMinidumpOnHandlerThread(exinfo)) {
+    // The handler fully handled the exception.  Returning
+    // EXCEPTION_EXECUTE_HANDLER indicates this to the system, and usually
+    // results in the applicaiton being terminated.
+    //
+    // Note: If the application was launched from within the Cygwin
+    // environment, returning EXCEPTION_EXECUTE_HANDLER seems to cause the
+    // application to be restarted.
+    action = EXCEPTION_EXECUTE_HANDLER;
+  } else {
+    // There was an exception, but the handler decided not to handle it.
+    // This could be because the filter callback didn't want it, because
+    // minidump writing failed for some reason, or because the post-minidump
+    // callback function indicated failure.  Give the previous handler a
+    // chance to do something with the exception.  If there is no previous
+    // handler, return EXCEPTION_CONTINUE_SEARCH, which will allow a debugger
+    // or native "crashed" dialog to handle the exception.
+    action = previous ? previous(exinfo) : EXCEPTION_CONTINUE_SEARCH;
+  }
+
+  // Put things back the way they were before entering this handler.
+  SetUnhandledExceptionFilter(restore);
+  EnterCriticalSection(&handler_stack_critical_section_);
+  --handler_stack_index_;
+  LeaveCriticalSection(&handler_stack_critical_section_);
+
+  return action;
 }
 
 bool ExceptionHandler::WriteMinidumpOnHandlerThread(EXCEPTION_POINTERS *exinfo) {
   EnterCriticalSection(&handler_critical_section_);
 
   // Set up data to be passed in to the handler thread.
-  requesting_handler_ = this;
   requesting_thread_id_ = GetCurrentThreadId();
   exception_info_ = exinfo;
 
@@ -148,7 +257,6 @@ bool ExceptionHandler::WriteMinidumpOnHandlerThread(EXCEPTION_POINTERS *exinfo)
   bool status = handler_return_value_;
 
   // Clean up.
-  requesting_handler_ = NULL;
   requesting_thread_id_ = 0;
   exception_info_ = NULL;
 
@@ -167,15 +275,25 @@ bool ExceptionHandler::WriteMinidump() {
 bool ExceptionHandler::WriteMinidump(const wstring &dump_path,
                                      MinidumpCallback callback,
                                      void *callback_context) {
-  ExceptionHandler handler(dump_path, callback, callback_context, false);
+  ExceptionHandler handler(dump_path, NULL, callback, callback_context, false);
   return handler.WriteMinidump();
 }
 
 bool ExceptionHandler::WriteMinidumpWithException(DWORD requesting_thread_id,
                                                   EXCEPTION_POINTERS *exinfo) {
+  // Give user code a chance to approve or prevent writing a minidump.  If the
+  // filter returns false, don't handle the exception at all.  If this method
+  // was called as a result of an exception, returning false will cause
+  // HandleException to call any previous handler or return
+  // EXCEPTION_CONTINUE_SEARCH on the exception thread, allowing it to appear
+  // as though this handler were not present at all.
+  if (filter_&& !filter_(callback_context_)) {
+    return false;
+  }
+
   bool success = false;
   if (minidump_write_dump_) {
-    HANDLE dump_file = CreateFile(next_minidump_path_.c_str(),
+    HANDLE dump_file = CreateFile(next_minidump_path_c_,
                                   GENERIC_WRITE,
                                   FILE_SHARE_WRITE,
                                   NULL,
@@ -223,9 +341,9 @@ bool ExceptionHandler::WriteMinidumpWithException(DWORD requesting_thread_id,
   }
 
   if (callback_) {
-    callback_(next_minidump_id_, callback_context_, success);
+    success = callback_(dump_path_c_, next_minidump_id_c_, callback_context_,
+                        success);
   }
-  // TODO(bryner): log an error on failure
 
   return success;
 }
@@ -234,12 +352,14 @@ void ExceptionHandler::UpdateNextID() {
   GUID id;
   CoCreateGuid(&id);
   next_minidump_id_ = GUIDString::GUIDToWString(&id);
+  next_minidump_id_c_ = next_minidump_id_.c_str();
 
   wchar_t minidump_path[MAX_PATH];
   WindowsStringUtils::safe_swprintf(minidump_path, MAX_PATH, L"%s\\%s.dmp",
-                                    dump_path_.c_str(),
-                                    next_minidump_id_.c_str());
+                                    dump_path_c_,
+                                    next_minidump_id_c_);
   next_minidump_path_ = minidump_path;
+  next_minidump_path_c_ = next_minidump_path_.c_str();
 }
 
 }  // namespace google_airbag