[Sync] Fix race during device info bridge loading.

This patch fixes the case when the sync service is restarted while
device info bridge is loading. Check that the device info provider was
actually initialized, IsTrackingMetadata does not guarantee that because
the code right after calling ModelReadyToSync might not run yet.

Bug: 1055584
Change-Id: Id0c8252bde40ba8814a48a3c06f8e628612b0f4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2072377Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#745433}

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -206,7 +206,15 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
       std::make_unique<ModelTypeProcessorProxy>(
           weak_ptr_factory_for_worker_.GetWeakPtr(),
           base::SequencedTaskRunnerHandle::Get());
-  std::move(start_callback_).Run(std::move(activation_response));
+
+  // Defer invoking of |start_callback_| to avoid synchronous call from the
+  // |bridge_|. It might cause a situation when inside the ModelReadyToSync()
+  // another methods of the bridge eventually were called. This behavior would
+  // be complicated and be unexpected in some bridges.
+  // See crbug.com/1055584 for more details.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(start_callback_),
+                                std::move(activation_response)));
 }
 
 bool ClientTagBasedModelTypeProcessor::IsConnected() const {

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -98,6 +98,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
                         UpdateResponseDataList updates) override;
 
   // ModelTypeControllerDelegate implementation.
+  // |start_callback| will never be called synchronously.
   void OnSyncStarting(const DataTypeActivationRequest& request,
                       StartCallback callback) override;
   void OnSyncStopping(SyncStopMetadataFate metadata_fate) override;
@@ -238,6 +239,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   ////////////////
 
   // Stores the start callback in between OnSyncStarting() and ReadyToConnect().
+  // |start_callback_| will never be called synchronously.
   StartCallback start_callback_;
 
   // The request context passed in as part of OnSyncStarting().

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -103,6 +103,19 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
     EXPECT_FALSE(data_callback_);
   }
 
+  void OnSyncStarting(
+      const syncer::DataTypeActivationRequest& request) override {
+    FakeModelTypeSyncBridge::OnSyncStarting(request);
+    sync_started_ = true;
+  }
+
+  void ApplyStopSyncChanges(std::unique_ptr<MetadataChangeList>
+                                delete_metadata_change_list) override {
+    sync_started_ = false;
+    FakeModelTypeSyncBridge::ApplyStopSyncChanges(
+        std::move(delete_metadata_change_list));
+  }
+
   std::string GetStorageKey(const EntityData& entity_data) override {
     get_storage_key_call_count_++;
     return FakeModelTypeSyncBridge::GetStorageKey(entity_data);
@@ -131,6 +144,8 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
   int get_storage_key_call_count() const { return get_storage_key_call_count_; }
   int commit_failures_count() const { return commit_failures_count_; }
 
+  bool sync_started() const { return sync_started_; }
+
   // FakeModelTypeSyncBridge overrides.
 
   bool SupportsIncrementalUpdates() const override {
@@ -193,6 +208,7 @@ class TestModelTypeSyncBridge : public FakeModelTypeSyncBridge {
 
   const ModelType model_type_;
   bool supports_incremental_updates_;
+  bool sync_started_ = false;
 
   // The number of times MergeSyncData has been called.
   int merge_call_count_ = 0;
@@ -251,6 +267,7 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
 
   void ModelReadyToSync() {
     type_processor()->ModelReadyToSync(db()->CreateMetadataBatch());
+    WaitForStartCallbackIfNeeded();
   }
 
   void OnCommitDataLoaded() { bridge()->OnCommitDataLoaded(); }
@@ -267,10 +284,16 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
     request.authenticated_account_id = CoreAccountId(authenticated_account_id);
     request.sync_mode = sync_mode;
     request.configuration_start_time = base::Time::Now();
+
+    // |run_loop_| may exist here if OnSyncStarting is called without resetting
+    // state. But it is safe to remove it.
+    ASSERT_TRUE(!run_loop_ || !run_loop_->running());
+    run_loop_ = std::make_unique<base::RunLoop>();
     type_processor()->OnSyncStarting(
         request,
         base::BindOnce(&ClientTagBasedModelTypeProcessorTest::OnReadyToConnect,
                        base::Unretained(this)));
+    WaitForStartCallbackIfNeeded();
   }
 
   void DisconnectSync() {
@@ -314,6 +337,7 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
                             IsCommitOnly(), GetModelType(),
                             SupportsIncrementalUpdates());
     worker_ = nullptr;
+    run_loop_.reset();
     CheckPostConditions();
   }
 
@@ -384,15 +408,30 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
     // side-stepping that completely and connecting directly to the real
     // processor, since these tests are single-threaded and don't need proxies.
     type_processor()->ConnectSync(std::move(worker));
+    ASSERT_TRUE(run_loop_);
+    run_loop_->Quit();
   }
 
   void ErrorReceived(const ModelError& error) {
     EXPECT_TRUE(expect_error_);
     expect_error_ = false;
+    // Do not expect for a start callback anymore.
+    if (run_loop_) {
+      run_loop_->Quit();
+    }
   }
 
   sync_pb::ModelTypeState model_type_state() { return model_type_state_; }
 
+  void WaitForStartCallbackIfNeeded() {
+    if (!type_processor()->IsModelReadyToSyncForTest() ||
+        !bridge()->sync_started()) {
+      return;
+    }
+    ASSERT_TRUE(run_loop_);
+    run_loop_->Run();
+  }
+
  private:
   std::unique_ptr<TestModelTypeSyncBridge> bridge_;
   sync_pb::ModelTypeState model_type_state_;
@@ -401,6 +440,9 @@ class ClientTagBasedModelTypeProcessorTest : public ::testing::Test {
   // processor will pick up as the sync task runner.
   base::test::SingleThreadTaskEnvironment task_environment_;
 
+  // This run loop is used to wait for OnReadyToConnect is called.
+  std::unique_ptr<base::RunLoop> run_loop_;
+
   // The current mock queue, which is owned by |type_processor()|.
   MockModelTypeWorker* worker_;
 

components/sync_device_info/device_info_sync_bridge.cc

@@ -552,6 +552,10 @@ void DeviceInfoSyncBridge::OnReadAllMetadata(
     return;
   }
 
+  // If OnSyncStarting() was already called then cache GUID must be the same.
+  // Otherwise IsTrackingMetadata would return false due to cache GUID mismatch.
+  DCHECK(local_cache_guid_.empty() ||
+         local_cache_guid_ == local_cache_guid_in_metadata);
   // If sync already enabled (usual case without data corruption), we can
   // initialize the provider immediately.
   local_cache_guid_ = local_cache_guid_in_metadata;