/C++ readability/ - Make Chromoting Host report crashes to Breakpad (Windows only). The user must e…

remoting/base/breakpad_win.cc

Index: remoting/base/breakpad_win.cc
diff --git a/remoting/base/breakpad_win.cc b/remoting/base/breakpad_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5757e64ccf26d7625f863ddb6381d5f501f0a603
--- /dev/null
+++ b/remoting/base/breakpad_win.cc
@@ -0,0 +1,216 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This module contains the necessary code to register the Breakpad exception
+// handler. This implementation is based on Chrome/Chrome Frame crash reporting
+// code. See:
+//   - src/chrome/app/breakpad_win.cc
+//   - src/chrome_frame/crash_server_init.cc
+//   - src/chrome/installer/setup/setup_main.cc
+//   - src/chrome_frame/crash_reporting/crash_report.cc
+
+#include "remoting/base/breakpad.h"
+
+#include <windows.h>
+#include <string>
+
+#include "base/atomicops.h"
+#include "base/logging.h"
+#include "base/file_version_info.h"
+#include "base/lazy_instance.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/process_util.h"
+#include "base/utf_string_conversions.h"
+#include "base/win/wrapped_window_proc.h"
+#include "breakpad/src/client/windows/handler/exception_handler.h"
+
+namespace remoting {
+void InitializeCrashReportingForTest(const wchar_t* pipe_name);
+}  // namespace remoting
+
+namespace {
+
+const wchar_t kBreakpadProductName[] = L"Chromoting";
+const wchar_t kBreakpadVersionEntry[] = L"ver";
+const wchar_t kBreakpadVersionDefault[] = L"0.1.0.0";
+const wchar_t kBreakpadProdEntry[] = L"prod";
+const wchar_t kBreakpadPlatformEntry[] = L"plat";
+const wchar_t kBreakpadPlatformWin32[] = L"Win32";
+
+// The protocol for connecting to the out-of-process Breakpad crash
+// reporter is different for x86-32 and x86-64: the message sizes
+// are different because the message struct contains a pointer.  As
+// a result, there are two different named pipes to connect to.  The
+// 64-bit one is distinguished with an "-x64" suffix.
+#if defined(_WIN64)
+const wchar_t kGoogleUpdatePipeName[] =
+    L"\\\\.\\pipe\\GoogleCrashServices\\S-1-5-18-x64";
+#else
+const wchar_t kGoogleUpdatePipeName[] =
+    L"\\\\.\\pipe\\GoogleCrashServices\\S-1-5-18";
+#endif
+
+using base::subtle::AtomicWord;
+using base::subtle::NoBarrier_CompareAndSwap;
+
+class BreakpadWin {
+ public:
+  BreakpadWin();
+  ~BreakpadWin();
+
+  static BreakpadWin* GetInstance();
+
+ private:
+  // Returns the Custom information to be used for crash reporting.
+  google_breakpad::CustomClientInfo* GetCustomInfo();
+
+  // Checks whether crash dump collection is allowed by the user.
+  bool IsCrashReportingEnabled();
+
+  // This callback is executed when the process has crashed and *before*
+  // the crash dump is created. To prevent duplicate crash reports we
+  // make every thread calling this method, except the very first one,
+  // go to sleep.
+  static bool OnExceptionCallback(void* context,
+                                  EXCEPTION_POINTERS* exinfo,
+                                  MDRawAssertionInfo* assertion);
+
+  // Crashes the process after generating a dump for the provided exception.
+  // Note that the crash reporter should be initialized before calling this
+  // function for it to do anything.
+  static int OnWindowProcedureException(EXCEPTION_POINTERS* exinfo);
+
+  // Breakpad's exception handler.
+  scoped_ptr<google_breakpad::ExceptionHandler> breakpad_;
+
+  // This flag is used to indicate that an exception is already being handled.
+  volatile AtomicWord handling_exception_;
+
+  // The testing hook below allows overriding the crash server pipe name.
+  static const wchar_t* pipe_name_;
+
+  friend void ::remoting::InitializeCrashReportingForTest(const wchar_t*);
+
+  DISALLOW_COPY_AND_ASSIGN(BreakpadWin);
+};
+
+// |LazyInstance| is used to guarantee that the exception handler will be
+// initialized exactly once.
+// N.B. LazyInstance does not allow this to be a static member of the class.
+static base::LazyInstance<BreakpadWin>::Leaky g_instance =
+    LAZY_INSTANCE_INITIALIZER;
+
+const wchar_t* BreakpadWin::pipe_name_ = kGoogleUpdatePipeName;
+
+BreakpadWin::BreakpadWin() : handling_exception_(0) {
+  // Disable the message box for assertions.
+  _CrtSetReportMode(_CRT_ASSERT, 0);
+
+  // Get the alternate dump directory. We use the temp path.
+  // N.B. We don't use base::GetTempDir() here to avoid running more code then
+  //      necessary before crashes can be properly reported.
+  wchar_t temp_directory[MAX_PATH + 1] = { 0 };
+  DWORD length = GetTempPath(MAX_PATH, temp_directory);
+  if (length == 0)
+    return;
+
+  // Minidump with stacks, PEB, TEB, unloaded module list and memory referenced
+  // from stack.
+  MINIDUMP_TYPE dump_type = static_cast<MINIDUMP_TYPE>(
+      MiniDumpWithProcessThreadData |
+      MiniDumpWithUnloadedModules |
+      MiniDumpWithIndirectlyReferencedMemory);
+  breakpad_.reset(
+      new google_breakpad::ExceptionHandler(
+          temp_directory, &OnExceptionCallback, NULL, NULL,
+          google_breakpad::ExceptionHandler::HANDLER_ALL, dump_type,
+          pipe_name_, GetCustomInfo()));
+
+  if (breakpad_->IsOutOfProcess()) {
+    // Tells breakpad to handle breakpoint and single step exceptions.
+    breakpad_->set_handle_debug_exceptions(true);
+  }
+
+  // Catch exceptions thrown from a window procedure.
+  base::win::WinProcExceptionFilter exception_filter =
+      base::win::SetWinProcExceptionFilter(&OnWindowProcedureException);
+  CHECK(!exception_filter);
+}
+
+BreakpadWin::~BreakpadWin() {
+  // This object should be leaked so that crashes occurred during the process
+  // shutdown will be caught.
+  NOTREACHED();
+}
+
+// static
+BreakpadWin* BreakpadWin::GetInstance() {
+  return &g_instance.Get();
+}
+
+// Returns the Custom information to be used for crash reporting.
+google_breakpad::CustomClientInfo* BreakpadWin::GetCustomInfo() {
+  HMODULE binary = base::GetModuleFromAddress(
+      reinterpret_cast<void*>(&remoting::InitializeCrashReporting));
+  scoped_ptr<FileVersionInfo> version_info(
+      FileVersionInfo::CreateFileVersionInfoForModule(binary));
+
+  std::wstring version;
+  if (version_info.get())
+    version = UTF16ToWide(version_info->product_version());
+  if (version.empty())
+    version = kBreakpadVersionDefault;
+
+  static google_breakpad::CustomInfoEntry ver_entry(
+      kBreakpadVersionEntry, version.c_str());
+  static google_breakpad::CustomInfoEntry prod_entry(
+      kBreakpadProdEntry, kBreakpadProductName);
+  static google_breakpad::CustomInfoEntry plat_entry(
+      kBreakpadPlatformEntry, kBreakpadPlatformWin32);
+  static google_breakpad::CustomInfoEntry entries[] = {
+      ver_entry, prod_entry, plat_entry  };
+  static google_breakpad::CustomClientInfo custom_info = {
+      entries, arraysize(entries) };
+  return &custom_info;
+}
+
+// static
+bool BreakpadWin::OnExceptionCallback(void* /* context */,
+                                      EXCEPTION_POINTERS* /* exinfo */,
+                                      MDRawAssertionInfo* /* assertion */) {

[Peter Kasting, 2012/06/15 21:19:30]

Nit: Don't comment out the parameter names.  MSVC doesn't warn for unused named
parameters.

[alexeypa (please no reviews), 2012/06/15 21:25:55]

This is required by Google's C++ style guide:
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Declarations...

[Peter Kasting, 2012/06/15 21:33:01]

True; however, the underlying thrust of that item is to always supply parameter
names and never leave things as bare types.  In google3, where almost everything
is compiled on Linux, compilers generally warn for unusued named parameters,
hence the style recommendation.  We don't normally follow this rule in Chrome
code (at least on Windows) because MSVC allows us to compile it without
warnings, and therefore we can supply names without having to resort to
comments.

Good catch though!

+  BreakpadWin* self = BreakpadWin::GetInstance();

[Peter Kasting, 2012/06/15 21:19:30]

Nit: You may also inline this in the following line.

[alexeypa (please no reviews), 2012/06/15 22:13:57]

This makes the line longer 80 characters.

[Peter Kasting, 2012/06/16 01:56:42]

Well, appropriate wrapping was assumed :)

In any case, it is perfectly fine not to inline as well.

+  if (NoBarrier_CompareAndSwap(&self->handling_exception_, 0, 1) != 0) {
+    // Capture every thread except the first one in the sleep. We don't
+    // want multiple threads to concurrently report exceptions.
+    ::Sleep(INFINITE);
+  }
+  return true;
+}
+
+// static
+int BreakpadWin::OnWindowProcedureException(EXCEPTION_POINTERS* exinfo) {
+  BreakpadWin* self = BreakpadWin::GetInstance();
+  if (self->breakpad_.get() != NULL) {
+    self->breakpad_->WriteMinidumpForException(exinfo);
+    ::TerminateProcess(::GetCurrentProcess(),

[Peter Kasting, 2012/06/15 21:19:30]

Nit: ::

[alexeypa (please no reviews), 2012/06/15 22:13:57]

Done.

+                       exinfo->ExceptionRecord->ExceptionCode);
+  }
+  return EXCEPTION_CONTINUE_SEARCH;
+}
+
+}  // namespace
+
+namespace remoting {
+
+void InitializeCrashReporting() {
+  // Touch the object to make sure it is initialized.
+  BreakpadWin::GetInstance();
+}
+
+void InitializeCrashReportingForTest(const wchar_t* pipe_name) {
+  BreakpadWin::pipe_name_ = pipe_name;
+  InitializeCrashReporting();
+}
+
+}  // namespace remoting