Breakpad support for ASan: take 2.

Breakpad support for ASan: take 2.
This is an updated version of https://chromiumcodereview.appspot.com/10703116/
which got reverted.

When the code is built with ASan, Chrome should register a callback which notifies the browser about a memory error.
The error report message is passed to the crash handler and is then sent to the crash server along with the minidump.

BUG=143657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152989

[Alexander Potapenko, 2012-08-17 14:11:21]

Lei, can you take a look again, please?

[Lei Zhang, 2012-08-21 07:57:11]

I'm having a bit of trouble playing "spot the difference" between this CL and
the last one, since the file has changed a bit in between. Do you remember what
the compile failure was for? The buildbot logs are gone.

[Lei Zhang, 2012-08-21 08:05:12]

http://codereview.chromium.org/10860006/diff/1/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/1/chrome/app/breakpad_linux.cc#n...
chrome/app/breakpad_linux.cc:85: static const char kUploadURL[] =
no need for static anymore, here and below.

http://codereview.chromium.org/10860006/diff/1/chrome/app/breakpad_linux.cc#n...
chrome/app/breakpad_linux.cc:398: log_path[log_path_len - 1] = '\0';
Shouldn't this be just |log_path_len| ?

[Alexander Potapenko, 2012-08-21 08:07:31]

On 2012/08/21 07:57:11, Lei Zhang wrote:
> I'm having a bit of trouble playing "spot the difference" between this CL and
> the last one, since the file has changed a bit in between. Do you remember
what
> the compile failure was for? The buildbot logs are gone.

There were two problems. First, some ASan-specific code was not guarded by
#ifdefs properly. Second, g_breakpad->HandleSignal was private, so I had to add
g_breakpad->SimulateSignalDelivery in Breakpad r1008.

[Alexander Potapenko, 2012-08-21 13:37:45]

Fixed the comments.

http://codereview.chromium.org/10860006/diff/1/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/1/chrome/app/breakpad_linux.cc#n...
chrome/app/breakpad_linux.cc:85: static const char kUploadURL[] =
On 2012/08/21 08:05:12, Lei Zhang wrote:
> no need for static anymore, here and below.

Done.

http://codereview.chromium.org/10860006/diff/1/chrome/app/breakpad_linux.cc#n...
chrome/app/breakpad_linux.cc:398: log_path[log_path_len - 1] = '\0';
On 2012/08/21 08:05:12, Lei Zhang wrote:
> Shouldn't this be just |log_path_len| ?

You're right. Fixed.

[Lei Zhang, 2012-08-21 18:35:17]

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:438: g_asan_report_str = const_cast<char*>(report);
nit: Can we make |g_asan_report_str| const char and avoid the const_cast here?

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:440: g_breakpad->SimulateSignalDelivery(9);
nit: Can we use a SIGKILL constant from signal.h here?

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:603: const char* filename) {
nit: Can we move |filename| in front of the out parameters?

http://codereview.chromium.org/10860006/diff/10001/chrome/browser/crash_handl...
File chrome/browser/crash_handler_host_linux.cc (right):

http://codereview.chromium.org/10860006/diff/10001/chrome/browser/crash_handl...
chrome/browser/crash_handler_host_linux.cc:394: FILE *logfile =
fopen(log_filename.c_str(), "w");
nit: FILE*

http://codereview.chromium.org/10860006/diff/10001/chrome/browser/crash_handl...
chrome/browser/crash_handler_host_linux.cc:394: FILE *logfile =
fopen(log_filename.c_str(), "w");
Check and make sure |logfile| is not NULL?

[Alexander Potapenko, 2012-08-22 12:15:07]

Fixed the comments

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:438: g_asan_report_str = const_cast<char*>(report);
On 2012/08/21 18:35:17, Lei Zhang wrote:
> nit: Can we make |g_asan_report_str| const char and avoid the const_cast here?

Ok. This however has introduced a const_cast below.

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:440: g_breakpad->SimulateSignalDelivery(9);
On 2012/08/21 18:35:17, Lei Zhang wrote:
> nit: Can we use a SIGKILL constant from signal.h here?

Done.

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:603: const char* filename) {
On 2012/08/21 18:35:17, Lei Zhang wrote:
> nit: Can we move |filename| in front of the out parameters?

Done.

http://codereview.chromium.org/10860006/diff/10001/chrome/browser/crash_handl...
File chrome/browser/crash_handler_host_linux.cc (right):

http://codereview.chromium.org/10860006/diff/10001/chrome/browser/crash_handl...
chrome/browser/crash_handler_host_linux.cc:394: FILE *logfile =
fopen(log_filename.c_str(), "w");
On 2012/08/21 18:35:17, Lei Zhang wrote:
> nit: FILE*

Done.

http://codereview.chromium.org/10860006/diff/10001/chrome/browser/crash_handl...
chrome/browser/crash_handler_host_linux.cc:394: FILE *logfile =
fopen(log_filename.c_str(), "w");
On 2012/08/21 18:35:17, Lei Zhang wrote:
> Check and make sure |logfile| is not NULL?

Done.

[Lei Zhang, 2012-08-22 18:41:55]

lgtm

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/10001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:438: g_asan_report_str = const_cast<char*>(report);
On 2012/08/22 12:15:08, Alexander Potapenko wrote:
> On 2012/08/21 18:35:17, Lei Zhang wrote:
> > nit: Can we make |g_asan_report_str| const char and avoid the const_cast
here?
> 
> Ok. This however has introduced a const_cast below.

Blah.

http://codereview.chromium.org/10860006/diff/13001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/13001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:440: g_breakpad->SimulateSignalDelivery(SIGKILL);
#include <signal.h> ?

[Alexander Potapenko, 2012-08-23 11:26:00]

http://codereview.chromium.org/10860006/diff/13001/chrome/app/breakpad_linux.cc
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/10860006/diff/13001/chrome/app/breakpad_linux....
chrome/app/breakpad_linux.cc:440: g_breakpad->SimulateSignalDelivery(SIGKILL);
On 2012/08/22 18:41:56, Lei Zhang wrote:
> #include <signal.h> ?

Done.