Implement CrashGeneration{Server,Client}, OOP ExceptionHandler for Mac

This patch implements a CrashGenerationServer and CrashGenerationClient for OS X
like we currently have on Windows and Linux. It also adds support for using
ExceptionHandler to communicate to a CrashGenerationServer for out-of-process
minidump generation. The IPC code is copied from the Breakpad framework right
now. I'd like to refactor the Breakpad framework to use an OOP ExceptionHandler,
and have the Inspector use a CrashGenerationServer so they can share the IPC
code, but I think that's best done as a followup.

[Mark Mentovai]

Preliminaries. It’s lunchtime. I’ll read more later.

http://breakpad.appspot.com/146001/diff/1/3
File src/client/mac/crash_generation/client_info.h (right):

http://breakpad.appspot.com/146001/diff/1/3#newcode33
Line 33: #include <stdint.h>
Not needed.

http://breakpad.appspot.com/146001/diff/1/3#newcode38
Line 38: int           exception_type;
Excess horizontal whitespace not helpful.

http://breakpad.appspot.com/146001/diff/1/3#newcode43
Line 43: }
}  // namespace google_breakpad

http://breakpad.appspot.com/146001/diff/1/4
File src/client/mac/crash_generation/crash_generation_client.h (right):

http://breakpad.appspot.com/146001/diff/1/4#newcode38
Line 38: public:
1-space indent on public:, protected:, and private:. Same on line 52.

http://breakpad.appspot.com/146001/diff/1/4#newcode39
Line 39: CrashGenerationClient(const char* mach_port_name) :
sender_(mach_port_name) {
This constructor should be marked explicit.

sender_ is MachPortSender, but mach_port_name is const char*. That’s cause for
concern.

(…investigation ensues…)

MachPortSender does have a constructor accepting const char*. It’s apparently
not marked explicit, either. It should be.

This should be written as sender_(MachPortSender(mach_port_name)).

http://breakpad.appspot.com/146001/diff/1/4#newcode45
Line 45: bool RequestDump(int exception_type, int exception_code, int
exception_subcode,
80 columns.

http://breakpad.appspot.com/146001/diff/1/4#newcode48
Line 48: bool RequestDump() {
Is overloading RequestDump by name helpful? Can we use RequestDumpForException
(or something) for the other one?

http://breakpad.appspot.com/146001/diff/1/4#newcode49
Line 49: return RequestDump(0, 0, 0, 0);
The fourth parameter should be MACH_PORT_NULL, not 0.

http://breakpad.appspot.com/146001/diff/1/4#newcode55
Line 55: // prevent copy construction and assignment
Prevent should have a capital P. Assignment should have a period after it.

http://breakpad.appspot.com/146001/diff/1/5
File src/client/mac/crash_generation/crash_generation_client.mm (right):

http://breakpad.appspot.com/146001/diff/1/5#newcode31
Line 31: #include "client/mac/crash_generation/crash_generation_client.h"
This #include should come before the others (because it’s the #include for this
file) and there should be a blank line between it and the other #includes.

http://breakpad.appspot.com/146001/diff/1/5#newcode37
Line 37: bool CrashGenerationClient::RequestDump(int exception_type,
Evidently, this will run inside the crashed process. Have you examined
everything that it uses (ReceivePort, MachSendMessage, etc.) to make sure that
they’re exception-time-safe? No memory allocation, etc.?

http://breakpad.appspot.com/146001/diff/1/5#newcode46
Line 46: message.AddDescriptor(mach_task_self());            // our task
Remember how I feel about “we” and “our” in comments. I don’t even really know
what “our task” is supposed to mean in the context of multiple tasks, IPC, etc.
I know what “this task” means.

http://breakpad.appspot.com/146001/diff/1/5#newcode57
Line 57: kern_return_t result = sender_.SendMessage(message, 2000);
These are timeouts. It’d be nice to write them symbolically so that the code is
a little more self-documenting.

  const mach_msg_timeout_t kSendTimeoutMs = 2 * 1000;
  kern_return_t result = sender_.SendMessage(message, kSendTimeoutMs);

Same on line 62.

http://breakpad.appspot.com/146001/diff/1/5#newcode67
Line 67: }  // namespace google_breakpad
There’s no good reason for this file to be .mm. It has no Objectiveness. You can
make it .cc.

http://breakpad.appspot.com/146001/diff/1/5#newcode68
Line 68: 
Blank line at EOF.

http://breakpad.appspot.com/146001/diff/1/6
File src/client/mac/crash_generation/crash_generation_server.h (right):

http://breakpad.appspot.com/146001/diff/1/6#newcode41
Line 41: // Messages the server can read via its mach port
Would this be better as a named enum?

http://breakpad.appspot.com/146001/diff/1/6#newcode49
Line 49: public:
1-space me. Line 91 also.

http://breakpad.appspot.com/146001/diff/1/6#newcode62
Line 62: // Parameter mach_port_name: Named server port to listen on.
These are obviously parameters. Save some space and don’t say “parameter” each
time. Or, if you like, you can write “Parameters:” once and then list them all
beneath that.

http://breakpad.appspot.com/146001/diff/1/6#newcode97
Line 97: static void *WaitForMessage(void *server);
The comment implies that this should be named WaitForMessages, plural.

http://breakpad.appspot.com/146001/diff/1/6#newcode125
Line 125: // disable copy constructor and operator=
As before.

[Mark Mentovai]

http://breakpad.appspot.com/146001/diff/1/7
File src/client/mac/crash_generation/crash_generation_server.mm (right):

http://breakpad.appspot.com/146001/diff/1/7#newcode1
Line 1: // Copyright (c) 2010 Google Inc.
This does not need to be an .mm file. .cc is fine.

http://breakpad.appspot.com/146001/diff/1/7#newcode37
Line 37: const char* mach_port_name,
4-space indent on continuation lines. This should be:

CrashGenerationServer::CrashGenerationServer(
    const char* mach_port_name,
    ...

http://breakpad.appspot.com/146001/diff/1/7#newcode43
Line 43: const std::string* dump_path) :
Usually read-only input parameters are passed by const reference, not const
pointer. But maybe you explicitly wanted to allow this to be NULL to mean
something special? (As opposed to empty?)

http://breakpad.appspot.com/146001/diff/1/7#newcode44
Line 44: dump_callback_(dump_callback),
And you should use the style guide-approved format here, too.  Four spaces on
the line before the :, as in:

    const std::string* dump_path)
    : dump_callback_(dump_callback),
      dump_context_(dump_context),
      ...

http://breakpad.appspot.com/146001/diff/1/7#newcode50
Line 50: receive_port_(mach_port_name),
ReceivePort’s constructor should be marked explicit and you should write this as
receive_port_(ReceivePort(mach_port_name)).

http://breakpad.appspot.com/146001/diff/1/7#newcode53
Line 53: dump_dir_ = *dump_path;
dump_dir_(dump_path ? *dump_path : "/tmp") in an initializer list would have
worked too. So would dump_dir_(dump_path.empty() ? "/tmp" : dump_path) if you
had done it as a const reference.

This is fine, too. Just pointing it out.

http://breakpad.appspot.com/146001/diff/1/7#newcode77
Line 77: kern_return_t result = sender.SendMessage(quit_message, 2000);
As before, a symbolic constant would be good here.

If you send and receive multiple messages and want to use the same timeouts for
them, move the constants out into some common location.

http://breakpad.appspot.com/146001/diff/1/7#newcode80
Line 80: started_ = false;
Wrong indent. Makes it hard to read.

http://breakpad.appspot.com/146001/diff/1/7#newcode81
Line 81: }
This would be clearer if you did something more like what you did in Start. For
example:

  if (result == KERN_SUCCESS) {
    int thread_join_result = pthread_join(server_thread_, NULL);
    started_ = thread_join_result != 0;
  }

http://breakpad.appspot.com/146001/diff/1/7#newcode87
Line 87: void *CrashGenerationServer::WaitForMessage(void *server) {
The * on the type name or on the variable name (or function name)? Which will it
be? Pick one and stick with it. Stay consistent within a single file.

(You did it the other way on lines 43, 89, and 92 and maybe others, and this way
on 88, 104, and probably others.)

http://breakpad.appspot.com/146001/diff/1/7#newcode89
Line 89: reinterpret_cast<CrashGenerationServer*>(server);
4-space indent on continuation lines.

http://breakpad.appspot.com/146001/diff/1/7#newcode90
Line 90: while (true) {
|while (true) if (!x) return;| can be rewritten as |while (x) {} return;|

http://breakpad.appspot.com/146001/diff/1/7#newcode92
Line 92: return (void*)0;
Why not |return NULL;|?

http://breakpad.appspot.com/146001/diff/1/7#newcode98
Line 98: // Wait for a message
This comment is extraneous.

http://breakpad.appspot.com/146001/diff/1/7#newcode102
Line 102: if (result == KERN_SUCCESS) {
You defined this function as returning false only when it receives a quit
message, but it seems like you might want to return false if WaitForMessage
returns ≠ KERN_SUCCESS, also. I can’t be sure of that without looking at the
implementation of WaitForMessage, though, and I’m not doing that right now.

What do you think?

http://breakpad.appspot.com/146001/diff/1/7#newcode103
Line 103: if (message.GetMessageID() == kDumpRequestMessage) {
This seems slightly better suited to switch.

http://breakpad.appspot.com/146001/diff/1/7#newcode114
Line 114: task_suspend(remote_task);
ScopedTaskSuspend would be cool.

http://breakpad.appspot.com/146001/diff/1/7#newcode132
Line 132: std::string *ptr_dump_path = (dump_path == "") ? NULL : &dump_path;
Use |dump_path.empty()| instead of |dump_path == ""|.

http://breakpad.appspot.com/146001/diff/1/7#newcode133
Line 133: dump_callback_(dump_context_, &info, ptr_dump_path);
Is there a reason that the callback needs to accept the dump path as a |const
std::string*| as opposed to a |const std::string&|?

http://breakpad.appspot.com/146001/diff/1/7#newcode136
Line 136: //TODO: support a way for the client to send additional data, perhaps
// TODO(ted):

(Note the space between // and TODO, and the username in parentheses.)

http://breakpad.appspot.com/146001/diff/1/7#newcode137
Line 137: // with a callback so users of the server can read it out themselves?
Read what out?

http://breakpad.appspot.com/146001/diff/1/7#newcode139
Line 139: if (ack_port != MACH_PORT_DEAD) {
Check for MACH_PORT_NULL too?

http://breakpad.appspot.com/146001/diff/1/7#newcode143
Line 143: sender.SendMessage(ack_message, 2000);
Symbolic constant?

http://breakpad.appspot.com/146001/diff/1/7#newcode152
Line 152: }
If the message wasn’t a dump request or a quit, what do you want to do?

I think that there are a lot of exceptional conditions that this code handles
with a silent “return false;” but probably deserve some form of logging.

http://breakpad.appspot.com/146001/diff/1/7#newcode158
Line 158: 
Remove the blank line at EOF.

http://breakpad.appspot.com/146001/diff/1/8
File src/client/mac/handler/exception_handler.cc (right):

http://breakpad.appspot.com/146001/diff/1/8#newcode221
Line 221: FilterCallback filter,
We have too many constructors now.

It seems like this one should be implemented in terms of the new one you’re
adding?

http://breakpad.appspot.com/146001/diff/1/8#newcode344
Line 344: // decided if this should be sent
decided -> decide. Also, sentences should end with periods.

http://breakpad.appspot.com/146001/diff/1/10
File src/client/mac/tests/crash_generation_server_test.cc (right):

http://breakpad.appspot.com/146001/diff/1/10#newcode1
Line 1: // Copyright (c) 2010, Google Inc.
(Not reading the tests until a future iteration when some of the other comments
have been addressed.)

http://breakpad.appspot.com/146001/diff/1/10#newcode126
Line 126: CrashGenerationServerTest* self =
reinterpret_cast<CrashGenerationServerTest*>(context);
80

http://breakpad.appspot.com/146001/diff/1/10#newcode216
Line 216: }
}  // namespace

http://breakpad.appspot.com/146001/diff/1/11
File src/client/mac/tests/exception_handler_test.cc (right):

http://breakpad.appspot.com/146001/diff/1/11#newcode104
Line 104: }
}  // namespace

[Mark Mentovai]

High-level comments (now that I’ve read this).

I don’t believe that this is the right model for the Mac (although it’s
difficult to say for sure without seeing where you take this). Mac
out-of-process exception handling shouldn’t require any code to execute in the
crash process (what you’ve termed the “client” in this code) at all.

On the Mac, we should be able to arrange for a process’ exception port to be in
a distinct process, which would handle suspending the crashed process, writing
the dump, and taking further action as needed. This is more robust from a
crash-handling perspective and would in fact solve a few bugs we’ve encountered
with the present in-process approach. One example is the wonky handling of
EXC_CRASH, which is “special.” The distinct dump-writing process on the “other”
end of the crash process’ exception port may not necessarily need to “hang
around,” it may be possible to set it up to run on demand.

As you’ve implemented this, exception handling is a hybrid of in-process and
out-of-process. The exception handler proper still runs in process, but dump
generation happens out of process. This is essentially the same thing that the
Breakpad.framework does. It’s not “wrong,” but it’s not “best” or “ideal”
either. Neal and I were discussing this a couple of days ago. We were hoping to
be able to do better.

[Ted Mielczarek]

On 2010/08/12 18:54:43, Mark Mentovai wrote:
...
> As you’ve implemented this, exception handling is a hybrid of in-process and
> out-of-process. The exception handler proper still runs in process, but dump
> generation happens out of process. This is essentially the same thing that the
> Breakpad.framework does. It’s not “wrong,” but it’s not “best” or “ideal”
> either. Neal and I were discussing this a couple of days ago. We were hoping
to
> be able to do better.

Right. As you can clearly see, this was a fairly straightforward refactoring of
the Breakpad framework / Inspector code into an API matching what's provided on
other platforms. Firefox isn't going to use the Breakpad framework, but needs
OOP minidump generation on Mac, and we already have glue code in place that uses
the CrashGeneration{Client,Server} APIs, so this seemed like the quickest path
to success. I'm certainly willing to do the right thing and write the better
implementation for Breakpad, but it's likely that I'll land some variation of
this in Mozilla as a stopgap so we can ship Firefox 4.

> I don’t believe that this is the right model for the Mac (although it’s
> difficult to say for sure without seeing where you take this). Mac
> out-of-process exception handling shouldn’t require any code to execute in the
> crash process (what you’ve termed the “client” in this code) at all.
> 
> On the Mac, we should be able to arrange for a process’ exception port to be
in
> a distinct process, which would handle suspending the crashed process, writing
> the dump, and taking further action as needed. This is more robust from a
> crash-handling perspective and would in fact solve a few bugs we’ve
encountered

I think I see what you're getting at. We could have an ExceptionHandler
constructor that took a |mach_port_t exception_port| which was either messaged
from the parent via the bootstrap port or set up as an on-demand port. It could
then register that port as the task's exception port, and the parent/server
process would directly receive the exception messages for the other task without
any Breakpad code running at the time of the exception in the crashing task.
Does that sound about right?

I think the only complication there will be communicating the key/value pairs
that the Breakpad framework uses currently, but I'm sure there's another way to
do it.

If that's what you'd like to do, then I'll abandon upstreaming this approach,
get my ducks in a row for my downstream work, and then look at implementing that
when I'm finished there.

[Ted Mielczarek]

http://breakpad.appspot.com/146001/diff/1/4
File src/client/mac/crash_generation/crash_generation_client.h (right):

http://breakpad.appspot.com/146001/diff/1/4#newcode39
Line 39: CrashGenerationClient(const char* mach_port_name) :
sender_(mach_port_name) {
On 2010/08/12 16:58:05, Mark Mentovai wrote:
> This should be written as sender_(MachPortSender(mach_port_name)).

That gives me "error: 'MachPortSender::MachPortSender(const MachPortSender&)' is
private", since we disallowed the copy constructor.

http://breakpad.appspot.com/146001/diff/1/5
File src/client/mac/crash_generation/crash_generation_client.mm (right):

http://breakpad.appspot.com/146001/diff/1/5#newcode37
Line 37: bool CrashGenerationClient::RequestDump(int exception_type,
On 2010/08/12 16:58:05, Mark Mentovai wrote:
> Evidently, this will run inside the crashed process. Have you examined
> everything that it uses (ReceivePort, MachSendMessage, etc.) to make sure that
> they’re exception-time-safe? No memory allocation, etc.?

Ah, I hadn't. I meant to look at the memory allocator strategy in the
ExceptionHandler itself, but I neglected to do so. The Breakpad framework is
already using these in the same context, FWIW.

http://breakpad.appspot.com/146001/diff/1/5#newcode46
Line 46: message.AddDescriptor(mach_task_self());            // our task
On 2010/08/12 16:58:05, Mark Mentovai wrote:
> Remember how I feel about “we” and “our” in comments. I don’t even really know
> what “our task” is supposed to mean in the context of multiple tasks, IPC,
etc.
> I know what “this task” means.

I copied these verbatim from Framework/Breakpad.mm, but I'll change them here.

http://breakpad.appspot.com/146001/diff/1/6
File src/client/mac/crash_generation/crash_generation_server.h (right):

http://breakpad.appspot.com/146001/diff/1/6#newcode41
Line 41: // Messages the server can read via its mach port
On 2010/08/12 16:58:05, Mark Mentovai wrote:
> Would this be better as a named enum?

I don't know that it adds any value, we only ever pass these to the
MachSendMessage constructor. We never use them in variables.

http://breakpad.appspot.com/146001/diff/1/6#newcode62
Line 62: // Parameter mach_port_name: Named server port to listen on.
On 2010/08/12 16:58:05, Mark Mentovai wrote:
> These are obviously parameters. Save some space and don’t say “parameter” each
> time. Or, if you like, you can write “Parameters:” once and then list them all
> beneath that.

This is all copy/pasted from the Windows crash_generation.h via the Linux
version. This is what we get when you're not constantly vigilant! (I'll fix this
copy.)

http://breakpad.appspot.com/146001/diff/1/7
File src/client/mac/crash_generation/crash_generation_server.mm (right):

http://breakpad.appspot.com/146001/diff/1/7#newcode43
Line 43: const std::string* dump_path) :
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> Usually read-only input parameters are passed by const reference, not const
> pointer. But maybe you explicitly wanted to allow this to be NULL to mean
> something special? (As opposed to empty?)

Again, I cargo-culted this from the Linux implementation via the Windows
implementation. I don't know why it was written that way. An empty value should
be fine as "no value".

http://breakpad.appspot.com/146001/diff/1/7#newcode87
Line 87: void *CrashGenerationServer::WaitForMessage(void *server) {
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> The * on the type name or on the variable name (or function name)? Which will
it
> be? Pick one and stick with it. Stay consistent within a single file.
> 
> (You did it the other way on lines 43, 89, and 92 and maybe others, and this
way
> on 88, 104, and probably others.)

I prefer it snuggled with the type name, but prevailing Breakpad style is
usually the opposite, so I probably made some mistakes. (It's also possible that
I imported some mismatched style while copy/pasting function prototypes around
from the Linux implementation.) I'll clean these all up.

http://breakpad.appspot.com/146001/diff/1/7#newcode92
Line 92: return (void*)0;
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> Why not |return NULL;|?

I don't know, I guess I thought it made more sense at the time.

http://breakpad.appspot.com/146001/diff/1/7#newcode102
Line 102: if (result == KERN_SUCCESS) {
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> You defined this function as returning false only when it receives a quit
> message, but it seems like you might want to return false if WaitForMessage
> returns ≠ KERN_SUCCESS, also. I can’t be sure of that without looking at the
> implementation of WaitForMessage, though, and I’m not doing that right now.
> 
> What do you think?

I thought about this but I never did anything about it. I suppose it's likely
that if WaitForMessage fails something is horribly wrong and looping around
again isn't likely to be useful anyway. I'll change it to return false on error.

http://breakpad.appspot.com/146001/diff/1/7#newcode133
Line 133: dump_callback_(dump_context_, &info, ptr_dump_path);
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> Is there a reason that the callback needs to accept the dump path as a |const
> std::string*| as opposed to a |const std::string&|?

More copy/pasting from the Windows implementation. I have no idea why it was
written that way.

http://breakpad.appspot.com/146001/diff/1/7#newcode152
Line 152: }
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> If the message wasn’t a dump request or a quit, what do you want to do?

I guess it's being ignored right now, which is probably what I do want, although
logging that an invalid message has been received seems sensible.

> I think that there are a lot of exceptional conditions that this code handles
> with a silent “return false;” but probably deserve some form of logging.

This is reasonable. Do we have logging code that we're currently using in the
Mac client?

http://breakpad.appspot.com/146001/diff/1/8
File src/client/mac/handler/exception_handler.cc (right):

http://breakpad.appspot.com/146001/diff/1/8#newcode344
Line 344: // decided if this should be sent
On 2010/08/12 18:48:24, Mark Mentovai wrote:
> decided -> decide. Also, sentences should end with periods.

Copied from the code below, I'll fix both.

[Mark Mentovai]

LGTM.

I started playing with something that can do out-of-process exception handling
without having any component of the exception handler in the crash process. I’ve
gotten something that starts an exception handler process on demand.

http://breakpad.appspot.com/146001/diff/15001/16005
File src/client/mac/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/146001/diff/15001/16005#newcode131
Line 131: // TODO(ted): support a way for the client to send additional data,
perhaps
80