Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(241)

Unified Diff: sync/engine/sync_scheduler_impl.cc

Issue 10825439: sync: remove buggy freshness condition in SyncSchedulerImpl add defensive DCHECKs. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: init Created 8 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: sync/engine/sync_scheduler_impl.cc
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index ff177b4f8347ec1bcd5582ee5c4873bb1cf948ec..fb97a62e1004b9dbd954c4b69f8eb5ea929f1c0a 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -275,6 +275,13 @@ void SyncSchedulerImpl::Start(Mode mode) {
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);
}
}
@@ -451,7 +458,7 @@ SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
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;
}
@@ -773,19 +780,30 @@ 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
@@ -974,10 +992,6 @@ void SyncSchedulerImpl::DoPendingJobIfPossible(bool is_canary_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()));

Powered by Google App Engine
This is Rietveld 408576698