[SyncInvalidations] Move deregistration to Initialize().

Deregistration from old invalidations is moved from the constructor to
the Initialize() method. The change is needed because
invalidator_->RegisterInvalidationHandler() calls
OnInvalidatorClientIdChange(), which cannot be called before the
SyncEngineImpl is initialized, since it uses the sync_task_runner_.

CreateBacked() helper is removed from the tests because it's not needed
anymore.

Bug: 1082115
Change-Id: I2846acf63eb0d245576b5ec65a435b522db05a21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2442969
Commit-Queue: Paula Vidas <paulavidas@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#813150}

components/sync/driver/glue/sync_engine_impl.cc

@@ -53,21 +53,6 @@ SyncEngineImpl::SyncEngineImpl(
 #endif
   backend_ = base::MakeRefCounted<SyncEngineBackend>(
       name_, sync_data_folder, weak_ptr_factory_.GetWeakPtr());
-
-  // If the new invalidations system (SyncInvalidationsService) is fully
-  // enabled, then the SyncService doesn't need to communicate with the old
-  // InvalidationService anymore.
-  if (invalidator_ &&
-      base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes) &&
-      base::FeatureList::IsEnabled(switches::kUseSyncInvalidations) &&
-      base::FeatureList::IsEnabled(
-          switches::kUseSyncInvalidationsForWalletAndOffer)) {
-    invalidator_->RegisterInvalidationHandler(this);
-    bool success = invalidator_->UpdateInterestedTopics(this, /*topics=*/{});
-    DCHECK(success);
-    invalidator_->UnregisterInvalidationHandler(this);
-    invalidator_ = nullptr;
-  }
 }
 
 SyncEngineImpl::~SyncEngineImpl() {
@@ -87,6 +72,22 @@ void SyncEngineImpl::Initialize(InitParams params) {
   sync_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&SyncEngineBackend::DoInitialize, backend_,
                                 std::move(params)));
+
+  // If the new invalidations system (SyncInvalidationsService) is fully
+  // enabled, then the SyncService doesn't need to communicate with the old
+  // InvalidationService anymore.
+  if (invalidator_ &&
+      base::FeatureList::IsEnabled(switches::kSyncSendInterestedDataTypes) &&
+      base::FeatureList::IsEnabled(switches::kUseSyncInvalidations) &&
+      base::FeatureList::IsEnabled(
+          switches::kUseSyncInvalidationsForWalletAndOffer)) {
+    DCHECK(!invalidation_handler_registered_);
+    invalidator_->RegisterInvalidationHandler(this);
+    bool success = invalidator_->UpdateInterestedTopics(this, /*topics=*/{});
+    DCHECK(success);
+    invalidator_->UnregisterInvalidationHandler(this);
+    invalidator_ = nullptr;
+  }
 }
 
 bool SyncEngineImpl::IsInitialized() const {

components/sync/driver/glue/sync_engine_impl_unittest.cc

@@ -199,6 +199,10 @@ class SyncEngineImplTest : public testing::Test {
     sync_thread_.StartAndWaitForTesting();
     ON_CALL(invalidator_, UpdateInterestedTopics(_, _))
         .WillByDefault(testing::Return(true));
+    backend_ = std::make_unique<SyncEngineImpl>(
+        "dummyDebugName", &invalidator_, GetSyncInvalidationsService(),
+        sync_prefs_->AsWeakPtr(),
+        temp_dir_.GetPath().Append(base::FilePath(kTestSyncDir)));
 
     fake_manager_factory_ = std::make_unique<FakeSyncManagerFactory>(
         &fake_manager_, network::TestNetworkConnectionTracker::GetInstance());
@@ -226,19 +230,8 @@ class SyncEngineImplTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
-  void CreateBackend() {
-    backend_ = std::make_unique<SyncEngineImpl>(
-        "dummyDebugName", &invalidator_, GetSyncInvalidationsService(),
-        sync_prefs_->AsWeakPtr(),
-        temp_dir_.GetPath().Append(base::FilePath(kTestSyncDir)));
-  }
-
   // Synchronously initializes the backend.
   void InitializeBackend(bool expect_success) {
-    if (!backend_) {
-      CreateBackend();
-    }
-
     host_.SetExpectSuccess(expect_success);
 
     SyncEngine::InitParams params;
@@ -683,7 +676,6 @@ TEST_F(SyncEngineImplTest, ShouldDestroyAfterInitFailure) {
 
 TEST_F(SyncEngineImplWithSyncInvalidationsTest,
        ShouldInvalidateDataTypesOnIncomingInvalidation) {
-  CreateBackend();
   EXPECT_CALL(mock_instance_id_driver_, AddListener(backend_.get()));
   InitializeBackend(/*expect_success=*/true);
 
@@ -724,12 +716,11 @@ TEST_F(SyncEngineImplWithSyncInvalidationsForWalletAndOfferTest,
 
   // Since the old invalidations system is not being used anymore (based on the
   // enabled feature flags), SyncEngine should call the (old) invalidator with
-  // an empty TopicSet upon construction.
+  // an empty TopicSet upon initialization.
   EXPECT_CALL(invalidator_, UpdateInterestedTopics(_, TopicSet()));
-  CreateBackend();
+  InitializeBackend(/*expect_success=*/true);
 
   EXPECT_CALL(invalidator_, UpdateInterestedTopics(_, _)).Times(0);
-  InitializeBackend(/*expect_success=*/true);
   ConfigureDataTypes();
 }