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

src/client/mac/crash_generation/crash_generation_client.mm

+// Copyright (c) 2010 Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "client/mac/crash_generation/client_info.h"
+#include "client/mac/crash_generation/crash_generation_client.h"

[Mark Mentovai, 2010/08/12 16:58:05]

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.

+#include "client/mac/crash_generation/crash_generation_server.h"
+#include "common/mac/MachIPC.h"
+
+namespace google_breakpad {
+
+bool CrashGenerationClient::RequestDump(int exception_type,

[Mark Mentovai, 2010/08/12 16:58:05]

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.?

[Ted Mielczarek, 2010/08/13 19:22:06]

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.

+                                        int exception_code,
+                                        int exception_subcode,
+                                        mach_port_t crashing_thread) {
+  // The server will send a message to this port indicating that it
+  // has finished its work.
+  ReceivePort acknowledge_port;
+
+  MachSendMessage message(kDumpRequestMessage);
+  message.AddDescriptor(mach_task_self());            // our task

[Mark Mentovai, 2010/08/12 16:58:05]

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.

[Ted Mielczarek, 2010/08/13 19:22:06]

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

+  message.AddDescriptor(crashing_thread);             // crashing thread
+  message.AddDescriptor(mach_thread_self());          // handler thread
+  message.AddDescriptor(acknowledge_port.GetPort());  // message receive port
+
+  ClientInfo info;
+  info.exception_type = exception_type;
+  info.exception_code = exception_code;
+  info.exception_subcode = exception_subcode;
+  message.SetData(&info, sizeof(info));
+  
+  kern_return_t result = sender_.SendMessage(message, 2000);

[Mark Mentovai, 2010/08/12 16:58:05]

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.

+  if (result != KERN_SUCCESS)
+    return false;
+
+  MachReceiveMessage acknowledge_message;
+  result = acknowledge_port.WaitForMessage(&acknowledge_message, 5000);
+  
+  return result == KERN_SUCCESS;
+}
+
+}  // namespace google_breakpad

[Mark Mentovai, 2010/08/12 16:58:05]

There’s no good reason for this file to be .mm. It has no Objectiveness. You can
make it .cc.

+

[Mark Mentovai, 2010/08/12 16:58:05]

Blank line at EOF.