[Sync] Refactor GetEncryptedTypes usage.

sync/internal_api/sync_encryption_handler_impl.h

 // Copyright (c) 2012 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_INTERNAL_API_SYNC_ENCRYPTION_HANDLER_IMPL_H_
 #define SYNC_INTERNAL_API_SYNC_ENCRYPTION_HANDLER_IMPL_H_
 
 #include <string>
 
 #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
 // encryption related changes/queries coming from both the chrome side and
 // the sync side (via NigoriHandler). It is capable of modifying all sync data
 // (re-encryption), updating the encrypted types, changing the encryption keys,
 // and creating/receiving nigori node updates.
 //
 // The class should live as long as the directory itself in order to ensure
 // any data read/written is properly decrypted/encrypted.
 //
 // 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.
   virtual void AddObserver(Observer* observer) OVERRIDE;
   virtual void RemoveObserver(Observer* observer) OVERRIDE;
   virtual void Init() OVERRIDE;
   virtual void SetEncryptionPassphrase(const std::string& passphrase,
                                        bool is_explicit) OVERRIDE;
   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.
   // Note: all methods are invoked while the caller holds a transaction.
   virtual void ApplyNigoriUpdate(
       const sync_pb::NigoriSpecifics& nigori,
       syncable::BaseTransaction* const trans) OVERRIDE;
   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,
                            NigoriEncryptionTypes);
   FRIEND_TEST_ALL_PREFIXES(SyncEncryptionHandlerImplTest,
                            EncryptEverythingExplicit);
   FRIEND_TEST_ALL_PREFIXES(SyncEncryptionHandlerImplTest,
                            EncryptEverythingImplicit);
   FRIEND_TEST_ALL_PREFIXES(SyncEncryptionHandlerImplTest,
                            UnknownSensitiveTypes);
@@ skipping 16 matching lines @@
   // the encrypted types/encrypt everything state, as well as the keybag/
   // explicit passphrase state (if the cryptographer is ready).
   void WriteEncryptionStateToNigori(WriteTransaction* trans);
 
   // Updates local encrypted types from |nigori|.
   // Returns true if the local set of encrypted types either matched or was
   // a subset of that in |nigori|. Returns false if the local state already
   // 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
   // the nigori node, and does re-encryption.
   // |success|: true if the operation was successful and false otherwise. If
   //            success == false, we send an OnPassphraseRequired notification.
   // |bootstrap_token|: used to inform observers if the cryptographer's
   //                    bootstrap token was updated.
   // |is_explicit|: used to differentiate between a custom passphrase (true) and
   //                a GAIA passphrase that is implicitly used for encryption
   //                (false).
   // |trans| and |nigori_node|: used to access data in the cryptographer.
   void FinishSetPassphrase(bool success,
                            const std::string& bootstrap_token,
                            bool is_explicit,
                            WriteTransaction* trans,
                            WriteNode* nigori_node);
 
   // 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_;
 
   ObserverList<SyncEncryptionHandler::Observer> observers_;
 
   // 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
   // conflict resolver) updated the nigori's encryption keys in this chrome
   // instantiation.
   int nigori_overwrite_count_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncEncryptionHandlerImpl);
 };
 
 }  // namespace syncer
 
 #endif  // SYNC_INTERNAL_API_PUBLIC_SYNC_ENCRYPTION_HANDLER_IMPL_H_