[Sync] Fix for not ignoring corrupted passwords in the worker

Before the patch: the worker would never ignore corrupted passwords,
which blocks other updates.

After this patch: the worker ignore corrupted passwords.

Change-Id: Ia8d3fd9a351f6949b9348fb57c7b721643d2e182
Bug: 1008403
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1826892
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700315}

components/sync/engine_impl/model_type_worker.cc

@@ -608,7 +608,10 @@ void ModelTypeWorker::DecryptStoredEntities() {
       }
       if (!DecryptPasswordSpecifics(*cryptographer_, data.specifics,
                                     &specifics)) {
-        ++it;
+        // Decryption error should be permanent (e.g. corrupt data), since
+        // CanDecrypt() above claims decryption keys are up-to-date. Let's
+        // ignore this update to avoid blocking other updates.
+        it = entries_pending_decryption_.erase(it);
         continue;
       }
     } else {

components/sync/engine_impl/model_type_worker.h

@@ -130,6 +130,11 @@ class ModelTypeWorker : public UpdateHandler,
   // clear the update data that has been added so far.
   void AbortMigration();
 
+  // Public for testing.
+  // Returns true if this type should stop communicating because of outstanding
+  // encryption issues and must wait for keys to be updated.
+  bool BlockForEncryption() const;
+
   // Returns the estimate of dynamically allocated memory in bytes.
   size_t EstimateMemoryUsage() const;
 
@@ -177,10 +182,6 @@ class ModelTypeWorker : public UpdateHandler,
   // settings in a good state.
   bool CanCommitItems() const;
 
-  // Returns true if this type should stop communicating because of outstanding
-  // encryption issues and must wait for keys to be updated.
-  bool BlockForEncryption() const;
-
   // Updates the encryption key name stored in |model_type_state_| if it differs
   // from the default encryption key name in |cryptographer_|. Returns whether
   // an update occurred.

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -1847,6 +1847,42 @@ TEST_F(ModelTypeWorkerPasswordsTest, ReceiveUndecryptablePasswordEntries) {
                            .password_value());
 }
 
+// Similar to ReceiveDecryptableEntities but for PASSWORDS, which have a custom
+// encryption mechanism.
+TEST_F(ModelTypeWorkerPasswordsTest, ReceiveCorruptedPasswordEntities) {
+  NormalInitialize();
+
+  sync_pb::PasswordSpecificsData unencrypted_password;
+  unencrypted_password.set_password_value(kPassword);
+  sync_pb::EntitySpecifics encrypted_specifics =
+      EncryptPasswordSpecifics(GetNthKeyParams(1), unencrypted_password);
+  // Manipulate the blob to be corrupted.
+  encrypted_specifics.mutable_password()->mutable_encrypted()->set_blob(
+      "corrupted blob");
+
+  // Receive an encrypted password, encrypted with a key that is already known.
+  SyncEntity entity = server()->UpdateFromServer(
+      /*version_offset=*/10, kHash1, encrypted_specifics);
+  worker()->ProcessGetUpdatesResponse(server()->GetProgress(),
+                                      server()->GetContext(), {&entity},
+                                      status_controller());
+  worker()->ApplyUpdates(status_controller());
+
+  // No updates should have reached the processor and worker is blocked for
+  // encryption because the cryptographer isn't ready yet.
+  EXPECT_FALSE(processor()->HasUpdateResponse(kHash1));
+  EXPECT_TRUE(worker()->BlockForEncryption());
+
+  // Allow the cryptographer to decrypt using the first key.
+  AddPendingKey();
+  DecryptPendingKey();
+
+  // Still, no updates should have reached the processor and worker is NOT
+  // blocked for encryption anymore.
+  EXPECT_FALSE(processor()->HasUpdateResponse(kHash1));
+  EXPECT_FALSE(worker()->BlockForEncryption());
+}
+
 // Analogous test fixture but uses BOOKMARKS instead of PREFERENCES, in order
 // to test some special encryption requirements for BOOKMARKS.
 class ModelTypeWorkerBookmarksTest : public ModelTypeWorkerTest {