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

12 files changed, 125 insertions, 117 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.
diff --git a/sync/internal_api/public/sessions/model_neutral_state.h b/sync/internal_api/public/sessions/model_neutral_state.h
index ee9c974..4979d3a 100644
--- a/sync/internal_api/public/sessions/model_neutral_state.h
+++ b/sync/internal_api/public/sessions/model_neutral_state.h
@@ -25,8 +25,6 @@ struct SYNC_EXPORT ModelNeutralState {
   // The set of types for which commits were sent to the server.
   ModelTypeSet commit_request_types;
 
-  sync_pb::ClientToServerResponse updates_response;
-
   int num_successful_commits;
 
   // This is needed for monitoring extensions activity.
diff --git a/sync/internal_api/public/util/syncer_error.cc b/sync/internal_api/public/util/syncer_error.cc
index 9c2609c..e7cb66f 100644
--- a/sync/internal_api/public/util/syncer_error.cc
+++ b/sync/internal_api/public/util/syncer_error.cc
@@ -27,6 +27,7 @@ const char* GetSyncerErrorString(SyncerError value) {
     ENUM_CASE(SERVER_RETURN_CONFLICT);
     ENUM_CASE(SERVER_RESPONSE_VALIDATION_FAILED);
     ENUM_CASE(SERVER_RETURN_DISABLED_BY_ADMIN);
+    ENUM_CASE(SERVER_MORE_TO_DOWNLOAD);
     ENUM_CASE(SYNCER_OK);
   }
   NOTREACHED();
@@ -35,7 +36,9 @@ const char* GetSyncerErrorString(SyncerError value) {
 #undef ENUM_CASE
 
 bool SyncerErrorIsError(SyncerError error) {
-  return error != UNSET && error != SYNCER_OK;
+  return error != UNSET
+      && error != SYNCER_OK
+      && error != SERVER_MORE_TO_DOWNLOAD;
 }
 
 }  // namespace syncer
diff --git a/sync/internal_api/public/util/syncer_error.h b/sync/internal_api/public/util/syncer_error.h
index db59b7d..02da22c 100644
--- a/sync/internal_api/public/util/syncer_error.h
+++ b/sync/internal_api/public/util/syncer_error.h
@@ -31,6 +31,8 @@ enum SYNC_EXPORT_PRIVATE SyncerError {
   SERVER_RESPONSE_VALIDATION_FAILED,
   SERVER_RETURN_DISABLED_BY_ADMIN,
 
+  SERVER_MORE_TO_DOWNLOAD,
+
   SYNCER_OK
 };
 
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 314a232..752b9ab 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -103,17 +103,6 @@ SyncerError StatusController::last_get_key_result() const {
   return model_neutral_.last_get_key_result;
 }
 
-// Returns the number of updates received from the sync server.
-int64 StatusController::CountUpdates() const {
-  const sync_pb::ClientToServerResponse& updates =
-      model_neutral_.updates_response;
-  if (updates.has_get_updates()) {
-    return updates.get_updates().entries().size();
-  } else {
-    return 0;
-  }
-}
-
 int StatusController::num_updates_applied() const {
   return model_neutral_.num_updates_applied;
 }
@@ -142,18 +131,5 @@ int StatusController::TotalNumConflictingItems() const {
   return sum;
 }
 
-bool StatusController::ServerSaysNothingMoreToDownload() const {
-  if (!download_updates_succeeded())
-    return false;
-
-  if (!updates_response().get_updates().has_changes_remaining()) {
-    NOTREACHED();  // Server should always send changes remaining.
-    return false;  // Avoid looping forever.
-  }
-  // Changes remaining is an estimate, but if it's estimated to be
-  // zero, that's firm and we don't have to ask again.
-  return updates_response().get_updates().changes_remaining() == 0;
-}
-
 }  // namespace sessions
 }  // namespace syncer
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 0e2c6f2..005f158 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -42,12 +42,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
   void set_commit_request_types(ModelTypeSet value) {
     model_neutral_.commit_request_types = value;
   }
-  const sync_pb::ClientToServerResponse& updates_response() const {
-    return model_neutral_.updates_response;
-  }
-  sync_pb::ClientToServerResponse* mutable_updates_response() {
-    return &model_neutral_.updates_response;
-  }
 
   // Changelog related state.
   int64 num_server_changes_remaining() const {
@@ -67,24 +61,6 @@ class SYNC_EXPORT_PRIVATE StatusController {
 
   int num_server_overwrites() const;
 
-  // Returns the number of updates received from the sync server.
-  int64 CountUpdates() const;
-
-  // Returns true if the last download_updates_command received a valid
-  // server response.
-  bool download_updates_succeeded() const {
-    return model_neutral_.last_download_updates_result
-        == SYNCER_OK;
-  }
-
-  // Returns true if the last updates response indicated that we were fully
-  // up to date.  This is subtle: if it's false, it could either mean that
-  // the server said there WAS more to download, or it could mean that we
-  // were unable to reach the server.  If we didn't request every enabled
-  // datatype, then we can't say for sure that there's nothing left to
-  // download: in that case, this also returns false.
-  bool ServerSaysNothingMoreToDownload() const;
-
   base::Time sync_start_time() const {
     // The time at which we sent the first GetUpdates command for this sync.
     return sync_start_time_;
diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc
index 32e7b64..c29bc5f 100644
--- a/sync/sessions/status_controller_unittest.cc
+++ b/sync/sessions/status_controller_unittest.cc
@@ -31,16 +31,6 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   EXPECT_EQ(14, status.model_neutral_state().num_successful_commits);
 }
 
-TEST_F(StatusControllerTest, CountUpdates) {
-  StatusController status;
-  EXPECT_EQ(0, status.CountUpdates());
-  sync_pb::ClientToServerResponse* response(status.mutable_updates_response());
-  sync_pb::SyncEntity* entity1 = response->mutable_get_updates()->add_entries();
-  sync_pb::SyncEntity* entity2 = response->mutable_get_updates()->add_entries();
-  ASSERT_TRUE(entity1 != NULL && entity2 != NULL);
-  EXPECT_EQ(2, status.CountUpdates());
-}
-
 // Test TotalNumConflictingItems
 TEST_F(StatusControllerTest, TotalNumConflictingItems) {
   StatusController status;
diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc
index c93a1ed..e712552 100644
--- a/sync/sessions/sync_session_unittest.cc
+++ b/sync/sessions/sync_session_unittest.cc
@@ -145,32 +145,6 @@ class SyncSessionTest : public testing::Test,
   scoped_refptr<ExtensionsActivity> extensions_activity_;
 };
 
-TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
-  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());
-}
-
-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());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-}
-
-TEST_F(SyncSessionTest, MoreToDownloadIfGotNoChangesRemaining) {
-  status()->set_last_download_updates_result(SYNCER_OK);
-  status()->mutable_updates_response()->mutable_get_updates()
-      ->set_changes_remaining(0);
-  EXPECT_TRUE(status()->ServerSaysNothingMoreToDownload());
-  EXPECT_TRUE(status()->download_updates_succeeded());
-}
-
 }  // namespace
 }  // namespace sessions
 }  // namespace syncer