sync: Support disconnect in non-blocking sync

Adds support for Disconnect() and Disable() operations to the
non-blocking sync engine.  Although these functions did exist prior to
this CL, until now they did not take into account the effect of these
operations on pending commit state.

Also adds some tests for the new functionality.

BUG=351005

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

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

5 files changed, 159 insertions, 1 deletions

diff --git a/sync/engine/model_thread_sync_entity.cc b/sync/engine/model_thread_sync_entity.cc
index 6e466f5..053a6c8 100644
--- a/sync/engine/model_thread_sync_entity.cc
+++ b/sync/engine/model_thread_sync_entity.cc
@@ -154,4 +154,19 @@ void ModelThreadSyncEntity::ReceiveCommitResponse(const std::string& id,
   base_version_ = response_version;
 }
 
+void ModelThreadSyncEntity::ClearTransientSyncState() {
+  // If we have any unacknowledged commit requests outstatnding, they've been
+  // dropped and we should forget about them.
+  commit_requested_sequence_number_ = acked_sequence_number_;
+}
+
+void ModelThreadSyncEntity::ClearSyncState() {
+  base_version_ = kUncommittedVersion;
+  is_dirty_ = true;
+  sequence_number_ = 1;
+  commit_requested_sequence_number_ = 0;
+  acked_sequence_number_ = 0;
+  id_.clear();
+}
+
 }  // namespace syncer
diff --git a/sync/engine/model_thread_sync_entity.h b/sync/engine/model_thread_sync_entity.h
index 31cc2b8..03a6d27 100644
--- a/sync/engine/model_thread_sync_entity.h
+++ b/sync/engine/model_thread_sync_entity.h
@@ -106,6 +106,12 @@ class SYNC_EXPORT_PRIVATE ModelThreadSyncEntity {
                              int64 sequence_number,
                              int64 response_version);
 
+  // Clears any in-memory sync state associated with outstanding commits.
+  void ClearTransientSyncState();
+
+  // Clears all sync state.  Invoked when a user signs out.
+  void ClearSyncState();
+
  private:
   ModelThreadSyncEntity(int64 sequence_number,
                         int64 commit_requested_sequence_number,
diff --git a/sync/engine/non_blocking_type_processor.cc b/sync/engine/non_blocking_type_processor.cc
index c1982b3..9b80543 100644
--- a/sync/engine/non_blocking_type_processor.cc
+++ b/sync/engine/non_blocking_type_processor.cc
@@ -62,6 +62,8 @@ void NonBlockingTypeProcessor::Disable() {
   DCHECK(CalledOnValidThread());
   is_preferred_ = false;
   Disconnect();
+
+  ClearSyncState();
 }
 
 void NonBlockingTypeProcessor::Disconnect() {
@@ -76,6 +78,8 @@ void NonBlockingTypeProcessor::Disconnect() {
 
   weak_ptr_factory_for_sync_.InvalidateWeakPtrs();
   core_interface_.reset();
+
+  ClearTransientSyncState();
 }
 
 base::WeakPtr<NonBlockingTypeProcessor>
@@ -227,4 +231,20 @@ void NonBlockingTypeProcessor::OnUpdateReceived(
   // TODO: Inform the model of the new or updated data.
 }
 
+void NonBlockingTypeProcessor::ClearTransientSyncState() {
+  for (EntityMap::iterator it = entities_.begin(); it != entities_.end();
+       ++it) {
+    it->second->ClearTransientSyncState();
+  }
+}
+
+void NonBlockingTypeProcessor::ClearSyncState() {
+  for (EntityMap::iterator it = entities_.begin(); it != entities_.end();
+       ++it) {
+    it->second->ClearSyncState();
+  }
+
+  data_type_state_ = DataTypeState();
+}
+
 }  // namespace syncer
diff --git a/sync/engine/non_blocking_type_processor.h b/sync/engine/non_blocking_type_processor.h
index 2274c5d..da680f1 100644
--- a/sync/engine/non_blocking_type_processor.h
+++ b/sync/engine/non_blocking_type_processor.h
@@ -86,6 +86,15 @@ class SYNC_EXPORT_PRIVATE NonBlockingTypeProcessor : base::NonThreadSafe {
   // Sends all commit requests that are due to be sent to the sync thread.
   void FlushPendingCommitRequests();
 
+  // Clears any state related to outstanding communications with the
+  // NonBlockingTypeProcessorCore.  Used when we want to disconnect from
+  // the current core.
+  void ClearTransientSyncState();
+
+  // Clears any state related to our communications with the current sync
+  // account.  Useful when a user signs out of the current account.
+  void ClearSyncState();
+
   ModelType type_;
   DataTypeState data_type_state_;
 
diff --git a/sync/engine/non_blocking_type_processor_unittest.cc b/sync/engine/non_blocking_type_processor_unittest.cc
index 20ecb24..2c42afc 100644
--- a/sync/engine/non_blocking_type_processor_unittest.cc
+++ b/sync/engine/non_blocking_type_processor_unittest.cc
@@ -51,6 +51,17 @@ class NonBlockingTypeProcessorTest : public ::testing::Test {
   // Initialize to a "ready-to-commit" state.
   void InitializeToReadyState();
 
+  // Disconnect the NonBlockingTypeProcessorCore from our
+  // NonBlockingTypeProcessor.
+  void Disconnect();
+
+  // Disable sync for this NonBlockingTypeProcessor.  Should cause sync state
+  // to be discarded.
+  void Disable();
+
+  // Re-enable sync after Disconnect() or Disable().
+  void ReEnable();
+
   // Local data modification.  Emulates signals from the model thread.
   void WriteItem(const std::string& tag, const std::string& value);
   void DeleteItem(const std::string& tag);
@@ -116,6 +127,31 @@ void NonBlockingTypeProcessorTest::InitializeToReadyState() {
   OnInitialSyncDone();
 }
 
+void NonBlockingTypeProcessorTest::Disconnect() {
+  processor_->Disconnect();
+  injectable_sync_core_proxy_.reset();
+  mock_processor_core_ = NULL;
+}
+
+void NonBlockingTypeProcessorTest::Disable() {
+  processor_->Disable();
+  injectable_sync_core_proxy_.reset();
+  mock_processor_core_ = NULL;
+}
+
+void NonBlockingTypeProcessorTest::ReEnable() {
+  DCHECK(!processor_->IsConnected());
+
+  // Prepare a new NonBlockingTypeProcesorCore instance, just as we would
+  // if this happened in the real world.
+  mock_processor_core_ = new MockNonBlockingTypeProcessorCore();
+  injectable_sync_core_proxy_.reset(
+      new InjectableSyncCoreProxy(mock_processor_core_));
+
+  // Re-enable sync with the new NonBlockingTypeProcessorCore.
+  processor_->Enable(injectable_sync_core_proxy_->Clone());
+}
+
 void NonBlockingTypeProcessorTest::WriteItem(const std::string& tag,
                                              const std::string& value) {
   const std::string tag_hash = GenerateTagHash(tag);
@@ -351,6 +387,78 @@ TEST_F(NonBlockingTypeProcessorTest, NoCommitsUntilInitialSyncDone) {
   EXPECT_TRUE(HasCommitRequestForTag("tag1"));
 }
 
-// TODO(rlarocque): Add more testing of non_unique_name fields.
+// Test proper handling of disconnect and reconnect.
+//
+// Creates items in various states of commit and verifies they re-attempt to
+// commit on reconnect.
+TEST_F(NonBlockingTypeProcessorTest, Disconnect) {
+  InitializeToReadyState();
+
+  // The first item is fully committed.
+  WriteItem("tag1", "value1");
+  ASSERT_TRUE(HasCommitRequestForTag("tag1"));
+  SuccessfulCommitResponse(GetLatestCommitRequestForTag("tag1"));
+
+  // The second item has a commit request in progress.
+  WriteItem("tag2", "value2");
+  EXPECT_TRUE(HasCommitRequestForTag("tag2"));
+
+  Disconnect();
+
+  // The third item is added after disconnection.
+  WriteItem("tag3", "value3");
+
+  ReEnable();
+
+  EXPECT_EQ(1U, GetNumCommitRequestLists());
+  EXPECT_EQ(2U, GetNthCommitRequestList(0).size());
+
+  // The first item was already in sync.
+  EXPECT_FALSE(HasCommitRequestForTag("tag1"));
+
+  // The second item's commit was interrupted and should be retried.
+  EXPECT_TRUE(HasCommitRequestForTag("tag2"));
+
+  // The third item's commit was not started until the reconnect.
+  EXPECT_TRUE(HasCommitRequestForTag("tag3"));
+}
+
+// Test proper handling of disable and re-enable.
+//
+// Creates items in various states of commit and verifies they re-attempt to
+// commit on re-enable.
+TEST_F(NonBlockingTypeProcessorTest, Disable) {
+  InitializeToReadyState();
+
+  // The first item is fully committed.
+  WriteItem("tag1", "value1");
+  ASSERT_TRUE(HasCommitRequestForTag("tag1"));
+  SuccessfulCommitResponse(GetLatestCommitRequestForTag("tag1"));
+
+  // The second item has a commit request in progress.
+  WriteItem("tag2", "value2");
+  EXPECT_TRUE(HasCommitRequestForTag("tag2"));
+
+  Disable();
+
+  // The third item is added after disable.
+  WriteItem("tag3", "value3");
+
+  // Now we re-enable.
+  ReEnable();
+
+  // There should be nothing to commit right away, since we need to
+  // re-initialize the client state first.
+  EXPECT_EQ(0U, GetNumCommitRequestLists());
+
+  // Once we're ready to commit, all three local items should consider
+  // themselves uncommitted and pending for commit.
+  OnInitialSyncDone();
+  EXPECT_EQ(1U, GetNumCommitRequestLists());
+  EXPECT_EQ(3U, GetNthCommitRequestList(0).size());
+  EXPECT_TRUE(HasCommitRequestForTag("tag1"));
+  EXPECT_TRUE(HasCommitRequestForTag("tag2"));
+  EXPECT_TRUE(HasCommitRequestForTag("tag3"));
+}
 
 }  // namespace syncer