diff options
Diffstat (limited to 'src/client/windows/handler')
| -rw-r--r-- | src/client/windows/handler/exception_handler.cc | 57 | ||||
| -rw-r--r-- | src/client/windows/handler/exception_handler.h | 13 |
2 files changed, 55 insertions, 15 deletions
diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc index 7217e19d..48cb8df3 100644 --- a/src/client/windows/handler/exception_handler.cc +++ b/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_) { - InitializeCriticalSection(&handler_stack_critical_section_); - handler_stack_critical_section_initialized_ = true; - } + // 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_); + } + 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; } diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h index d02f7080..2cacdc38 100644 --- a/src/client/windows/handler/exception_handler.h +++ b/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 &); |