Refactor PrefHashStore's BeginTransaction to take a rawptr

To support a new HashStoreContents which needs to be passed around in
a different manner, the PrefHashStoreTransaction now has a raw ptr
instead of owning the HashStoreContents.

BUG=624858

Review-Url: https://codereview.chromium.org/2297373002
Cr-Commit-Position: refs/heads/master@{#417023}

components/user_prefs/tracked/pref_hash_filter.cc

@@ -132,9 +132,9 @@ void PrefHashFilter::ClearResetTime(PrefService* user_prefs) {
 }
 
 void PrefHashFilter::Initialize(base::DictionaryValue* pref_store_contents) {
+  DictionaryHashStoreContents dictionary_contents(pref_store_contents);
   std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
-      pref_hash_store_->BeginTransaction(std::unique_ptr<HashStoreContents>(
-          new DictionaryHashStoreContents(pref_store_contents))));
+      pref_hash_store_->BeginTransaction(&dictionary_contents));
   for (TrackedPreferencesMap::const_iterator it = tracked_paths_.begin();
        it != tracked_paths_.end(); ++it) {
     const std::string& initialized_path = it->first;
@@ -161,9 +161,9 @@ void PrefHashFilter::FilterSerializeData(
   if (!changed_paths_.empty()) {
     base::TimeTicks checkpoint = base::TimeTicks::Now();
     {
+      DictionaryHashStoreContents dictionary_contents(pref_store_contents);
       std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
-          pref_hash_store_->BeginTransaction(std::unique_ptr<HashStoreContents>(
-              new DictionaryHashStoreContents(pref_store_contents))));
+          pref_hash_store_->BeginTransaction(&dictionary_contents));
       for (ChangedPathsMap::const_iterator it = changed_paths_.begin();
            it != changed_paths_.end(); ++it) {
         const std::string& changed_path = it->first;
@@ -191,9 +191,9 @@ void PrefHashFilter::FinalizeFilterOnLoad(
 
   bool did_reset = false;
   {
+    DictionaryHashStoreContents dictionary_contents(pref_store_contents.get());
     std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
-        pref_hash_store_->BeginTransaction(std::unique_ptr<HashStoreContents>(
-            new DictionaryHashStoreContents(pref_store_contents.get()))));
+        pref_hash_store_->BeginTransaction(&dictionary_contents));
 
     CleanupDeprecatedTrackedPreferences(
         pref_store_contents.get(), hash_store_transaction.get());

components/user_prefs/tracked/pref_hash_filter_unittest.cc

@@ -157,7 +157,7 @@ class MockPrefHashStore : public PrefHashStore {
 
   // PrefHashStore implementation.
   std::unique_ptr<PrefHashStoreTransaction> BeginTransaction(
-      std::unique_ptr<HashStoreContents> storage) override;
+      HashStoreContents* storage) override;
 
  private:
   // A MockPrefHashStoreTransaction is handed to the caller on
@@ -247,7 +247,7 @@ void MockPrefHashStore::SetInvalidKeysResult(
 }
 
 std::unique_ptr<PrefHashStoreTransaction> MockPrefHashStore::BeginTransaction(
-    std::unique_ptr<HashStoreContents> storage) {
+    HashStoreContents* storage) {
   EXPECT_FALSE(transaction_active_);
   return std::unique_ptr<PrefHashStoreTransaction>(
       new MockPrefHashStoreTransaction(this));

components/user_prefs/tracked/pref_hash_store.h

@@ -21,8 +21,9 @@ class PrefHashStore {
   // of operations on the hash store. |storage| MAY be used as the backing store
   // depending on the implementation. Therefore the HashStoreContents used for
   // related transactions should correspond to the same underlying data store.
+  // |storage| must outlive the returned transaction.
   virtual std::unique_ptr<PrefHashStoreTransaction> BeginTransaction(
-      std::unique_ptr<HashStoreContents> storage) = 0;
+      HashStoreContents* storage) = 0;
 };
 
 #endif  // COMPONENTS_PREFS_TRACKED_PREF_HASH_STORE_H_

components/user_prefs/tracked/pref_hash_store_impl.cc

@@ -20,7 +20,7 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl
   // Constructs a PrefHashStoreTransactionImpl which can use the private
   // members of its |outer| PrefHashStoreImpl.
   PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer,
-                               std::unique_ptr<HashStoreContents> storage);
+                               HashStoreContents* storage);
   ~PrefHashStoreTransactionImpl() override;
 
   // PrefHashStoreTransaction implementation.
@@ -41,7 +41,7 @@ class PrefHashStoreImpl::PrefHashStoreTransactionImpl
 
  private:
   PrefHashStoreImpl* outer_;
-  std::unique_ptr<HashStoreContents> contents_;
+  HashStoreContents* contents_;
 
   bool super_mac_valid_;
   bool super_mac_dirty_;
@@ -59,14 +59,14 @@ PrefHashStoreImpl::~PrefHashStoreImpl() {
 }
 
 std::unique_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction(
-    std::unique_ptr<HashStoreContents> storage) {
+    HashStoreContents* storage) {
   return std::unique_ptr<PrefHashStoreTransaction>(
       new PrefHashStoreTransactionImpl(this, std::move(storage)));
 }
 
 PrefHashStoreImpl::PrefHashStoreTransactionImpl::PrefHashStoreTransactionImpl(
     PrefHashStoreImpl* outer,
-    std::unique_ptr<HashStoreContents> storage)
+    HashStoreContents* storage)
     : outer_(outer),
       contents_(std::move(storage)),
       super_mac_valid_(false),

components/user_prefs/tracked/pref_hash_store_impl.h

@@ -47,7 +47,7 @@ class PrefHashStoreImpl : public PrefHashStore {
 
   // PrefHashStore implementation.
   std::unique_ptr<PrefHashStoreTransaction> BeginTransaction(
-      std::unique_ptr<HashStoreContents> storage) override;
+      HashStoreContents* storage) override;
 
  private:
   class PrefHashStoreTransactionImpl;

components/user_prefs/tracked/pref_hash_store_impl_unittest.cc

@@ -15,14 +15,19 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 class PrefHashStoreImplTest : public testing::Test {
+ public:
+  PrefHashStoreImplTest() : contents_(&pref_store_contents_) {}
+
  protected:
-  std::unique_ptr<HashStoreContents> CreateHashStoreContents() {
-    return std::unique_ptr<HashStoreContents>(
-        new DictionaryHashStoreContents(&pref_store_contents_));
-  }
+  HashStoreContents* GetHashStoreContents() { return &contents_; }
 
  private:
   base::DictionaryValue pref_store_contents_;
+  // Must be declared after |pref_store_contents_| as it needs to be outlived
+  // by it.
+  DictionaryHashStoreContents contents_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefHashStoreImplTest);
 };
 
 TEST_F(PrefHashStoreImplTest, AtomicHashStoreAndCheck) {
@@ -33,7 +38,7 @@ TEST_F(PrefHashStoreImplTest, AtomicHashStoreAndCheck) {
     // 32 NULL bytes is the seed that was used to generate the legacy hash.
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
 
     // Only NULL should be trusted in the absence of a hash.
     EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE,
@@ -63,14 +68,14 @@ TEST_F(PrefHashStoreImplTest, AtomicHashStoreAndCheck) {
               transaction->CheckValue("path1", &dict));
   }
 
-  ASSERT_FALSE(CreateHashStoreContents()->GetSuperMac().empty());
+  ASSERT_FALSE(GetHashStoreContents()->GetSuperMac().empty());
 
   {
     // |pref_hash_store2| should trust its initial hashes dictionary and thus
     // trust new unknown values.
     PrefHashStoreImpl pref_hash_store2(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store2.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store2.BeginTransaction(GetHashStoreContents()));
     EXPECT_EQ(PrefHashStoreTransaction::TRUSTED_UNKNOWN_VALUE,
               transaction->CheckValue("new_path", &string_1));
     EXPECT_EQ(PrefHashStoreTransaction::TRUSTED_UNKNOWN_VALUE,
@@ -80,14 +85,14 @@ TEST_F(PrefHashStoreImplTest, AtomicHashStoreAndCheck) {
   }
 
   // Manually corrupt the super MAC.
-  CreateHashStoreContents()->SetSuperMac(std::string(64, 'A'));
+  GetHashStoreContents()->SetSuperMac(std::string(64, 'A'));
 
   {
     // |pref_hash_store3| should no longer trust its initial hashes dictionary
     // and thus shouldn't trust non-NULL unknown values.
     PrefHashStoreImpl pref_hash_store3(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store3.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store3.BeginTransaction(GetHashStoreContents()));
     EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE,
               transaction->CheckValue("new_path", &string_1));
     EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE,
@@ -105,7 +110,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_FALSE(transaction->IsSuperMACValid());
 
     ASSERT_FALSE(transaction->HasHash("path1"));
@@ -122,7 +127,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
 
   // Make a copy of the stored hash for future use.
   const base::Value* hash = NULL;
-  ASSERT_TRUE(CreateHashStoreContents()->GetContents()->Get("path1", &hash));
+  ASSERT_TRUE(GetHashStoreContents()->GetContents()->Get("path1", &hash));
   std::unique_ptr<base::Value> path_1_string_1_hash_copy(hash->DeepCopy());
   hash = NULL;
 
@@ -130,7 +135,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_TRUE(transaction->IsSuperMACValid());
     ASSERT_TRUE(transaction->HasHash("path1"));
 
@@ -149,18 +154,18 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_TRUE(transaction->IsSuperMACValid());
     ASSERT_FALSE(transaction->HasHash("path1"));
   }
 
   // Invalidate the super MAC.
-  CreateHashStoreContents()->SetSuperMac(std::string());
+  GetHashStoreContents()->SetSuperMac(std::string());
 
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_FALSE(transaction->IsSuperMACValid());
     ASSERT_FALSE(transaction->HasHash("path1"));
 
@@ -178,7 +183,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_FALSE(transaction->IsSuperMACValid());
     ASSERT_TRUE(transaction->HasHash("path1"));
     EXPECT_EQ(PrefHashStoreTransaction::UNCHANGED,
@@ -196,7 +201,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_FALSE(transaction->IsSuperMACValid());
 
     // Test StampSuperMac.
@@ -207,7 +212,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_TRUE(transaction->IsSuperMACValid());
 
     // Store the hash of a different value to test an "over-import".
@@ -221,7 +226,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_TRUE(transaction->IsSuperMACValid());
 
     // "Over-import". An import should preserve validity.
@@ -236,7 +241,7 @@ TEST_F(PrefHashStoreImplTest, ImportExportOperations) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
     ASSERT_TRUE(transaction->IsSuperMACValid());
     EXPECT_EQ(PrefHashStoreTransaction::UNCHANGED,
               transaction->CheckValue("path1", &string_1));
@@ -253,19 +258,19 @@ TEST_F(PrefHashStoreImplTest, SuperMACDisabled) {
     // Pass |use_super_mac| => false.
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", false);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
 
     transaction->StoreHash("path1", &string_2);
     EXPECT_EQ(PrefHashStoreTransaction::UNCHANGED,
               transaction->CheckValue("path1", &string_2));
   }
 
-  ASSERT_TRUE(CreateHashStoreContents()->GetSuperMac().empty());
+  ASSERT_TRUE(GetHashStoreContents()->GetSuperMac().empty());
 
   {
     PrefHashStoreImpl pref_hash_store2(std::string(32, 0), "device_id", false);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store2.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store2.BeginTransaction(GetHashStoreContents()));
     EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE,
               transaction->CheckValue("new_path", &string_1));
   }
@@ -289,7 +294,7 @@ TEST_F(PrefHashStoreImplTest, SplitHashStoreAndCheck) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
 
     // No hashes stored yet and hashes dictionary is empty (and thus not
     // trusted).
@@ -359,21 +364,21 @@ TEST_F(PrefHashStoreImplTest, SplitHashStoreAndCheck) {
     // trust new unknown values.
     PrefHashStoreImpl pref_hash_store2(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store2.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store2.BeginTransaction(GetHashStoreContents()));
     EXPECT_EQ(PrefHashStoreTransaction::TRUSTED_UNKNOWN_VALUE,
               transaction->CheckSplitValue("new_path", &dict, &invalid_keys));
     EXPECT_TRUE(invalid_keys.empty());
   }
 
   // Manually corrupt the super MAC.
-  CreateHashStoreContents()->SetSuperMac(std::string(64, 'A'));
+  GetHashStoreContents()->SetSuperMac(std::string(64, 'A'));
 
   {
     // |pref_hash_store3| should no longer trust its initial hashes dictionary
     // and thus shouldn't trust unknown values.
     PrefHashStoreImpl pref_hash_store3(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store3.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store3.BeginTransaction(GetHashStoreContents()));
     EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE,
               transaction->CheckSplitValue("new_path", &dict, &invalid_keys));
     EXPECT_TRUE(invalid_keys.empty());
@@ -388,7 +393,7 @@ TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
 
     // Store hashes for a random dict to be overwritten below.
     base::DictionaryValue initial_dict;
@@ -424,7 +429,7 @@ TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) {
     // update the stored hash of hashes).
     PrefHashStoreImpl pref_hash_store2(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store2.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store2.BeginTransaction(GetHashStoreContents()));
 
     base::DictionaryValue tested_dict;
     tested_dict.Set("a", new base::StringValue("foo"));
@@ -454,7 +459,7 @@ TEST_F(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) {
   {
     PrefHashStoreImpl pref_hash_store(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store.BeginTransaction(GetHashStoreContents()));
 
     transaction->StoreHash("path1", &string);
     EXPECT_EQ(PrefHashStoreTransaction::UNCHANGED,
@@ -465,7 +470,7 @@ TEST_F(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) {
     // Load a new |pref_hash_store2| in which the hashes dictionary is trusted.
     PrefHashStoreImpl pref_hash_store2(std::string(32, 0), "device_id", true);
     std::unique_ptr<PrefHashStoreTransaction> transaction(
-        pref_hash_store2.BeginTransaction(CreateHashStoreContents()));
+        pref_hash_store2.BeginTransaction(GetHashStoreContents()));
     std::vector<std::string> invalid_keys;
     EXPECT_EQ(PrefHashStoreTransaction::TRUSTED_UNKNOWN_VALUE,
               transaction->CheckSplitValue("path1", &dict, &invalid_keys));

components/user_prefs/tracked/tracked_preferences_migration.cc

@@ -120,10 +120,9 @@ void ScheduleSourcePrefStoreCleanup(
 void CleanupMigratedHashes(const std::set<std::string>& migrated_pref_names,
                            PrefHashStore* origin_pref_hash_store,
                            base::DictionaryValue* origin_pref_store) {
+  DictionaryHashStoreContents dictionary_contents(origin_pref_store);
   std::unique_ptr<PrefHashStoreTransaction> transaction(
-      origin_pref_hash_store->BeginTransaction(
-          std::unique_ptr<HashStoreContents>(
-              new DictionaryHashStoreContents(origin_pref_store))));
+      origin_pref_hash_store->BeginTransaction(&dictionary_contents));
   for (std::set<std::string>::const_iterator it = migrated_pref_names.begin();
        it != migrated_pref_names.end();
        ++it) {
@@ -143,9 +142,9 @@ void MigratePrefsFromOldToNewStore(const std::set<std::string>& pref_names,
                                    bool* new_store_altered) {
   const base::DictionaryValue* old_hash_store_contents =
       DictionaryHashStoreContents(old_store).GetContents();
+  DictionaryHashStoreContents dictionary_contents(new_store);
   std::unique_ptr<PrefHashStoreTransaction> new_hash_store_transaction(
-      new_hash_store->BeginTransaction(std::unique_ptr<HashStoreContents>(
-          new DictionaryHashStoreContents(new_store))));
+      new_hash_store->BeginTransaction(&dictionary_contents));
 
   for (std::set<std::string>::const_iterator it = pref_names.begin();
        it != pref_names.end();

components/user_prefs/tracked/tracked_preferences_migration_unittest.cc

@@ -162,10 +162,8 @@ class TrackedPreferencesMigrationTest : public testing::Test {
     DCHECK(store);
 
     base::StringValue string_value(value);
-    pref_hash_store
-        ->BeginTransaction(std::unique_ptr<HashStoreContents>(
-            new DictionaryHashStoreContents(store)))
-        ->StoreHash(key, &string_value);
+    DictionaryHashStoreContents contents(store);
+    pref_hash_store->BeginTransaction(&contents)->StoreHash(key, &string_value);
   }
 
   // Returns true if the store opposite to |store_id| is observed for its next