race condition between ExceptionHandler::Teardown and ExceptionHandler::WaitForMessage

[GoogleCodeExporter]

If you delete an ExceptionHandler object on mac, the destructor calls
::Teardown, which calls ::SendEmptyMachMessage, which signals the handler
thread to shut down. Unfortunately, the mach_msg call only blocks until the
message is received by the other thread, then the two threads will race. If
the main thread finishes the destructor before the handler thread gets to
run, then the handler thread will touch deallocated memory when it checks
self->is_in_teardown_.

valgrind picks this up when running one of our unit tests that sets/unsets
the exception handler:
==46682== Thread 5:
==46682== Invalid read of size 1
==46682==    at 0x2E130BE:
google_breakpad::ExceptionHandler::WaitForMessage(void*) (in
/Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x605154: _pthread_start (in /usr/lib/libSystem.B.dylib)
==46682==    by 0x605011: thread_start (in /usr/lib/libSystem.B.dylib)
==46682==  Address 0xafdcff5 is 53 bytes inside a block of size 104 free'd
==46682==    at 0x3FB1A: operator delete(void*) (vg_replace_malloc.c:346)
==46682==    by 0x2E0DFC9: CrashReporter::UnsetExceptionHandler() (in
/Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x2DE7481: nsXULAppInfo::SetEnabled(int) (in
/Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x3F9F142: NS_InvokeByIndex_P (in
/Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x2E70CAF: XPCWrappedNative::CallMethod(XPCCallContext&,
XPCWrappedNative::CallMode) (in /Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x2E81CA0: XPCWrappedNative::SetAttribute(XPCCallContext&) (in
/Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x2E7D30E: XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned
int, long*, long*) (in /Users/sayrer/dev/mc-debug/toolkit/library/XUL)
==46682==    by 0x33E6E2: js_Invoke (in
/Users/sayrer/dev/mc-debug/js/src/libmozjs.dylib)
==46682==    by 0x33E9FA: js_InternalInvoke (in
/Users/sayrer/dev/mc-debug/js/src/libmozjs.dylib)
==46682==    by 0x33EC42: js_InternalGetOrSet (in
/Users/sayrer/dev/mc-debug/js/src/libmozjs.dylib)
==46682==    by 0x34C02E: js_SetSprop (in
/Users/sayrer/dev/mc-debug/js/src/libmozjs.dylib)
==46682==    by 0x357C20: js_NativeSet (in
/Users/sayrer/dev/mc-debug/js/src/libmozjs.dylib)
==46682== 

I don't know what the right solution is here. Clearly we need some sort of
synchronization to prevent this from happening. This also manifests in our
unit tests as occasional crashes.

Original issue reported on code.google.com by ted.mielczarek on 10 Sep 2009 at 7:21

[GoogleCodeExporter]

Patch at: http://breakpad.appspot.com/165001

Original comment by ted.mielczarek on 23 Aug 2010 at 7:19

• Changed state: Started

[GoogleCodeExporter]

Fixed in r744.

Original comment by ted.mielczarek on 15 Dec 2010 at 5:29

• Changed state: Fixed