Don't do work inside assert(). Ever.

Don't do work inside assert(). Ever.

The Mac crash key manipulation code was intended to be thread-safe through the
provision of a mutex. The mutex operations were done inside an assert().
assert() is a no-op in NDEBUG (release) builds. Therefore, in release builds,
these operations were occurring without being protected by any mutex at all,
and were nowhere near thread-safe.

BUG=chromium:331268
R=rsesek@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1270

[Mark Mentovai]

I looked through all of the other uses of assert() in the rest of Breakpad and
they look like they’re all actually just asserting things instead of doing real
work.

[rsesek]

LGTM. This has bitten me before with DCHECK.

[Mark Mentovai]

https://breakpad.appspot.com/1034002/diff/1/src/client/mac/Framework/Breakpad.mm
File src/client/mac/Framework/Breakpad.mm (left):

https://breakpad.appspot.com/1034002/diff/1/src/client/mac/Framework/Breakpad...
src/client/mac/Framework/Breakpad.mm:108: if (allocator_ ) {
I got rid of the condition here and on old line 115, and the comment above on
old line 99, because nothing ever called this with allocator_ being NULL. In
fact, everything that creates a ProtectedMemoryLocker explicitly checks that the
thing being passed in for allocator_ is non-NULL.

[rsesek]

https://breakpad.appspot.com/1034002/diff/1/src/client/mac/Framework/Breakpad.mm
File src/client/mac/Framework/Breakpad.mm (left):

https://breakpad.appspot.com/1034002/diff/1/src/client/mac/Framework/Breakpad...
src/client/mac/Framework/Breakpad.mm:108: if (allocator_ ) {
On 2014/01/10 19:37:46, Mark Mentovai wrote:
> I got rid of the condition here and on old line 115, and the comment above on
> old line 99, because nothing ever called this with allocator_ being NULL. In
> fact, everything that creates a ProtectedMemoryLocker explicitly checks that
the
> thing being passed in for allocator_ is non-NULL.

What would this be protecting if the allocator_ were NULL, anyways?

[Mark Mentovai]

I guess the original thought was that it would just be a mutex in that case. I
don’t know. It doesn’t make any sense given how the code works now.

[Mark Mentovai]

Committed patchset #1 manually as r1270 (presubmit successful).