Browser watcher part three.

components/browser_watcher/watcher_metrics_provider_win.cc

Index: components/browser_watcher/watcher_metrics_provider_win.cc
diff --git a/components/browser_watcher/watcher_metrics_provider_win.cc b/components/browser_watcher/watcher_metrics_provider_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f4c582ec6754408c596373bcb52ea3dd6c8252ea
--- /dev/null
+++ b/components/browser_watcher/watcher_metrics_provider_win.cc
@@ -0,0 +1,108 @@
+// Copyright (c) 2014 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/watcher_metrics_provider_win.h"
+
+#include "base/metrics/sparse_histogram.h"
+#include "base/process/process_handle.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
+#include "base/win/registry.h"
+
+namespace browser_watcher {
+
+namespace {
+
+// This function does soft matching on the PID recorded in the key only.
+// Due to PID reuse, the possibility exists that the process that's now live
+// with the given PID is not the same process the data was recorded for.
+// This doesn't matter for the purpose, as eventually the data will be
+// scavenged and reported.
+bool IsDeadProcess(base::StringPiece16 key_name) {
+  // Truncate the input string to the first occurrence of '-', if one exists.
+  size_t num_end = key_name.find(L'-');
+  if (num_end != base::StringPiece16::npos)
+    key_name = key_name.substr(0, num_end);
+
+  // Convert to the numeric PID.
+  int pid = 0;

[erikwright (departed), 2014/11/18 21:13:36]

this CL and the other one I noticed you are using int to store PIDs. But it
seems there's a risk of overflow since GetProcessId (for example) returns DWORD.

http://msdn.microsoft.com/en-ca/library/windows/desktop/ms683215(v=vs.85).aspx

[Sigurður Ásgeirsson, 2014/11/18 21:52:36]

Yeah, in practice the PID space is pretty small though. I've added a compile
assert that the type is at least large enough - are you suggesting some other
change?

+  if (!base::StringToInt(key_name, &pid) || pid == 0)
+    return true;
+
+  // This is a very inexpensive check for the common case of our own PID.
+  if (static_cast<base::ProcessId>(pid) == base::GetCurrentProcId())
+    return false;
+
+  // The process is not our own - see whether a process with this PID exists.
+  // This is more expensive than the above check, but should also be very rare,
+  // as this only happens more than once for a given PID if a user is running
+  // multiple Chrome instances concurrently.
+  base::ProcessHandle process = base::kNullProcessHandle;
+  if (base::OpenProcessHandle(static_cast<base::ProcessId>(pid), &process)) {
+    base::CloseProcessHandle(process);
+
+    // The fact that it was possible to open the process says it's live.
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
+// TODO(siggi): is this an acceptable name?

[erikwright (departed), 2014/11/18 21:13:36]

Seems reasonable to me.

[Sigurður Ásgeirsson, 2014/11/18 21:52:37]

Acknowledged.

+const char WatcherMetricsProviderWin::kBrowserExitCodeHistogramName[] =
+    "Stability.BrowserExitCodes";
+
+WatcherMetricsProviderWin::WatcherMetricsProviderWin(
+    const base::char16* registry_path) : registry_path_(registry_path) {
+}
+
+WatcherMetricsProviderWin::~WatcherMetricsProviderWin() {
+}
+
+void WatcherMetricsProviderWin::ProvideStabilityMetrics(
+    metrics::SystemProfileProto* /* system_profile_proto */) {
+  // Note that if there are multiple instances of Chrome running in the same
+  // user account, there's a small race that will double-reporti the exit codes

[erikwright (departed), 2014/11/18 21:13:36]

reporti -> report

[Sigurður Ásgeirsson, 2014/11/18 21:52:37]

Done.

+  // from both/multiple instances. This ought to be vanishingly rare and will
+  // only manifest as low-level "random" noise. To work around this it would be
+  // necessary to implement some form of global locking, which is not worth it
+  // here.
+  base::win::RegKey regkey(HKEY_CURRENT_USER,
+                           registry_path_.c_str(),
+                           KEY_QUERY_VALUE | KEY_SET_VALUE);
+
+  std::vector<base::string16> to_delete;

[erikwright (departed), 2014/11/18 21:13:36]

move inside the if(num!=0) block

[erikwright (departed), 2014/11/18 21:13:36]

<vector>

[Sigurður Ásgeirsson, 2014/11/18 21:52:37]

Done.

[Sigurður Ásgeirsson, 2014/11/18 21:52:37]

Done.

+  size_t num = regkey.GetValueCount();
+  if (num != 0) {
+    // Record the exit codes in a sparse stability histogram, as the range of
+    // values used to report failures is large.
+    base::HistogramBase* exit_code_histogram =
+        base::SparseHistogram::FactoryGet(kBrowserExitCodeHistogramName,

[erikwright (departed), 2014/11/18 21:13:36]

Is this not available using the macros?

[Sigurður Ásgeirsson, 2014/11/18 21:52:37]

This particular variant is not present in the macros. Alexei indicated just use
the API.

+            base::HistogramBase::kUmaStabilityHistogramFlag);
+
+    for (size_t i = 0; i < num; ++i) {
+      base::string16 name;
+      if (regkey.GetValueNameAt(i, &name) == ERROR_SUCCESS) {
+        DWORD exit_code = 0;
+        if (regkey.ReadValueDW(name.c_str(), &exit_code) == ERROR_SUCCESS) {
+          // Do not report exit codes for processes that are still live,
+          // notably for our own process.
+          if (exit_code != STILL_ACTIVE || IsDeadProcess(name)) {
+            // Tag the value for deletion.

[erikwright (departed), 2014/11/18 21:13:36]

Obvious.

[Sigurður Ásgeirsson, 2014/11/18 21:52:37]

Done.

+            to_delete.push_back(name);
+            exit_code_histogram->Add(exit_code);
+          }
+        }
+      }
+    }
+
+    // Delete the values reported above.
+    for (size_t i = 0; i < to_delete.size(); ++i)
+      regkey.DeleteValue(to_delete[i].c_str());
+  }
+}
+
+}  // namespace browser_watcher