MetricsService: ensure in-memory/pref execution phase are in sync

components/metrics/metrics_service.cc

Index: components/metrics/metrics_service.cc
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index 5c9448987ab9b42348f2ed6a8b0743936542dacb..82380f59d271db15be58baf0bbe78325d7ca8308 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -222,6 +222,7 @@ ResponseStatus ResponseCodeToStatus(int response_code) {
 void MarkAppCleanShutdownAndCommit(CleanExitBeacon* clean_exit_beacon,
                                    PrefService* local_state) {
   clean_exit_beacon->WriteBeaconValue(true);
+  // Note: the in-memory MetricsService::execution_phase_ is not updated.
   local_state->SetInteger(prefs::kStabilityExecutionPhase,
                           MetricsService::SHUTDOWN_COMPLETE);
   // Start writing right away (write happens on a different thread).
@@ -480,6 +481,8 @@ void MetricsService::OnAppEnterBackground() {
 
 void MetricsService::OnAppEnterForeground() {
   clean_exit_beacon_.WriteBeaconValue(false);
+  // Restore the execution phase stored in prefs.

[Alexei Svitkine (slow), 2016/11/24 21:20:20]

Nit: Mention MarkAppCleanShutdownAndCommit() here.

[manzagop (departed), 2016/11/24 22:40:42]

Done.

+  local_state_->SetInteger(prefs::kStabilityExecutionPhase, execution_phase_);
   StartSchedulerIfNecessary();
 }
 #else
@@ -521,8 +524,7 @@ void MetricsService::ClearSavedStabilityMetrics() {
   local_state_->SetInteger(prefs::kStabilityCrashCount, 0);
   local_state_->SetInteger(prefs::kStabilityDebuggerPresent, 0);
   local_state_->SetInteger(prefs::kStabilityDebuggerNotPresent, 0);
-  local_state_->SetInteger(prefs::kStabilityExecutionPhase,
-                           UNINITIALIZED_PHASE);
+  local_state_->SetInteger(prefs::kStabilityExecutionPhase, execution_phase_);

[Alexei Svitkine (slow), 2016/11/24 21:20:20]

I'm not sure this makes sense. Maybe better to just not set it?

[manzagop (departed), 2016/11/24 22:40:42]

Done.

The value needs to be preserved up to when it's consumed in
MetricsService::InitializeMetricsState.
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?sq...

But it's different from the other metrics, in that it's not an accumulation
metric.

After that, it gets shortly after overwritten to START_METRICS_RECORDING (and
this period it outside the uma crash mechanism coverage).
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?sq...

So I think it's fine to not set it.

   local_state_->SetInteger(prefs::kStabilityIncompleteSessionEndCount, 0);
   local_state_->SetInteger(prefs::kStabilityLaunchCount, 0);
   local_state_->SetBoolean(prefs::kStabilitySessionEndCompleted, true);
@@ -1211,8 +1213,7 @@ void MetricsService::LogCleanShutdown() {
   client_->OnLogCleanShutdown();
   clean_exit_beacon_.WriteBeaconValue(true);
   RecordCurrentState(local_state_);
-  local_state_->SetInteger(prefs::kStabilityExecutionPhase,
-                           MetricsService::SHUTDOWN_COMPLETE);
+  SetExecutionPhase(MetricsService::SHUTDOWN_COMPLETE, local_state_);
 }
 
 void MetricsService::RecordBooleanPrefValue(const char* path, bool value) {