Report dump counts as a stability metric.

Previously these were reported as a regular metric, against the currently running version, even though they were recorded from the previous execution, possibly from a different version.

Stability metrics are attributed to the version of the previous execution.

Also, there was an attempt to mitigate the cross-version reporting by associating each even with the version that recorded it. This is problematic for short-lived versions such as Canary.

I extracted the code, fixed the above deficiencies, wrote tests, and added functionality (also reporting the outcome of the Breakpad invocation).

BUG=460512

Committed: https://crrev.com/a11510d8e5175aa451f11b183a6668f581b4ec60
Cr-Commit-Position: refs/heads/master@{#323131}

Review URL: https://codereview.chromium.org/1038503002

Cr-Commit-Position: refs/heads/master@{#323182}

4 files changed, 330 insertions, 0 deletions

diff --git a/components/browser_watcher/BUILD.gn b/components/browser_watcher/BUILD.gn
index 6720895..376699d 100644
--- a/components/browser_watcher/BUILD.gn
+++ b/components/browser_watcher/BUILD.gn
@@ -22,6 +22,8 @@ source_set("browser_watcher") {
 
 source_set("browser_watcher_client") {
   sources = [
+    "crash_reporting_metrics_win.cc",
+    "crash_reporting_metrics_win.h",
     "watcher_client_win.cc",
     "watcher_client_win.h",
     "watcher_metrics_provider_win.cc",
diff --git a/components/browser_watcher/crash_reporting_metrics_win.cc b/components/browser_watcher/crash_reporting_metrics_win.cc
new file mode 100644
index 0000000..ee83c4c
--- /dev/null
+++ b/components/browser_watcher/crash_reporting_metrics_win.cc
@@ -0,0 +1,161 @@
+// Copyright 2015 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 "components/browser_watcher/crash_reporting_metrics_win.h"
+
+#include <algorithm>
+#include "base/atomicops.h"
+#include "base/guid.h"
+#include "base/logging.h"
+#include "base/strings/safe_sprintf.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/win/registry.h"
+
+namespace browser_watcher {
+
+namespace {
+
+// The prior implementation used 0 and 1. Its data collection is inconsistent
+// with ours and it's best to just ignore its records. So we start at 2.
+const DWORD kCrashDumpAttempt = 2;
+const DWORD kDumpWithoutCrashAttempt = 3;
+const DWORD kCrashDumpSuccess = 4;
+const DWORD kCrashDumpFailure = 5;
+const DWORD kDumpWithoutCrashSuccess = 6;
+const DWORD kDumpWithoutCrashFailure = 7;
+
+// A prefix intended to avoid registry value name collisions with other
+// processes (or modules within a single process). It is not necessary for the
+// reading and writing instances to share the prefix value. This array is sized
+// to hold a null-terminated string as generated by base::GenerateGUID.
+char g_unique_prefix[36 + 1] = {0};
+
+// An atomic counter intended  to make each registry value name unique within
+// this process and module.
+base::subtle::Atomic32 g_record_count = 0;
+
+// The length of a null-terminated string consisting of "{GUID}-{COUNT}".
+const size_t kValueNameSize = 36 + 1 + 8 + 1;
+
+void WriteValue(const base::string16& key_path, DWORD value) {
+  // Generate the final value name we'll use (appends the crash number to the
+  // base value name).
+  char value_name_ascii[kValueNameSize] = "";
+  base::char16 value_name_utf16[kValueNameSize] = L"";
+
+  if (base::strings::SafeSPrintf(
+          value_name_ascii, "%s-%x", g_unique_prefix,
+          base::subtle::NoBarrier_AtomicIncrement(&g_record_count, 1)) <= 0) {
+    NOTREACHED();
+  } else {
+    // Since it's an ASCII string, the UTF-16 form is identical with leading 0
+    // bytes. We're avoiding unnecessary heap operations since we're running in
+    // a compromised process.
+    std::copy(value_name_ascii, value_name_ascii + kValueNameSize,
+              value_name_utf16);
+
+    base::win::RegKey reg_key;
+    if (reg_key.Create(HKEY_CURRENT_USER, key_path.c_str(), KEY_SET_VALUE) !=
+        ERROR_SUCCESS) {
+      NOTREACHED();
+    } else {
+      reg_key.WriteValue(value_name_utf16, value);
+    }
+  }
+
+}
+
+}  // namespace
+
+CrashReportingMetrics::CrashReportingMetrics(
+    const base::string16& registry_path)
+    : registry_path_(registry_path) {
+  if (g_unique_prefix[0] == 0) {
+    std::string guid = base::GenerateGUID();
+    // It seems reasonable to assume that the worst possible outcome of two
+    // separate threads trying to do the following would be to store a GUID
+    // value that is a hybrid of the two intended values. Hence we can avoid any
+    // thread-safety caveats in our public API.
+    size_t copied_size = base::strlcpy(g_unique_prefix, guid.c_str(),
+                                       arraysize(g_unique_prefix));
+    DCHECK_EQ(copied_size, guid.length());
+  }
+}
+
+void CrashReportingMetrics::RecordCrashDumpAttempt() {
+  WriteValue(registry_path_, kCrashDumpAttempt);
+}
+
+void CrashReportingMetrics::RecordDumpWithoutCrashAttempt() {
+  WriteValue(registry_path_, kDumpWithoutCrashAttempt);
+}
+void CrashReportingMetrics::RecordCrashDumpAttemptResult(bool succeeded) {
+  WriteValue(registry_path_, succeeded ? kCrashDumpSuccess : kCrashDumpFailure);
+}
+void CrashReportingMetrics::RecordDumpWithoutCrashAttemptResult(
+    bool succeeded) {
+  WriteValue(registry_path_,
+             succeeded ? kDumpWithoutCrashSuccess : kDumpWithoutCrashFailure);
+}
+
+CrashReportingMetrics::Values CrashReportingMetrics::RetrieveAndResetMetrics() {
+  Values values = {0};
+
+  // Open the registry key for iteration.
+  base::win::RegKey regkey;
+  if (regkey.Open(HKEY_CURRENT_USER, registry_path_.c_str(),
+                  KEY_QUERY_VALUE | KEY_SET_VALUE) != ERROR_SUCCESS) {
+    return values;
+  }
+
+  // Track a list of values to delete. We don't modify the registry key while
+  // we're iterating over its values.
+  typedef std::vector<base::string16> StringVector;
+  StringVector to_delete;
+
+  // Iterate over the values in the key counting dumps with and without crashes.
+  // We directly walk the values instead of using RegistryValueIterator in order
+  // to read all of the values as DWORDS instead of strings.
+  base::string16 name;
+  DWORD value = 0;
+  for (int i = regkey.GetValueCount() - 1; i >= 0; --i) {
+    if (regkey.GetValueNameAt(i, &name) == ERROR_SUCCESS &&
+        regkey.ReadValueDW(name.c_str(), &value) == ERROR_SUCCESS) {
+      to_delete.push_back(name);
+      switch (value) {
+        case kCrashDumpAttempt:
+          ++values.crash_dump_attempts;
+          break;
+        case kCrashDumpSuccess:
+          ++values.successful_crash_dumps;
+          break;
+        case kCrashDumpFailure:
+          ++values.failed_crash_dumps;
+          break;
+        case kDumpWithoutCrashAttempt:
+          ++values.dump_without_crash_attempts;
+          break;
+        case kDumpWithoutCrashSuccess:
+          ++values.successful_dumps_without_crash;
+          break;
+        case kDumpWithoutCrashFailure:
+          ++values.failed_dumps_without_crash;
+          break;
+        default:
+          // Presumably a pre-existing record from the previous implementation.
+          // We will delete it.
+          break;
+      }
+    }
+  }
+
+  // Delete the registry keys we've just counted.
+  for (const auto& value_name : to_delete)
+    regkey.DeleteValue(value_name.c_str());
+
+  return values;
+}
+
+}  // namespace browser_watcher
diff --git a/components/browser_watcher/crash_reporting_metrics_win.h b/components/browser_watcher/crash_reporting_metrics_win.h
new file mode 100644
index 0000000..6a63414
--- /dev/null
+++ b/components/browser_watcher/crash_reporting_metrics_win.h
@@ -0,0 +1,56 @@
+// Copyright 2015 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.
+
+#ifndef COMPONENTS_BROWSER_WATCHER_CRASH_REPORTING_METRICS_WIN_H_
+#define COMPONENTS_BROWSER_WATCHER_CRASH_REPORTING_METRICS_WIN_H_
+
+#include "base/strings/string16.h"
+
+namespace browser_watcher {
+
+// Stores and retrieves metrics related to crash dump attempts.
+class CrashReportingMetrics {
+ public:
+  // Represents the currently stored metrics.
+  struct Values {
+    // A count of crash dump attempts.
+    int crash_dump_attempts;
+    // A count of successful crash dump attempts.
+    int successful_crash_dumps;
+    // A count of failed crash dump attempts.
+    int failed_crash_dumps;
+    // A count of dump without crash attempts.
+    int dump_without_crash_attempts;
+    // A count of successful dump without crash attempts.
+    int successful_dumps_without_crash;
+    // A count of failed dump without crash attempts.
+    int failed_dumps_without_crash;
+  };
+
+  // Instantiates an instance that will store and retrieve its metrics from
+  // |registry_path|.
+  explicit CrashReportingMetrics(const base::string16& registry_path);
+
+  // Records that a crash dump is being attempted.
+  void RecordCrashDumpAttempt();
+
+  // Records that a dump without crash is being attempted.
+  void RecordDumpWithoutCrashAttempt();
+
+  // Records the result of a crash dump attempt.
+  void RecordCrashDumpAttemptResult(bool succeeded);
+
+  // Records the result of a dump without crash attempt.
+  void RecordDumpWithoutCrashAttemptResult(bool succeeded);
+
+  // Returns the currently stored metrics and resets them to 0.
+  Values RetrieveAndResetMetrics();
+
+ private:
+  base::string16 registry_path_;
+};
+
+}  // namespace browser_watcher
+
+#endif  // COMPONENTS_BROWSER_WATCHER_CRASH_REPORTING_METRICS_WIN_H_
diff --git a/components/browser_watcher/crash_reporting_metrics_win_unittest.cc b/components/browser_watcher/crash_reporting_metrics_win_unittest.cc
new file mode 100644
index 0000000..e21f428
--- /dev/null
+++ b/components/browser_watcher/crash_reporting_metrics_win_unittest.cc
@@ -0,0 +1,111 @@
+// Copyright 2015 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 "components/browser_watcher/crash_reporting_metrics_win.h"
+
+#include "base/macros.h"
+#include "base/strings/string16.h"
+#include "base/win/registry.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browser_watcher {
+
+namespace {
+
+const base::char16 test_root[] = L"TmpTmp";
+
+class CrashReportingMetricsTest : public testing::Test {
+ protected:
+  CrashReportingMetricsTest() {}
+
+  virtual void SetUp() {
+    // Create a temporary key for testing
+    base::win::RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
+    key.DeleteKey(test_root);
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, test_root, KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, test_root,
+                                        KEY_READ));
+  }
+
+  virtual void TearDown() {
+    // Clean up the temporary key
+    base::win::RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(test_root));
+  }
+
+  base::string16 registry_path() {
+    return base::string16(test_root) + L"\\CrashReportingMetricsTest";
+  };
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CrashReportingMetricsTest);
+};
+
+}  // namespace
+
+TEST_F(CrashReportingMetricsTest, BasicTest) {
+  CrashReportingMetrics::Values values =
+      CrashReportingMetrics(registry_path()).RetrieveAndResetMetrics();
+  EXPECT_EQ(0, values.crash_dump_attempts);
+  EXPECT_EQ(0, values.successful_crash_dumps);
+  EXPECT_EQ(0, values.failed_crash_dumps);
+  EXPECT_EQ(0, values.dump_without_crash_attempts);
+  EXPECT_EQ(0, values.successful_dumps_without_crash);
+  EXPECT_EQ(0, values.failed_dumps_without_crash);
+
+  CrashReportingMetrics writer(registry_path());
+  writer.RecordCrashDumpAttempt();
+  writer.RecordCrashDumpAttempt();
+  writer.RecordCrashDumpAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+
+  writer.RecordCrashDumpAttemptResult(true);
+  writer.RecordCrashDumpAttemptResult(false);
+  writer.RecordCrashDumpAttemptResult(false);
+  writer.RecordDumpWithoutCrashAttemptResult(true);
+  writer.RecordDumpWithoutCrashAttemptResult(true);
+  writer.RecordDumpWithoutCrashAttemptResult(true);
+  writer.RecordDumpWithoutCrashAttemptResult(false);
+
+  values = CrashReportingMetrics(registry_path()).RetrieveAndResetMetrics();
+  EXPECT_EQ(3, values.crash_dump_attempts);
+  EXPECT_EQ(1, values.successful_crash_dumps);
+  EXPECT_EQ(2, values.failed_crash_dumps);
+  EXPECT_EQ(4, values.dump_without_crash_attempts);
+  EXPECT_EQ(3, values.successful_dumps_without_crash);
+  EXPECT_EQ(1, values.failed_dumps_without_crash);
+
+  values = CrashReportingMetrics(registry_path()).RetrieveAndResetMetrics();
+  EXPECT_EQ(0, values.crash_dump_attempts);
+  EXPECT_EQ(0, values.successful_crash_dumps);
+  EXPECT_EQ(0, values.failed_crash_dumps);
+  EXPECT_EQ(0, values.dump_without_crash_attempts);
+  EXPECT_EQ(0, values.successful_dumps_without_crash);
+  EXPECT_EQ(0, values.failed_dumps_without_crash);
+
+  writer.RecordCrashDumpAttempt();
+  writer.RecordCrashDumpAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+  writer.RecordDumpWithoutCrashAttempt();
+
+  writer.RecordCrashDumpAttemptResult(true);
+  writer.RecordCrashDumpAttemptResult(true);
+  writer.RecordDumpWithoutCrashAttemptResult(true);
+  writer.RecordDumpWithoutCrashAttemptResult(false);
+  writer.RecordDumpWithoutCrashAttemptResult(false);
+
+  values = CrashReportingMetrics(registry_path()).RetrieveAndResetMetrics();
+  EXPECT_EQ(2, values.crash_dump_attempts);
+  EXPECT_EQ(2, values.successful_crash_dumps);
+  EXPECT_EQ(0, values.failed_crash_dumps);
+  EXPECT_EQ(3, values.dump_without_crash_attempts);
+  EXPECT_EQ(1, values.successful_dumps_without_crash);
+  EXPECT_EQ(2, values.failed_dumps_without_crash);
+}
+
+} // namespace browser_watcher