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

2 files changed, 270 insertions, 82 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
diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h
index b24a37b7..f5ca31b9 100644
--- a/src/client/windows/handler/exception_handler.h
+++ b/src/client/windows/handler/exception_handler.h
@@ -67,27 +67,57 @@
 #pragma warning( disable : 4530 ) 
 
 #include <string>
+#include <vector>
 
 namespace google_airbag {
 
+using std::vector;
 using std::wstring;
 
 class ExceptionHandler {
  public:
+  // A callback function to run before Airbag performs any substantial
+  // processing of an exception.  A FilterCallback is called before writing
+  // a minidump.  context is the parameter supplied by the user as
+  // callback_context when the handler was created.
+  //
+  // If a FilterCallback returns true, Airbag will continue processing,
+  // attempting to write a minidump.  If a FilterCallback returns false, Airbag
+  // will immediately report the exception as unhandled without writing a
+  // minidump, allowing another handler the opportunity to handle it.
+  typedef bool (*FilterCallback)(void *context);
+
   // A callback function to run after the minidump has been written.
   // minidump_id is a unique id for the dump, so the minidump
-  // file is <dump_path>\<minidump_id>.dmp.  succeeded indicates whether
-  // a minidump file was successfully written.
-  typedef void (*MinidumpCallback)(const wstring &minidump_id,
-                                   void *context, bool succeeded);
+  // file is <dump_path>\<minidump_id>.dmp.  context is the parameter supplied
+  // by the user as callback_context when the handler was created.  succeeded
+  // indicates whether a minidump file was successfully written.
+  //
+  // If an exception occurred and the callback returns true, Airbag will treat
+  // the exception as fully-handled, suppressing any other handlers from being
+  // notified of the exception.  If the callback returns false, Airbag will
+  // treat the exception as unhandled, and allow another handler to handle it.
+  // If there are no other handlers, Airbag will report the exception to the
+  // system as unhandled, allowing a debugger or native crash dialog the
+  // opportunity to handle the exception.  Most callback implementations
+  // should normally return the value of |succeeded|, or when they wish to
+  // not report an exception of handled, false.  Callbacks will rarely want to
+  // return true directly (unless |succeeded| is true).
+  typedef bool (*MinidumpCallback)(const wchar_t *dump_path,
+                                   const wchar_t *minidump_id,
+                                   void *context,
+                                   bool succeeded);
 
   // Creates a new ExceptionHandler instance to handle writing minidumps.
+  // Before writing a minidump, the optional filter callback will be called.
+  // Its return value determines whether or not Airbag should write a minidump.
   // Minidump files will be written to dump_path, and the optional callback
   // is called after writing the dump file, as described above.
   // If install_handler is true, then a minidump will be written whenever
   // an unhandled exception occurs.  If it is false, minidumps will only
   // be written when WriteMinidump is called.
-  ExceptionHandler(const wstring &dump_path, MinidumpCallback callback,
+  ExceptionHandler(const wstring &dump_path,
+                   FilterCallback filter, MinidumpCallback callback,
                    void *callback_context, bool install_handler);
   ~ExceptionHandler();
 
@@ -95,6 +125,7 @@ class ExceptionHandler {
   wstring dump_path() const { return dump_path_; }
   void set_dump_path(const wstring &dump_path) {
     dump_path_ = dump_path;
+    dump_path_c_ = dump_path_.c_str();
     UpdateNextID();  // Necessary to put dump_path_ in next_minidump_path_.
   }
 
@@ -147,56 +178,93 @@ class ExceptionHandler {
   // path of the next minidump to be written in next_minidump_path_.
   void UpdateNextID();
 
+  FilterCallback filter_;
   MinidumpCallback callback_;
   void *callback_context_;
 
+  // The directory in which a minidump will be written, set by the dump_path
+  // argument to the constructor, or set_dump_path.
   wstring dump_path_;
+
+  // The basename of the next minidump to be written, without the extension.
   wstring next_minidump_id_;
+
+  // The full pathname of the next minidump to be written, including the file
+  // extension.
   wstring next_minidump_path_;
 
+  // Pointers to C-string representations of the above.  These are set when
+  // the above wstring versions are set in order to avoid calling c_str during
+  // an exception, as c_str may attempt to allocate heap memory.  These
+  // pointers are not owned by the ExceptionHandler object, but their lifetimes
+  // should be equivalent to the lifetimes of the associated wstring, provided
+  // that the wstrings are not altered.
+  const wchar_t *dump_path_c_;
+  const wchar_t *next_minidump_id_c_;
+  const wchar_t *next_minidump_path_c_;
+
   HMODULE dbghelp_module_;
   MiniDumpWriteDump_type minidump_write_dump_;
 
-  ExceptionHandler *previous_handler_;  // current_handler_ before us
+  // True if the ExceptionHandler installed an unhandled exception filter

+  // when created (with an install_handler parameter set to true).

+  bool installed_handler_;

+

+  // When installed_handler_ is true, previous_filter_ is the unhandled

+  // exception filter that was set prior to installing ExceptionHandler as

+  // the unhandled exception filter and pointing it to |this|.  NULL indicates

+  // that there is no previous unhandled exception filter.

   LPTOP_LEVEL_EXCEPTION_FILTER previous_filter_;
-
-  // the currently-installed ExceptionHandler, of which there can be only 1
-  static ExceptionHandler *current_handler_;
-
-  // The exception handler thread, if one has been created.
-  static HANDLE handler_thread_;
+

+  // The exception handler thread.
+  HANDLE handler_thread_;
 
   // The critical section enforcing the requirement that only one exception be
-  // handled at a time.
-  static CRITICAL_SECTION handler_critical_section_;
+  // handled by a handler at a time.
+  CRITICAL_SECTION handler_critical_section_;
 
   // Semaphores used to move exception handling between the exception thread
   // and the handler thread.  handler_start_semaphore_ is signalled by the
   // exception thread to wake up the handler thread when an exception occurs.
   // handler_finish_semaphore_ is signalled by the handler thread to wake up
   // the exception thread when handling is complete.
-  static HANDLE handler_start_semaphore_;
-  static HANDLE handler_finish_semaphore_;
+  HANDLE handler_start_semaphore_;
+  HANDLE handler_finish_semaphore_;
 
-  // The next 3 fields are static data passed from the requesting thread to
+  // The next 2 fields contain data passed from the requesting thread to
   // the handler thread.
 
-  // The ExceptionHandler through which a request to write a dump was routed.
-  // This will be the same as current_handler_ for exceptions, but
-  // user-requested dumps may be routed through any live ExceptionHandler.
-  static ExceptionHandler *requesting_handler_;
-
   // The thread ID of the thread requesting the dump (either the exception
   // thread or any other thread that called WriteMinidump directly).
-  static DWORD requesting_thread_id_;
+  DWORD requesting_thread_id_;
 
   // The exception info passed to the exception handler on the exception
   // thread, if an exception occurred.  NULL for user-requested dumps.
-  static EXCEPTION_POINTERS *exception_info_;
+  EXCEPTION_POINTERS *exception_info_;
 
   // The return value of the handler, passed from the handler thread back to
   // the requesting thread.
-  static bool handler_return_value_;
+  bool handler_return_value_;
+
+  // A stack of ExceptionHandler objects that have installed unhandled

+  // exception filters.  This vector is used by HandleException to determine

+  // which ExceptionHandler object to route an exception to.  When an

+  // ExceptionHandler is created with install_handler true, it will append

+  // itself to this list.

+  static vector<ExceptionHandler *> *handler_stack_;

+

+  // The index of the ExceptionHandler in handler_stack_ that will handle the

+  // next exception.  Note that 0 means the last entry in handler_stack_, 1

+  // means the next-to-last entry, and so on.  This is used by HandleException

+  // to support multiple stacked Airbag handlers.

+  static LONG handler_stack_index_;

+

+  // handler_stack_critical_section_ guards operations on handler_stack_ and

+  // handler_stack_index_.

+  static CRITICAL_SECTION handler_stack_critical_section_;
+
+  // True when handler_stack_critical_section_ has been initialized.
+  static bool handler_stack_critical_section_initialized_;
 
   // disallow copy ctor and operator=
   explicit ExceptionHandler(const ExceptionHandler &);