[Sync] Refactor GetEncryptedTypes usage.

sync/internal_api/sync_encryption_handler_impl.h

Index: sync/internal_api/sync_encryption_handler_impl.h
diff --git a/sync/internal_api/sync_encryption_handler_impl.h b/sync/internal_api/sync_encryption_handler_impl.h
index 6c605a88cc9f05c83db379d8baa9a8f3689424c0..50fd15ca7a1a5dc3f2ff7011da2577ecb9b1d5af 100644
--- a/sync/internal_api/sync_encryption_handler_impl.h
+++ b/sync/internal_api/sync_encryption_handler_impl.h
@@ -9,18 +9,48 @@
 
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
+#include "base/threading/thread_checker.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "sync/internal_api/public/sync_encryption_handler.h"
+#include "sync/internal_api/public/user_share.h"
 #include "sync/syncable/nigori_handler.h"
+#include "sync/util/cryptographer.h"
 
 namespace syncer {
 
+class Encryptor;
 struct UserShare;
 class WriteNode;
 class WriteTransaction;
 
+// Helper class for ensuring non-thread-safe objects are only accessed
+// while a valid transaction is open. Does not take ownership, so the held
+// object must live longer than the transaction holder.
+template <typename T>
+class TransactionalHolder {

[tim (not reviewing), 2012/08/23 18:38:10]

I don't think this needs to be exposed outside of the class below.

Also does the templating buy us much?  It requires that you don't access the
wrong member variable (since the unsafe_ ones are exposed as well). 

How about having a private struct (call it Vault, or something, like we used to
have in SyncerThread) that houses all of the unsafe_ variables, and then having
an internal accessor to get at the struct that performs the DCHECKs you want. It
still has the downside of "you might access illegally", but the paradigm of
going through an accessor with DCHECKs is a bit more common / obvious.

[Nicolas Zea, 2012/08/23 22:49:32]

Yeah, I guess they're pretty much the same. Done.

+ public:
+  TransactionalHolder(UserShare* user_share, T* obj)
+      : user_share_(user_share),
+        obj_(obj) {}
+  ~TransactionalHolder() {}
+
+  // Given an open transaction |trans| that matches the directory in
+  // |user_share_|, returns the held object.
+  const T& Get(syncable::BaseTransaction* const trans) const;
+  T* GetMutable(syncable::BaseTransaction* const trans);
+
+ private:
+  // The current user share. Used to enforce the transaction belongs to this
+  // sync profile.
+  const UserShare* user_share_;
+
+  // The object being held. Can only be publicly accessed while a transaction
+  // is open.
+  T* obj_;
+};
+
 // Sync encryption handler implementation.
 //
 // This class acts as the respository of all sync encryption state, and handles
@@ -35,16 +65,14 @@ class WriteTransaction;
 // Note: See sync_encryption_handler.h for a description of the chrome visible
 // methods and what they do, and nigori_handler.h for a description of the
 // sync methods.
-//
-// TODO(zea): Make this class explicitly non-thread safe and ensure its only
-// accessed from the sync thread, with the possible exception of
-// GetEncryptedTypes. Need to cache explicit passphrase state on the UI thread.
+// All methods are non-thread-safe and should only be called from the sync
+// thread unless explicitly noted otherwise.
 class SyncEncryptionHandlerImpl
     : public SyncEncryptionHandler,
       public syncable::NigoriHandler {
  public:
   SyncEncryptionHandlerImpl(UserShare* user_share,
-                            Cryptographer* cryptographer);
+                            Encryptor* encryptor);
   virtual ~SyncEncryptionHandlerImpl();
 
   // SyncEncryptionHandler implementation.
@@ -56,6 +84,8 @@ class SyncEncryptionHandlerImpl
   virtual void SetDecryptionPassphrase(const std::string& passphrase) OVERRIDE;
   virtual void EnableEncryptEverything() OVERRIDE;
   virtual bool EncryptEverythingEnabled() const OVERRIDE;
+  // Can be called from any thread.
+  // TODO(zea): enforce this is only called on sync thread.
   virtual bool IsUsingExplicitPassphrase() const OVERRIDE;
 
   // NigoriHandler implementation.
@@ -66,7 +96,13 @@ class SyncEncryptionHandlerImpl
   virtual void UpdateNigoriFromEncryptedTypes(
       sync_pb::NigoriSpecifics* nigori,
       syncable::BaseTransaction* const trans) const OVERRIDE;
-  virtual ModelTypeSet GetEncryptedTypes() const OVERRIDE;
+  // Can be called from any thread.
+  virtual ModelTypeSet GetEncryptedTypes(
+      syncable::BaseTransaction* const trans) const OVERRIDE;
+
+  // Getters.
+  Cryptographer* cryptographer_unsafe() { return &cryptographer_unsafe_; }
+  ModelTypeSet encrypted_types_unsafe() { return encrypted_types_unsafe_; }
 
  private:
   FRIEND_TEST_ALL_PREFIXES(SyncEncryptionHandlerImplTest,
@@ -103,7 +139,9 @@ class SyncEncryptionHandlerImpl
   // had stricter encryption than |nigori|, and the nigori node needs to be
   // updated with the newer encryption state.
   // Note: must be called from within a transaction.
-  bool UpdateEncryptedTypesFromNigori(const sync_pb::NigoriSpecifics& nigori);
+  bool UpdateEncryptedTypesFromNigori(
+      const sync_pb::NigoriSpecifics& nigori,
+      syncable::BaseTransaction* const trans);
 
   // The final step of SetEncryptionPassphrase and SetDecryptionPassphrase that
   // notifies observers of the result of the set passphrase operation, updates
@@ -125,7 +163,10 @@ class SyncEncryptionHandlerImpl
   // Merges the given set of encrypted types with the existing set and emits a
   // notification if necessary.
   // Note: must be called from within a transaction.
-  void MergeEncryptedTypes(ModelTypeSet encrypted_types);
+  void MergeEncryptedTypes(ModelTypeSet new_encrypted_types,
+                           syncable::BaseTransaction* const trans);
+
+  base::ThreadChecker thread_checker_;
 
   base::WeakPtrFactory<SyncEncryptionHandlerImpl> weak_ptr_factory_;
 
@@ -134,18 +175,24 @@ class SyncEncryptionHandlerImpl
   // The current user share (for creating transactions).
   UserShare* user_share_;
 
-  // TODO(zea): have the sync encryption handler own the cryptographer, and live
-  // in the directory.
-  Cryptographer* cryptographer_;
+  // Sync encryption state that is accessed from any thread. Must only be
+  // accessed while holding a transaction.
+  // Sync's cryptographer.
+  Cryptographer cryptographer_unsafe_;
+  // The set of types that require encryption.
+  ModelTypeSet encrypted_types_unsafe_;
 
-  // The set of types that require encryption. This is accessed on all sync
-  // datatype threads when we write to a node, so we must hold a transaction
-  // whenever we touch/read it.
-  ModelTypeSet encrypted_types_;
+  // Helpers to enforce thread safety.
+  // Sync's cryptographer (wrapped within a transactional holder).
+  TransactionalHolder<Cryptographer> cryptographer_holder_;
+  // Sync's set of encrypted types (wrapped within a transactional holder).
+  TransactionalHolder<ModelTypeSet> encrypted_types_holder_;
 
-  // Sync encryption state. These are only modified and accessed from the sync
+  // Sync encryption state that is only modified and accessed from the sync
   // thread.
+  // Whether all current and future types should be encrypted.
   bool encrypt_everything_;
+  // Whether the user is using a custom passphrase for encryption.
   bool explicit_passphrase_;
 
   // The number of times we've automatically (i.e. not via SetPassphrase or