Minor fixes to windows exception handler.

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

src/client/windows/handler/exception_handler.cc

@@ -40,12 +40,13 @@
 
 namespace google_breakpad {
 
+static const int kWaitForHandlerThreadMs = 60000;
 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;
+volatile LONG ExceptionHandler::instance_count_ = 0;
 
 ExceptionHandler::ExceptionHandler(const wstring& dump_path,
                                    FilterCallback filter,
@@ -88,6 +89,7 @@ void ExceptionHandler::Initialize(const wstring& dump_path,
                                   MINIDUMP_TYPE dump_type,
                                   const wchar_t* pipe_name,
                                   const CustomClientInfo* custom_info) {
+  LONG instance_count = InterlockedIncrement(&instance_count_);
   filter_ = filter;
   callback_ = callback;
   callback_context_ = callback_context;
@@ -106,6 +108,7 @@ void ExceptionHandler::Initialize(const wstring& dump_path,
 #endif  // _MSC_VER >= 1400
   previous_pch_ = NULL;
   handler_thread_ = NULL;
+  is_shutdown_ = false;
   handler_start_semaphore_ = NULL;
   handler_finish_semaphore_ = NULL;
   requesting_thread_id_ = 0;
@@ -177,12 +180,22 @@ void ExceptionHandler::Initialize(const wstring& dump_path,
     set_dump_path(dump_path);
   }
 
-  if (handler_types != HANDLER_NONE) {
-    if (!handler_stack_critical_section_initialized_) {
+  // There is a race condition here. If the first instance has not yet
+  // initialized the critical section, the second (and later) instances will
+  // try to use uninitialized critical section object. The featuer of multiple
+  // instnaces in one module is not used much, so leave it as is for now.
+  // One way to solve this in the current design (that is, keeping the static
+  // handler stack) is to use spin locks with volatile bools to synchronize
+  // the handler stack. This works only if the compiler guarntees to generate
+  // cache coherent code for volatile.
+  // TODO(munjal): Fix this in a better way by changing the design if possible.
+
+  // Lazy initialization of the handler_stack_critical_section_
+  if (instance_count == 1) {
     InitializeCriticalSection(&handler_stack_critical_section_);
-      handler_stack_critical_section_initialized_ = true;
   }
 
+  if (handler_types != HANDLER_NONE) {
     EnterCriticalSection(&handler_stack_critical_section_);
 
     // The first time an ExceptionHandler that installs a handler is
@@ -259,12 +272,27 @@ ExceptionHandler::~ExceptionHandler() {
   // Some of the objects were only initialized if out of process
   // registration was not done.
   if (!IsOutOfProcess()) {
-    // Clean up the handler thread and synchronization primitives.
-    TerminateThread(handler_thread_, 1);
+    // Clean up the handler thread and synchronization primitives. The handler
+    // thread is either waiting on the semaphore to handle a crash or it is
+    // handling a crash. Coming out of the wait is fast but wait more in the
+    // eventuality a crash is handled.
+    is_shutdown_ = true;
+    ReleaseSemaphore(handler_start_semaphore_, 1, NULL);
+    WaitForSingleObject(handler_thread_, kWaitForHandlerThreadMs);
     DeleteCriticalSection(&handler_critical_section_);
     CloseHandle(handler_start_semaphore_);
     CloseHandle(handler_finish_semaphore_);
   }
+
+  // There is a race condition in the code below: if this instance is
+  // deleting the static critical section and a new instance of the class
+  // is created, then there is a possibility that the critical section be
+  // initialized while the same critical section is being deleted. Given the
+  // usage pattern for the code, this race condition is unlikely to hit, but it
+  // is a race condition nonetheless.
+  if (InterlockedDecrement(&instance_count_) == 0) {
+    DeleteCriticalSection(&handler_stack_critical_section_);
+  }
 }
 
 // static
@@ -278,16 +306,21 @@ DWORD ExceptionHandler::ExceptionHandlerThreadMain(void* lpParameter) {
     if (WaitForSingleObject(self->handler_start_semaphore_, INFINITE) ==
         WAIT_OBJECT_0) {
       // Perform the requested action.
-      self->handler_return_value_ = self->WriteMinidumpWithException(
-          self->requesting_thread_id_, self->exception_info_, self->assertion_);
+      if (self->is_shutdown_) {
+        // The instance of the exception handler is being destroyed.
+        break;
+      } else {
+        self->handler_return_value_ =
+            self->WriteMinidumpWithException(self->requesting_thread_id_,
+                                             self->exception_info_,
+                                             self->assertion_);
+      }
 
       // Allow the requesting thread to proceed.
       ReleaseSemaphore(self->handler_finish_semaphore_, 1, NULL);
     }
   }
 
-  // Not reached.  This thread will be terminated by ExceptionHandler's
-  // destructor.
   return 0;
 }
 

src/client/windows/handler/exception_handler.h

@@ -340,6 +340,12 @@ class ExceptionHandler {
   // The exception handler thread.
   HANDLE handler_thread_;
 
+  // True if the exception handler is being destroyed.
+  // Starting with MSVC 2005, Visual C has stronger guarantees on volatile vars.
+  // It has release semantics on write and acquire semantics on reads.
+  // See the msdn documentation.
+  volatile bool is_shutdown_;
+
   // The critical section enforcing the requirement that only one exception be
   // handled by a handler at a time.
   CRITICAL_SECTION handler_critical_section_;
@@ -390,11 +396,12 @@ class ExceptionHandler {
   static LONG handler_stack_index_;
 
   // handler_stack_critical_section_ guards operations on handler_stack_ and
-  // handler_stack_index_.
+  // handler_stack_index_. The critical section is initialized by the
+  // first instance of the class and destroyed by the last instance of it.
   static CRITICAL_SECTION handler_stack_critical_section_;
 
-  // True when handler_stack_critical_section_ has been initialized.
-  static bool handler_stack_critical_section_initialized_;
+  // The number of instances of this class.
+  volatile static LONG instance_count_;
 
   // disallow copy ctor and operator=
   explicit ExceptionHandler(const ExceptionHandler &);