diff options
| author | tim@chromium.org <tim@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-09-07 12:57:14 +0000 |
|---|---|---|
| committer | tim@chromium.org <tim@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-09-07 12:57:14 +0000 |
| commit | d9f025b59828ab356d2dab40b9c7a6a1fff7d0df (patch) | |
| tree | 61e58d1ac643c26c2bdae3ceb61c49f9b42ce22a /sync/sessions | |
| parent | c11e43e23e5f5155c445b430d805d861b2fe24b3 (diff) | |
| download | chromium_src-d9f025b59828ab356d2dab40b9c7a6a1fff7d0df.zip chromium_src-d9f025b59828ab356d2dab40b9c7a6a1fff7d0df.tar.gz chromium_src-d9f025b59828ab356d2dab40b9c7a6a1fff7d0df.tar.bz2 | |
sync: remove buggy freshness condition in SyncSchedulerImpl add defensive DCHECKs.
We were using last_sync_session_end_time, which would effectively allow config jobs to cancel nudges that were scheduled to start before the config ended. There was code to reset the start time of nudges to hack around this, (which is removed in this patch) which was pretty confusing because after executing
if (scheduled_start < Now())
scheduled_start = Now();
scheduled_start is less than Now() again. This check was purely to get around the freshness check (AFAICT), and I think the new code is clearer. It also affects the Sync.Freq histogram, see bug for detail.
The integration tests also caught the fact that we weren't checking Succeeded() before updating that time, which means failed jobs could have caused legitimate nudges to be cancelled -- note this probably wasn't a big problem in practice due to the hack mentioned above. We now check Succeeded().
This also adds a few DCHECKs for situations that were previously unsupported but would technically squeak through. For example, there's a DCHECK in ScheduleConfiguration that there is no pending config job, but that situation could have arisen if we swapped to NORMAL_MODE and back before the config job executed -- in fact, one of the tests was doing this, but it's clearly unsupported.
Also fixes an issue with test_util::SimulateSuccess where we would not set ModelNeutralState results for config jobs, which meant that in some cases, tests would *not* reset wait_interval_ on successful backoff relief. The changes to the test file now ensure this can't regress.
BUG=143595
Review URL: https://chromiumcodereview.appspot.com/10825439
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@155370 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sync/sessions')
| -rw-r--r-- | sync/sessions/test_util.cc | 18 |
1 files changed, 13 insertions, 5 deletions
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc index ef734b7..4214c3d 100644 --- a/sync/sessions/test_util.cc +++ b/sync/sessions/test_util.cc @@ -48,11 +48,19 @@ void SimulateSuccess(sessions::SyncSession* session, } ASSERT_EQ(0U, session->status_controller().num_server_changes_remaining()); session->SetFinished(); - if (end == SYNCER_END) { - session->mutable_status_controller()->set_last_get_key_result(SYNCER_OK); - session->mutable_status_controller()->set_last_download_updates_result( - SYNCER_OK); - session->mutable_status_controller()->set_commit_result(SYNCER_OK); + switch(end) { + case SYNCER_END: + session->mutable_status_controller()->set_commit_result(SYNCER_OK); + // Fall through. + case APPLY_UPDATES: + DCHECK_EQ(end == APPLY_UPDATES, session->source().updates_source == + sync_pb::GetUpdatesCallerInfo::RECONFIGURATION); + session->mutable_status_controller()->set_last_get_key_result(SYNCER_OK); + session->mutable_status_controller()->set_last_download_updates_result( + SYNCER_OK); + break; + default: + ADD_FAILURE() << "Not a valid END state."; } } |