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

remoting/base/breakpad_win_unittest.cc

+// 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.
+
+#include <stdio.h>
+
+#include "base/compiler_specific.h"
+#include "base/environment.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/string16.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "breakpad/src/client/windows/crash_generation/client_info.h"
+#include "breakpad/src/client/windows/crash_generation/crash_generation_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {

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

Nit: Does the anonymous namespace need to be inside the remoting namespace?

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

Done.

+
+namespace {
+
+// The name of the environment variable used to pass the crash server pipe name
+// to the crashing child process.
+const char kPipeVariableName[] = "REMOTING_BREAKPAD_WIN_DEATH_TEST_PIPE_NAME";
+
+// The prefix string used to generate a unique crash server pipe name.
+// The name has to be unique as multiple test instances can be running
+// simultaneously.
+const wchar_t kPipeNamePrefix[] = L"\\\\.\\pipe\\";
+
+class MockCrashServerCallbacks {
+ public:
+  MockCrashServerCallbacks() {}
+  virtual ~MockCrashServerCallbacks() {}

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

Nit: Even in tests, try to avoid inlining functions into the class declaration
unless you would inline them under normal circumstances.  This helps keep the
class declaration shorter and simpler to read.  In this case, you should at
least not inline the destructor (since it's virtual) and
OnClientDumpRequestCallback() (since it's not a simple accessor).

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

Done. I removed inlining of the constructor as well to match the style used
elsewhere in the code.

+
+  // |google_breakpad::CrashGenerationServer| invokes callbacks from artitrary
+  // thread pool threads. |OnClientDumpRequested| is the only one that happened
+  // to be called in synchronous manner. While it is still called on
+  // a thread pool thread, the crashing process will wait until the server
+  // signals an event after |OnClientDumpRequested| completes (or until 15
+  // seconds timeout expires).
+  MOCK_METHOD0(OnClientDumpRequested, void());
+
+  static void OnClientDumpRequestCallback(
+      void* context,
+      const google_breakpad::ClientInfo* client_info,
+      const string16* file_path) {
+    reinterpret_cast<MockCrashServerCallbacks*>(context)->
+        OnClientDumpRequested();
+  }
+};
+
+}  // namespace
+
+void InitializeCrashReportingForTest(const wchar_t*);
+
+class BreakpadWinDeathTest : public testing::Test {
+ public:
+  BreakpadWinDeathTest() {}
+  virtual void SetUp() OVERRIDE {
+    scoped_ptr<base::Environment> environment(base::Environment::Create());
+    std::string pipe_name;
+    if (environment->GetVar(kPipeVariableName, &pipe_name)) {
+      // This is a child process. Initialize crash dump reporting to the crash
+      // dump server.
+      pipe_name_ = UTF8ToUTF16(pipe_name);

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

Use UTF8ToWide() here (see below).

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

Done.

+      ::remoting::InitializeCrashReportingForTest(pipe_name_.c_str());
+    } else {
+      // This is the parent process. Generate a unique pipe name and setup
+      // a dummy crash dump server.
+      UUID guid = {0};
+      RPC_STATUS status = ::UuidCreate(&guid);

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

Nit: Is the :: really necessary?

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

Done.

+      EXPECT_TRUE(status == RPC_S_OK || status == RPC_S_UUID_LOCAL_ONLY);
+
+      pipe_name_ = kPipeNamePrefix +

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

Nit: If using StringPrintf() anyway, add a "%s" to the front of the format
string and supply |kPipeNamePrefix| as the first subsequent arg.

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

Done.

+          base::StringPrintf(
+              L"%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
+              guid.Data1, guid.Data2, guid.Data3,
+              guid.Data4[0], guid.Data4[1], guid.Data4[2],
+              guid.Data4[3], guid.Data4[4], guid.Data4[5],
+              guid.Data4[6], guid.Data4[7]);
+      bool result = environment->SetVar(kPipeVariableName,
+                                        UTF16ToUTF8(pipe_name_));

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

Use WideToUTF8 here (see below).

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

Done.

+      EXPECT_TRUE(result);

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

Nit: Unlike with DCHECK(), it's OK to directly wrap function calls in ASSERT or
EXPECT checks without using a temp to store the call result.

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

Done.

+
+      // Setup a dummy crash dump server.
+      callbacks_.reset(new MockCrashServerCallbacks());
+      crash_server_.reset(
+          new google_breakpad::CrashGenerationServer(
+              pipe_name_, NULL,

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

Nit: Strange wrapping here.  More common choices are "as many args on each line
as will fit" and "one arg on each line".

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

Done.

+              NULL, 0,
+              MockCrashServerCallbacks::OnClientDumpRequestCallback,
+              callbacks_.get(),
+              NULL, 0,
+              false, NULL));
+
+      result = crash_server_->Start();
+      ASSERT_TRUE(result);
+    }
+  }
+
+ protected:
+  scoped_ptr<google_breakpad::CrashGenerationServer> crash_server_;
+  scoped_ptr<MockCrashServerCallbacks> callbacks_;
+  string16 pipe_name_;

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

This should be a wstring.

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

Done.

+};
+
+TEST_F(BreakpadWinDeathTest, TestAccessViolation) {
+  if (callbacks_.get()) {
+    EXPECT_CALL(*callbacks_, OnClientDumpRequested());
+  }
+
+  // Generate access violation exception.
+  ASSERT_DEATH(*reinterpret_cast<int*>(0) = 1, "");

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

Nit: 0 -> NULL?

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

Done.

+}
+
+TEST_F(BreakpadWinDeathTest, TestInvalidParameter) {
+  if (callbacks_.get()) {
+    EXPECT_CALL(*callbacks_, OnClientDumpRequested());
+  }
+
+  // Cause the invalid parameter callback to be called.
+  ASSERT_EXIT(printf(NULL), ::testing::ExitedWithCode(0), "");
+}
+
+TEST_F(BreakpadWinDeathTest, TestDebugbreak) {
+  if (callbacks_.get()) {
+    EXPECT_CALL(*callbacks_, OnClientDumpRequested());
+  }
+
+  // See if __debugbreak() is intercepted.
+  ASSERT_DEATH(__debugbreak(), "");
+}
+
+}  // namespace remoting