[sync] Add feature flag to drop updates undecryptable for too long

If the new (DISABLED_BY_DEFAULT) IgnoreEncryptionKeysLongMissing feature
is enabled, ModelTypeWorker will ignore both future and pending updates
encrypted with keys that remained unknown for 'too long', as measured by
UnknownEncryptionKeyInfo::gu_responses_while_should_have_been_known.
The threshold for 'too long' is arbitrary for the moment. Two different
metrics are added: the first is logged when a key starts being ignored,
and records the number of pending updates that were dropped. The second
is recorded individually for each incoming update entity that's
encrypted with an ignored key.

Future CLs will start persisting the notion of unknown keys across
browser restarts.

Bug: 1109221
Change-Id: I52b841368385ce347fbaa8746501ab07a5b7752a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567883Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#835108}

components/sync/base/data_type_histogram.cc

@@ -25,6 +25,8 @@ std::string GetHistogramSuffixForUpdateDropReason(UpdateDropReason reason) {
       return "TombstoneForNonexistentInIncrementalUpdate";
     case UpdateDropReason::kDecryptionPending:
       return "DecryptionPending";
+    case UpdateDropReason::kDecryptionPendingForTooLong:
+      return "DecryptionPendingForTooLong";
     case UpdateDropReason::kFailedToDecrypt:
       return "FailedToDecrypt";
   }

components/sync/base/data_type_histogram.h

@@ -22,6 +22,7 @@ enum class UpdateDropReason {
   kTombstoneInFullUpdate,
   kTombstoneForNonexistentInIncrementalUpdate,
   kDecryptionPending,
+  kDecryptionPendingForTooLong,
   kFailedToDecrypt
 };
 

components/sync/engine/sync_engine_switches.cc

@@ -23,4 +23,10 @@ const base::Feature kSyncSupportTrustedVaultPassphrase{
 const base::Feature kSyncTriggerFullKeystoreMigration{
     "SyncTriggerFullKeystoreMigration", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Causes Sync to ignore updates encrypted with keys that have been missing for
+// too long from this client; Sync will proceed normally as if those updates
+// didn't exist.
+const base::Feature kIgnoreSyncEncryptionKeysLongMissing{
+    "IgnoreSyncEncryptionKeysLongMissing", base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace switches

components/sync/engine/sync_engine_switches.h

@@ -13,6 +13,7 @@ extern const base::Feature kSyncResetPollIntervalOnStart;
 extern const base::Feature kSyncUseScryptForNewCustomPassphrases;
 extern const base::Feature kSyncSupportTrustedVaultPassphrase;
 extern const base::Feature kSyncTriggerFullKeystoreMigration;
+extern const base::Feature kIgnoreSyncEncryptionKeysLongMissing;
 
 }  // namespace switches
 

components/sync/engine_impl/model_type_worker.cc

@@ -27,6 +27,7 @@
 #include "components/sync/base/time.h"
 #include "components/sync/base/unique_position.h"
 #include "components/sync/engine/model_type_processor.h"
+#include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/engine_impl/bookmark_update_preprocessing.h"
 #include "components/sync/engine_impl/cancelation_signal.h"
 #include "components/sync/engine_impl/commit_contribution.h"
@@ -41,6 +42,11 @@ namespace {
 const char kTimeUntilEncryptionKeyFoundHistogramPrefix[] =
     "Sync.ModelTypeTimeUntilEncryptionKeyFound.";
 
+const char kUndecryptablePendingUpdatesDroppedHistogramPrefix[] =
+    "Sync.ModelTypeUndecryptablePendingUpdatesDropped.";
+
+const int kMinGuResponsesToIgnoreKey = 50;
+
 void AdaptClientTagForFullUpdateData(ModelType model_type,
                                      syncer::EntityData* data) {
   // Server does not send any client tags for wallet data entities or offer data
@@ -104,6 +110,7 @@ ModelTypeWorker::ModelTypeWorker(
       cryptographer_(std::move(cryptographer)),
       passphrase_type_(passphrase_type),
       nudge_handler_(nudge_handler),
+      min_gu_responses_to_ignore_key_(kMinGuResponsesToIgnoreKey),
       cancelation_signal_(cancelation_signal) {
   DCHECK(model_type_processor_);
   DCHECK(type_ != PASSWORDS || cryptographer_);
@@ -221,17 +228,33 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
         // Override any previously undecryptable update for the same id.
         entries_pending_decryption_.erase(update_entity->id_string());
         break;
-      case DECRYPTION_PENDING:
-        // Cannot decrypt now, copy the sync entity for later decryption.
-        entries_pending_decryption_[update_entity->id_string()] =
-            *update_entity;
-        // If there's no entry for this unknown encryption key, create one.
-        DCHECK(!response_data.encryption_key_name.empty());
-        unknown_encryption_keys_by_name_.emplace(
-            response_data.encryption_key_name, UnknownEncryptionKeyInfo());
+      case DECRYPTION_PENDING: {
         SyncRecordModelTypeUpdateDropReason(
             UpdateDropReason::kDecryptionPending, type_);
+
+        const std::string& key_name = response_data.encryption_key_name;
+        DCHECK(!key_name.empty());
+        // If there's no entry for this unknown encryption key, create one.
+        unknown_encryption_keys_by_name_.emplace(key_name,
+                                                 UnknownEncryptionKeyInfo());
+
+        const std::string& server_id = update_entity->id_string();
+        if (ShouldIgnoreUpdatesEncryptedWith(key_name)) {
+          // Don't queue the incoming update. If there's a queued entry for
+          // |server_id|, don't clear it: outdated data is better than nothing.
+          // Such entry should be encrypted with another key, since |key_name|'s
+          // queued updates would've have been dropped by now.
+          DCHECK(!base::Contains(entries_pending_decryption_, server_id) ||
+                 GetEncryptionKeyName(entries_pending_decryption_[server_id]) !=
+                     key_name);
+          SyncRecordModelTypeUpdateDropReason(
+              UpdateDropReason::kDecryptionPendingForTooLong, type_);
+          break;
+        }
+        // Copy the sync entity for later decryption.
+        entries_pending_decryption_[server_id] = *update_entity;
         break;
+      }
       case FAILED_TO_DECRYPT:
         // Failed to decrypt the entity. Likely it is corrupt. Move on.
         SyncRecordModelTypeUpdateDropReason(UpdateDropReason::kFailedToDecrypt,
@@ -245,8 +268,12 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
   RemoveKeysNoLongerUnknown();
 
   if (!cryptographer_ || cryptographer_->CanEncrypt()) {
+    // Encryption keys should've been known in this state.
     for (auto& key_and_info : unknown_encryption_keys_by_name_) {
       key_and_info.second.gu_responses_while_should_have_been_known++;
+      // If the key is now missing for too long, drop pending updates encrypted
+      // with it. This eventually unblocks a worker having undecryptable data.
+      MaybeDropPendingUpdatesEncryptedWith(key_and_info.first);
     }
   }
 
@@ -676,6 +703,40 @@ bool ModelTypeWorker::DecryptPasswordSpecifics(
   return true;
 }
 
+bool ModelTypeWorker::ShouldIgnoreUpdatesEncryptedWith(
+    const std::string& key_name) {
+  if (!base::Contains(unknown_encryption_keys_by_name_, key_name)) {
+    return false;
+  }
+  if (unknown_encryption_keys_by_name_.at(key_name)
+          .gu_responses_while_should_have_been_known <
+      min_gu_responses_to_ignore_key_) {
+    return false;
+  }
+  return base::FeatureList::IsEnabled(
+      switches::kIgnoreSyncEncryptionKeysLongMissing);
+}
+
+void ModelTypeWorker::MaybeDropPendingUpdatesEncryptedWith(
+    const std::string& key_name) {
+  if (!ShouldIgnoreUpdatesEncryptedWith(key_name)) {
+    return;
+  }
+
+  size_t updates_before_dropping = entries_pending_decryption_.size();
+  base::EraseIf(entries_pending_decryption_, [&](const auto& id_and_update) {
+    return key_name == GetEncryptionKeyName(id_and_update.second);
+  });
+
+  // If updates were dropped, record how many.
+  if (entries_pending_decryption_.size() < updates_before_dropping) {
+    base::UmaHistogramCounts1000(
+        base::StrCat({kUndecryptablePendingUpdatesDroppedHistogramPrefix,
+                      ModelTypeToString(GetModelType())}),
+        updates_before_dropping - entries_pending_decryption_.size());
+  }
+}
+
 std::vector<ModelTypeWorker::UnknownEncryptionKeyInfo>
 ModelTypeWorker::RemoveKeysNoLongerUnknown() {
   std::set<std::string> keys_blocking_updates;

components/sync/engine_impl/model_type_worker.h

@@ -103,8 +103,6 @@ class ModelTypeWorker : public UpdateHandler,
   std::unique_ptr<CommitContribution> GetContribution(
       size_t max_entries) override;
 
-  bool HasLocalChangesForTest() const;
-
   // An alternative way to drive sending data to the processor, that should be
   // called when a new encryption mechanism is ready.
   void EncryptionAcceptedMaybeApplyUpdates();
@@ -119,6 +117,12 @@ class ModelTypeWorker : public UpdateHandler,
 
   base::WeakPtr<ModelTypeWorker> AsWeakPtr();
 
+  bool HasLocalChangesForTest() const;
+
+  void SetMinGuResponsesToIgnoreKeyForTest(int min_gu_responses_to_ignore_key) {
+    min_gu_responses_to_ignore_key_ = min_gu_responses_to_ignore_key;
+  }
+
  private:
   struct UnknownEncryptionKeyInfo {
     // Not increased if the cryptographer knows it's in a pending state
@@ -204,8 +208,18 @@ class ModelTypeWorker : public UpdateHandler,
   // response body.
   void OnFullCommitFailure(SyncCommitError commit_error);
 
-  // Removes |unknown_encryption_keys_| that no longer fit the definition of
-  // an unknown key, and returns their info.
+  // Returns true for keys that have remained unknown for so long that they are
+  // not expected to arrive anytime soon. The worker ignores incoming updates
+  // encrypted with them, and drops pending ones on the next GetUpdatesResponse.
+  // Those keys remain in |unknown_encryption_keys_by_name_|.
+  bool ShouldIgnoreUpdatesEncryptedWith(const std::string& key_name);
+
+  // If |key_name| should be ignored (cf. ShouldIgnoreUpdatesEncryptedWith()),
+  // drops elements of |entries_pending_decryption_| encrypted with it.
+  void MaybeDropPendingUpdatesEncryptedWith(const std::string& key_name);
+
+  // Removes elements of |unknown_encryption_keys_by_name_| that no longer fit
+  // the definition of an unknown key, and returns their info.
   std::vector<UnknownEncryptionKeyInfo> RemoveKeysNoLongerUnknown();
 
   ModelType type_;
@@ -234,10 +248,12 @@ class ModelTypeWorker : public UpdateHandler,
   // TODO(crbug.com/1109221): Use a name mentioning "updates" and "server id".
   std::map<std::string, sync_pb::SyncEntity> entries_pending_decryption_;
 
-  // A key is unknown if it encrypts a subset of |entries_pending_decryption_|.
-  // It'll be added here when the worker receives the first update entity
+  // A key is said to be unknown if one of these is true:
+  // a) It encrypts some updates(s) in |entries_pending_decryption_|.
+  // b) (a) happened for so long that the worker dropped those updates in an
+  // attempt to unblock itself (cf. ShouldIgnoreUpdatesEncryptedWith()).
+  // The key is added here when the worker receives the first update entity
   // encrypted with it.
-  // TODO(crbug.com/1109221): Enlarge this concept to cover ignored keys.
   std::map<std::string, UnknownEncryptionKeyInfo>
       unknown_encryption_keys_by_name_;
 
@@ -249,6 +265,11 @@ class ModelTypeWorker : public UpdateHandler,
   // and worker might not be ready to commit entities at the time.
   bool has_local_changes_ = false;
 
+  // Remains constant in production code. Can be overridden in tests.
+  // |UnknownEncryptionKeyInfo::gu_responses_while_should_have_been_known| must
+  // be above this value before updates encrypted with the key are ignored.
+  int min_gu_responses_to_ignore_key_;
+
   // Cancellation signal is used to cancel blocking operation on engine
   // shutdown.
   CancelationSignal* cancelation_signal_;

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -11,12 +11,15 @@
 
 #include "base/bind.h"
 #include "base/guid.h"
+#include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/threading/thread.h"
 #include "components/sync/base/client_tag_hash.h"
 #include "components/sync/base/unique_position.h"
 #include "components/sync/engine/model_type_processor.h"
+#include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/engine_impl/cancelation_signal.h"
 #include "components/sync/engine_impl/commit_contribution.h"
 #include "components/sync/engine_impl/cycle/entity_change_metric_recording.h"
@@ -1361,6 +1364,48 @@ TEST_F(ModelTypeWorkerTest, TimeUntilEncryptionKeyFoundMetric) {
       histogram_name, gu_responses_while_should_have_been_known, 1);
 }
 
+TEST_F(ModelTypeWorkerTest, IgnoreUpdatesEncryptedWithKeysMissingForTooLong) {
+  base::HistogramTester histogram_tester;
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      switches::kIgnoreSyncEncryptionKeysLongMissing);
+
+  NormalInitialize();
+  worker()->SetMinGuResponsesToIgnoreKeyForTest(2);
+
+  // Send an update encrypted with a key that shall remain unknown.
+  SetUpdateEncryptionFilter(1);
+  TriggerUpdateFromServer(10, kTag1, kValue1);
+
+  // The undecryptable update has been around for only 1 GetUpdatesResponse, so
+  // the worker is still blocked.
+  EXPECT_TRUE(worker()->BlockForEncryption());
+
+  // Send empty GetUpdatesResponse, reaching the threshold of 2.
+  worker()->ProcessGetUpdatesResponse(
+      server()->GetProgress(), server()->GetContext(), {}, status_controller());
+
+  // The undecryptable update should have been dropped and the worker is no
+  // longer blocked.
+  EXPECT_FALSE(worker()->BlockForEncryption());
+
+  // Should have recorded that 1 entity was dropped.
+  histogram_tester.ExpectUniqueSample(
+      base::StrCat({"Sync.ModelTypeUndecryptablePendingUpdatesDropped.",
+                    ModelTypeToString(worker()->GetModelType())}),
+      1, 1);
+
+  // From now on, incoming updates encrypted with the missing key don't block
+  // the worker.
+  TriggerUpdateFromServer(10, kTag2, kValue2);
+  EXPECT_FALSE(worker()->BlockForEncryption());
+
+  // Should have recorded that 1 incoming update was ignored.
+  histogram_tester.ExpectUniqueSample(
+      "Sync.ModelTypeUpdateDrop.DecryptionPendingForTooLong",
+      worker()->GetModelType(), 1);
+}
+
 // Test that processor has been disconnected from Sync when worker got
 // disconnected.
 TEST_F(ModelTypeWorkerTest, DisconnectProcessorFromSyncTest) {

tools/metrics/histograms/histograms_xml/histogram_suffixes_list.xml

@@ -18427,6 +18427,7 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   <affected-histogram name="Sync.ModelTypeMemoryKB"/>
   <affected-histogram name="Sync.ModelTypeStoreCommitWriteBatchOutcome"/>
   <affected-histogram name="Sync.ModelTypeTimeUntilEncryptionKeyFound"/>
+  <affected-histogram name="Sync.ModelTypeUndecryptablePendingUpdatesDropped"/>
   <affected-histogram name="Sync.NonReflectionUpdateFreshnessPossiblySkewed">
     <obsolete>
       Deprecated 06/2019. Replaced by
@@ -18532,6 +18533,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   <suffix name="DecryptionPending"
       label="Decryption keys are missing at the moment (it is queued for
              later decryption which may or may not happen)."/>
+  <suffix name="DecryptionPendingForTooLong"
+      label="Decryption keys were missing for so long that the update was
+             ignored."/>
   <suffix name="FailedToDecrypt"
       label="Decryption is not successful (maybe the data is corrupt)."/>
   <suffix name="InconsistentClientTag"

tools/metrics/histograms/histograms_xml/sync/histograms.xml

@@ -600,6 +600,18 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram base="true" name="Sync.ModelTypeUndecryptablePendingUpdatesDropped"
+    units="SyncEntity" expires_after="2021-06-05">
+  <owner>victorvianna@google.com</owner>
+  <owner>mastiz@chromium.org</owner>
+  <summary>
+    Records the number of entities dropped when the data type decided that a
+    certain encryption key was lost and dropped all pending updates encrypted
+    with it. Future updates encrypted with such key will also be ignored by the
+    data type, but those are *not* counted in this metric.
+  </summary>
+</histogram>
+
 <histogram base="true" name="Sync.ModelTypeUpdateDrop" enum="SyncModelTypes"
     expires_after="2021-05-01">
 <!-- Name completed by histogram_suffixes name="SyncModelTypeUpdateDrop" -->