[ModelTypeWorker] Store SyncEntities if encryption pending

This CL changes the way we process incoming SyncEntities when decryption
is pending. After the change:
 - PopulateUpdateResponseData() early returns and does not populate
   anything if decryption is pending.
 - ProcessGetUpdatesResponse() then stores the original SyncEntity
   instead of the previous (partially) populated UpdateResponseData.
 - DecryptStoredEntities() gets much simpler because it can now reuse
   PopulateUpdateResponseData() and does not have to duplicate the
   decryption logic.

As a result this CL reconciles all bookmarks adaptations into a single
place in PopulateUpdateResponseData().

This CL does not introduce any behavioral change.

Bug: 1007199
Change-Id: I5ebbfcbaf5dd6bd677aba1697e7ee8d8cd08d75f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1827427
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#723431}

components/sync/engine_impl/bookmark_update_preprocessing.cc

@@ -142,25 +142,4 @@ void AdaptGuidForBookmark(const sync_pb::SyncEntity& update_entity,
   }
 }
 
-void AdaptGuidForBookmarkEntityData(EntityData* entity_data) {
-  DCHECK(entity_data);
-  DCHECK(entity_data->specifics.has_bookmark());
-  DCHECK(!entity_data->specifics.has_encrypted());
-
-  if (entity_data->is_deleted() ||
-      !entity_data->server_defined_unique_tag.empty()) {
-    return;
-  }
-
-  if (entity_data->specifics.bookmark().has_guid()) {
-    LogGuidSource(BookmarkGuidSource::kSpecifics);
-  } else if (base::IsValidGUID(entity_data->originator_client_item_id)) {
-    entity_data->specifics.mutable_bookmark()->set_guid(
-        entity_data->originator_client_item_id);
-    LogGuidSource(BookmarkGuidSource::kValidOCII);
-  } else {
-    LogGuidSource(BookmarkGuidSource::kLeftEmpty);
-  }
-}
-
 }  // namespace syncer

components/sync/engine_impl/bookmark_update_preprocessing.h

@@ -37,11 +37,6 @@ void AdaptTitleForBookmark(const sync_pb::SyncEntity& update_entity,
 void AdaptGuidForBookmark(const sync_pb::SyncEntity& update_entity,
                           sync_pb::EntitySpecifics* specifics);
 
-// Analogous to AdaptGuidForBookmark() but deals with EntityData structs.
-// |entity_data| must not be null.
-// TODO(crbug.com/1007199): Reconcile with AdaptGuidForBookmark().
-void AdaptGuidForBookmarkEntityData(EntityData* entity_data);
-
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_ENGINE_IMPL_BOOKMARK_UPDATE_PREPROCESSING_H_

components/sync/engine_impl/model_type_worker.cc

@@ -29,22 +29,6 @@
 
 namespace syncer {
 
-namespace {
-
-void AdaptUpdateForCompatibilityAfterDecryption(
-    ModelType model_type,
-    const sync_pb::SyncEntity& update_entity,
-    sync_pb::EntitySpecifics* specifics,
-    bool specifics_were_encrypted) {
-  DCHECK(specifics);
-  if (model_type == BOOKMARKS) {
-    AdaptTitleForBookmark(update_entity, specifics, specifics_were_encrypted);
-    AdaptGuidForBookmark(update_entity, specifics);
-  }
-}
-
-}  // namespace
-
 ModelTypeWorker::ModelTypeWorker(
     ModelType type,
     const sync_pb::ModelTypeState& initial_state,
@@ -185,8 +169,9 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
         pending_updates_.push_back(std::move(response_data));
         break;
       case DECRYPTION_PENDING:
+        // Cannot decrypt now, copy the sync entity for later decryption.
         entries_pending_decryption_[update_entity->id_string()] =
-            std::move(response_data);
+            *update_entity;
         break;
       case FAILED_TO_DECRYPT:
         // Failed to decrypt the entity. Likely it is corrupt. Move on.
@@ -206,39 +191,17 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
     ModelType model_type,
     const sync_pb::SyncEntity& update_entity,
     UpdateResponseData* response_data) {
-  response_data->response_version = update_entity.version();
   auto data = std::make_unique<syncer::EntityData>();
-  // Prepare the message for the model thread.
-  data->id = update_entity.id_string();
-  data->client_tag_hash =
-      ClientTagHash::FromHashed(update_entity.client_defined_unique_tag());
-  data->creation_time = ProtoTimeToTime(update_entity.ctime());
-  data->modification_time = ProtoTimeToTime(update_entity.mtime());
-  data->name = update_entity.name();
-  data->is_folder = update_entity.folder();
-  data->parent_id = update_entity.parent_id_string();
-  data->server_defined_unique_tag = update_entity.server_defined_unique_tag();
-
-  // Populate |originator_cache_guid| and |originator_client_item_id|. This is
-  // currently relevant only for bookmarks.
-  data->originator_cache_guid = update_entity.originator_cache_guid();
-  data->originator_client_item_id = update_entity.originator_client_item_id();
-
-  // Adapt the update for compatibility (all except specifics that may need
-  // encryption).
-  if (model_type == BOOKMARKS) {
-    AdaptUniquePositionForBookmark(update_entity, data.get());
-  }
-  // TODO(crbug.com/881289): Generate client tag hash for wallet data.
 
   // Deleted entities must use the default instance of EntitySpecifics in
   // order for EntityData to correctly reflect that they are deleted.
   const sync_pb::EntitySpecifics& specifics =
       update_entity.deleted() ? sync_pb::EntitySpecifics::default_instance()
                               : update_entity.specifics();
+  bool specifics_were_encrypted = false;
 
-  // Passwords use their own legacy encryption scheme.
   if (specifics.password().has_encrypted()) {
+    // Passwords use their own legacy encryption scheme.
     DCHECK(cryptographer);
     // TODO(crbug.com/516866): If we switch away from the password legacy
     // encryption, this method and DecryptStoredEntities() )should be already
@@ -247,51 +210,62 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
     // Make sure the worker defers password entities if the encryption key
     // hasn't been received yet.
     if (!cryptographer->CanDecrypt(specifics.password().encrypted())) {
-      data->specifics = specifics;
-      response_data->entity = std::move(data);
       return DECRYPTION_PENDING;
     }
-    response_data->encryption_key_name =
-        specifics.password().encrypted().key_name();
     if (!DecryptPasswordSpecifics(*cryptographer, specifics,
                                   &data->specifics)) {
       return FAILED_TO_DECRYPT;
     }
-    AdaptUpdateForCompatibilityAfterDecryption(
-        model_type, update_entity, &data->specifics,
-        /*specifics_were_encrypted=*/true);
-    response_data->entity = std::move(data);
-    return SUCCESS;
-  }
-
-  // Check if specifics are encrypted and try to decrypt if so.
-  if (!specifics.has_encrypted()) {
-    // No encryption.
-    data->specifics = specifics;
-    AdaptUpdateForCompatibilityAfterDecryption(
-        model_type, update_entity, &data->specifics,
-        /*specifics_were_encrypted=*/false);
-    response_data->entity = std::move(data);
-    return SUCCESS;
-  }
-  // Deleted entities should not be encrypted.
-  DCHECK(!update_entity.deleted());
-  if (cryptographer && cryptographer->CanDecrypt(specifics.encrypted())) {
+    response_data->encryption_key_name =
+        specifics.password().encrypted().key_name();
+    specifics_were_encrypted = true;
+  } else if (specifics.has_encrypted()) {
+    // Check if specifics are encrypted and try to decrypt if so.
+    // Deleted entities should not be encrypted.
+    DCHECK(!update_entity.deleted());
+    if (!cryptographer || !cryptographer->CanDecrypt(specifics.encrypted())) {
+      // Can't decrypt right now.
+      return DECRYPTION_PENDING;
+    }
     // Encrypted and we know the key.
     if (!DecryptSpecifics(*cryptographer, specifics, &data->specifics)) {
       return FAILED_TO_DECRYPT;
     }
-    AdaptUpdateForCompatibilityAfterDecryption(
-        model_type, update_entity, &data->specifics,
-        /*specifics_were_encrypted=*/true);
-    response_data->entity = std::move(data);
     response_data->encryption_key_name = specifics.encrypted().key_name();
-    return SUCCESS;
+    specifics_were_encrypted = true;
+  } else {
+    // No encryption.
+    data->specifics = specifics;
+  }
+
+  response_data->response_version = update_entity.version();
+  // Prepare the message for the model thread.
+  data->id = update_entity.id_string();
+  data->client_tag_hash =
+      ClientTagHash::FromHashed(update_entity.client_defined_unique_tag());
+  data->creation_time = ProtoTimeToTime(update_entity.ctime());
+  data->modification_time = ProtoTimeToTime(update_entity.mtime());
+  data->name = update_entity.name();
+  data->is_folder = update_entity.folder();
+  data->parent_id = update_entity.parent_id_string();
+  data->server_defined_unique_tag = update_entity.server_defined_unique_tag();
+
+  // Populate |originator_cache_guid| and |originator_client_item_id|. This is
+  // currently relevant only for bookmarks.
+  data->originator_cache_guid = update_entity.originator_cache_guid();
+  data->originator_client_item_id = update_entity.originator_client_item_id();
+
+  // Adapt the update for compatibility.
+  if (model_type == BOOKMARKS) {
+    AdaptUniquePositionForBookmark(update_entity, data.get());
+    AdaptTitleForBookmark(update_entity, &data->specifics,
+                          specifics_were_encrypted);
+    AdaptGuidForBookmark(update_entity, &data->specifics);
   }
-  // Can't decrypt right now.
-  data->specifics = specifics;
+  // TODO(crbug.com/881289): Generate client tag hash for wallet data.
+
   response_data->entity = std::move(data);
-  return DECRYPTION_PENDING;
+  return SUCCESS;
 }
 
 void ModelTypeWorker::ApplyUpdates(StatusController* status) {
@@ -475,57 +449,26 @@ bool ModelTypeWorker::UpdateEncryptionKeyName() {
 void ModelTypeWorker::DecryptStoredEntities() {
   for (auto it = entries_pending_decryption_.begin();
        it != entries_pending_decryption_.end();) {
-    const UpdateResponseData& encrypted_update = *it->second;
-    const EntityData& data = *encrypted_update.entity;
-    DCHECK(!data.is_deleted());
-
-    sync_pb::EntitySpecifics specifics;
-    std::string encryption_key_name;
+    const sync_pb::SyncEntity& encrypted_update = it->second;
 
-    if (data.specifics.password().has_encrypted()) {
-      encryption_key_name = data.specifics.password().encrypted().key_name();
-      if (!cryptographer_->CanDecrypt(data.specifics.password().encrypted())) {
-        ++it;
-        continue;
-      }
-      if (!DecryptPasswordSpecifics(*cryptographer_, data.specifics,
-                                    &specifics)) {
-        // 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.
+    auto response_data = std::make_unique<UpdateResponseData>();
+    switch (PopulateUpdateResponseData(cryptographer_.get(), type_,
+                                       encrypted_update, response_data.get())) {
+      case SUCCESS:
+        pending_updates_.push_back(std::move(response_data));
         it = entries_pending_decryption_.erase(it);
-        continue;
-      }
-    } else {
-      DCHECK(data.specifics.has_encrypted());
-      encryption_key_name = data.specifics.encrypted().key_name();
-
-      if (!cryptographer_->CanDecrypt(data.specifics.encrypted())) {
+        break;
+      case DECRYPTION_PENDING:
+        // Still cannot decrypt, move on and keep this one for later.
         ++it;
-        continue;
-      }
-
-      if (!DecryptSpecifics(*cryptographer_, data.specifics, &specifics)) {
+        break;
+      case FAILED_TO_DECRYPT:
         // 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.
+        // decryption keys are up-to-date. Let's ignore this update to avoid
+        // blocking other updates.
         it = entries_pending_decryption_.erase(it);
-        continue;
-      }
-    }
-
-    auto decrypted_update = std::make_unique<UpdateResponseData>();
-    decrypted_update->response_version = encrypted_update.response_version;
-    decrypted_update->encryption_key_name = encryption_key_name;
-    decrypted_update->entity = std::move(it->second->entity);
-    decrypted_update->entity->specifics = std::move(specifics);
-
-    if (decrypted_update->entity->specifics.has_bookmark()) {
-      AdaptGuidForBookmarkEntityData(decrypted_update->entity.get());
+        break;
     }
-
-    pending_updates_.push_back(std::move(decrypted_update));
-    it = entries_pending_decryption_.erase(it);
   }
 }
 

components/sync/engine_impl/model_type_worker.h

@@ -231,10 +231,10 @@ class ModelTypeWorker : public UpdateHandler,
   // Interface used to access and send nudges to the sync scheduler. Not owned.
   NudgeHandler* nudge_handler_;
 
-  // A map of update responses, keyed by server_id.
-  // Holds updates encrypted with pending keys.
-  std::map<std::string, std::unique_ptr<UpdateResponseData>>
-      entries_pending_decryption_;
+  // A map of sync entities, keyed by server_id. Holds updates encrypted with
+  // pending keys. Entries are stored in a map for de-duplication (applying only
+  // the latest).
+  std::map<std::string, sync_pb::SyncEntity> entries_pending_decryption_;
 
   // Accumulates all the updates from a single GetUpdates cycle in memory so
   // they can all be sent to the processor at once.

components/sync/protocol/proto_memory_estimations.cc

@@ -127,6 +127,7 @@ INSTANTIATE(EntityMetadata)
 INSTANTIATE(EntitySpecifics)
 INSTANTIATE(ModelTypeState)
 INSTANTIATE(PersistedEntityData)
+INSTANTIATE(SyncEntity)
 INSTANTIATE(UniquePosition)
 
 }  // namespace sync_pb