Fix sync metadata persistence for pseudo-USS

All changes to the in-memory sync metadata should be reflected in
persisted sync metadata, and a bug in the implementation prevented that.
The end result is that all sync entities would be refetched on every
restart, for the experimental pseudo-USS codepath (behind feature
toggle).

Given that we've already done some experimentation on canary channel,
we introduce the logic to recover cleanly from the resulting
inconsistent state, which seems like a desirable safeguard anyway.

Bug: 870624
Change-Id: I28b170a6ff62ecb78c94c7026796df56687507dc
Reviewed-on: https://chromium-review.googlesource.com/c/1267498
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597516}

components/sync/model_impl/syncable_service_based_bridge.cc

@@ -439,6 +439,18 @@ void SyncableServiceBasedBridge::OnReadAllMetadataForInit(
     return;
   }
 
+  // Guard against inconsistent state, and recover from it by starting from
+  // scratch, which will cause the eventual refetching of all entities from the
+  // server.
+  if (!metadata_batch->GetModelTypeState().initial_sync_done() &&
+      !in_memory_store_.empty()) {
+    in_memory_store_.clear();
+    store_->DeleteAllDataAndMetadata(base::DoNothing());
+    change_processor()->ModelReadyToSync(std::make_unique<MetadataBatch>());
+    DCHECK(!change_processor()->IsTrackingMetadata());
+    return;
+  }
+
   change_processor()->ModelReadyToSync(std::move(metadata_batch));
   MaybeStartSyncableService();
 }
@@ -487,10 +499,11 @@ void SyncableServiceBasedBridge::MaybeStartSyncableService() {
 }
 
 SyncChangeList SyncableServiceBasedBridge::StoreAndConvertRemoteChanges(
-    std::unique_ptr<MetadataChangeList> metadata_change_list,
+    std::unique_ptr<MetadataChangeList> initial_metadata_change_list,
     EntityChangeList entity_change_list) {
   std::unique_ptr<ModelTypeStore::WriteBatch> batch =
       store_->CreateWriteBatch();
+  batch->TakeMetadataChangesFrom(std::move(initial_metadata_change_list));
 
   SyncChangeList output_change_list;
   output_change_list.reserve(entity_change_list.size());
@@ -523,7 +536,7 @@ SyncChangeList SyncableServiceBasedBridge::StoreAndConvertRemoteChanges(
         // know.
         change_processor()->UpdateStorageKey(
             change.data(), /*storage_key=*/change.data().client_tag_hash,
-            metadata_change_list.get());
+            batch->GetMetadataChangeList());
         FALLTHROUGH;
 
       case EntityChange::ACTION_UPDATE: {

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -79,7 +79,8 @@ class MockSyncableService : public SyncableService {
 
 class SyncableServiceBasedBridgeTest : public ::testing::Test {
  protected:
-  SyncableServiceBasedBridgeTest() {
+  SyncableServiceBasedBridgeTest()
+      : store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()) {
     ON_CALL(syncable_service_, MergeDataAndStartSyncing(_, _, _, _))
         .WillByDefault(
             [&](ModelType type, const SyncDataList& initial_sync_data,
@@ -99,7 +100,8 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
             /*commit_only=*/false);
     mock_processor_.DelegateCallsByDefaultTo(real_processor_.get());
     bridge_ = std::make_unique<SyncableServiceBasedBridge>(
-        kModelType, ModelTypeStoreTestUtil::FactoryForInMemoryStoreForTest(),
+        kModelType,
+        ModelTypeStoreTestUtil::FactoryForForwardingStore(store_.get()),
         mock_processor_.CreateForwardingProcessor(), &syncable_service_);
   }
 
@@ -154,6 +156,7 @@ class SyncableServiceBasedBridgeTest : public ::testing::Test {
   testing::NiceMock<MockSyncableService> syncable_service_;
   testing::NiceMock<MockModelTypeChangeProcessor> mock_processor_;
   base::MockCallback<ModelErrorHandler> mock_error_handler_;
+  const std::unique_ptr<ModelTypeStore> store_;
   std::unique_ptr<syncer::ClientTagBasedModelTypeProcessor> real_processor_;
   std::unique_ptr<SyncableServiceBasedBridge> bridge_;
   std::unique_ptr<MockModelTypeWorker> worker_;
@@ -259,7 +262,7 @@ TEST_F(SyncableServiceBasedBridgeTest, ShouldPropagateErrorDuringStart) {
 }
 
 TEST_F(SyncableServiceBasedBridgeTest,
-       ShouldStartSyncingWithPreviousDirectoryData) {
+       ShouldStartSyncingWithPreviousDirectoryDataWithoutRestart) {
   InitializeBridge();
   StartSyncing();
   worker_->UpdateFromServer(kClientTagHash, GetTestSpecifics("name1"));
@@ -273,6 +276,25 @@ TEST_F(SyncableServiceBasedBridgeTest,
   StartSyncing();
 }
 
+TEST_F(SyncableServiceBasedBridgeTest,
+       ShouldStartSyncingWithPreviousDirectoryDataAfterRestart) {
+  InitializeBridge();
+  StartSyncing();
+  worker_->UpdateFromServer(kClientTagHash, GetTestSpecifics("name1"));
+
+  // Mimic restart, which shouldn't start syncing until OnSyncStarting() is
+  // received (exercised in StartSyncing()).
+  EXPECT_CALL(syncable_service_, MergeDataAndStartSyncing(_, _, _, _)).Times(0);
+  ShutdownBridge();
+  InitializeBridge();
+
+  EXPECT_CALL(syncable_service_,
+              MergeDataAndStartSyncing(
+                  kModelType, ElementsAre(SyncDataRemoteMatches("name1")),
+                  NotNull(), NotNull()));
+  StartSyncing();
+}
+
 TEST_F(SyncableServiceBasedBridgeTest, ShouldSupportDisableReenableSequence) {
   InitializeBridge();
   StartSyncing();