sync: Refactor GetUpdates loop

This is a follow-up to r238532.  I had wanted to implement this change
with that CL, but decided it would be easier to split them up into
independent changes.

This refactoring pulls the GetUpdates looping logic out of the
StatusController.  It was implemented that way in a time when the only
way to pass state between different parts of the sync cycle was through
a shared SyncSession and its members.  Now that this is no longer the
case, we can keep the "should continue looping" logic more or less local
to the code that performs  the looping.

The change allows us to remove the updates response message proto member
out of the StatusController.  That could save us a small amount of time,
since copying protos can be somewhat expensive.  More importantly, it
reduces complexity, since we no longer need to worry about the effects
of state stored in the StatusController.  The StatusController is now
used only to collect some counts for debugging.

This has already uncovered a bug.  There was a unit test that failed to
reset its SyncSession in the same manner that it would have been reset
in a non-test scenario.  It depended on the state that was "leaked" from
one sync cycle to the next via the unreset SyncSession in order to meet
its test expectations.

BUG=278484

Review URL: https://codereview.chromium.org/104653003

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

5 files changed, 119 insertions, 30 deletions

diff --git a/sync/engine/download.cc b/sync/engine/download.cc
index 7d00beb..2bc7f7a 100644
--- a/sync/engine/download.cc
+++ b/sync/engine/download.cc
@@ -143,7 +143,7 @@ void PartitionProgressMarkersByType(
 
 // Examines the contents of the GetUpdates response message and forwards
 // relevant data to the UpdateHandlers for processing and persisting.
-bool ProcessUpdateResponseMessage(
+bool ProcessUpdateResponseContents(
     const sync_pb::GetUpdatesResponse& gu_response,
     ModelTypeSet proto_request_types,
     UpdateHandlerMap* handler_map,
@@ -353,13 +353,10 @@ SyncerError ExecuteDownloadUpdates(
       update_response);
 
   if (result != SYNCER_OK) {
-    status->mutable_updates_response()->Clear();
     LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
     return result;
   }
 
-  status->mutable_updates_response()->CopyFrom(update_response);
-
   DVLOG(1) << "GetUpdates "
            << " returned " << update_response.get_updates().entries_size()
            << " updates and indicated "
@@ -379,22 +376,41 @@ SyncerError ExecuteDownloadUpdates(
         HandleGetEncryptionKeyResponse(update_response, dir));
   }
 
-  const sync_pb::GetUpdatesResponse& gu_response =
-      update_response.get_updates();
+  const ModelTypeSet proto_request_types =
+      Intersection(request_types, ProtocolTypes());
+
+  return ProcessResponse(update_response.get_updates(),
+                         proto_request_types,
+                         session->context()->update_handler_map(),
+                         status);
+}
+
+SyncerError ProcessResponse(
+    const sync_pb::GetUpdatesResponse& gu_response,
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* handler_map,
+    StatusController* status) {
   status->increment_num_updates_downloaded_by(gu_response.entries_size());
-  DCHECK(gu_response.has_changes_remaining());
+
+  // The changes remaining field is used to prevent the client from looping.  If
+  // that field is being set incorrectly, we're in big trouble.
+  if (!gu_response.has_changes_remaining()) {
+    return SERVER_RESPONSE_VALIDATION_FAILED;
+  }
   status->set_num_server_changes_remaining(gu_response.changes_remaining());
 
-  const ModelTypeSet proto_request_types =
-      Intersection(request_types, ProtocolTypes());
 
-  if (!ProcessUpdateResponseMessage(gu_response,
-                                    proto_request_types,
-                                    session->context()->update_handler_map(),
-                                    status)) {
+  if (!ProcessUpdateResponseContents(gu_response,
+                                     proto_request_types,
+                                     handler_map,
+                                     status)) {
     return SERVER_RESPONSE_VALIDATION_FAILED;
+  }
+
+  if (gu_response.changes_remaining() == 0) {
+    return SYNCER_OK;
   } else {
-    return result;
+    return SERVER_MORE_TO_DOWNLOAD;
   }
 }
 
diff --git a/sync/engine/download.h b/sync/engine/download.h
index 952823f..5bc08d4 100644
--- a/sync/engine/download.h
+++ b/sync/engine/download.h
@@ -37,7 +37,7 @@ SYNC_EXPORT_PRIVATE void BuildNormalDownloadUpdates(
     sync_pb::ClientToServerMessage* client_to_server_message);
 
 // Helper function.  Defined here for testing.
-void SYNC_EXPORT_PRIVATE BuildNormalDownloadUpdatesImpl(
+SYNC_EXPORT_PRIVATE void BuildNormalDownloadUpdatesImpl(
     ModelTypeSet proto_request_types,
     UpdateHandlerMap* update_handler_map,
     const sessions::NudgeTracker& nudge_tracker,
@@ -54,7 +54,7 @@ SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForConfigure(
     sync_pb::ClientToServerMessage* client_to_server_message);
 
 // Helper function.  Defined here for testing.
-void SYNC_EXPORT_PRIVATE BuildDownloadUpdatesForConfigureImpl(
+SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForConfigureImpl(
     ModelTypeSet proto_request_types,
     UpdateHandlerMap* update_handler_map,
     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource source,
@@ -70,7 +70,7 @@ SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForPoll(
     sync_pb::ClientToServerMessage* client_to_server_message);
 
 // Helper function.  Defined here for testing.
-void SYNC_EXPORT_PRIVATE BuildDownloadUpdatesForPollImpl(
+SYNC_EXPORT_PRIVATE void BuildDownloadUpdatesForPollImpl(
     ModelTypeSet proto_request_types,
     UpdateHandlerMap* update_handler_map,
     sync_pb::GetUpdatesMessage* get_updates);
@@ -82,9 +82,17 @@ SYNC_EXPORT_PRIVATE SyncerError
                            sessions::SyncSession* session,
                            sync_pb::ClientToServerMessage* msg);
 
+// Helper function for processing responses from the server.
+// Defined here for testing.
+SYNC_EXPORT_PRIVATE SyncerError ProcessResponse(
+    const sync_pb::GetUpdatesResponse& gu_response,
+    ModelTypeSet proto_request_types,
+    UpdateHandlerMap* handler_map,
+    sessions::StatusController* status);
+
 // Helper function to copy client debug info from debug_info_getter to
 // debug_info.  Defined here for testing.
-void SYNC_EXPORT_PRIVATE CopyClientDebugInfo(
+SYNC_EXPORT_PRIVATE void CopyClientDebugInfo(
     sessions::DebugInfoGetter* debug_info_getter,
     sync_pb::DebugInfo* debug_info);
 
diff --git a/sync/engine/download_unittest.cc b/sync/engine/download_unittest.cc
index 8fd08d1..eae6277 100644
--- a/sync/engine/download_unittest.cc
+++ b/sync/engine/download_unittest.cc
@@ -58,6 +58,19 @@ class DownloadUpdatesTest : public ::testing::Test {
     return &update_handler_map_;
   }
 
+  void InitFakeUpdateResponse(sync_pb::GetUpdatesResponse* response) {
+    ModelTypeSet types = proto_request_types();
+
+    for (ModelTypeSet::Iterator it = types.First(); it.Good(); it.Inc()) {
+      sync_pb::DataTypeProgressMarker* marker =
+          response->add_new_progress_marker();
+      marker->set_data_type_id(GetSpecificsFieldNumberFromModelType(it.Get()));
+      marker->set_token("foobarbaz");
+    }
+
+    response->set_changes_remaining(0);
+  }
+
  private:
   void AddUpdateHandler(ModelType type, ModelSafeGroup group) {
     DCHECK(directory());
@@ -206,6 +219,51 @@ TEST_F(DownloadUpdatesTest, PollTest) {
   EXPECT_TRUE(proto_request_types().Equals(progress_types));
 }
 
+// Verify that a bogus response message is detected.
+TEST_F(DownloadUpdatesTest, InvalidResponse) {
+  sync_pb::GetUpdatesResponse gu_response;
+  InitFakeUpdateResponse(&gu_response);
+
+  // This field is essential for making the client stop looping.  If it's unset
+  // then something is very wrong.  The client should detect this.
+  gu_response.clear_changes_remaining();
+
+  sessions::StatusController status;
+  SyncerError error = download::ProcessResponse(gu_response,
+                                                proto_request_types(),
+                                                update_handler_map(),
+                                                &status);
+  EXPECT_EQ(error, SERVER_RESPONSE_VALIDATION_FAILED);
+}
+
+// Verify that we correctly detect when there's more work to be done.
+TEST_F(DownloadUpdatesTest, MoreToDownloadResponse) {
+  sync_pb::GetUpdatesResponse gu_response;
+  InitFakeUpdateResponse(&gu_response);
+  gu_response.set_changes_remaining(1);
+
+  sessions::StatusController status;
+  SyncerError error = download::ProcessResponse(gu_response,
+                                                proto_request_types(),
+                                                update_handler_map(),
+                                                &status);
+  EXPECT_EQ(error, SERVER_MORE_TO_DOWNLOAD);
+}
+
+// A simple scenario: No updates returned and nothing more to download.
+TEST_F(DownloadUpdatesTest, NormalResponseTest) {
+  sync_pb::GetUpdatesResponse gu_response;
+  InitFakeUpdateResponse(&gu_response);
+  gu_response.set_changes_remaining(0);
+
+  sessions::StatusController status;
+  SyncerError error = download::ProcessResponse(gu_response,
+                                                proto_request_types(),
+                                                update_handler_map(),
+                                                &status);
+  EXPECT_EQ(error, SYNCER_OK);
+}
+
 class DownloadUpdatesDebugInfoTest : public ::testing::Test {
  public:
   DownloadUpdatesDebugInfoTest() {}
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 39b068b..13e1f79 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -130,20 +130,23 @@ bool Syncer::DownloadAndApplyUpdates(
     ModelTypeSet request_types,
     SyncSession* session,
     base::Callback<void(sync_pb::ClientToServerMessage*)> build_fn) {
-  while (!session->status_controller().ServerSaysNothingMoreToDownload()) {
+  SyncerError download_result = UNSET;
+  do {
     TRACE_EVENT0("sync", "DownloadUpdates");
     sync_pb::ClientToServerMessage msg;
     build_fn.Run(&msg);
-    SyncerError download_result =
+    download_result =
         download::ExecuteDownloadUpdates(request_types, session, &msg);
     session->mutable_status_controller()->set_last_download_updates_result(
         download_result);
-    if (download_result != SYNCER_OK) {
-      return false;
-    }
-  }
+  } while (download_result == SERVER_MORE_TO_DOWNLOAD);
+
+  // Exit without applying if we're shutting down or an error was detected.
+  if (download_result != SYNCER_OK)
+    return false;
   if (ExitRequested())
     return false;
+
   ApplyUpdates(session);
   if (ExitRequested())
     return false;
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 1678ba6..19aff7c 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -181,8 +181,12 @@ class SyncerTest : public testing::Test,
     saw_syncer_event_ = true;
   }
 
-  void SyncShareNudge() {
+  void ResetSession() {
     session_.reset(SyncSession::Build(context_.get(), this));
+  }
+
+  void SyncShareNudge() {
+    ResetSession();
 
     // Pretend we've seen a local change, to make the nudge_tracker look normal.
     nudge_tracker_.RecordLocalChange(ModelTypeSet(BOOKMARKS));
@@ -195,7 +199,7 @@ class SyncerTest : public testing::Test,
   }
 
   void SyncShareConfigure() {
-    session_.reset(SyncSession::Build(context_.get(), this));
+    ResetSession();
     EXPECT_TRUE(syncer_->ConfigureSyncShare(
             context_->enabled_types(),
             sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
@@ -543,6 +547,9 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) {
   }
 
   // Now sync without enabling bookmarks.
+  mock_server_->ExpectGetUpdatesRequestTypes(
+      Difference(context_->enabled_types(), ModelTypeSet(BOOKMARKS)));
+  ResetSession();
   syncer_->NormalSyncShare(
       Difference(context_->enabled_types(), ModelTypeSet(BOOKMARKS)),
       nudge_tracker_,
@@ -557,10 +564,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) {
   }
 
   // Sync again with bookmarks enabled.
-  syncer_->NormalSyncShare(
-      context_->enabled_types(),
-      nudge_tracker_,
-      session_.get());
+  mock_server_->ExpectGetUpdatesRequestTypes(context_->enabled_types());
   SyncShareNudge();
   {
     // It should have been committed.