Revise system profile prefs clearing

components/metrics/metrics_service.cc

 // 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.
 
 //------------------------------------------------------------------------------
 // Description of the life cycle of a instance of MetricsService.
 //
 //  OVERVIEW
 //
 // A MetricsService instance is typically created at application startup.  It is
@@ skipping 566 matching lines @@
 
   session_id_ = local_state_->GetInteger(prefs::kMetricsSessionID);
 
   if (!clean_exit_beacon_.exited_cleanly()) {
     IncrementPrefValue(prefs::kStabilityCrashCount);
     // Reset flag, and wait until we call LogNeedForCleanShutdown() before
     // monitoring.
     clean_exit_beacon_.WriteBeaconValue(true);
   }
 
+  bool is_initial_stability_log_required =

[Alexei Svitkine (slow), 2016/10/20 16:47:26]

Nit: const

[manzagop (departed), 2016/10/20 18:38:37]

Done.

+      !clean_exit_beacon_.exited_cleanly() ||
+      ProvidersHaveInitialStabilityMetrics();
   bool has_initial_stability_log = false;
-  bool providers_have_initial_stability_metrics =
-      ProvidersHaveInitialStabilityMetrics();
-  if (!clean_exit_beacon_.exited_cleanly() ||
-      providers_have_initial_stability_metrics) {
+  if (is_initial_stability_log_required) {
     // TODO(rtenneti): On windows, consider saving/getting execution_phase from
     // the registry.
     int execution_phase =
         local_state_->GetInteger(prefs::kStabilityExecutionPhase);
     UMA_HISTOGRAM_SPARSE_SLOWLY("Chrome.Browser.CrashedExecutionPhase",
                                 execution_phase);
 
     // 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()) {
       has_initial_stability_log = PrepareInitialStabilityLog(previous_version);
       if (!has_initial_stability_log)
         IncrementPrefValue(prefs::kStabilityDeferredCount);
     }
   }
 
-  // If no initial stability log was generated and there was a version upgrade,
-  // clear the stability stats from the previous version (so that they don't get
+  // If the version changed, but no initial stability log was generated, clear
+  // the stability stats from the previous version (so that they don't get
   // attributed to the current version). This could otherwise happen due to a
   // number of different edge cases, such as if the last version crashed before
   // it could save off a system profile or if UMA reporting is disabled (which
   // normally results in stats being accumulated).
-  if (!has_initial_stability_log && version_changed) {
+  if (version_changed && !has_initial_stability_log) {
     ClearSavedStabilityMetrics();
     IncrementPrefValue(prefs::kStabilityDiscardCount);
   }
 
+  // If the version changed, the system profile is obsolete and needs to be
+  // cleared.

[Alexei Svitkine (slow), 2016/10/20 16:47:26]

I think I do agree with this logic, but the comment should be expanded to
provide more context since originally it wasn't obvious to me that this is
desired.

The reason i think this is OK is because by this point if there stability stats
that warranted creating an initial stability log, those stats would already have
been included in that log. So we're not losing those. But the reason we want to
clear it is if we crash before we have a chance to save the new system profile,
so that those new crashes don't get attributed to the old version.

Can you make the comment include the above reasoning? Thanks!

[manzagop (departed), 2016/10/20 18:38:37]

Done.

+  if (version_changed) {
+    local_state->ClearPref(prefs::kStabilitySavedSystemProfile);
+    local_state->ClearPref(prefs::kStabilitySavedSystemProfileHash);
+  }
+
   // Update session ID.
   ++session_id_;
   local_state_->SetInteger(prefs::kMetricsSessionID, session_id_);
 
   // Stability bookkeeping
   IncrementPrefValue(prefs::kStabilityLaunchCount);
 
   DCHECK_EQ(UNINITIALIZED_PHASE, execution_phase_);
   SetExecutionPhase(START_METRICS_RECORDING, local_state_);
 
@@ skipping 578 matching lines @@
                  base::Time::Now().ToTimeT());
 }
 
 void MetricsService::SkipAndDiscardUpload() {
   log_manager_.DiscardStagedLog();
   scheduler_->UploadCancelled();
   log_upload_in_progress_ = false;
 }
 
 }  // namespace metrics