Don't commit prefs::kStabilitySessionEndCompleted change immediately on chromeos

chrome/browser/metrics/metrics_service.cc

 // Copyright (c) 2012 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 719 matching lines @@
 void MetricsService::HandleIdleSinceLastTransmission(bool in_idle) {
   // If there wasn't a lot of action, maybe the computer was asleep, in which
   // case, the log transmissions should have stopped.  Here we start them up
   // again.
   if (!in_idle && idle_since_last_transmission_)
     StartSchedulerIfNecessary();
   idle_since_last_transmission_ = in_idle;
 }
 
 void MetricsService::RecordStartOfSessionEnd() {
-  LogCleanShutdown();
-  RecordBooleanPrefValue(prefs::kStabilitySessionEndCompleted, false);
+  RecordStabilitySessionEndCompleted(false);
 }
 
 void MetricsService::RecordCompletedSessionEnd() {
-  LogCleanShutdown();
-  RecordBooleanPrefValue(prefs::kStabilitySessionEndCompleted, true);
+  RecordStabilitySessionEndCompleted(true);
 }
 
 #if defined(OS_ANDROID) || defined(OS_IOS)
 void MetricsService::OnAppEnterBackground() {
   scheduler_->Stop();
 
   MarkAppCleanShutdownAndCommit();
 
   // At this point, there's no way of knowing when the process will be
   // killed, so this has to be treated similar to a shutdown, closing and
@@ skipping 834 matching lines @@
   } else if (status == base::TERMINATION_STATUS_STILL_RUNNING) {
     UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.DisconnectedAlive",
                                was_extension_process ? 2 : 1);
     }
   }
 
 void MetricsService::LogRendererHang() {
   IncrementPrefValue(prefs::kStabilityRendererHangCount);
 }
 
+void MetricsService::RecordStabilitySessionEndCompleted(bool value) {
+  PrefService* pref = g_browser_process->local_state();
+  DCHECK(pref);
+  // Redundant setting to assure that we always reset this value at shutdown
+  // (and that we don't use some alternate path, and not call
+  // RecordStartOfEndSession/RecordCompletedSessionEnd)
+  clean_shutdown_status_ = CLEANLY_SHUTDOWN;
+  pref->SetBoolean(prefs::kStabilitySessionEndCompleted, value);
+  RecordCurrentState(pref);
+  MarkAppCleanShutdownAndCommit();

[jar (doing other things), 2013/10/11 22:55:44]

I'm trying to understand what you are doing here, vs what the code used to do.

In the past, we *first* did a LogCleanShutdown() on lines 740 or 745.  This
function did a MarkAppCleanShutdownAndCommit(), and *then* set the pref.

Now, you first change the prefs, and then call MarkAppCleanShutdownAndCommit().

It appears you have changed the order (setting the state, before the commit). 


IMO, it would be great to add a file-level comment indicating the exact shutdown
sequence, and exactly what state is observable (via prefs) to deduce where (if
any) the previous run of the browser crashed (stopped updating prefs files).  We
could use that as the spec, verify that it is indeed what was currently done....
and then verify that it is preserved by your change.

WDYT?

+}
+
 bool MetricsService::UmaMetricsProperlyShutdown() {
   CHECK(clean_shutdown_status_ == CLEANLY_SHUTDOWN ||
         clean_shutdown_status_ == NEED_TO_SHUTDOWN);
   return clean_shutdown_status_ == CLEANLY_SHUTDOWN;
 }
 
-void MetricsService::LogCleanShutdown() {
-  // Redundant hack to write pref ASAP.
-  MarkAppCleanShutdownAndCommit();
-
-  // Redundant setting to assure that we always reset this value at shutdown
-  // (and that we don't use some alternate path, and not call LogCleanShutdown).
-  clean_shutdown_status_ = CLEANLY_SHUTDOWN;
-
-  RecordBooleanPrefValue(prefs::kStabilityExitedCleanly, true);
-}
-
 #if defined(OS_CHROMEOS)
 void MetricsService::LogChromeOSCrash(const std::string &crash_type) {
   if (crash_type == "user")
     IncrementPrefValue(prefs::kStabilityOtherUserCrashCount);
   else if (crash_type == "kernel")
     IncrementPrefValue(prefs::kStabilityKernelCrashCount);
   else if (crash_type == "uncleanshutdown")
     IncrementPrefValue(prefs::kStabilitySystemUncleanShutdownCount);
   else
     NOTREACHED() << "Unexpected Chrome OS crash type " << crash_type;
@@ skipping 119 matching lines @@
   child_process_stats_buffer_.clear();
 }
 
 bool MetricsService::CanLogNotification() {
   // We simply don't log anything to UMA if there is a single incognito
   // session visible. The problem is that we always notify using the orginal
   // profile in order to simplify notification processing.
   return !chrome::IsOffTheRecordSessionActive();
 }
 
-void MetricsService::RecordBooleanPrefValue(const char* path, bool value) {
-  DCHECK(IsSingleThreaded());
-
-  PrefService* pref = g_browser_process->local_state();
-  DCHECK(pref);
-
-  pref->SetBoolean(path, value);
-  RecordCurrentState(pref);
-}
-
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(prefs::kStabilityLastTimestampSec, Time::Now().ToTimeT());
 
   RecordPluginChanges(pref);
 }
 
 // static
 bool MetricsService::IsPluginProcess(int process_type) {
   return (process_type == content::PROCESS_TYPE_PLUGIN ||
           process_type == content::PROCESS_TYPE_PPAPI_PLUGIN ||
@@ skipping 14 matching lines @@
   if (local_state) {
     const PrefService::Preference* uma_pref =
         local_state->FindPreference(prefs::kMetricsReportingEnabled);
     if (uma_pref) {
       bool success = uma_pref->GetValue()->GetAsBoolean(&result);
       DCHECK(success);
     }
   }
   return result;
 }