Make metrics state machine more lenient about OLD vs. CURRENT

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
@@ skipping 737 matching lines @@
 
   PrefService* pref = g_browser_process->local_state();
   pref->SetBoolean(prefs::kStabilityExitedCleanly, true);
 
   // 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
   // persisting all logs. Unlinke a shutdown, the state is primed to be ready
   // to continue logging and uploading if the process does return.
   if (recording_active() && state_ >= INITIAL_LOG_READY) {
     PushPendingLogsToPersistentStorage();
-    if (state_ == SENDING_CURRENT_LOGS)
-      state_ = SENDING_OLD_LOGS;

[Ilya Sherman, 2012/07/09 16:12:54]

Hmm, why is this no longer needed?

[jar (doing other things), 2012/07/09 16:38:04]

Can you motivate this removal?

[stuartmorgan, 2012/07/10 07:26:25]

It was cruft almost the moment it was added; it landed almost simultaneously
with my change to move the state_-changing logic into
PushPendingLogsToPersistentStorage.

And it's wrong, because while the PPLTPS() version checks that there are
actually stored logs after storing (there might not be, if the logs were huge),
this doesn't and could thus be a way of incorrectly entering SENDING_OLD_LOGS.

     // Persisting logs stops recording, so start recording a new log immediately
     // to capture any background work that might be done before the process is
     // killed.
     StartRecording();
   }
 
   // Start writing right away (write happens on a different thread).
   pref->CommitPendingWrite();
 }
 
@@ skipping 376 matching lines @@
   // Similarly, if logs should no longer be uploaded, stop here.
   // TODO(stuartmorgan): Call Stop() on the schedule when reporting and/or
   // recording are turned off instead of letting it fire and then aborting.
   if (idle_since_last_transmission_ ||
       !recording_active() || !reporting_active()) {
     scheduler_->Stop();
     scheduler_->UploadCancelled();
     return;
   }
 
+  // If the callback was to upload an old log, but there no longer is one,
+  // just report success back to the scheduler to begin the ongoing log
+  // callbacks.
+  // TODO(stuartmorgan): Consider removing the distinction between
+  // SENDING_OLD_LOGS and SENDING_CURRENT_LOGS to simplify the state machine
+  // now that the log upload flow is the same for both modes.
+  if (state_ == SENDING_OLD_LOGS && !log_manager_.has_unsent_logs()) {
+    state_ = SENDING_CURRENT_LOGS;

[jar (doing other things), 2012/07/09 16:38:04]

This seems nice, conservative, and defenive.

+    scheduler_->UploadFinished(true /* healthy */, false /* no unsent logs */);

[jar (doing other things), 2012/07/09 16:38:04]

Why do we need to do this now?  Won't this (eventually) get called after the
upload attempt we instigate below completes?

[stuartmorgan, 2012/07/10 07:26:25]

Whoops, it's because I'm missing the |return| I intended to add. Fixed. (We want
to return rather than continue because if the callback was for sending an old
log, it was much shorter that we want for sending a new log.)

+  }
   // If there are unsent logs, send the next one. If not, start the asynchronous
   // process of finalizing the current log for upload.
   if (state_ == SENDING_OLD_LOGS) {
     DCHECK(log_manager_.has_unsent_logs());
     log_manager_.StageNextLogForUpload();
     SendStagedLog();
   } else {
     StartFinalLogInfoCollection();
   }
 }
@@ skipping 734 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;
 }