Mac: Add support for in-process crash reporting

Mac: Add support for in-process crash reporting to Breakpad.

Add new option BREAKPAD_IN_PROCESS.
If YES, Breakpad will write the dump file in-process and then launch the reporter
executable as a child  process.

BUG=414239
Moved to https://breakpad.appspot.com/1714002/

[Andre, 2014-09-12 18:04:58]

andresantoso@chromium.org changed reviewers:
+ mark@chromium.org

[Andre, 2014-09-12 18:04:59]

Hi Mark, I'm still working on this but I thought it would be good to get your
early feedback if I'm on the right track or not.

https://codereview.chromium.org/571523004/diff/1/client/mac/Framework/Breakpa...
File client/mac/Framework/Breakpad.mm (right):

https://codereview.chromium.org/571523004/diff/1/client/mac/Framework/Breakpa...
client/mac/Framework/Breakpad.mm:763: //     config_file.GetFilePath());
This was my first naive attempt, but it didn't link.
I'm still looking into it, but let me know if you have advice.

[Andre, 2014-09-12 18:22:00]

https://codereview.chromium.org/571523004/diff/1/client/mac/Framework/Breakpa...
File client/mac/Framework/Breakpad.mm (right):

https://codereview.chromium.org/571523004/diff/1/client/mac/Framework/Breakpa...
client/mac/Framework/Breakpad.mm:763: //     config_file.GetFilePath());
On 2014/09/12 18:04:59, Andre wrote:
> This was my first naive attempt, but it didn't link.
> I'm still looking into it, but let me know if you have advice.
> 

I'm considering promoting ConfigFile and LaunchReporter to common/mac and
linking to it from both the Breakpad framework and the Inspector tool.

[Andre, 2014-09-12 21:11:59]

Uploaded patchset 2 with implementation of reporter launching.
Tested with staging server, results here:
https://crash-staging.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%2...

[Mark Mentovai, 2014-09-12 21:58:21]

Yes! This is absolutely heading in the right direction. Let me know when you’d
like a full review. Should be easy.

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
File common/mac/launch_reporter.cc (right):

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
common/mac/launch_reporter.cc:37: const char *argv[3];
Don’t line this up with the parameters: that makes it look like a parameter too.

const char* argv[] = { reporterExecutablePath, configFilePath, NULL }; is a
little cleaner than this, too.

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
common/mac/launch_reporter.cc:49: unlink(configFilePath);  // launch failed -
get rid of config file
fprintf something to stderr here about exec’s failure, too?

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
common/mac/launch_reporter.cc:61: pid_t result = waitpid(pid, &status, WNOHANG);
I know that this is old code that you’re just moving, but it’s kind of weird…but
since it’s old code, I guess it’s all right. We’re not looking to reinvent the
world here.

[Andre, 2014-09-12 22:53:33]

Patchset #3 (id:40001) has been deleted

[Andre, 2014-09-12 22:56:56]

Patchset #3 (id:60001) has been deleted

[Andre, 2014-09-12 22:59:40]

Thanks Mark.
I've uploaded patchset 3, let's start the full review.
The gyp/gn changes are in the other patch (http://crrev.com/571663002/), so the
build will break if they don't land together. What's the best way to deal with
this?

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
File common/mac/launch_reporter.cc (right):

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
common/mac/launch_reporter.cc:37: const char *argv[3];
On 2014/09/12 21:58:21, Mark Mentovai wrote:
> Don’t line this up with the parameters: that makes it look like a parameter
too.
> 
> const char* argv[] = { reporterExecutablePath, configFilePath, NULL }; is a
> little cleaner than this, too.

Done.

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
common/mac/launch_reporter.cc:49: unlink(configFilePath);  // launch failed -
get rid of config file
On 2014/09/12 21:58:21, Mark Mentovai wrote:
> fprintf something to stderr here about exec’s failure, too?

Done.

https://codereview.chromium.org/571523004/diff/20001/common/mac/launch_report...
common/mac/launch_reporter.cc:61: pid_t result = waitpid(pid, &status, WNOHANG);
On 2014/09/12 21:58:21, Mark Mentovai wrote:
> I know that this is old code that you’re just moving, but it’s kind of
weird…but
> since it’s old code, I guess it’s all right. We’re not looking to reinvent the
> world here.

Acknowledged.

[Mark Mentovai, 2014-09-12 23:01:04]

We’ll land this side first, and we’ll land the other side along with a DEPS roll
to pull in the new version of Breakpad.

[Andre, 2014-09-12 23:03:38]

On 2014/09/12 23:01:04, Mark Mentovai wrote:
> We’ll land this side first, and we’ll land the other side along with a DEPS
roll
> to pull in the new version of Breakpad.

But will Breakpad itself build without the other side?
Inspector.mm will be looking for the LaunchReporter function.
launch_reporter.cc will not get compiled and linked without the gyp change
right?

[Mark Mentovai, 2014-09-15 13:27:17]

The .gyp files in Chromium dictate how Breakpad operates in Chromium. For
Breakpad’s own standalone build, you’ll need to make changes to
breakpad/src/client/mac/Breakpad.xcodeproj. You may find that this .xcodeproj is
currently broken. It’s not used by Chromium, and I’m not sure if it’s used by
any other project, other than to test standalone Breakpad.

[Mark Mentovai, 2014-09-15 13:38:44]

This is in pretty good shape already, most of the comments amount to minor nits.

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
File client/mac/Framework/Breakpad.mm (right):

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:199: void *context, bool succeeded);
If you have to break parameters up to take more than one line, you should put
each parameter on its own line, otherwise the last two melt into one another.

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:201: bool HandleMinidump(const char *dump_dir,
This should also have a comment explaining that it’s only used with
BREAKPAD_IN_PROCESS.

It might be better to name it more descriptively, like
WriteMinidumpInProcessAndLaunchReporter().

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:202: const char *minidump_id);
Nit (may be irrelevant if the method is renamed): it doesn’t look like you
actually needed to put this one on its own line at all.

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:284: void *context, bool succeeded) {
As above.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
File common/mac/launch_reporter.cc (right):

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.cc:30: #include <cstdio>
<cstdio> only guarantees that you’ll have names in the std namespace, such as
std::fprintf. You really want <stdio.h> here.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.cc:47: fprintf(stderr, "Failed to launch reporter
process\n");
Gimme a strerror(errno) or a perror("exec"), and maybe include the
reporterExecutablePath, so that we know why it failed when this happens.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.cc:52: // Wait until the Reporter child process
exits.
fork() may have returned -1 indicating failure. In that case, there’s no child
process.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
File common/mac/launch_reporter.h (right):

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.h:29: 
This file needs #include guards.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.h:35: void LaunchReporter(const char
*reporterExecutablePath,
You’re not supposed to use camelCase naming for parameter names. Use
reporter_executable_path, etc. Same in the comment above and in the .cc file.

[Andre, 2014-09-15 17:32:48]

Patchset #5 (id:120001) has been deleted

[Andre, 2014-09-15 17:37:32]

Patchset #4 (id:100001) has been deleted

[Andre, 2014-09-15 17:42:39]

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
File client/mac/Framework/Breakpad.mm (right):

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:199: void *context, bool succeeded);
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> If you have to break parameters up to take more than one line, you should put
> each parameter on its own line, otherwise the last two melt into one another.

Done.

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:201: bool HandleMinidump(const char *dump_dir,
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> This should also have a comment explaining that it’s only used with
> BREAKPAD_IN_PROCESS.
> 
> It might be better to name it more descriptively, like
> WriteMinidumpInProcessAndLaunchReporter().

Done.
This method does not actually write the minidump file (the minidump was already
written by
ExceptionHandler before this callback in invoked), so WriteMinidump may not be a
good name.
I'm leaning towards keeping the name HandleMinidump (and it matches iOS
breakpad), WDYT?

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:202: const char *minidump_id);
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> Nit (may be irrelevant if the method is renamed): it doesn’t look like you
> actually needed to put this one on its own line at all.

Done.

https://codereview.chromium.org/571523004/diff/80001/client/mac/Framework/Bre...
client/mac/Framework/Breakpad.mm:284: void *context, bool succeeded) {
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> As above.

Done.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
File common/mac/launch_reporter.cc (right):

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.cc:30: #include <cstdio>
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> <cstdio> only guarantees that you’ll have names in the std namespace, such as
> std::fprintf. You really want <stdio.h> here.

Done.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.cc:47: fprintf(stderr, "Failed to launch reporter
process\n");
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> Gimme a strerror(errno) or a perror("exec"), and maybe include the
> reporterExecutablePath, so that we know why it failed when this happens.

Done.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.cc:52: // Wait until the Reporter child process
exits.
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> fork() may have returned -1 indicating failure. In that case, there’s no child
> process.

Done.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
File common/mac/launch_reporter.h (right):

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.h:29: 
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> This file needs #include guards.

Done.

https://codereview.chromium.org/571523004/diff/80001/common/mac/launch_report...
common/mac/launch_reporter.h:35: void LaunchReporter(const char
*reporterExecutablePath,
On 2014/09/15 13:38:44, Mark Mentovai wrote:
> You’re not supposed to use camelCase naming for parameter names. Use
> reporter_executable_path, etc. Same in the comment above and in the .cc file.

Done.
Renamed the local variable timeoutCount to timeout_count as well.

[Mark Mentovai, 2014-09-15 19:15:45]

LGTM

I don’t like the Handle naming because it makes it sound like it’s doing
something interesting with the minidump, but all it’s really doing with it is
triggering the uploader. I suppose it’s fine to leave as-is for consistency. I
don’t think that the Mac OS X code is going to live for too much longer.

https://codereview.chromium.org/571523004/diff/140001/common/mac/launch_repor...
File common/mac/launch_reporter.h (right):

https://codereview.chromium.org/571523004/diff/140001/common/mac/launch_repor...
common/mac/launch_reporter.h:41: } // namespace google_breakpad
Lost a space here—there are supposed to be two between the } and //.

[Andre, 2014-09-15 20:16:32]

https://codereview.chromium.org/571523004/diff/140001/common/mac/launch_repor...
File common/mac/launch_reporter.h (right):

https://codereview.chromium.org/571523004/diff/140001/common/mac/launch_repor...
common/mac/launch_reporter.h:41: } // namespace google_breakpad
On 2014/09/15 19:15:44, Mark Mentovai wrote:
> Lost a space here—there are supposed to be two between the } and //.

Done.

[Andre, 2014-09-15 20:19:55]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-09-15 20:22:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 20:22:49]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Andre, 2014-09-15 20:37:55]

Mark, can you tell me how to land this patch?
Do we use CQ?

[Mark Mentovai, 2014-09-15 20:39:47]

No commit queue for Breakpad. You have to land it manually. “gcl commit” or “git
cl land”. I’ve given you write access to the Breakpad repository, see
https://code.google.com/p/google-breakpad/source/checkout to get your svn
password.

[Andre, 2014-09-15 21:18:38]

I'm getting "Git access forbidden" when trying to run "git cl land".
What am I doing wrong?

fatal: remote error: Git access forbidden
Failed to push. If this persists, please file a bug.

[Mark Mentovai, 2014-09-15 21:33:20]

Is the svn side of it using https://? If it’s only using http://, you won’t be
able to commit.