sync: remove buggy freshness condition in SyncSchedulerImpl add defensive DCHECKs.

sync/engine/sync_scheduler_impl.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.
 
 #include "sync/engine/sync_scheduler_impl.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/bind.h"
@@ skipping 257 matching lines @@
 
   DCHECK(!session_context_->account_name().empty());
   DCHECK(syncer_.get());
   Mode old_mode = mode_;
   mode_ = mode;
   AdjustPolling(NULL);  // Will kick start poll timer if needed.
 
   if (old_mode != mode_) {
     // We just changed our mode. See if there are any pending jobs that we could
     // execute in the new mode.
+    if (mode_ == NORMAL_MODE) {
+      // It is illegal to switch to NORMAL_MODE if a previous CONFIGURATION job
+      // has not yet completed.
+      DCHECK(!wait_interval_.get() ||
+             !wait_interval_->pending_configure_job.get());
+    }
+
     DoPendingJobIfPossible(false);
   }
 }
 
 void SyncSchedulerImpl::SendInitialSnapshot() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   scoped_ptr<SyncSession> dummy(new SyncSession(session_context_, this,
       SyncSourceInfo(), ModelSafeRoutingInfo(),
       std::vector<ModelSafeWorker*>()));
   SyncEngineEvent event(SyncEngineEvent::STATUS_CHANGED);
@@ skipping 156 matching lines @@
       return CONTINUE;
     else
       return DROP;
   }
 
   // We are in normal mode.
   DCHECK_EQ(mode_, NORMAL_MODE);
   DCHECK_NE(job.purpose, SyncSessionJob::CONFIGURATION);
 
   // Freshness condition
-  if (job.scheduled_start < last_sync_session_end_time_) {
+  if (job.scheduled_start < last_nudge_or_poll_end_time_) {
     SDVLOG(2) << "Dropping job because of freshness";
     return DROP;
   }
 
   if (!session_context_->connection_manager()->HasInvalidAuthToken())
     return CONTINUE;
 
   SDVLOG(2) << "No valid auth token. Using that to decide on job.";
   return job.purpose == SyncSessionJob::NUDGE ? SAVE : DROP;
 }
@@ skipping 301 matching lines @@
   }
   SDVLOG(2) << "Done SyncShare looping.";
 
   FinishSyncSessionJob(job);
 }
 
 void SyncSchedulerImpl::FinishSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   // Update timing information for how often datatypes are triggering nudges.
   base::TimeTicks now = TimeTicks::Now();
-  if (!last_sync_session_end_time_.is_null()) {
+  if (!last_nudge_or_poll_end_time_.is_null()) {
+    // TODO(tim): We should probably track NUDGE time separately to avoid
+    // skewing the histogram?  Does it make sense to only compare against
+    // successful sessions?
     ModelTypePayloadMap::const_iterator iter;
+    DCHECK_EQ(job.purpose == SyncSessionJob::POLL,
+              job.session->source().types.empty());
     for (iter = job.session->source().types.begin();
          iter != job.session->source().types.end();
          ++iter) {
 #define PER_DATA_TYPE_MACRO(type_str) \
     SYNC_FREQ_HISTOGRAM("Sync.Freq" type_str, \
-                        now - last_sync_session_end_time_);
+                        now - last_nudge_or_poll_end_time_);
       SYNC_DATA_TYPE_HISTOGRAM(iter->first);
 #undef PER_DATA_TYPE_MACRO
     }
   }
-  last_sync_session_end_time_ = now;
+
+  // TODO(tim): Consolidate Succeeded() checks by breaking this Finish job
+  // flow into successful / unsuccessful handler methods.
+  if (job.session->Succeeded() && (job.purpose == SyncSessionJob::NUDGE ||
+                                   job.purpose == SyncSessionJob::POLL)) {
+    last_nudge_or_poll_end_time_ = now;
+  }
 
   // Now update the status of the connection from SCM. We need this to decide
   // whether we need to save/run future jobs. The notifications from SCM are not
   // reliable.
   //
   // TODO(rlarocque): crbug.com/110954
   // We should get rid of the notifications and it is probably not needed to
   // maintain this status variable in 2 places. We should query it directly from
   // SCM when needed.
   ServerConnectionManager* scm = session_context_->connection_manager();
@@ skipping 168 matching lines @@
 
 void SyncSchedulerImpl::DoPendingJobIfPossible(bool is_canary_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SyncSessionJob* job_to_execute = NULL;
   if (mode_ == CONFIGURATION_MODE && wait_interval_.get()
       && wait_interval_->pending_configure_job.get()) {
     SDVLOG(2) << "Found pending configure job";
     job_to_execute = wait_interval_->pending_configure_job.get();
   } else if (mode_ == NORMAL_MODE && pending_nudge_.get()) {
     SDVLOG(2) << "Found pending nudge job";
-    // Pending jobs mostly have time from the past. Reset it so this job
-    // will get executed.
-    if (pending_nudge_->scheduled_start < TimeTicks::Now())
-      pending_nudge_->scheduled_start = TimeTicks::Now();
 
     scoped_ptr<SyncSession> session(CreateSyncSession(
         pending_nudge_->session->source()));
 
     // Also the routing info might have been changed since we cached the
     // pending nudge. Update it by coalescing to the latest.
     pending_nudge_->session->Coalesce(*(session.get()));
     // The pending nudge would be cleared in the DoSyncSessionJob function.
     job_to_execute = pending_nudge_.get();
   }
@@ skipping 128 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer