/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..732c186b5bb25662b9974736f73af8728a5c7682
--- /dev/null
+++ b/remoting/base/breakpad_win.cc
@@ -0,0 +1,213 @@
+// 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/Crome Frame crash reporitng

[Peter Kasting, 2012/06/14 20:16:27]

Nit: s/reporitng/reporting/

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+// 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 "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/string16.h"
+#include "base/win/wrapped_window_proc.h"
+#include "breakpad/src/client/windows/handler/exception_handler.h"
+
+namespace remoting {
+void InitializeCrashReportingForTest(const wchar_t*);
+}  // 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();

[Peter Kasting, 2012/06/14 20:16:27]

Nit: While I'm not sure non-const refs are banned as return types (as they are
as function types), they're less common in Google code; consider returning a
pointer instead.

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+
+ 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*, EXCEPTION_POINTERS*,

[Peter Kasting, 2012/06/14 20:16:27]

Nit: One arg per line; name the arguments.

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+                                  MDRawAssertionInfo*);
+
+  // 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* info);
+
+  // 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.
+  wchar_t temp_directory[MAX_PATH + 1] = { 0 };
+  DWORD length = ::GetTempPath(MAX_PATH, temp_directory);

[Peter Kasting, 2012/06/14 20:16:27]

Nit: Is :: necessary?

Also consider using GetTempDir from base/file_util_win.cc, though this might not
make your code any simpler or safer.

[alexeypa (please no reviews), 2012/06/15 18:45:49]

It is a common pattern in Chromium code. This is however a Windows-only file, so
I don't think it makes much sense.
The reason not to use GetTempDir from base is to run as little code as we can
before all the hooks are installed. This way we minimize the chance of crashing
before we can report it.

[Peter Kasting, 2012/06/15 19:02:42]

(FWIW, I object when I see this in other code too, if it's not actually
necessary -- I don't see it all that often.)
I see.  Might want a quick comment to that effect.

[alexeypa (please no reviews), 2012/06/15 19:51:24]

Done.

+  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));
+
+  string16 version;

[Peter Kasting, 2012/06/14 20:16:27]

This should be a wstring (and you should call UTF16ToWide() on
product_version()).

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+  if (version_info.get())
+    version = 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*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
+  BreakpadWin& self = BreakpadWin::GetInstance();
+  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* info) {
+  BreakpadWin& self = BreakpadWin::GetInstance();
+  if (self.breakpad_.get() != NULL) {
+    self.breakpad_->WriteMinidumpForException(info);
+    ::TerminateProcess(::GetCurrentProcess(),
+                       info->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;
+
+  // Touch the object to make sure it is initialized.

[Peter Kasting, 2012/06/14 20:16:27]

Nit: For maintenance safety, I suggest calling InitializeCrashReporting() here
instead (and ommitting the comment), in case later someon wants to add a step to
that function.

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+  BreakpadWin::GetInstance();
+}
+
+}  // namespace remoting