Fix a bug that causes the metrics loop to be broken.

components/metrics/metrics_service.cc

 // Copyright 2014 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 1034 matching lines @@
 
 void MetricsService::OnLogUploadComplete(int response_code) {
   DCHECK(log_upload_in_progress_);
   log_upload_in_progress_ = false;
 
   // Log a histogram to track response success vs. failure rates.
   UMA_HISTOGRAM_ENUMERATION("UMA.UploadResponseStatus.Protobuf",
                             ResponseCodeToStatus(response_code),
                             NUM_RESPONSE_STATUSES);
 
+  bool suppress_reschedule = false;
+
   bool upload_succeeded = response_code == 200;
 
   // Provide boolean for error recovery (allow us to ignore response_code).
   bool discard_log = false;
   const size_t log_size = log_manager_.staged_log().length();
   if (upload_succeeded) {
     UMA_HISTOGRAM_COUNTS_10000("UMA.LogSize.OnSuccess", log_size / 1024);
   } else if (log_size > kUploadLogAvoidRetransmitSize) {
     UMA_HISTOGRAM_COUNTS("UMA.Large Rejected Log was Discarded",
                          static_cast<int>(log_size));
@@ skipping 13 matching lines @@
     switch (state_) {
       case SENDING_INITIAL_STABILITY_LOG:
         if (NewInitialMetricsTimingEnabled()) {
           // The initial metrics log is already in the queue of unsent logs.
           state_ = SENDING_OLD_LOGS;
         } else {
           PrepareInitialMetricsLog();
           log_manager_.StageNextLogForUpload();
           SendStagedLog();
           state_ = SENDING_INITIAL_METRICS_LOG;
+          suppress_reschedule = true;
         }
         break;
 
       case SENDING_INITIAL_METRICS_LOG:
         state_ = log_manager_.has_unsent_logs() ? SENDING_OLD_LOGS
                                                 : SENDING_CURRENT_LOGS;
         break;
 
       case SENDING_OLD_LOGS:
         if (!log_manager_.has_unsent_logs())
@@ skipping 10 matching lines @@
 
     if (log_manager_.has_unsent_logs())
       DCHECK_LT(state_, SENDING_CURRENT_LOGS);
   }
 
   // Error 400 indicates a problem with the log, not with the server, so
   // don't consider that a sign that the server is in trouble.
   bool server_is_healthy = upload_succeeded || response_code == 400;
   // Don't notify the scheduler that the upload is finished if we've only sent
   // the initial stability log, but not yet the initial metrics log (treat the
   // two as a single unit of work as far as the scheduler is concerned).

[jwd, 2014/12/11 20:39:54]

Please update this comment.

[Steven Holte, 2014/12/11 20:46:54]

Slightly tweaked the comment, but it's basically consistent with the intended
behaviour.  The problem was state_ could be SENDING_INITIAL_METRICS_LOG for
other reasons (i.e. there was no initial stability log, but we the first upload
attempt failed due to timeout).

-  if (state_ != SENDING_INITIAL_METRICS_LOG) {
+  if (!suppress_reschedule) {
     scheduler_->UploadFinished(server_is_healthy,
                                log_manager_.has_unsent_logs());
   }
 
   if (server_is_healthy)
     client_->OnLogUploadComplete();
 }
 
 void MetricsService::IncrementPrefValue(const char* path) {
   int value = local_state_->GetInteger(path);
@@ skipping 124 matching lines @@
   local_state_->SetBoolean(path, value);
   RecordCurrentState(local_state_);
 }
 
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(prefs::kStabilityLastTimestampSec,
                  base::Time::Now().ToTimeT());
 }
 
 }  // namespace metrics