Add ExitFunnel to prepare for instrumenting browser exits.

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
index c6863f7723b6cbd2dca5baeafa35d6b983e77385..5ad40a230b67ce151645ff9a4d31a058a14bc211 100644
--- a/components/browser_watcher/watcher_metrics_provider_win.cc
+++ b/components/browser_watcher/watcher_metrics_provider_win.cc
@@ -4,12 +4,14 @@
 
 #include "components/browser_watcher/watcher_metrics_provider_win.h"
 
+#include <limits>
 #include <vector>
 
 #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/strings/utf_string_conversions.h"
 #include "base/win/registry.h"
 
 namespace browser_watcher {
@@ -60,10 +62,143 @@ bool IsDeadProcess(base::StringPiece16 key_name) {
   return true;
 }
 
+void RecordExitCodes(const base::string16& registry_path) {
+  base::win::RegKey regkey(HKEY_CURRENT_USER,
+                           registry_path.c_str(),
+                           KEY_QUERY_VALUE | KEY_SET_VALUE);
+  if (!regkey.Valid())
+    return;
+
+  size_t num = regkey.GetValueCount();
+  if (num == 0)
+    return;
+  std::vector<base::string16> to_delete;
+
+  // 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(
+          WatcherMetricsProviderWin::kBrowserExitCodeHistogramName,
+          base::HistogramBase::kUmaStabilityHistogramFlag);
+
+  for (size_t i = 0; i < num; ++i) {
+    base::string16 name;
+    if (regkey.GetValueNameAt(static_cast<int>(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)) {
+          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());
+}
+
+void RecordSingleExitFunnel(base::win::RegKey* parent_key,
+                            const base::char16* name) {
+  DCHECK(parent_key);
+  DCHECK(name);
+
+  base::win::RegKey regkey(parent_key->Handle(), name, KEY_READ | KEY_WRITE);
+  if (!regkey.Valid())
+    return;
+
+  // Exit early if no work to do.
+  size_t num = regkey.GetValueCount();
+  if (num == 0)
+    return;
+
+  // Enumerate the recorded events for this process for processing.
+  std::vector<std::pair<base::string16, int64>> events;
+  int64 min_time = std::numeric_limits<int64>::max();

[erikwright (departed), 2014/12/11 15:00:46]

Everything in this method needs to be extracted out into something tested. It's
probably the thing that the unit test should use.

[Sigurður Ásgeirsson, 2014/12/12 16:21:40]

Maybe so, though the tests also want to verify that the data contract isn't
violated across versions.
In any case, I think it's good enough for what we need it for.

+  for (size_t i = 0; i < num; ++i) {
+    base::string16 event_name;
+    LONG res = regkey.GetValueNameAt(static_cast<int>(i), &event_name);
+    if (res == ERROR_SUCCESS) {
+      int64 event_time = 0;
+      res = regkey.ReadInt64(event_name.c_str(), &event_time);
+      if (res == ERROR_SUCCESS) {
+        events.push_back(std::make_pair(event_name, event_time));
+        min_time = std::min(min_time, event_time);
+      }
+    }
+  }
+
+  // Attempt to delete the values before reporting anything.
+  // Exit if this fails to make sure there is no double-reporting on e.g.
+  // permission problems or other corruption.
+  for (size_t i = 0; i < events.size(); ++i) {
+    const base::string16& event_name = events[i].first;
+    LONG res = regkey.DeleteValue(event_name.c_str());
+    if (res != ERROR_SUCCESS) {
+      LOG(ERROR) << "Failed to delete value " << event_name;
+      return;
+    }
+  }
+
+  // Record the exit funnel event times in a sparse stability histogram.
+  for (size_t i = 0; i < events.size(); ++i) {
+    std::string histogram_name(
+        WatcherMetricsProviderWin::kExitFunnelHistogramPrefix);
+    histogram_name.append(base::WideToUTF8(events[i].first));
+    base::TimeDelta event_time =
+        base::Time::FromInternalValue(events[i].second) -
+            base::Time::FromInternalValue(min_time);
+    base::HistogramBase* histogram =
+        base::SparseHistogram::FactoryGet(
+            histogram_name.c_str(),
+            base::HistogramBase::kUmaStabilityHistogramFlag);
+
+    // Record the time rounded up to the nearest millisecond.
+    histogram->Add(event_time.InMillisecondsRoundedUp());
+  }
+}
+
+void RecordExitFunnels(const base::string16& registry_path) {
+  base::win::RegistryKeyIterator it(HKEY_CURRENT_USER, registry_path.c_str());
+  if (!it.Valid())
+    return;
+
+  // Exit early if no work to do.
+  if (it.SubkeyCount() == 0)
+    return;
+
+  // Open the key we use for deletion preemptively to prevent reporting
+  // multiple times on permission problems.
+  base::win::RegKey key(HKEY_CURRENT_USER,
+                        registry_path.c_str(),
+                        KEY_QUERY_VALUE);
+  if (!key.Valid()) {
+    LOG(ERROR) << "Failed to open " << registry_path << " for writing.";
+    return;
+  }
+
+  std::vector<base::string16> to_delete;
+  for (; it.Valid(); ++it) {
+    RecordSingleExitFunnel(&key, it.Name());
+    to_delete.push_back(it.Name());
+  }
+
+  for (size_t i = 0; i < to_delete.size(); ++i) {
+    LONG res = key.DeleteEmptyKey(to_delete[i].c_str());
+    if (res != ERROR_SUCCESS)
+      LOG(ERROR) << "Failed to delete key " << to_delete[i];
+  }
+}
+
 }  // namespace
 
 const char WatcherMetricsProviderWin::kBrowserExitCodeHistogramName[] =
     "Stability.BrowserExitCodes";
+const char WatcherMetricsProviderWin::kExitFunnelHistogramPrefix[] =
+    "Stability.ExitFunnel.";
 
 WatcherMetricsProviderWin::WatcherMetricsProviderWin(
     const base::char16* registry_path) : registry_path_(registry_path) {
@@ -80,39 +215,8 @@ void WatcherMetricsProviderWin::ProvideStabilityMetrics(
   // 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);
-
-  size_t num = regkey.GetValueCount();
-  if (num != 0) {
-    std::vector<base::string16> to_delete;
-
-    // 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,
-            base::HistogramBase::kUmaStabilityHistogramFlag);
-
-    for (size_t i = 0; i < num; ++i) {
-      base::string16 name;
-      if (regkey.GetValueNameAt(static_cast<int>(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)) {
-            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());
-  }
+  RecordExitCodes(registry_path_);
+  RecordExitFunnels(registry_path_);
 }
 
 }  // namespace browser_watcher