Abort sync cycles when download step fails

This change causes the syncer to exit from its download then commit
function if the download fails.  This helps prevent the creation of
server-side conflicts, which would be more likely to happen if a
download failed but the following commit succeeded.

The main changes are as follows:
- Rather than proceed to the next step when a download updates failure
  is detected, the syncer will exit.  This should cause the
SyncScheduler to schedule a retry at a later time.  
- The definition of a download updates failure is now based on the
  return code of the download attempt, rather than checking the contents
of the (possible non-existent) returned protobuf.  This makes the error
detection logic used here more closely match the logic used to decide
whether or not to schedule retries.

This implementation has a side-effect on configure sync cycles.  The old
behaviour was to handle failures by applying whatever updates we had
downloaded at that point.  The new behaviour will leave updates
unapplied if any error occurs.  This better matches a nearby comment's
assertion which states that we should not attempt to apply updates until
we have downloaded all of them.  I believe the author of that comment
would approve of this change.

This change moves around some of the ExtensionActivityMonitor logic.
It's important that we not take the data from the extensions acitivity
monitor at the start of the cycle.  Restoring that data is handled in
the commit building and response code, which might not be executed if we
need to break out early.  This also fixes issue 123270.

Most of the diffs for this change are concentrated in the unit tests:
- Expose more of the SyncScheduler to the SyncerTest test harness.
- Add functions to SyncerTest for testing specific types of sync jobs.
- Add some functions that allow us to better control failures in
MockConnectionManager.
- Added tests for configure job success and failure.
- Added tests for update then commit job success and failure.
- Removed some redundant tests.

BUG=122033,123270
TEST=sync_unit_tests, specifically:
SyncerTest.UpdateThenCommit,
SyncerTest.UpdateFailsThenDontCommit,
SyncerTest.ConfigureDownloadsTwoBatchesSuccess,
SyncerTest.ConfigureFailsDontApplyUpdates

Review URL: http://codereview.chromium.org/10006046

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132455 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 4 insertions, 48 deletions

diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index 48524a4..d5ce46a 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -215,6 +215,8 @@ TEST_F(SyncSessionTest, MoreToSyncIfUnsyncedGreaterThanCommitted) {
 TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
   status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());
 
+  status()->set_last_download_updates_result(NETWORK_IO_ERROR);
+
   // When DownloadUpdatesCommand fails, these should be false.
   EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
   EXPECT_FALSE(status()->download_updates_succeeded());
@@ -229,6 +231,7 @@ TEST_F(SyncSessionTest, MoreToDownloadIfGotChangesRemaining) {
 
   // When the server returns changes_remaining, that means there's
   // more to download.
+  status()->set_last_download_updates_result(SYNCER_OK);
   status()->mutable_updates_response()->mutable_get_updates()
      ->set_changes_remaining(1000L);
   EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
@@ -242,54 +245,7 @@ TEST_F(SyncSessionTest, MoreToDownloadIfGotChangesRemaining) {
 TEST_F(SyncSessionTest, MoreToDownloadIfGotNoChangesRemaining) {
   status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());
 
-  // When the server returns a timestamp, that means we're up to date.
-  status()->mutable_updates_response()->mutable_get_updates()
-      ->set_changes_remaining(0);
-  EXPECT_TRUE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-
-  // Download updates has its own loop in the syncer; it shouldn't factor
-  // into HasMoreToSync.
-  EXPECT_FALSE(session_->HasMoreToSync());
-}
-
-TEST_F(SyncSessionTest, MoreToDownloadIfGotNoChangesRemainingForSubset) {
-  status()->set_updates_request_types(ParamsMeaningJustOneEnabledType());
-
-  // When the server returns a timestamp, that means we're up to date for that
-  // type.  But there may still be more to download if there are other
-  // datatypes that we didn't request on this go-round.
-  status()->mutable_updates_response()->mutable_get_updates()
-      ->set_changes_remaining(0);
-
-  EXPECT_TRUE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-
-  // Download updates has its own loop in the syncer; it shouldn't factor
-  // into HasMoreToSync.
-  EXPECT_FALSE(session_->HasMoreToSync());
-}
-
-TEST_F(SyncSessionTest, MoreToDownloadIfGotChangesRemainingAndEntries) {
-  status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());
-  // The actual entry count should not factor into the HasMoreToSync
-  // determination.
-  status()->mutable_updates_response()->mutable_get_updates()->add_entries();
-  status()->mutable_updates_response()->mutable_get_updates()
-      ->set_changes_remaining(1000000L);;
-  EXPECT_FALSE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-
-  // Download updates has its own loop in the syncer; it shouldn't factor
-  // into HasMoreToSync.
-  EXPECT_FALSE(session_->HasMoreToSync());
-}
-
-TEST_F(SyncSessionTest, MoreToDownloadIfGotNoChangesRemainingAndEntries) {
-  status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());
-  // The actual entry count should not factor into the HasMoreToSync
-  // determination.
-  status()->mutable_updates_response()->mutable_get_updates()->add_entries();
+  status()->set_last_download_updates_result(SYNCER_OK);
   status()->mutable_updates_response()->mutable_get_updates()
       ->set_changes_remaining(0);
   EXPECT_TRUE(status()->ServerSaysNothingMoreToDownload());