Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(617)

Unified Diff: components/metrics/metrics_service.cc

Issue 2428413005: Revise system profile prefs clearing (Closed)
Patch Set: Update LoadSavedEnvironmentFromPrefs's comment Created 4 years, 2 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « components/metrics/metrics_log_unittest.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: components/metrics/metrics_service.cc
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index 2acc9cb0b88cbec1f0538b42147f7c800af98958..fb49c5cc6d0a0a7ef3bf31f70246b00c80cabd08 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -584,11 +584,13 @@ void MetricsService::InitializeMetricsState() {
clean_exit_beacon_.WriteBeaconValue(true);
}
+ // ProvidersHaveInitialStabilityMetrics is called first to ensure it is never
+ // bypassed.
+ const bool is_initial_stability_log_required =
+ ProvidersHaveInitialStabilityMetrics() ||
+ !clean_exit_beacon_.exited_cleanly();
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 =
@@ -606,17 +608,29 @@ void MetricsService::InitializeMetricsState() {
}
}
- // 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. This is to avoid the stability data misattribution that could
+ // occur if the current version crashed before saving its own system profile.
+ // Note however this clearing occurs only after preparing the initial
+ // stability log, an operation that requires the previous version's system
+ // profile. At this point, stability metrics pertaining to the previous
+ // version have been cleared.
+ 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_);
« no previous file with comments | « components/metrics/metrics_log_unittest.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698