[Sync] Adding integration tests for USS encryption and fixing a worker bug.

components/sync/engine_impl/model_type_worker.cc

Index: components/sync/engine_impl/model_type_worker.cc
diff --git a/components/sync/engine_impl/model_type_worker.cc b/components/sync/engine_impl/model_type_worker.cc
index fa9bea2d044608b5841872afd6f0aa34a6bd8b73..76a74d1e97f14058f7b9988c9ddaae6fce771cc9 100644
--- a/components/sync/engine_impl/model_type_worker.cc
+++ b/components/sync/engine_impl/model_type_worker.cc
@@ -35,6 +35,7 @@ ModelTypeWorker::ModelTypeWorker(
       model_type_processor_(std::move(model_type_processor)),
       cryptographer_(std::move(cryptographer)),
       nudge_handler_(nudge_handler),
+      decrypted_everything_(true),
       weak_ptr_factory_(this) {
   DCHECK(model_type_processor_);
 
@@ -149,6 +150,7 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
       data.specifics = specifics;
       response_data.entity = data.PassToPtr();
       entity->ReceiveEncryptedUpdate(response_data);
+      decrypted_everything_ = false;
     }
   }
 
@@ -175,11 +177,14 @@ void ModelTypeWorker::PassiveApplyUpdates(StatusController* status) {
 }
 
 void ModelTypeWorker::ApplyPendingUpdates() {
-  DVLOG(1) << ModelTypeToString(type_) << ": "
-           << base::StringPrintf("Delivering %" PRIuS " applicable updates.",
-                                 pending_updates_.size());
-  model_type_processor_->OnUpdateReceived(model_type_state_, pending_updates_);
-  pending_updates_.clear();
+  if (!BlockForEncryption()) {
+    DVLOG(1) << ModelTypeToString(type_) << ": "
+             << base::StringPrintf("Delivering %" PRIuS " applicable updates.",
+                                   pending_updates_.size());
+    model_type_processor_->OnUpdateReceived(model_type_state_,
+                                            pending_updates_);
+    pending_updates_.clear();
+  }
 }
 
 void ModelTypeWorker::EnqueueForCommit(const CommitRequestDataList& list) {
@@ -204,8 +209,6 @@ void ModelTypeWorker::EnqueueForCommit(const CommitRequestDataList& list) {
 std::unique_ptr<CommitContribution> ModelTypeWorker::GetContribution(
     size_t max_entries) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  // There shouldn't be a GetUpdates in progress when a commit is triggered.
-  DCHECK(pending_updates_.empty());
 
   size_t space_remaining = max_entries;
   google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> commit_entities;
@@ -269,14 +272,26 @@ bool ModelTypeWorker::CanCommitItems() const {
   if (!IsTypeInitialized())
     return false;
 
-  // Don't commit if we should be encrypting but don't have the required keys.
-  if (cryptographer_ && !cryptographer_->is_ready()) {
+  // We shouldn't commit anything if we're missing encryption keys.
+  if (BlockForEncryption())
     return false;
-  }
 
   return true;
 }
 
+bool ModelTypeWorker::BlockForEncryption() const {
+  // Should be using encryption, but we do not have the keys.
+  if (cryptographer_ && !cryptographer_->is_ready())
+    return true;
+
+  // Encrypted entities have been returned by the server and decrypting them was
+  // unsuccessful. This means there is a key we do not have.
+  if (!decrypted_everything_)
+    return true;
+
+  return false;
+}
+
 void ModelTypeWorker::AdjustCommitProto(sync_pb::SyncEntity* sync_entity) {
   DCHECK(CanCommitItems());
 
@@ -318,18 +333,17 @@ void ModelTypeWorker::AdjustCommitProto(sync_pb::SyncEntity* sync_entity) {
 void ModelTypeWorker::OnCryptographerUpdated() {
   DCHECK(cryptographer_);
 
-  bool new_encryption_key = false;
-  UpdateResponseDataList response_datas;
-
-  const std::string& new_key_name = cryptographer_->GetDefaultNigoriKeyName();
+  // Reset the flag, if anything undecryptable is encountered below this flag

[maxbogue, 2016/10/11 15:56:39]

"Reset the flag. If anything undecryptable is encountered below, then this
flag.."

[skym, 2016/10/11 16:41:00]

Done.

+  // will be updated back to false.
+  decrypted_everything_ = true;
 
   // Handle a change in encryption key.
-  if (model_type_state_.encryption_key_name() != new_key_name) {
+  const std::string& new_key_name = cryptographer_->GetDefaultNigoriKeyName();
+  const std::string& old_key_name = model_type_state_.encryption_key_name();
+  if (old_key_name != new_key_name) {
     DVLOG(1) << ModelTypeToString(type_) << ": Updating encryption key "
-             << model_type_state_.encryption_key_name() << " -> "
-             << new_key_name;
+             << old_key_name << " -> " << new_key_name;
     model_type_state_.set_encryption_key_name(new_key_name);
-    new_encryption_key = true;
   }
 
   for (const auto& kv : entities_) {
@@ -355,21 +369,25 @@ void ModelTypeWorker::OnCryptographerUpdated() {
           decrypted_update.response_version = encrypted_update.response_version;
           decrypted_update.encryption_key_name =
               data.specifics.encrypted().key_name();
-          response_datas.push_back(decrypted_update);
+          pending_updates_.push_back(decrypted_update);
 
           entity->ClearEncryptedUpdate();
         }
+      } else {
+        // It is possible that we could get into a weird state where an entity
+        // was encrypted with an old key that has since been overwritten. We are
+        // never going to recieve the new key, and |decrypted_everything_| is

[maxbogue, 2016/10/11 15:56:39]

"receive"

[skym, 2016/10/11 16:41:00]

Done.

+        // perpetually stuck false. Every time we start up, we use a really old
+        // progress marker to download lots of entities, and that old and
+        // undecryptable entity contiuously blocks progress. This would cause

[maxbogue, 2016/10/11 15:56:39]

"continuously"

[skym, 2016/10/11 16:41:00]

Done.

+        // the data type to silently stop working, which is definitely less than

[maxbogue, 2016/10/11 15:56:39]

Should we add a UMA counter to track this case...?

[skym, 2016/10/11 16:41:00]

It would be great to have an UMA counter that helped us understand how often we
got stuck in the can't decrypt everything state. But it's really hard to know
you're in that, and instead you'll just get a lot of noise from normal usage.

+        // ideal. Hopefully this never happens since nigori updates are rare and
+        // should be using optimistic locking.
+        decrypted_everything_ = false;
       }
     }
   }
-
-  if (new_encryption_key || response_datas.size() > 0) {
-    DVLOG(1) << ModelTypeToString(type_) << ": "
-             << base::StringPrintf("Delivering encryption key and %" PRIuS
-                                   " decrypted updates.",
-                                   response_datas.size());
-    model_type_processor_->OnUpdateReceived(model_type_state_, response_datas);
-  }
+  ApplyPendingUpdates();
 }
 
 bool ModelTypeWorker::DecryptSpecifics(const sync_pb::EntitySpecifics& in,