Analyze and record Windows system crashes on browser crash.

Bug: 799526
Change-Id: Ie61ff4ada2c1ed57949414c0c400ea93481a68c5
Reviewed-on: https://chromium-review.googlesource.com/897904
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534149}

components/metrics/clean_exit_beacon.cc

@@ -71,7 +71,7 @@ void CleanExitBeacon::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kStabilityExitedCleanly, true);
 
   registry->RegisterTimePref(prefs::kStabilityBrowserLastLiveTimeStamp,
-                             base::Time::Now(), PrefRegistry::LOSSY_PREF);
+                             base::Time(), PrefRegistry::LOSSY_PREF);
 }
 
 void CleanExitBeacon::WriteBeaconValue(bool value) {

components/metrics/metrics_pref_names.cc

@@ -193,6 +193,12 @@ const char kStabilityStatsBuildTime[] =
 const char kStabilityStatsVersion[] =
     "user_experience_metrics.stability.stats_version";
 
+// Number of times the application exited uncleanly and the system session
+// embedding the browser session ended abnormally since the last report.
+// Windows only.
+const char kStabilitySystemCrashCount[] =
+    "user_experience_metrics.stability.system_crash_count";
+
 // Number of times the version number stored in prefs did not match the
 // serialized system profile version number.
 const char kStabilityVersionMismatchCount[] =

components/metrics/metrics_pref_names.h

@@ -60,6 +60,7 @@ extern const char kStabilitySavedSystemProfileHash[];
 extern const char kStabilitySessionEndCompleted[];
 extern const char kStabilityStatsBuildTime[];
 extern const char kStabilityStatsVersion[];
+extern const char kStabilitySystemCrashCount[];
 extern const char kStabilityVersionMismatchCount[];
 
 // Preferences for generating metrics at uninstall time.

components/metrics/metrics_service.cc

@@ -485,7 +485,8 @@ void MetricsService::InitializeMetricsState() {
 
   StabilityMetricsProvider provider(local_state_);
   if (!state_manager_->clean_exit_beacon()->exited_cleanly()) {
-    provider.LogCrash();
+    provider.LogCrash(
+        state_manager_->clean_exit_beacon()->browser_last_live_timestamp());
     // Reset flag, and wait until we call LogNeedForCleanShutdown() before
     // monitoring.
     state_manager_->clean_exit_beacon()->WriteBeaconValue(true);

components/metrics/stability_metrics_provider.cc

@@ -14,6 +14,9 @@
 #if defined(OS_ANDROID)
 #include "base/android/build_info.h"
 #endif
+#if defined(OS_WIN)
+#include "components/metrics/system_session_analyzer_win.h"
+#endif
 
 namespace metrics {
 
@@ -66,6 +69,9 @@ void StabilityMetricsProvider::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterIntegerPref(prefs::kStabilityCrashCountWithoutGmsCoreUpdate,
                                 0);
 #endif
+#if defined(OS_WIN)
+  registry->RegisterIntegerPref(prefs::kStabilitySystemCrashCount, 0);
+#endif
 }
 
 void StabilityMetricsProvider::ClearSavedStabilityMetrics() {
@@ -81,6 +87,9 @@ void StabilityMetricsProvider::ClearSavedStabilityMetrics() {
   // Note: kStabilityDiscardCount is not cleared as its intent is to measure
   // the number of times data is discarded, even across versions.
   local_state_->SetInteger(prefs::kStabilityVersionMismatchCount, 0);
+#if defined(OS_WIN)
+  local_state_->SetInteger(prefs::kStabilitySystemCrashCount, 0);
+#endif
 }
 
 void StabilityMetricsProvider::ProvideStabilityMetrics(
@@ -135,6 +144,13 @@ void StabilityMetricsProvider::ProvideStabilityMetrics(
     UMA_STABILITY_HISTOGRAM_COUNTS_100(
         "Stability.Internals.VersionMismatchCount", pref_value);
   }
+
+#if defined(OS_WIN)
+  if (GetPrefValue(prefs::kStabilitySystemCrashCount, &pref_value)) {
+    UMA_STABILITY_HISTOGRAM_COUNTS_100("Stability.Internals.SystemCrashCount",
+                                       pref_value);
+  }
+#endif
 }
 
 void StabilityMetricsProvider::RecordBreakpadRegistration(bool success) {
@@ -163,7 +179,7 @@ void StabilityMetricsProvider::MarkSessionEndCompleted(bool end_completed) {
   local_state_->SetBoolean(prefs::kStabilitySessionEndCompleted, end_completed);
 }
 
-void StabilityMetricsProvider::LogCrash() {
+void StabilityMetricsProvider::LogCrash(base::Time last_live_timestamp) {
   IncrementPrefValue(prefs::kStabilityCrashCount);
 
 #if defined(OS_ANDROID)
@@ -173,6 +189,10 @@ void StabilityMetricsProvider::LogCrash() {
   if (UpdateGmsCoreVersionPref(local_state_))
     IncrementPrefValue(prefs::kStabilityCrashCountWithoutGmsCoreUpdate);
 #endif
+
+#if defined(OS_WIN)
+  MaybeLogSystemCrash(last_live_timestamp);
+#endif
 }
 
 void StabilityMetricsProvider::LogStabilityLogDeferred() {
@@ -191,6 +211,34 @@ void StabilityMetricsProvider::LogStabilityVersionMismatch() {
   IncrementPrefValue(prefs::kStabilityVersionMismatchCount);
 }
 
+#if defined(OS_WIN)
+bool StabilityMetricsProvider::IsUncleanSystemSession(
+    base::Time last_live_timestamp) {
+  DCHECK_NE(base::Time(), last_live_timestamp);
+  // There's a non-null last live timestamp, see if this occurred in
+  // a Windows system session that ended uncleanly. The expectation is that
+  // |last_live_timestamp| will have occurred in the immediately previous system
+  // session, but if the system has been restarted many times since Chrome last
+  // ran, that's not necessarily true. Log traversal can be expensive, so we
+  // limit the analyzer to reaching back three previous system sessions to bound
+  // the cost of the traversal.
+  SystemSessionAnalyzer analyzer(3);
+
+  SystemSessionAnalyzer::Status status =
+      analyzer.IsSessionUnclean(last_live_timestamp);
+
+  return status == SystemSessionAnalyzer::UNCLEAN;
+}
+
+void StabilityMetricsProvider::MaybeLogSystemCrash(
+    base::Time last_live_timestamp) {
+  if (last_live_timestamp != base::Time() &&
+      IsUncleanSystemSession(last_live_timestamp)) {
+    IncrementPrefValue(prefs::kStabilitySystemCrashCount);
+  }
+}
+#endif
+
 void StabilityMetricsProvider::IncrementPrefValue(const char* path) {
   int value = local_state_->GetInteger(path);
   local_state_->SetInteger(path, value + 1);

components/metrics/stability_metrics_provider.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_METRICS_STABILITY_METRICS_PROVIDER_H_
 #define COMPONENTS_METRICS_STABILITY_METRICS_PROVIDER_H_
 
+#include "base/time/time.h"
+#include "build/build_config.h"
 #include "components/metrics/metrics_provider.h"
 
 class PrefService;
@@ -28,13 +30,21 @@ class StabilityMetricsProvider : public MetricsProvider {
   void CheckLastSessionEndCompleted();
   void MarkSessionEndCompleted(bool end_completed);
 
-  void LogCrash();
+  void LogCrash(base::Time last_live_timestamp);
   void LogStabilityLogDeferred();
   void LogStabilityDataDiscarded();
   void LogLaunch();
   void LogStabilityVersionMismatch();
 
  private:
+#if defined(OS_WIN)
+  // This function is virtual for testing. The |last_live_timestamp| is a
+  // time point where the previous browser was known to be alive, and is used
+  // to determine whether the system session embedding that timestamp terminated
+  // uncleanly.
+  virtual bool IsUncleanSystemSession(base::Time last_live_timestamp);
+  void MaybeLogSystemCrash(base::Time last_live_timestamp);
+#endif
   // Increments an Integer pref value specified by |path|.
   void IncrementPrefValue(const char* path);
 

components/metrics/stability_metrics_provider_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "components/metrics/stability_metrics_provider.h"
 
+#include "base/test/histogram_tester.h"
+#include "build/build_config.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/metrics_proto/system_profile.pb.h"
@@ -46,7 +48,7 @@ TEST_F(StabilityMetricsProviderTest, RecordStabilityMetrics) {
   {
     StabilityMetricsProvider recorder(&prefs_);
     recorder.LogLaunch();
-    recorder.LogCrash();
+    recorder.LogCrash(base::Time());
     recorder.MarkSessionEndCompleted(false);
     recorder.CheckLastSessionEndCompleted();
     recorder.RecordBreakpadRegistration(true);
@@ -73,4 +75,58 @@ TEST_F(StabilityMetricsProviderTest, RecordStabilityMetrics) {
   }
 }
 
+#if defined(OS_WIN)
+namespace {
+
+class TestingStabilityMetricsProvider : public StabilityMetricsProvider {
+ public:
+  TestingStabilityMetricsProvider(PrefService* local_state,
+                                  base::Time unclean_session_time)
+      : StabilityMetricsProvider(local_state),
+        unclean_session_time_(unclean_session_time) {}
+
+  bool IsUncleanSystemSession(base::Time last_live_timestamp) override {
+    return last_live_timestamp == unclean_session_time_;
+  }
+
+ private:
+  const base::Time unclean_session_time_;
+};
+
+}  // namespace
+
+TEST_F(StabilityMetricsProviderTest, RecordSystemCrashMetrics) {
+  {
+    base::Time unclean_time = base::Time::Now();
+    TestingStabilityMetricsProvider recorder(&prefs_, unclean_time);
+
+    // Any crash with a last_live_timestamp equal to unclean_time will
+    // be logged as a system crash as per the implementation of
+    // TestingStabilityMetricsProvider, so this will log a system crash.
+    recorder.LogCrash(unclean_time);
+
+    // Record a crash with no system crash.
+    recorder.LogCrash(unclean_time - base::TimeDelta::FromMinutes(1));
+  }
+
+  {
+    StabilityMetricsProvider stability_provider(&prefs_);
+    MetricsProvider* provider = &stability_provider;
+    SystemProfileProto system_profile;
+
+    base::HistogramTester histogram_tester;
+
+    provider->ProvideStabilityMetrics(&system_profile);
+
+    const SystemProfileProto_Stability& stability = system_profile.stability();
+    // Two crashes, one system crash.
+    EXPECT_EQ(2, stability.crash_count());
+
+    histogram_tester.ExpectTotalCount("Stability.Internals.SystemCrashCount",
+                                      1);
+  }
+}
+
+#endif
+
 }  // namespace metrics

tools/metrics/histograms/histograms.xml

@@ -85264,6 +85264,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Stability.Internals.SystemCrashCount" units="crashes">
+  <owner>siggi@chromium.org</owner>
+  <summary>
+    Number of times a browser crash was detected, where the browser was last
+    known to be alive in a Windows system session that terminated abnormally.
+  </summary>
+</histogram>
+
 <histogram name="Stability.Internals.VersionMismatchCount" units="counts">
   <owner>manzagop@chromium.org</owner>
   <summary>