sync: Finish non-blocking type encryption support

Undoes some previous work towards encryption support.  That approach
suffered from some subtle deadlock issues that could not be easily
worked around.  The new approach involves less sharing and less locks.

Gives the ModelTypeSyncWorker its own copy of the Cryptographer.  By
passing around copies, it no longer needs to worry about acquiring locks
in order to access the Directory's cryptographer.  This required
a rewrite of some changes to the way the ModelTypeSyncWorker detects the
current encryption state.  Most notably, its Cryptographer is NULL if
encryption is not enabled for its model type.

Makes the ModelTypeSyncRegistry responsible for observing changes
emitted by the SyncEncryptionHandler and forwarding them to the
ModelTypeSyncWorkers.  It should receive callbacks from the
SyncEncryptionHandler during startup, so it does not need to cache or
query any new data.

Removes the CryptographerProviders.  Since the ModelTypeSyncWorker no
longer need to access the directory's cryptographer, it's no longer
necessary.

BUG=351005

Review URL: https://codereview.chromium.org/452283003

Cr-Commit-Position: refs/heads/master@{#290067}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290067 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 151 insertions, 293 deletions

diff --git a/sync/engine/cryptographer_provider.cc b/sync/engine/cryptographer_provider.cc
deleted file mode 100644
index 2c16bc4..0000000
--- a/sync/engine/cryptographer_provider.cc
+++ /dev/null
@@ -1,42 +0,0 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "sync/engine/cryptographer_provider.h"
-
-namespace syncer {
-
-CryptographerProvider::CryptographerProvider() {
-}
-
-CryptographerProvider::~CryptographerProvider() {
-}
-
-ScopedCryptographerRef::ScopedCryptographerRef() {
-}
-
-ScopedCryptographerRef::~ScopedCryptographerRef() {
-}
-
-bool ScopedCryptographerRef::Initialize(ScopedCryptographerInternal* impl) {
-  scoped_cryptographer_internal_.reset(impl);
-  return IsValid();
-}
-
-bool ScopedCryptographerRef::IsValid() const {
-  return !!Get();
-}
-
-Cryptographer* ScopedCryptographerRef::Get() const {
-  if (!scoped_cryptographer_internal_)
-    return NULL;
-  return scoped_cryptographer_internal_->Get();
-}
-
-ScopedCryptographerInternal::ScopedCryptographerInternal() {
-}
-
-ScopedCryptographerInternal::~ScopedCryptographerInternal() {
-}
-
-}  // namespace syncer
diff --git a/sync/engine/cryptographer_provider.h b/sync/engine/cryptographer_provider.h
deleted file mode 100644
index f38bd185..0000000
--- a/sync/engine/cryptographer_provider.h
+++ /dev/null
@@ -1,64 +0,0 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef SYNC_ENGINE_CRYPTOGRAPHER_PROVIDER_H_
-#define SYNC_ENGINE_CRYPTOGRAPHER_PROVIDER_H_
-
-#include "base/memory/scoped_ptr.h"
-#include "sync/base/sync_export.h"
-
-namespace syncer {
-
-class Cryptographer;
-class ScopedCryptographerRef;
-class ScopedCryptographerInternal;
-
-// An interface for providing clients with a ScopedCryptographerRef.
-//
-// Used to expose the syncable::Directory's cryptographer to clients that
-// would otherwise not have access to the Directory.
-class SYNC_EXPORT_PRIVATE CryptographerProvider {
- public:
-  CryptographerProvider();
-  virtual ~CryptographerProvider();
-
-  virtual bool InitScopedCryptographerRef(ScopedCryptographerRef* scoped) = 0;
-};
-
-// A concrete class representing a reference to a cryptographer.
-//
-// Access to the cryptographer is lost when this class goes out of scope.
-class SYNC_EXPORT_PRIVATE ScopedCryptographerRef {
- public:
-  ScopedCryptographerRef();
-  ~ScopedCryptographerRef();
-
-  bool Initialize(ScopedCryptographerInternal* impl);
-  bool IsValid() const;
-  Cryptographer* Get() const;
-
- private:
-  scoped_ptr<ScopedCryptographerInternal> scoped_cryptographer_internal_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedCryptographerRef);
-};
-
-// An interface class used in the implementation of the ScopedCryptographerRef.
-//
-// We use this class to allow different implementations of
-// CryptographerProvider to implement InitScopedCryptographer in different
-// ways.  The ScopedCryptographerRef itself must be stack-allocated, so it
-// can't vary depending on which kind of CryptographerProvider is used to
-// intialize it.
-class SYNC_EXPORT_PRIVATE ScopedCryptographerInternal {
- public:
-  ScopedCryptographerInternal();
-  virtual ~ScopedCryptographerInternal();
-
-  virtual Cryptographer* Get() const = 0;
-};
-
-}  // namespace syncer
-
-#endif  // SYNC_ENGINE_CRYPTOGRAPHER_PROVIDER_H_
diff --git a/sync/engine/directory_cryptographer_provider.cc b/sync/engine/directory_cryptographer_provider.cc
deleted file mode 100644
index a9cd2a2..0000000
--- a/sync/engine/directory_cryptographer_provider.cc
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "sync/engine/directory_cryptographer_provider.h"
-
-#include "sync/syncable/directory.h"
-#include "sync/syncable/syncable_read_transaction.h"
-
-namespace syncer {
-
-DirectoryCryptographerProvider::DirectoryCryptographerProvider(
-    syncable::Directory* dir)
-    : dir_(dir) {
-}
-
-DirectoryCryptographerProvider::~DirectoryCryptographerProvider() {
-}
-
-bool DirectoryCryptographerProvider::InitScopedCryptographerRef(
-    ScopedCryptographerRef* scoped) {
-  scoped->Initialize(new ScopedDirectoryCryptographerInternal(dir_));
-  return scoped->IsValid();
-}
-
-ScopedDirectoryCryptographerInternal::ScopedDirectoryCryptographerInternal(
-    syncable::Directory* dir)
-    : dir_(dir), trans_(FROM_HERE, dir) {
-}
-
-ScopedDirectoryCryptographerInternal::~ScopedDirectoryCryptographerInternal() {
-}
-
-Cryptographer* ScopedDirectoryCryptographerInternal::Get() const {
-  return dir_->GetCryptographer(&trans_);
-}
-
-}  // namespace syncer
diff --git a/sync/engine/directory_cryptographer_provider.h b/sync/engine/directory_cryptographer_provider.h
deleted file mode 100644
index 36b1c0f..0000000
--- a/sync/engine/directory_cryptographer_provider.h
+++ /dev/null
@@ -1,51 +0,0 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef SYNC_ENGINE_DIRECTORY_CRYPTOGRAPHER_PROVIDER_H_
-#define SYNC_ENGINE_DIRECTORY_CRYPTOGRAPHER_PROVIDER_H_
-
-#include "sync/engine/cryptographer_provider.h"
-
-#include "sync/syncable/syncable_read_transaction.h"
-
-namespace syncer {
-
-namespace syncable {
-class Directory;
-}
-
-// Provides access to the Directory's cryptographer through the
-// CryptographerProvider interface.
-class DirectoryCryptographerProvider : public CryptographerProvider {
- public:
-  DirectoryCryptographerProvider(syncable::Directory* dir);
-  virtual ~DirectoryCryptographerProvider();
-
-  virtual bool InitScopedCryptographerRef(
-      ScopedCryptographerRef* scoped) OVERRIDE;
-
- private:
-  syncable::Directory* dir_;
-};
-
-// An implementation detail of the DirectoryCryptographerProvider.  Contains
-// the ReadTransaction used to safely access the Cryptographer.
-class ScopedDirectoryCryptographerInternal
-    : public ScopedCryptographerInternal {
- public:
-  ScopedDirectoryCryptographerInternal(syncable::Directory* dir);
-  virtual ~ScopedDirectoryCryptographerInternal();
-
-  virtual Cryptographer* Get() const OVERRIDE;
-
- private:
-  syncable::Directory* dir_;
-  syncable::ReadTransaction trans_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedDirectoryCryptographerInternal);
-};
-
-}  // namespace syncer
-
-#endif  // SYNC_ENGINE_DIRECTORY_CRYPTOGRAPHER_PROVIDER_H_
diff --git a/sync/engine/model_type_entity.cc b/sync/engine/model_type_entity.cc
index 2f758a8..3249a16 100644
--- a/sync/engine/model_type_entity.cc
+++ b/sync/engine/model_type_entity.cc
@@ -131,6 +131,9 @@ void ModelTypeEntity::UpdateDesiredEncryptionKey(const std::string& name) {
   if (encryption_key_name_ == name)
     return;
 
+  DVLOG(2) << id_ << ": Encryption triggered commit: " << encryption_key_name_
+           << " -> " << name;
+
   // Schedule commit with the expectation that the worker will re-encrypt with
   // the latest encryption key as it does.
   sequence_number_++;
diff --git a/sync/engine/model_type_sync_proxy_impl.cc b/sync/engine/model_type_sync_proxy_impl.cc
index 7ea5d52..d3e1d77 100644
--- a/sync/engine/model_type_sync_proxy_impl.cc
+++ b/sync/engine/model_type_sync_proxy_impl.cc
@@ -240,6 +240,9 @@ void ModelTypeSyncProxyImpl::OnUpdateReceived(
     // commit to fix it.
     if (data_type_state_.encryption_key_name !=
         response_data.encryption_key_name) {
+      DVLOG(2) << ModelTypeToString(type_) << ": Requesting re-encrypt commit "
+               << response_data.encryption_key_name << " -> "
+               << data_type_state_.encryption_key_name;
       EntityMap::iterator it2 = entities_.find(client_tag_hash);
       it2->second->UpdateDesiredEncryptionKey(
           data_type_state_.encryption_key_name);
diff --git a/sync/engine/model_type_sync_worker_impl.cc b/sync/engine/model_type_sync_worker_impl.cc
index a58400f4..5a47f5e 100644
--- a/sync/engine/model_type_sync_worker_impl.cc
+++ b/sync/engine/model_type_sync_worker_impl.cc
@@ -22,13 +22,13 @@ ModelTypeSyncWorkerImpl::ModelTypeSyncWorkerImpl(
     ModelType type,
     const DataTypeState& initial_state,
     const UpdateResponseDataList& saved_pending_updates,
-    CryptographerProvider* cryptographer_provider,
+    scoped_ptr<Cryptographer> cryptographer,
     NudgeHandler* nudge_handler,
     scoped_ptr<ModelTypeSyncProxy> type_sync_proxy)
     : type_(type),
       data_type_state_(initial_state),
       type_sync_proxy_(type_sync_proxy.Pass()),
-      cryptographer_provider_(cryptographer_provider),
+      cryptographer_(cryptographer.Pass()),
       nudge_handler_(nudge_handler),
       entities_deleter_(&entities_),
       weak_ptr_factory_(this) {
@@ -47,7 +47,11 @@ ModelTypeSyncWorkerImpl::ModelTypeSyncWorkerImpl(
     entities_.insert(std::make_pair(it->client_tag_hash, entity_tracker));
   }
 
-  TryDecryptPendingUpdates();
+  if (cryptographer_) {
+    DVLOG(1) << ModelTypeToString(type_) << ": Starting with encryption key "
+             << cryptographer_->GetDefaultNigoriKeyName();
+    OnCryptographerUpdated();
+  }
 }
 
 ModelTypeSyncWorkerImpl::~ModelTypeSyncWorkerImpl() {
@@ -59,29 +63,19 @@ ModelType ModelTypeSyncWorkerImpl::GetModelType() const {
 }
 
 bool ModelTypeSyncWorkerImpl::IsEncryptionRequired() const {
-  return !data_type_state_.encryption_key_name.empty();
+  return !!cryptographer_;
 }
 
-void ModelTypeSyncWorkerImpl::SetEncryptionKeyName(const std::string& name) {
-  if (data_type_state_.encryption_key_name == name)
-    return;
-
-  data_type_state_.encryption_key_name = name;
-
-  // Pretend to send an update.  This will cause the TypeSyncProxy to notice
-  // the new encryption key and take appropriate action.
-  type_sync_proxy_->OnUpdateReceived(
-      data_type_state_, UpdateResponseDataList(), UpdateResponseDataList());
-}
+void ModelTypeSyncWorkerImpl::UpdateCryptographer(
+    scoped_ptr<Cryptographer> cryptographer) {
+  DCHECK(cryptographer);
+  cryptographer_ = cryptographer.Pass();
 
-void ModelTypeSyncWorkerImpl::OnCryptographerStateChanged() {
-  TryDecryptPendingUpdates();
+  // Update our state and that of the proxy.
+  OnCryptographerUpdated();
 
-  ScopedCryptographerRef scoped_cryptographer_ref;
-  cryptographer_provider_->InitScopedCryptographerRef(
-      &scoped_cryptographer_ref);
-  Cryptographer* cryptographer = scoped_cryptographer_ref.Get();
-  if (CanCommitItems(cryptographer))
+  // Nudge the scheduler if we're now allowed to commit.
+  if (CanCommitItems())
     nudge_handler_->NudgeForCommit(type_);
 }
 
@@ -109,12 +103,6 @@ SyncerError ModelTypeSyncWorkerImpl::ProcessGetUpdatesResponse(
   data_type_state_.type_context = mutated_context;
   data_type_state_.progress_marker = progress_marker;
 
-  ScopedCryptographerRef scoped_cryptographer_ref;
-  cryptographer_provider_->InitScopedCryptographerRef(
-      &scoped_cryptographer_ref);
-  Cryptographer* cryptographer = scoped_cryptographer_ref.Get();
-  DCHECK(cryptographer);
-
   UpdateResponseDataList response_datas;
   UpdateResponseDataList pending_updates;
 
@@ -166,17 +154,18 @@ SyncerError ModelTypeSyncWorkerImpl::ProcessGetUpdatesResponse(
         entity_tracker->ReceiveUpdate(update_entity->version());
         response_data.specifics = specifics;
         response_datas.push_back(response_data);
-      } else if (specifics.has_encrypted() &&
-                 cryptographer->CanDecrypt(specifics.encrypted())) {
+      } else if (specifics.has_encrypted() && cryptographer_ &&
+                 cryptographer_->CanDecrypt(specifics.encrypted())) {
         // Encrypted, but we know the key.
         if (DecryptSpecifics(
-                cryptographer, specifics, &response_data.specifics)) {
+                cryptographer_.get(), specifics, &response_data.specifics)) {
           entity_tracker->ReceiveUpdate(update_entity->version());
           response_data.encryption_key_name = specifics.encrypted().key_name();
           response_datas.push_back(response_data);
         }
       } else if (specifics.has_encrypted() &&
-                 !cryptographer->CanDecrypt(specifics.encrypted())) {
+                 (!cryptographer_ ||
+                  !cryptographer_->CanDecrypt(specifics.encrypted()))) {
         // Can't decrypt right now.  Ask the entity tracker to handle it.
         response_data.specifics = specifics;
         if (entity_tracker->ReceivePendingUpdate(response_data)) {
@@ -188,6 +177,12 @@ SyncerError ModelTypeSyncWorkerImpl::ProcessGetUpdatesResponse(
     }
   }
 
+  DVLOG(1) << ModelTypeToString(type_) << ": "
+           << base::StringPrintf(
+                  "Delivering %zd applicable and %zd pending updates.",
+                  response_datas.size(),
+                  pending_updates.size());
+
   // Forward these updates to the model thread so it can do the rest.
   type_sync_proxy_->OnUpdateReceived(
       data_type_state_, response_datas, pending_updates);
@@ -202,6 +197,8 @@ void ModelTypeSyncWorkerImpl::ApplyUpdates(sessions::StatusController* status) {
   // cycle, we should update our state so the ModelTypeSyncProxy knows that
   // it's safe to commit items now.
   if (!data_type_state_.initial_sync_done) {
+    DVLOG(1) << "Delivering 'initial sync done' ping.";
+
     data_type_state_.initial_sync_done = true;
 
     type_sync_proxy_->OnUpdateReceived(
@@ -230,11 +227,7 @@ void ModelTypeSyncWorkerImpl::EnqueueForCommit(
     StorePendingCommit(*it);
   }
 
-  ScopedCryptographerRef scoped_cryptographer_ref;
-  cryptographer_provider_->InitScopedCryptographerRef(
-      &scoped_cryptographer_ref);
-  Cryptographer* cryptographer = scoped_cryptographer_ref.Get();
-  if (CanCommitItems(cryptographer))
+  if (CanCommitItems())
     nudge_handler_->NudgeForCommit(type_);
 }
 
@@ -247,12 +240,7 @@ scoped_ptr<CommitContribution> ModelTypeSyncWorkerImpl::GetContribution(
   std::vector<int64> sequence_numbers;
   google::protobuf::RepeatedPtrField<sync_pb::SyncEntity> commit_entities;
 
-  ScopedCryptographerRef scoped_cryptographer_ref;
-  cryptographer_provider_->InitScopedCryptographerRef(
-      &scoped_cryptographer_ref);
-  Cryptographer* cryptographer = scoped_cryptographer_ref.Get();
-
-  if (!CanCommitItems(cryptographer))
+  if (!CanCommitItems())
     return scoped_ptr<CommitContribution>();
 
   // TODO(rlarocque): Avoid iterating here.
@@ -265,7 +253,7 @@ scoped_ptr<CommitContribution> ModelTypeSyncWorkerImpl::GetContribution(
       int64 sequence_number = -1;
 
       entity->PrepareCommitProto(commit_entity, &sequence_number);
-      HelpInitializeCommitEntity(cryptographer, commit_entity);
+      HelpInitializeCommitEntity(commit_entity);
       sequence_numbers.push_back(sequence_number);
 
       space_remaining--;
@@ -350,17 +338,15 @@ bool ModelTypeSyncWorkerImpl::IsTypeInitialized() const {
          data_type_state_.initial_sync_done;
 }
 
-bool ModelTypeSyncWorkerImpl::CanCommitItems(
-    Cryptographer* cryptographer) const {
+bool ModelTypeSyncWorkerImpl::CanCommitItems() const {
   // We can't commit anything until we know the type's parent node.
   // We'll get it in the first update response.
   if (!IsTypeInitialized())
     return false;
 
   // Don't commit if we should be encrypting but don't have the required keys.
-  if (IsEncryptionRequired() && (!cryptographer || !cryptographer->is_ready() ||
-                                 cryptographer->GetDefaultNigoriKeyName() !=
-                                     data_type_state_.encryption_key_name)) {
+  if (IsEncryptionRequired() &&
+      (!cryptographer_ || !cryptographer_->is_ready())) {
     return false;
   }
 
@@ -368,8 +354,9 @@ bool ModelTypeSyncWorkerImpl::CanCommitItems(
 }
 
 void ModelTypeSyncWorkerImpl::HelpInitializeCommitEntity(
-    Cryptographer* cryptographer,
     sync_pb::SyncEntity* sync_entity) {
+  DCHECK(CanCommitItems());
+
   // Initial commits need our help to generate a client ID.
   if (!sync_entity->has_id_string()) {
     DCHECK_EQ(kUncommittedVersion, sync_entity->version());
@@ -380,9 +367,12 @@ void ModelTypeSyncWorkerImpl::HelpInitializeCommitEntity(
 
   // Encrypt the specifics and hide the title if necessary.
   if (IsEncryptionRequired()) {
+    // IsEncryptionRequired() && CanCommitItems() implies
+    // that the cryptographer is valid and ready to encrypt.
     sync_pb::EntitySpecifics encrypted_specifics;
-    cryptographer->Encrypt(sync_entity->specifics(),
-                           encrypted_specifics.mutable_encrypted());
+    bool result = cryptographer_->Encrypt(
+        sync_entity->specifics(), encrypted_specifics.mutable_encrypted());
+    DCHECK(result);
     sync_entity->mutable_specifics()->CopyFrom(encrypted_specifics);
     sync_entity->set_name("encrypted");
   }
@@ -395,14 +385,21 @@ void ModelTypeSyncWorkerImpl::HelpInitializeCommitEntity(
   sync_entity->set_parent_id_string(data_type_state_.type_root_id);
 }
 
-void ModelTypeSyncWorkerImpl::TryDecryptPendingUpdates() {
+void ModelTypeSyncWorkerImpl::OnCryptographerUpdated() {
+  DCHECK(cryptographer_);
+
+  bool new_encryption_key = false;
   UpdateResponseDataList response_datas;
 
-  ScopedCryptographerRef scoped_cryptographer_ref;
-  cryptographer_provider_->InitScopedCryptographerRef(
-      &scoped_cryptographer_ref);
-  Cryptographer* cryptographer = scoped_cryptographer_ref.Get();
-  DCHECK(cryptographer);
+  const std::string& new_key_name = cryptographer_->GetDefaultNigoriKeyName();
+
+  // Handle a change in encryption key.
+  if (data_type_state_.encryption_key_name != new_key_name) {
+    DVLOG(1) << ModelTypeToString(type_) << ": Updating encryption key "
+             << data_type_state_.encryption_key_name << " -> " << new_key_name;
+    data_type_state_.encryption_key_name = new_key_name;
+    new_encryption_key = true;
+  }
 
   for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end();
        ++it) {
@@ -413,9 +410,9 @@ void ModelTypeSyncWorkerImpl::TryDecryptPendingUpdates() {
       // don't have the key.
       DCHECK(saved_pending.specifics.has_encrypted());
 
-      if (cryptographer->CanDecrypt(saved_pending.specifics.encrypted())) {
+      if (cryptographer_->CanDecrypt(saved_pending.specifics.encrypted())) {
         UpdateResponseData decrypted_response = saved_pending;
-        if (DecryptSpecifics(cryptographer,
+        if (DecryptSpecifics(cryptographer_.get(),
                              saved_pending.specifics,
                              &decrypted_response.specifics)) {
           decrypted_response.encryption_key_name =
@@ -428,7 +425,11 @@ void ModelTypeSyncWorkerImpl::TryDecryptPendingUpdates() {
     }
   }
 
-  if (!response_datas.empty()) {
+  if (new_encryption_key || response_datas.size() > 0) {
+    DVLOG(1) << ModelTypeToString(type_) << ": "
+             << base::StringPrintf(
+                    "Delivering encryption key and %zd decrypted updates.",
+                    response_datas.size());
     type_sync_proxy_->OnUpdateReceived(
         data_type_state_, response_datas, UpdateResponseDataList());
   }
diff --git a/sync/engine/model_type_sync_worker_impl.h b/sync/engine/model_type_sync_worker_impl.h
index f2df976..10aa080 100644
--- a/sync/engine/model_type_sync_worker_impl.h
+++ b/sync/engine/model_type_sync_worker_impl.h
@@ -10,7 +10,6 @@
 #include "base/threading/non_thread_safe.h"
 #include "sync/base/sync_export.h"
 #include "sync/engine/commit_contributor.h"
-#include "sync/engine/cryptographer_provider.h"
 #include "sync/engine/model_type_sync_worker.h"
 #include "sync/engine/nudge_handler.h"
 #include "sync/engine/update_handler.h"
@@ -18,6 +17,7 @@
 #include "sync/internal_api/public/non_blocking_sync_common.h"
 #include "sync/internal_api/public/sync_encryption_handler.h"
 #include "sync/protocol/sync.pb.h"
+#include "sync/util/cryptographer.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -56,7 +56,7 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler,
   ModelTypeSyncWorkerImpl(ModelType type,
                           const DataTypeState& initial_state,
                           const UpdateResponseDataList& saved_pending_updates,
-                          CryptographerProvider* cryptographer_provider,
+                          scoped_ptr<Cryptographer> cryptographer,
                           NudgeHandler* nudge_handler,
                           scoped_ptr<ModelTypeSyncProxy> type_sync_proxy);
   virtual ~ModelTypeSyncWorkerImpl();
@@ -64,9 +64,7 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler,
   ModelType GetModelType() const;
 
   bool IsEncryptionRequired() const;
-  void SetEncryptionKeyName(const std::string& name);
-
-  void OnCryptographerStateChanged();
+  void UpdateCryptographer(scoped_ptr<Cryptographer> cryptographer);
 
   // UpdateHandler implementation.
   virtual void GetDownloadProgress(
@@ -109,19 +107,19 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler,
   // Returns true if this type is prepared to commit items.  Currently, this
   // depends on having downloaded the initial data and having the encryption
   // settings in a good state.
-  bool CanCommitItems(Cryptographer* cryptographer) const;
+  bool CanCommitItems() const;
 
   // Initializes the parts of a commit entity that are the responsibility of
   // this class, and not the EntityTracker.  Some fields, like the
   // client-assigned ID, can only be set by an entity with knowledge of the
   // entire data type's state.
-  void HelpInitializeCommitEntity(Cryptographer* cryptographer,
-                                  sync_pb::SyncEntity* commit_entity);
+  void HelpInitializeCommitEntity(sync_pb::SyncEntity* commit_entity);
 
   // Attempts to decrypt pending updates stored in the EntityMap.  If
   // successful, will remove the update from the its EntityTracker and forward
-  // it to the proxy thread for application.
-  void TryDecryptPendingUpdates();
+  // it to the proxy thread for application.  Will forward any new encryption
+  // keys to the proxy to trigger re-encryption if necessary.
+  void OnCryptographerUpdated();
 
   // Attempts to decrypt the given specifics and return them in the |out|
   // parameter.  Assumes cryptographer->CanDecrypt(specifics) returned true.
@@ -145,9 +143,10 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler,
   // This is NULL when no proxy is connected..
   scoped_ptr<ModelTypeSyncProxy> type_sync_proxy_;
 
-  // A helper to provide access to the syncable::Directory's cryptographer.
-  // Not owned.
-  CryptographerProvider* cryptographer_provider_;
+  // A private copy of the most recent cryptographer known to sync.
+  // Initialized at construction time and updated with UpdateCryptographer().
+  // NULL if encryption is not enabled for this type.
+  scoped_ptr<Cryptographer> cryptographer_;
 
   // Interface used to access and send nudges to the sync scheduler.  Not owned.
   NudgeHandler* nudge_handler_;
diff --git a/sync/engine/model_type_sync_worker_impl_unittest.cc b/sync/engine/model_type_sync_worker_impl_unittest.cc
index df2ed66..5c62c9a 100644
--- a/sync/engine/model_type_sync_worker_impl_unittest.cc
+++ b/sync/engine/model_type_sync_worker_impl_unittest.cc
@@ -14,7 +14,6 @@
 #include "sync/syncable/syncable_util.h"
 #include "sync/test/engine/mock_model_type_sync_proxy.h"
 #include "sync/test/engine/mock_nudge_handler.h"
-#include "sync/test/engine/simple_cryptographer_provider.h"
 #include "sync/test/engine/single_type_mock_server.h"
 #include "sync/test/fake_encryptor.h"
 
@@ -40,6 +39,7 @@ namespace syncer {
 // - Commit requests from the model thread.
 // - Update responses from the server.
 // - Commit responses from the server.
+// - The cryptographer, if encryption is enabled.
 //
 // Outputs:
 // - Commit requests to the server.
@@ -168,6 +168,11 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test {
   // Returns the number of initial sync nudges sent to the mock nudge handler.
   int GetNumInitialDownloadNudges() const;
 
+  // Returns the name of the encryption key in the cryptographer last passed to
+  // the ModelTypeSyncWorker.  Returns an empty string if no crypgorapher is
+  // in use.  See also: UpdateLocalCryptographer().
+  std::string GetLocalCryptographerKeyName() const;
+
   // Helpers for building various messages and structures.
   static std::string GenerateTagHash(const std::string& tag);
   static sync_pb::EntitySpecifics GenerateSpecifics(const std::string& tag,
@@ -192,11 +197,8 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test {
   // An encryptor for our cryptographer.
   FakeEncryptor fake_encryptor_;
 
-  // The cryptographer itself.
-  Cryptographer cryptographer_;
-
-  // A CryptographerProvider for the ModelTypeSyncWorkerImpl.
-  SimpleCryptographerProvider cryptographer_provider_;
+  // The cryptographer itself.  NULL if we're not encrypting the type.
+  scoped_ptr<Cryptographer> cryptographer_;
 
   // The number of the most recent foreign encryption key known to our
   // cryptographer.  Note that not all of these will be decryptable.
@@ -224,9 +226,7 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test {
 };
 
 ModelTypeSyncWorkerImplTest::ModelTypeSyncWorkerImplTest()
-    : cryptographer_(&fake_encryptor_),
-      cryptographer_provider_(&cryptographer_),
-      foreign_encryption_key_index_(0),
+    : foreign_encryption_key_index_(0),
       update_encryption_filter_index_(0),
       mock_type_sync_proxy_(NULL),
       mock_server_(kModelType) {
@@ -273,15 +273,24 @@ void ModelTypeSyncWorkerImplTest::InitializeWithState(
   mock_type_sync_proxy_ = new MockModelTypeSyncProxy();
   scoped_ptr<ModelTypeSyncProxy> proxy(mock_type_sync_proxy_);
 
+  scoped_ptr<Cryptographer> cryptographer_copy;
+  if (cryptographer_) {
+    cryptographer_copy.reset(new Cryptographer(*cryptographer_));
+  }
+
   worker_.reset(new ModelTypeSyncWorkerImpl(kModelType,
                                             state,
                                             initial_pending_updates,
-                                            &cryptographer_provider_,
+                                            cryptographer_copy.Pass(),
                                             &mock_nudge_handler_,
                                             proxy.Pass()));
 }
 
 void ModelTypeSyncWorkerImplTest::NewForeignEncryptionKey() {
+  if (!cryptographer_) {
+    cryptographer_.reset(new Cryptographer(&fake_encryptor_));
+  }
+
   foreign_encryption_key_index_++;
 
   sync_pb::NigoriKeyBag bag;
@@ -314,20 +323,29 @@ void ModelTypeSyncWorkerImplTest::NewForeignEncryptionKey() {
   last_nigori.Encrypt(serialized_bag, encrypted.mutable_blob());
 
   // Update the cryptographer with new pending keys.
-  cryptographer_.SetPendingKeys(encrypted);
+  cryptographer_->SetPendingKeys(encrypted);
 
-  // Update the worker with the latest encryption key name.
-  if (worker_)
-    worker_->SetEncryptionKeyName(encrypted.key_name());
+  // Update the worker with the latest cryptographer.
+  if (worker_) {
+    worker_->UpdateCryptographer(
+        make_scoped_ptr<Cryptographer>(new Cryptographer(*cryptographer_)));
+  }
 }
 
 void ModelTypeSyncWorkerImplTest::UpdateLocalCryptographer() {
+  if (!cryptographer_) {
+    cryptographer_.reset(new Cryptographer(&fake_encryptor_));
+  }
+
   KeyParams params = GetNthKeyParams(foreign_encryption_key_index_);
-  bool success = cryptographer_.DecryptPendingKeys(params);
+  bool success = cryptographer_->DecryptPendingKeys(params);
   DCHECK(success);
 
-  if (worker_)
-    worker_->OnCryptographerStateChanged();
+  // Update the worker with the latest cryptographer.
+  if (worker_) {
+    worker_->UpdateCryptographer(
+        make_scoped_ptr<Cryptographer>(new Cryptographer(*cryptographer_)));
+  }
 }
 
 void ModelTypeSyncWorkerImplTest::SetUpdateEncryptionFilter(int n) {
@@ -558,6 +576,14 @@ int ModelTypeSyncWorkerImplTest::GetNumInitialDownloadNudges() const {
   return mock_nudge_handler_.GetNumInitialDownloadNudges();
 }
 
+std::string ModelTypeSyncWorkerImplTest::GetLocalCryptographerKeyName() const {
+  if (!cryptographer_) {
+    return std::string();
+  }
+
+  return cryptographer_->GetDefaultNigoriKeyName();
+}
+
 // static.
 std::string ModelTypeSyncWorkerImplTest::GenerateTagHash(
     const std::string& tag) {
@@ -829,9 +855,15 @@ TEST_F(ModelTypeSyncWorkerImplTest, ReceiveUpdates) {
 TEST_F(ModelTypeSyncWorkerImplTest, EncryptedCommit) {
   NormalInitialize();
 
+  ASSERT_EQ(0U, GetNumModelThreadUpdateResponses());
+
   NewForeignEncryptionKey();
   UpdateLocalCryptographer();
 
+  ASSERT_EQ(1U, GetNumModelThreadUpdateResponses());
+  EXPECT_EQ(GetLocalCryptographerKeyName(),
+            GetNthModelThreadUpdateState(0).encryption_key_name);
+
   // Normal commit request stuff.
   CommitRequest("tag1", "value1");
   DoSuccessfulCommit();
@@ -917,27 +949,42 @@ TEST_F(ModelTypeSyncWorkerImplTest, ReceiveDecryptableEntities) {
   EXPECT_FALSE(update2.encryption_key_name.empty());
 }
 
+// Test initializing a ModelTypeSyncWorker with a cryptographer at startup.
+TEST_F(ModelTypeSyncWorkerImplTest, InitializeWithCryptographer) {
+  // Set up some encryption state.
+  NewForeignEncryptionKey();
+  UpdateLocalCryptographer();
+
+  // Then initialize.
+  NormalInitialize();
+
+  // The worker should tell the model thread about encryption as soon as
+  // possible, so that it will have the chance to re-encrypt local data if
+  // necessary.
+  ASSERT_EQ(1U, GetNumModelThreadUpdateResponses());
+  EXPECT_EQ(GetLocalCryptographerKeyName(),
+            GetNthModelThreadUpdateState(0).encryption_key_name);
+}
+
 // Receive updates that are initially undecryptable, then ensure they get
 // delivered to the model thread when decryption becomes possible.
 TEST_F(ModelTypeSyncWorkerImplTest, ReceiveUndecryptableEntries) {
   NormalInitialize();
 
-  // Set a new encryption key.  The model thread will be notified of the new
-  // encryption key through a faked update response.
+  // Receive a new foreign encryption key that we can't decrypt.
   NewForeignEncryptionKey();
-  EXPECT_EQ(1U, GetNumModelThreadUpdateResponses());
 
-  // Send an update using that new key.
+  // Receive an encrypted with that new key, which we can't access.
   SetUpdateEncryptionFilter(1);
   TriggerUpdateFromServer(10, "tag1", "value1");
 
   // At this point, the cryptographer does not have access to the key, so the
   // updates will be undecryptable.  They'll be transfered to the model thread
   // for safe-keeping as pending updates.
-  ASSERT_EQ(2U, GetNumModelThreadUpdateResponses());
-  UpdateResponseDataList updates_list = GetNthModelThreadUpdateResponse(1);
+  ASSERT_EQ(1U, GetNumModelThreadUpdateResponses());
+  UpdateResponseDataList updates_list = GetNthModelThreadUpdateResponse(0);
   EXPECT_EQ(0U, updates_list.size());
-  UpdateResponseDataList pending_updates = GetNthModelThreadPendingUpdates(1);
+  UpdateResponseDataList pending_updates = GetNthModelThreadPendingUpdates(0);
   EXPECT_EQ(1U, pending_updates.size());
 
   // The update will be delivered as soon as decryption becomes possible.