[Sync] Fix missing wait for initial sync to send crypto to proc

This avoid hitting a DCHECK that was introduced in
https://chromiumcodereview.appspot.com/2401083003 where we tried clean
up notifications to the model thread about encryption information.
By re-using the existing ApplyUpdates methods (and DCHECKs) we now
realize that we sometimes call ApplyUpdates before the initial sync is
completed. This is probably wrong and definitely, since the initial
sync will call ApplyUpdates and pass encryption information. Sending it
before that will just be confusing for the proc.

Bug: 789950
Change-Id: Ifc2b01efbc71145574f9106c67ce9aebfc8f6b99
Reviewed-on: https://chromium-review.googlesource.com/807664Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521852}

components/sync/engine_impl/model_type_registry.cc

@@ -296,7 +296,7 @@ void ModelTypeRegistry::OnPassphraseRequired(
 void ModelTypeRegistry::OnPassphraseAccepted() {
   for (const auto& worker : model_type_workers_) {
     if (encrypted_types_.Has(worker->GetModelType())) {
-      worker->EncryptionAcceptedApplyUpdates();
+      worker->EncryptionAcceptedMaybeApplyUpdates();
     }
   }
 }

components/sync/engine_impl/model_type_worker.cc

@@ -218,11 +218,16 @@ void ModelTypeWorker::PassiveApplyUpdates(StatusController* status) {
   ApplyPendingUpdates();
 }
 
-void ModelTypeWorker::EncryptionAcceptedApplyUpdates() {
+void ModelTypeWorker::EncryptionAcceptedMaybeApplyUpdates() {
   DCHECK(cryptographer_);
   DCHECK(cryptographer_->is_ready());
-  // Reuse ApplyUpdates(...) to get its DCHECKs as well.
-  ApplyUpdates(nullptr);
+
+  // Only push the encryption to the processor if we're already connected.
+  // Otherwise this information can wait for the initial sync's first apply.
+  if (model_type_state_.initial_sync_done()) {
+    // Reuse ApplyUpdates(...) to get its DCHECKs as well.
+    ApplyUpdates(nullptr);
+  }
 }
 
 void ModelTypeWorker::ApplyPendingUpdates() {

components/sync/engine_impl/model_type_worker.h

@@ -95,7 +95,7 @@ class ModelTypeWorker : public UpdateHandler,
 
   // An alternative way to drive sending data to the processor, that should be
   // called when a new encryption mechanism is ready.
-  void EncryptionAcceptedApplyUpdates();
+  void EncryptionAcceptedMaybeApplyUpdates();
 
   // Callback for when our contribution gets a response.
   void OnCommitResponse(CommitResponseDataList* response_list);

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -285,6 +285,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
     if (worker()) {
       worker()->UpdateCryptographer(
           std::make_unique<Cryptographer>(*cryptographer_));
+      worker()->EncryptionAcceptedMaybeApplyUpdates();
     }
   }
 
@@ -667,7 +668,7 @@ TEST_F(ModelTypeWorkerTest, ReceiveUpdates) {
 
   ASSERT_EQ(1U, processor()->GetNumUpdateResponses());
   UpdateResponseDataList updates_list = processor()->GetNthUpdateResponse(0);
-  ASSERT_EQ(1U, updates_list.size());
+  EXPECT_EQ(1U, updates_list.size());
 
   ASSERT_TRUE(processor()->HasUpdateResponse(kHash1));
   UpdateResponseData update = processor()->GetUpdateResponse(kHash1);
@@ -694,7 +695,7 @@ TEST_F(ModelTypeWorkerTest, ReceiveMultiPartUpdates) {
 
   // A partial update response doesn't pass anything to the processor.
   TriggerPartialUpdateFromServer(10, kTag1, kValue1);
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
   // Trigger the completion of the update.
   TriggerUpdateFromServer(10, kTag2, kValue2);
@@ -721,7 +722,7 @@ TEST_F(ModelTypeWorkerTest, EmptyUpdates) {
   server()->SetProgressMarkerToken("token2");
   DeliverRawUpdates(SyncEntityList());
   ASSERT_EQ(1U, processor()->GetNumUpdateResponses());
-  ASSERT_EQ(
+  EXPECT_EQ(
       server()->GetProgress().SerializeAsString(),
       processor()->GetNthUpdateState(0).progress_marker().SerializeAsString());
 }
@@ -730,16 +731,11 @@ TEST_F(ModelTypeWorkerTest, EmptyUpdates) {
 TEST_F(ModelTypeWorkerTest, EncryptedCommit) {
   NormalInitialize();
 
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
-  // Tell the worker about the new encryption key but no ApplyUpdates yet.
+  // Init the Cryptographer, it'll cause the EKN to be pushed.
   AddPendingKey();
   DecryptPendingKey();
-  EXPECT_EQ(0, nudge_handler()->GetNumCommitNudges());
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
-
-  // Now the information is allowed to reach the processor.
-  ApplyUpdates();
   ASSERT_EQ(1U, processor()->GetNumUpdateResponses());
   EXPECT_EQ(GetLocalCryptographerKeyName(),
             processor()->GetNthUpdateState(0).encryption_key_name());
@@ -770,19 +766,18 @@ TEST_F(ModelTypeWorkerTest, EncryptionBlocksUpdates) {
 
   // Update encryption key name, should be blocked.
   AddPendingKey();
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
   // Update progress marker, should be blocked.
   server()->SetProgressMarkerToken("token2");
   DeliverRawUpdates(SyncEntityList());
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
   // Update local cryptographer, verify everything is pushed to processor.
   DecryptPendingKey();
-  ApplyUpdates();
   ASSERT_EQ(1U, processor()->GetNumUpdateResponses());
   UpdateResponseDataList updates_list = processor()->GetNthUpdateResponse(0);
-  ASSERT_EQ(
+  EXPECT_EQ(
       server()->GetProgress().SerializeAsString(),
       processor()->GetNthUpdateState(0).progress_marker().SerializeAsString());
 }
@@ -877,14 +872,10 @@ TEST_F(ModelTypeWorkerTest, InitializeWithPendingCryptographer) {
   NormalInitialize();
 
   // Shouldn't be informed of the EKN, since there's a pending key.
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
-  // Although the cryptographer is ready, it should wait until being told to
-  // update the processor.
+  // Init the cryptographer, it'll push the EKN.
   DecryptPendingKey();
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
-
-  ApplyUpdates();
   ASSERT_EQ(1U, processor()->GetNumUpdateResponses());
   EXPECT_EQ(GetLocalCryptographerKeyName(),
             processor()->GetNthUpdateState(0).encryption_key_name());
@@ -900,7 +891,25 @@ TEST_F(ModelTypeWorkerTest, FirstInitializeWithCryptographer) {
   FirstInitialize();
 
   // Shouldn't be informed of the EKN, since normal activation will drive this.
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
+
+  // Now perform first sync and make sure the EKN makes it.
+  TriggerTypeRootUpdateFromServer();
+  ASSERT_EQ(1U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(GetLocalCryptographerKeyName(),
+            processor()->GetNthUpdateState(0).encryption_key_name());
+}
+
+TEST_F(ModelTypeWorkerTest, CryptographerDuringInitialization) {
+  // Initialize with initial sync done to false.
+  FirstInitialize();
+
+  // Set up the Cryptographer logic after initialization but before first sync.
+  AddPendingKey();
+  DecryptPendingKey();
+
+  // Shouldn't be informed of the EKN, since normal activation will drive this.
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
   // Now perform first sync and make sure the EKN makes it.
   TriggerTypeRootUpdateFromServer();
@@ -924,15 +933,11 @@ TEST_F(ModelTypeWorkerTest, ReceiveUndecryptableEntries) {
 
   // At this point, the cryptographer does not have access to the key, so the
   // updates will be undecryptable. This will block all updates.
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
+  EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
 
-  // The update can be delivered once the cryptographer is ready, but it'll
-  // still wait for the apply call.
+  // The update should know that the cryptographer is ready.
   DecryptPendingKey();
-  ASSERT_EQ(0U, processor()->GetNumUpdateResponses());
-
-  // ApplyUpdates() will cause everything to finally be delivered.
-  ApplyUpdates();
+  EXPECT_EQ(1U, processor()->GetNumUpdateResponses());
   ASSERT_TRUE(processor()->HasUpdateResponse(kHash1));
   UpdateResponseData update = processor()->GetUpdateResponse(kHash1);
   EXPECT_EQ(kTag1, update.entity->specifics.preference().name());
@@ -961,7 +966,7 @@ TEST_F(ModelTypeWorkerTest, RestorePendingEntries) {
 
   // Update will be undecryptable at first.
   EXPECT_EQ(0U, processor()->GetNumUpdateResponses());
-  ASSERT_FALSE(processor()->HasUpdateResponse(kHash1));
+  EXPECT_FALSE(processor()->HasUpdateResponse(kHash1));
 
   // Update the cryptographer so it can decrypt that update.
   AddPendingKey();