Fix processor stop before initial sync done

From the bridge's perspective, it's confusing to receive
ApplyStopSyncChanges() with a non-null parameter, which reflects sync
being disabled, without a prior MergeSyncData() call, which represents
sync being enabled.

Instead, make sure that a null parameter is passed to the bridge if the
initial sync is not done.

Prior to this patch, bridges would be overly conservative and clear
sync metadata unnecessarily (because there couldn't be any). Other than
that, no real user-facing issue exists.

Credits for feuunk@ for surfacing and investigating the scenario.

Bug: 890709
Change-Id: Ic39fb83596bd4e5e4c6f921b2df0812d0d94515c
Reviewed-on: https://chromium-review.googlesource.com/c/1288578Reviewed-by: Florian Uunk <feuunk@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600884}

components/sync/model/fake_model_type_sync_bridge.h

@@ -147,7 +147,8 @@ class FakeModelTypeSyncBridge : public ModelTypeSyncBridge {
   // Sets storage key which will be ignored by bridge.
   void SetKeyToIgnore(const std::string key);
 
-  const Store& db() { return *db_; }
+  const Store& db() const { return *db_; }
+  Store* mutable_db() { return db_.get(); }
 
  protected:
   // Contains all of the data and metadata state.

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -271,13 +271,22 @@ void ClientTagBasedModelTypeProcessor::OnSyncStopping(
 
 ModelTypeSyncBridge::StopSyncResponse
 ClientTagBasedModelTypeProcessor::ClearMetadataAndResetState() {
-  // Clear metadata.
-  std::unique_ptr<MetadataChangeList> change_list =
-      bridge_->CreateMetadataChangeList();
-  for (const auto& kv : entities_) {
-    change_list->ClearMetadata(kv.second->storage_key());
+  std::unique_ptr<MetadataChangeList> change_list;
+
+  // Clear metadata if MergeSyncData() was called before.
+  if (model_type_state_.initial_sync_done()) {
+    change_list = bridge_->CreateMetadataChangeList();
+    for (const auto& kv : entities_) {
+      change_list->ClearMetadata(kv.second->storage_key());
+    }
+    change_list->ClearModelTypeState();
+  } else {
+    // All changes before the initial sync is done are ignored and in fact they
+    // were never persisted by the bridge (prior to MergeSyncData), so we should
+    // be tracking no entities.
+    DCHECK(entities_.empty());
   }
-  change_list->ClearModelTypeState();
+
   const ModelTypeSyncBridge::StopSyncResponse response =
       bridge_->ApplyStopSyncChanges(std::move(change_list));
 
@@ -436,6 +445,7 @@ void ClientTagBasedModelTypeProcessor::UpdateStorageKey(
   DCHECK(!client_tag_hash.empty());
   DCHECK(!storage_key.empty());
   DCHECK(!bridge_->SupportsGetStorageKey());
+  DCHECK(model_type_state_.initial_sync_done());
 
   ProcessorEntityTracker* entity = GetEntityForTagHash(client_tag_hash);
   DCHECK(entity);
@@ -452,6 +462,7 @@ void ClientTagBasedModelTypeProcessor::UpdateStorageKey(
 void ClientTagBasedModelTypeProcessor::UntrackEntity(
     const EntityData& entity_data) {
   const std::string& client_tag_hash = entity_data.client_tag_hash;
+  DCHECK(model_type_state_.initial_sync_done());
   DCHECK(!client_tag_hash.empty());
   DCHECK(GetEntityForTagHash(client_tag_hash)->storage_key().empty());
   entities_.erase(client_tag_hash);
@@ -459,6 +470,8 @@ void ClientTagBasedModelTypeProcessor::UntrackEntity(
 
 void ClientTagBasedModelTypeProcessor::UntrackEntityForStorageKey(
     const std::string& storage_key) {
+  DCHECK(model_type_state_.initial_sync_done());
+
   // Look-up the client tag hash.
   auto iter = storage_key_to_tag_hash_.find(storage_key);
   if (iter == storage_key_to_tag_hash_.end()) {

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -1348,6 +1348,26 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldStopAndClearMetadata) {
   worker()->VerifyPendingCommits({{kHash1}, {kHash2}, {kHash3}});
 }
 
+// Test proper handling of disable-sync before initial sync done.
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldNotClearBridgeMetadataPriorToMergeSyncData) {
+  // Populate the bridge's metadata with some non-empty values for us to later
+  // check that it hasn't been cleared.
+  const std::string kTestEncryptionKeyName = "TestEncryptionKey";
+  ModelTypeState model_type_state(db().model_type_state());
+  model_type_state.set_encryption_key_name(kTestEncryptionKeyName);
+  bridge()->mutable_db()->set_model_type_state(model_type_state);
+
+  ModelReadyToSync();
+  OnSyncStarting();
+  ASSERT_FALSE(type_processor()->IsTrackingMetadata());
+
+  type_processor()->OnSyncStopping(CLEAR_METADATA);
+  EXPECT_FALSE(type_processor()->IsTrackingMetadata());
+  EXPECT_EQ(kTestEncryptionKeyName,
+            db().model_type_state().encryption_key_name());
+}
+
 // Test re-encrypt everything when desired encryption key changes.
 TEST_F(ClientTagBasedModelTypeProcessorTest, ShouldReencryptCommitsWithNewKey) {
   InitializeToReadyState();