Fix replying to OnSyncStarting() early for cache GUID mismatches

OnSyncStarting() should be completed (replied to) when a model is ready
to sync and the initial sync metadata has been loaded. This is important
to honor the processor's invariant that no remote changes are expected
(from the worker) unless the model is ready to sync.

For the (unlikely) case where OnSyncStarting() detects a cache GUID
mismatch (e.g. database in a corrupt state), *AND* the bridge becomes
unready to sync (currently the case for SESSIONS) as a reaction to that,
we should make sure no reply is issued for OnSyncStarting() until the
model becomes ready to sync.

Specifically, for SESSIONS (SessionSyncBridge), this scenario could lead
to MergeSyncData() being called too early, before the model was ready to
handle it, running into DCHECKs (for DCHECK-enabled builds) or crashes.
This becomes more likely if the UssMigrator is exercised (i.e. clients
that transition from the directory-based sessions implementation to the
USS implementation for the first time), because the migrator mimics a
response from the server that is very fast (no actual network involved).

Bug: 876490
Change-Id: I83e851fbbff235bb49ee03fd7f98d9764830ed83
Reviewed-on: https://chromium-review.googlesource.com/1256565
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595783}

components/sync/model/fake_model_type_sync_bridge.cc

@@ -360,12 +360,24 @@ ConflictResolution FakeModelTypeSyncBridge::ResolveConflict(
   return std::move(*conflict_resolution_);
 }
 
+ModelTypeSyncBridge::StopSyncResponse
+FakeModelTypeSyncBridge::ApplyStopSyncChanges(
+    std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
+  ModelTypeSyncBridge::ApplyStopSyncChanges(
+      std::move(delete_metadata_change_list));
+  return stop_sync_response_;
+}
+
 void FakeModelTypeSyncBridge::SetConflictResolution(
     ConflictResolution resolution) {
   conflict_resolution_ =
       std::make_unique<ConflictResolution>(std::move(resolution));
 }
 
+void FakeModelTypeSyncBridge::SetStopSyncResponse(StopSyncResponse response) {
+  stop_sync_response_ = response;
+}
+
 void FakeModelTypeSyncBridge::ErrorOnNextCall() {
   EXPECT_FALSE(error_next_);
   error_next_ = true;

components/sync/model/fake_model_type_sync_bridge.h

@@ -126,11 +126,16 @@ class FakeModelTypeSyncBridge : public ModelTypeSyncBridge {
   ConflictResolution ResolveConflict(
       const EntityData& local_data,
       const EntityData& remote_data) const override;
+  StopSyncResponse ApplyStopSyncChanges(
+      std::unique_ptr<MetadataChangeList> delete_metadata_change_list) override;
 
-  // Store a resolution for the next call to ResolveConflict. Note that if this
+  // Stores a resolution for the next call to ResolveConflict. Note that if this
   // is a USE_NEW resolution, the data will only exist for one resolve call.
   void SetConflictResolution(ConflictResolution resolution);
 
+  // Stores the value returned by future calls to ApplyStopSyncChanges().
+  void SetStopSyncResponse(StopSyncResponse response);
+
   // Sets an error that the next fallible call to the bridge will generate.
   void ErrorOnNextCall();
 
@@ -139,7 +144,7 @@ class FakeModelTypeSyncBridge : public ModelTypeSyncBridge {
   // test code here, this function is needed to manually copy it.
   static std::unique_ptr<EntityData> CopyEntityData(const EntityData& old_data);
 
-  // Set storage key which will be ignored by bridge.
+  // Sets storage key which will be ignored by bridge.
   void SetKeyToIgnore(const std::string key);
 
   const Store& db() { return *db_; }
@@ -157,6 +162,9 @@ class FakeModelTypeSyncBridge : public ModelTypeSyncBridge {
   // The conflict resolution to use for calls to ResolveConflict.
   std::unique_ptr<ConflictResolution> conflict_resolution_;
 
+  StopSyncResponse stop_sync_response_ =
+      StopSyncResponse::kModelStillReadyToSync;
+
   // The storage keys which bridge will ignore.
   std::unordered_set<std::string> keys_to_ignore_;
 

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -185,16 +185,19 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
         // For commit-only types, no updates are expected and hence we can
         // consider initial_sync_done().
         model_type_state_.set_initial_sync_done(commit_only_);
+        // Notify the bridge sync is starting to simulate an enable event.
+        bridge_->OnSyncStarting(activation_request_);
         break;
       case ModelTypeSyncBridge::StopSyncResponse::kModelNoLongerReadyToSync:
         // Model not ready to sync, so wait until the bridge calls
         // ModelReadyToSync().
         DCHECK(!model_ready_to_sync_);
-        break;
+        // Notify the bridge sync is starting to simulate an enable event.
+        bridge_->OnSyncStarting(activation_request_);
+        // Return early to avoid replying to OnSyncStarting() immediately. This
+        // will be handled in ModelReadyToSync().
+        return;
     }
-
-    // Notify the bridge sync is starting to simulate an enable event.
-    bridge_->OnSyncStarting(activation_request_);
   }
 
   // Cache GUID verification guarantees the user is the same.
@@ -1178,6 +1181,10 @@ bool ClientTagBasedModelTypeProcessor::HasLocalChangesForTest() const {
   return HasLocalChanges();
 }
 
+bool ClientTagBasedModelTypeProcessor::IsModelReadyToSyncForTest() const {
+  return model_ready_to_sync_;
+}
+
 void ClientTagBasedModelTypeProcessor::ExpireEntriesIfNeeded(
     const sync_pb::DataTypeProgressMarker& progress_marker) {
   if (!progress_marker.has_gc_directive())

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -98,6 +98,8 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
 
   bool HasLocalChangesForTest() const;
 
+  bool IsModelReadyToSyncForTest() const;
+
  private:
   friend class ModelTypeDebugInfo;
   friend class ClientTagBasedModelTypeProcessorTest;

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -1918,13 +1918,55 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   metadata_batch->SetModelTypeState(model_type_state);
 
   type_processor()->ModelReadyToSync(std::move(metadata_batch));
+  ASSERT_TRUE(type_processor()->IsModelReadyToSyncForTest());
 
   OnSyncStarting();
 
+  // Model should still be ready to sync.
+  ASSERT_TRUE(type_processor()->IsModelReadyToSyncForTest());
+  // OnSyncStarting() should have completed.
+  EXPECT_NE(nullptr, worker());
   // Upon a mismatch, metadata should have been cleared.
   EXPECT_EQ(0U, db().metadata_count());
 }
 
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldNotConnectImmediatelyAfterGuidMismatchIfNotReadyToSync) {
+  // Commit item.
+  InitializeToReadyState();
+  WriteItemAndAck(kKey1, kValue1);
+  // Reset the processor to simulate a restart.
+  ResetState(/*keep_db=*/true);
+
+  // Force future stops cause the model to become unready.
+  bridge()->SetStopSyncResponse(
+      ModelTypeSyncBridge::StopSyncResponse::kModelNoLongerReadyToSync);
+
+  // A new processor loads the metadata after changing the cache GUID.
+  bridge()->SetInitialSyncDone(true);
+
+  std::unique_ptr<MetadataBatch> metadata_batch = db().CreateMetadataBatch();
+  sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
+  model_type_state.set_cache_guid("WRONG_CACHE_GUID");
+  metadata_batch->SetModelTypeState(model_type_state);
+
+  type_processor()->ModelReadyToSync(std::move(metadata_batch));
+  ASSERT_TRUE(type_processor()->IsModelReadyToSyncForTest());
+
+  OnSyncStarting();
+
+  // Model should not be ready to sync.
+  ASSERT_FALSE(type_processor()->IsModelReadyToSyncForTest());
+  // OnSyncStarting() should NOT have completed.
+  EXPECT_EQ(nullptr, worker());
+  // Upon a mismatch, metadata should have been cleared.
+  EXPECT_EQ(0U, db().metadata_count());
+
+  // Calling ModelReadyToSync() should complete OnSyncStarting().
+  type_processor()->ModelReadyToSync(std::make_unique<MetadataBatch>());
+  EXPECT_NE(nullptr, worker());
+}
+
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldClearOrphanMetadataInGetLocalChangesWhenDataIsMissing) {
   InitializeToReadyState();