[Sync] Refactor GetEncryptedTypes usage.

The cryptographer is now owned by the SyncEncryptionHandlerImpl, and
no longer needs any connection to the nigori handler or encrypted types.
Those are accessed via the directory, which now has a GetNigoriHandler
method.

Because of this, we can now clean up the thread safety guarantees in the
sync encryption handler impl, enforcing that everything except
GetEncryptedTypes (and IsUsingExplicitPassphrase, which will be fixed in a
followup patch) are invoked on the syncer thread. This lets us not have to
open unnecessary transactions.

At the chrome layer, encrypted types are accessed via BaseTransaction::
GetEncryptedTypes(). At the syncer layer, they're accessed via directory::
GetNigoriHandler()->GetEncryptedTypes(BaseTransaction* trans const).

BUG=142476,139848


Review URL: https://chromiumcodereview.appspot.com/10844005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@153335 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 53 insertions, 43 deletions

diff --git a/sync/engine/apply_updates_command_unittest.cc b/sync/engine/apply_updates_command_unittest.cc
index 91fa259..5e14c59 100644
--- a/sync/engine/apply_updates_command_unittest.cc
+++ b/sync/engine/apply_updates_command_unittest.cc
@@ -65,22 +65,14 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest {
     SyncerCommandTest::SetUp();
     entry_factory_.reset(new TestEntryFactory(directory()));
     ExpectNoGroupsToChange(apply_updates_command_);
-
-    syncable::ReadTransaction trans(FROM_HERE, directory());
-    directory()->GetCryptographer(&trans)->SetNigoriHandler(
-        &fake_encryption_handler_);
-    fake_encryption_handler_.set_cryptographer(
-        directory()->GetCryptographer(&trans));
   }
 
  protected:
   DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesCommandTest);
 
   ApplyUpdatesCommand apply_updates_command_;
-  FakeEncryptor encryptor_;
   TestIdFactory id_factory_;
   scoped_ptr<TestEntryFactory> entry_factory_;
-  FakeSyncEncryptionHandler fake_encryption_handler_;
 };
 
 TEST_F(ApplyUpdatesCommandTest, Simple) {
@@ -466,6 +458,7 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) {
 }
 
 TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {
+  Cryptographer* cryptographer;
   // Only decryptable password updates should be applied.
   {
     sync_pb::EntitySpecifics specifics;
@@ -473,7 +466,7 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {
     data.set_origin("http://example.com/1");
     {
       syncable::ReadTransaction trans(FROM_HERE, directory());
-      Cryptographer* cryptographer = directory()->GetCryptographer(&trans);
+      cryptographer = directory()->GetCryptographer(&trans);
 
       KeyParams params = {"localhost", "dummy", "foobar"};
       cryptographer->AddKey(params);
@@ -485,15 +478,15 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {
   }
   {
     // Create a new cryptographer, independent of the one in the session.
-    Cryptographer cryptographer(&encryptor_);
+    Cryptographer other_cryptographer(cryptographer->encryptor());
     KeyParams params = {"localhost", "dummy", "bazqux"};
-    cryptographer.AddKey(params);
+    other_cryptographer.AddKey(params);
 
     sync_pb::EntitySpecifics specifics;
     sync_pb::PasswordSpecificsData data;
     data.set_origin("http://example.com/2");
 
-    cryptographer.Encrypt(data,
+    other_cryptographer.Encrypt(data,
         specifics.mutable_password()->mutable_encrypted());
     entry_factory_->CreateUnappliedNewItem("item2", specifics, false);
   }
@@ -532,11 +525,12 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) {
   {
     syncable::ReadTransaction trans(FROM_HERE, directory());
     cryptographer = directory()->GetCryptographer(&trans);
-    EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(encrypted_types));
   }
 
   // Nigori node updates should update the Cryptographer.
-  Cryptographer other_cryptographer(&encryptor_);
+  Cryptographer other_cryptographer(cryptographer->encryptor());
   KeyParams params = {"localhost", "dummy", "foobar"};
   other_cryptographer.AddKey(params);
 
@@ -564,7 +558,11 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) {
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
-  EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All()));
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(ModelTypeSet::All()));
+  }
 }
 
 TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) {
@@ -577,11 +575,12 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) {
   {
     syncable::ReadTransaction trans(FROM_HERE, directory());
     cryptographer = directory()->GetCryptographer(&trans);
-    EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(encrypted_types));
   }
 
   // Nigori node updates should update the Cryptographer.
-  Cryptographer other_cryptographer(&encryptor_);
+  Cryptographer other_cryptographer(cryptographer->encryptor());
   KeyParams params = {"localhost", "dummy", "foobar"};
   other_cryptographer.AddKey(params);
 
@@ -609,7 +608,11 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) {
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
-  EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All()));
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(ModelTypeSet::All()));
+  }
 }
 
 // Create some local unsynced and unencrypted data. Apply a nigori update that
@@ -627,7 +630,8 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
   {
     syncable::ReadTransaction trans(FROM_HERE, directory());
     cryptographer = directory()->GetCryptographer(&trans);
-    EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(encrypted_types));
 
     // With default encrypted_types, this should be true.
     EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
@@ -704,8 +708,8 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
 
     // If ProcessUnsyncedChangesForEncryption worked, all our unsynced changes
     // should be encrypted now.
-    EXPECT_TRUE(ModelTypeSet::All().Equals(
-        cryptographer->GetEncryptedTypes()));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(ModelTypeSet::All()));
     EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
 
     Syncer::UnsyncedMetaHandles handles;
@@ -744,8 +748,8 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
     syncable::ReadTransaction trans(FROM_HERE, directory());
 
     // All our changes should still be encrypted.
-    EXPECT_TRUE(ModelTypeSet::All().Equals(
-        cryptographer->GetEncryptedTypes()));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(ModelTypeSet::All()));
     EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
 
     Syncer::UnsyncedMetaHandles handles;
@@ -764,7 +768,8 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) {
   {
     syncable::ReadTransaction trans(FROM_HERE, directory());
     cryptographer = directory()->GetCryptographer(&trans);
-    EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(encrypted_types));
 
     // With default encrypted_types, this should be true.
     EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
@@ -798,7 +803,7 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) {
 
   // We encrypt with new keys, triggering the local cryptographer to be unready
   // and unable to decrypt data (once updated).
-  Cryptographer other_cryptographer(&encryptor_);
+  Cryptographer other_cryptographer(cryptographer->encryptor());
   KeyParams params = {"localhost", "dummy", "foobar"};
   other_cryptographer.AddKey(params);
   sync_pb::EntitySpecifics specifics;
@@ -844,8 +849,8 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) {
     // Since we have pending keys, we would have failed to encrypt, but the
     // cryptographer should be updated.
     EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
-    EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(
-        ModelTypeSet().All()));
+    EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans)
+        .Equals(ModelTypeSet::All()));
     EXPECT_FALSE(cryptographer->is_ready());
     EXPECT_TRUE(cryptographer->has_pending_keys());
 
diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc
index 5912e46..736e668 100644
--- a/sync/engine/conflict_resolver.cc
+++ b/sync/engine/conflict_resolver.cc
@@ -17,6 +17,7 @@
 #include "sync/sessions/status_controller.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/mutable_entry.h"
+#include "sync/syncable/nigori_handler.h"
 #include "sync/syncable/write_transaction.h"
 #include "sync/util/cryptographer.h"
 
@@ -229,7 +230,9 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
       sync_pb::NigoriSpecifics* server_nigori = specifics.mutable_nigori();
       // Store the merged set of encrypted types (cryptographer->Update(..) will
       // have merged the local types already).
-      cryptographer->UpdateNigoriFromEncryptedTypes(server_nigori, trans);
+      trans->directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes(
+          server_nigori,
+          trans);
       // The cryptographer has the both the local and remote encryption keys
       // (added at cryptographer->Update(..) time).
       // If the cryptographer is ready, then it already merged both sets of keys
diff --git a/sync/engine/download_updates_command.cc b/sync/engine/download_updates_command.cc
index 0c413b6..fea8f88 100644
--- a/sync/engine/download_updates_command.cc
+++ b/sync/engine/download_updates_command.cc
@@ -12,6 +12,7 @@
 #include "sync/internal_api/public/base/model_type_state_map.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/read_transaction.h"
+#include "sync/util/cryptographer.h"
 
 using sync_pb::DebugInfo;
 
diff --git a/sync/engine/get_commit_ids_command.cc b/sync/engine/get_commit_ids_command.cc
index a7a52dd..bb6d228 100644
--- a/sync/engine/get_commit_ids_command.cc
+++ b/sync/engine/get_commit_ids_command.cc
@@ -12,6 +12,7 @@
 #include "sync/engine/throttled_data_type_tracker.h"
 #include "sync/syncable/entry.h"
 #include "sync/syncable/mutable_entry.h"
+#include "sync/syncable/nigori_handler.h"
 #include "sync/syncable/nigori_util.h"
 #include "sync/syncable/syncable_util.h"
 #include "sync/syncable/write_transaction.h"
@@ -49,7 +50,8 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
       session->context()->
       directory()->GetCryptographer(session->write_transaction());
   if (cryptographer) {
-    encrypted_types = cryptographer->GetEncryptedTypes();
+    encrypted_types = session->context()->directory()->GetNigoriHandler()->
+        GetEncryptedTypes(session->write_transaction());
     passphrase_missing = cryptographer->has_pending_keys();
   };
 
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 506fa3f..df587a7 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -256,7 +256,6 @@ class SyncerTest : public testing::Test,
     child_id_ = ids_.MakeServer("child id");
     directory()->set_store_birthday(mock_server_->store_birthday());
     mock_server_->SetKeystoreKey("encryption_key");
-    GetCryptographer(&trans)->SetNigoriHandler(&fake_encryption_handler_);
   }
 
   virtual void TearDown() {
@@ -568,8 +567,6 @@ class SyncerTest : public testing::Test,
   ModelTypeSet enabled_datatypes_;
   TrafficRecorder traffic_recorder_;
 
-  FakeSyncEncryptionHandler fake_encryption_handler_;
-
   DISALLOW_COPY_AND_ASSIGN(SyncerTest);
 };
 
@@ -727,7 +724,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersUnreadyEntries) {
     sync_pb::EntitySpecifics specifics;
     sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori();
     other_cryptographer.GetKeys(nigori->mutable_encrypted());
-    fake_encryption_handler_.EnableEncryptEverything();
+    dir_maker_.encryption_handler()->EnableEncryptEverything();
     // Set up with an old passphrase, but have pending keys
     GetCryptographer(&wtrans)->AddKey(key_params);
     GetCryptographer(&wtrans)->Encrypt(bookmark,
@@ -840,7 +837,7 @@ TEST_F(SyncerTest, EncryptionAwareConflicts) {
     sync_pb::EntitySpecifics specifics;
     sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori();
     other_cryptographer.GetKeys(nigori->mutable_encrypted());
-    fake_encryption_handler_.EnableEncryptEverything();
+    dir_maker_.encryption_handler()->EnableEncryptEverything();
     GetCryptographer(&wtrans)->SetPendingKeys(nigori->encrypted());
     EXPECT_TRUE(GetCryptographer(&wtrans)->has_pending_keys());
   }
@@ -1018,8 +1015,8 @@ TEST_F(SyncerTest, NigoriConflicts) {
         our_encrypted_specifics.mutable_encrypted());
     GetCryptographer(&wtrans)->GetKeys(
         nigori->mutable_encrypted());
-    fake_encryption_handler_.EnableEncryptEverything();
-    GetCryptographer(&wtrans)->UpdateNigoriFromEncryptedTypes(
+    dir_maker_.encryption_handler()->EnableEncryptEverything();
+    directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes(
         nigori,
         &wtrans);
     MutableEntry nigori_entry(&wtrans, GET_BY_SERVER_TAG,
@@ -1029,8 +1026,7 @@ TEST_F(SyncerTest, NigoriConflicts) {
     nigori_entry.Put(IS_UNSYNCED, true);
     EXPECT_FALSE(GetCryptographer(&wtrans)->has_pending_keys());
     EXPECT_TRUE(encrypted_types.Equals(
-            GetCryptographer(&wtrans)->GetEncryptedTypes()));
-    fake_encryption_handler_.set_cryptographer(GetCryptographer(&wtrans));
+        directory()->GetNigoriHandler()->GetEncryptedTypes(&wtrans)));
   }
   {
     sync_pb::EntitySpecifics specifics;
@@ -1061,8 +1057,8 @@ TEST_F(SyncerTest, NigoriConflicts) {
     sync_pb::EntitySpecifics specifics = nigori_entry.Get(SPECIFICS);
     ASSERT_TRUE(GetCryptographer(&wtrans)->has_pending_keys());
     EXPECT_TRUE(encrypted_types.Equals(
-        GetCryptographer(&wtrans)->GetEncryptedTypes()));
-    EXPECT_TRUE(fake_encryption_handler_.EncryptEverythingEnabled());
+        directory()->GetNigoriHandler()->GetEncryptedTypes(&wtrans)));
+    EXPECT_TRUE(dir_maker_.encryption_handler()->EncryptEverythingEnabled());
     EXPECT_TRUE(specifics.nigori().using_explicit_passphrase());
     // Supply the pending keys. Afterwards, we should be able to decrypt both
     // our own encrypted data and data encrypted by the other cryptographer,
@@ -1072,7 +1068,9 @@ TEST_F(SyncerTest, NigoriConflicts) {
     EXPECT_FALSE(GetCryptographer(&wtrans)->has_pending_keys());
     sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori();
     GetCryptographer(&wtrans)->GetKeys(nigori->mutable_encrypted());
-    GetCryptographer(&wtrans)->UpdateNigoriFromEncryptedTypes(nigori, &wtrans);
+    directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes(
+        nigori,
+        &wtrans);
     // Normally this would be written as part of SetPassphrase, but we do it
     // manually for the test.
     nigori_entry.Put(SPECIFICS, specifics);
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index ca21f2e..8a243df 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -22,6 +22,7 @@
 #include "sync/syncable/directory.h"
 #include "sync/syncable/entry.h"
 #include "sync/syncable/mutable_entry.h"
+#include "sync/syncable/nigori_handler.h"
 #include "sync/syncable/nigori_util.h"
 #include "sync/syncable/read_transaction.h"
 #include "sync/syncable/syncable_changes_version.h"
@@ -217,7 +218,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
   // the nigori node (e.g. on restart), they will commit without issue.
   if (specifics.has_nigori()) {
     const sync_pb::NigoriSpecifics& nigori = specifics.nigori();
-    cryptographer->ApplyNigoriUpdate(nigori, trans);
+    trans->directory()->GetNigoriHandler()->ApplyNigoriUpdate(nigori, trans);
 
     // Make sure any unsynced changes are properly encrypted as necessary.
     // We only perform this if the cryptographer is ready. If not, these are
@@ -235,7 +236,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
       // If this fails, something is wrong with the cryptographer, but there's
       // nothing we can do about it here.
       DVLOG(1) << "Received new nigori, encrypting unsynced changes.";
-      syncable::ProcessUnsyncedChangesForEncryption(trans, cryptographer);
+      syncable::ProcessUnsyncedChangesForEncryption(trans);
     }
   }