Make metrics state machine more lenient about OLD vs. CURRENT

chrome/browser/metrics/metrics_service.cc

Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index 42f2c02924f4ddd7f0c6f540980cf41ef871adf7..19896aef72e61e679ceca27255685909fe5f030a 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -755,8 +755,6 @@ void MetricsService::OnAppEnterBackground() {
   // 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.
@@ -1153,6 +1151,16 @@ void MetricsService::StartScheduledUpload() {
     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) {