Allow MetricsProviders to request an initial stability log.

BUG=412384

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

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

7 files changed, 172 insertions, 12 deletions

diff --git a/components/metrics.gypi b/components/metrics.gypi
index 25c17eb..945d1c16 100644
--- a/components/metrics.gypi
+++ b/components/metrics.gypi
@@ -37,6 +37,7 @@
         'metrics/metrics_log_uploader.h',
         'metrics/metrics_pref_names.cc',
         'metrics/metrics_pref_names.h',
+        'metrics/metrics_provider.cc',
         'metrics/metrics_provider.h',
         'metrics/metrics_reporting_scheduler.cc',
         'metrics/metrics_reporting_scheduler.h',
diff --git a/components/metrics/BUILD.gn b/components/metrics/BUILD.gn
index cb25367..92f9e89 100644
--- a/components/metrics/BUILD.gn
+++ b/components/metrics/BUILD.gn
@@ -24,6 +24,7 @@ source_set("metrics") {
     "metrics_log_uploader.h",
     "metrics_pref_names.cc",
     "metrics_pref_names.h",
+    "metrics_provider.cc",
     "metrics_provider.h",
     "metrics_reporting_scheduler.cc",
     "metrics_reporting_scheduler.h",
diff --git a/components/metrics/metrics_provider.cc b/components/metrics/metrics_provider.cc
new file mode 100644
index 0000000..0bfdd73
--- /dev/null
+++ b/components/metrics/metrics_provider.cc
@@ -0,0 +1,40 @@
+// Copyright 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/metrics/metrics_provider.h"
+
+namespace metrics {
+
+MetricsProvider::MetricsProvider() {
+}
+
+MetricsProvider::~MetricsProvider() {
+}
+
+void MetricsProvider::OnDidCreateMetricsLog() {
+}
+
+void MetricsProvider::OnRecordingEnabled() {
+}
+
+void MetricsProvider::OnRecordingDisabled() {
+}
+
+void MetricsProvider::ProvideSystemProfileMetrics(
+    SystemProfileProto* system_profile_proto) {
+}
+
+bool MetricsProvider::HasStabilityMetrics() {
+  return false;
+}
+
+void MetricsProvider::ProvideStabilityMetrics(
+    SystemProfileProto* system_profile_proto) {
+}
+
+void MetricsProvider::ProvideGeneralMetrics(
+    ChromeUserMetricsExtension* uma_proto) {
+}
+
+}  // namespace metrics
diff --git a/components/metrics/metrics_provider.h b/components/metrics/metrics_provider.h
index 001e66a..abfbc6f 100644
--- a/components/metrics/metrics_provider.h
+++ b/components/metrics/metrics_provider.h
@@ -17,32 +17,36 @@ class SystemProfileProto_Stability;
 // be filled out by different classes.
 class MetricsProvider {
  public:
-  MetricsProvider() {}
-  virtual ~MetricsProvider() {}
+  MetricsProvider();
+  virtual ~MetricsProvider();
 
   // Called when a new MetricsLog is created.
-  virtual void OnDidCreateMetricsLog() {}
+  virtual void OnDidCreateMetricsLog();
 
   // Called when metrics recording has been enabled.
-  virtual void OnRecordingEnabled() {}
+  virtual void OnRecordingEnabled();
 
   // Called when metrics recording has been disabled.
-  virtual void OnRecordingDisabled() {}
+  virtual void OnRecordingDisabled();
 
   // Provides additional metrics into the system profile.
   virtual void ProvideSystemProfileMetrics(
-      SystemProfileProto* system_profile_proto) {}
+      SystemProfileProto* system_profile_proto);
+
+  // Called once at startup to see whether this provider has stability events
+  // to share. Default implementation always returns false.
+  virtual bool HasStabilityMetrics();
 
   // Provides additional stability metrics. Stability metrics can be provided
   // directly into |stability_proto| fields or by logging stability histograms
   // via the UMA_STABILITY_HISTOGRAM_ENUMERATION() macro.
   virtual void ProvideStabilityMetrics(
-      SystemProfileProto* system_profile_proto) {}
+      SystemProfileProto* system_profile_proto);
 
   // Provides general metrics that are neither system profile nor stability
   // metrics.
   virtual void ProvideGeneralMetrics(
-      ChromeUserMetricsExtension* uma_proto) {}
+      ChromeUserMetricsExtension* uma_proto);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MetricsProvider);
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index cf71f14..cdcb1f5 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -564,13 +564,16 @@ void MetricsService::InitializeMetricsState() {
   log_manager_.LoadPersistedUnsentLogs();
 
   session_id_ = local_state_->GetInteger(metrics::prefs::kMetricsSessionID);
-
+  bool exited_cleanly = true;
   if (!local_state_->GetBoolean(metrics::prefs::kStabilityExitedCleanly)) {
     IncrementPrefValue(metrics::prefs::kStabilityCrashCount);
     // Reset flag, and wait until we call LogNeedForCleanShutdown() before
     // monitoring.
     local_state_->SetBoolean(metrics::prefs::kStabilityExitedCleanly, true);
+    exited_cleanly = false;
+  }
 
+  if (!exited_cleanly || ProvidersHaveStabilityMetrics()) {
     // TODO(rtenneti): On windows, consider saving/getting execution_phase from
     // the registry.
     int execution_phase =
@@ -578,8 +581,9 @@ void MetricsService::InitializeMetricsState() {
     UMA_HISTOGRAM_SPARSE_SLOWLY("Chrome.Browser.CrashedExecutionPhase",
                                 execution_phase);
 
-    // If the previous session didn't exit cleanly, then prepare an initial
-    // stability log if UMA is enabled.
+    // If the previous session didn't exit cleanly, or if any provider
+    // explicitly requests it, prepare an initial stability log -
+    // provided UMA is enabled.
     if (state_manager_->IsMetricsReportingEnabled())
       PrepareInitialStabilityLog();
   }
@@ -896,9 +900,18 @@ void MetricsService::StageNewLog() {
   DCHECK(log_manager_.has_staged_log());
 }
 
+bool MetricsService::ProvidersHaveStabilityMetrics() {
+  // Check whether any metrics provider has stability metrics.
+  for (size_t i = 0; i < metrics_providers_.size(); ++i) {
+    if (metrics_providers_[i]->HasStabilityMetrics())
+      return true;
+  }
+
+  return false;
+}
+
 void MetricsService::PrepareInitialStabilityLog() {
   DCHECK_EQ(INITIALIZED, state_);
-  DCHECK_NE(0, local_state_->GetInteger(metrics::prefs::kStabilityCrashCount));
 
   scoped_ptr<MetricsLog> initial_stability_log(
       CreateLog(MetricsLog::INITIAL_STABILITY_LOG));
diff --git a/components/metrics/metrics_service.h b/components/metrics/metrics_service.h
index fb92e9b..a1d7ad7 100644
--- a/components/metrics/metrics_service.h
+++ b/components/metrics/metrics_service.h
@@ -315,6 +315,10 @@ class MetricsService : public base::HistogramFlattener {
   // (depending on |state_|), and stages it for upload.
   void StageNewLog();
 
+  // Returns true if any of the registered metrics providers have stability
+  // metrics to report.
+  bool ProvidersHaveStabilityMetrics();
+
   // Prepares the initial stability log, which is only logged when the previous
   // run of Chrome crashed.  This log contains any stability metrics left over
   // from that previous run, and only these stability metrics.  It uses the
diff --git a/components/metrics/metrics_service_unittest.cc b/components/metrics/metrics_service_unittest.cc
index e85f351..3e4f504 100644
--- a/components/metrics/metrics_service_unittest.cc
+++ b/components/metrics/metrics_service_unittest.cc
@@ -31,6 +31,30 @@ scoped_ptr<ClientInfo> ReturnNoBackup() {
   return scoped_ptr<ClientInfo>();
 }
 
+class TestMetricsProvider : public metrics::MetricsProvider {
+ public:
+  explicit TestMetricsProvider(bool has_stability_metrics) :
+      has_stability_metrics_(has_stability_metrics),
+      provide_stability_metrics_called_(false) {
+  }
+
+  virtual bool HasStabilityMetrics() OVERRIDE { return has_stability_metrics_; }
+  virtual void ProvideStabilityMetrics(
+      SystemProfileProto* system_profile_proto) OVERRIDE {
+    provide_stability_metrics_called_ = true;
+  }
+
+  bool provide_stability_metrics_called() const {
+    return provide_stability_metrics_called_;
+  }
+
+ private:
+  bool has_stability_metrics_;
+  bool provide_stability_metrics_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestMetricsProvider);
+};
+
 class TestMetricsService : public MetricsService {
  public:
   TestMetricsService(MetricsStateManager* state_manager,
@@ -138,10 +162,83 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCleanShutDown) {
   TestMetricsServiceClient client;
   TestMetricsService service(
       GetMetricsStateManager(), &client, GetLocalState());
+
+  TestMetricsProvider* test_provider = new TestMetricsProvider(false);
+  service.RegisterMetricsProvider(
+      scoped_ptr<metrics::MetricsProvider>(test_provider));
+
   service.InitializeMetricsRecordingState();
   // No initial stability log should be generated.
   EXPECT_FALSE(service.log_manager()->has_unsent_logs());
   EXPECT_FALSE(service.log_manager()->has_staged_log());
+
+  // The test provider should not have been called upon to provide stability
+  // metrics.
+  EXPECT_FALSE(test_provider->provide_stability_metrics_called());
+}
+
+TEST_F(MetricsServiceTest, InitialStabilityLogAtProviderRequest) {
+  EnableMetricsReporting();
+
+  // Save an existing system profile to prefs, to correspond to what would be
+  // saved from a previous session.
+  TestMetricsServiceClient client;
+  TestMetricsLog log("client", 1, &client, GetLocalState());
+  log.RecordEnvironment(std::vector<metrics::MetricsProvider*>(),
+                        std::vector<variations::ActiveGroupId>(),
+                        0);
+
+  // Record stability build time and version from previous session, so that
+  // stability metrics (including exited cleanly flag) won't be cleared.
+  GetLocalState()->SetInt64(prefs::kStabilityStatsBuildTime,
+                            MetricsLog::GetBuildTime());
+  GetLocalState()->SetString(prefs::kStabilityStatsVersion,
+                             client.GetVersionString());
+
+  // Set the clean exit flag, as that will otherwise cause a stabilty
+  // log to be produced, irrespective provider requests.
+  GetLocalState()->SetBoolean(prefs::kStabilityExitedCleanly, true);
+
+  TestMetricsService service(
+      GetMetricsStateManager(), &client, GetLocalState());
+  // Add a metrics provider that requests a stability log.
+  TestMetricsProvider* test_provider = new TestMetricsProvider(true);
+  service.RegisterMetricsProvider(
+      scoped_ptr<MetricsProvider>(test_provider));
+
+  service.InitializeMetricsRecordingState();
+
+  // The initial stability log should be generated and persisted in unsent logs.
+  MetricsLogManager* log_manager = service.log_manager();
+  EXPECT_TRUE(log_manager->has_unsent_logs());
+  EXPECT_FALSE(log_manager->has_staged_log());
+
+  // The test provider should have been called upon to provide stability
+  // metrics.
+  EXPECT_TRUE(test_provider->provide_stability_metrics_called());
+
+  // Stage the log and retrieve it.
+  log_manager->StageNextLogForUpload();
+  EXPECT_TRUE(log_manager->has_staged_log());
+
+  std::string uncompressed_log;
+  EXPECT_TRUE(GzipUncompress(log_manager->staged_log(),
+                                      &uncompressed_log));
+
+  ChromeUserMetricsExtension uma_log;
+  EXPECT_TRUE(uma_log.ParseFromString(uncompressed_log));
+
+  EXPECT_TRUE(uma_log.has_client_id());
+  EXPECT_TRUE(uma_log.has_session_id());
+  EXPECT_TRUE(uma_log.has_system_profile());
+  EXPECT_EQ(0, uma_log.user_action_event_size());
+  EXPECT_EQ(0, uma_log.omnibox_event_size());
+  EXPECT_EQ(0, uma_log.histogram_event_size());
+  EXPECT_EQ(0, uma_log.profiler_event_size());
+  EXPECT_EQ(0, uma_log.perf_data_size());
+
+  // As there wasn't an unclean shutdown, this log has zero crash count.
+  EXPECT_EQ(0, uma_log.system_profile().stability().crash_count());
 }
 
 TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {