Integrate registry_hash_store_contents with the rest of tracked prefs.

components/user_prefs/tracked/pref_hash_filter_unittest.cc

Index: components/user_prefs/tracked/pref_hash_filter_unittest.cc
diff --git a/components/user_prefs/tracked/pref_hash_filter_unittest.cc b/components/user_prefs/tracked/pref_hash_filter_unittest.cc
index 97201b5a1e618fa85ecca089bd38e578e71522d2..5bf2cbb9e75a1fde0e82dfb66c71b18b9346ade3 100644
--- a/components/user_prefs/tracked/pref_hash_filter_unittest.cc
+++ b/components/user_prefs/tracked/pref_hash_filter_unittest.cc
@@ -18,7 +18,7 @@
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_samples.h"
 #include "base/metrics/statistics_recorder.h"
@@ -129,6 +129,9 @@ class MockPrefHashStore : public PrefHashStore {
   // Returns the number of paths stored.
   size_t stored_paths_count() const { return stored_values_.size(); }
 
+  // Returns the number of paths cleared.
+  size_t cleared_paths_count() const { return cleared_values_.size(); }
+
   // Returns the pointer value and strategy that was passed to
   // |CheckHash/CheckSplitHash| for |path|. The returned pointer could since
   // have been freed and is thus not safe to dereference.
@@ -157,7 +160,12 @@ class MockPrefHashStore : public PrefHashStore {
 
   // PrefHashStore implementation.
   std::unique_ptr<PrefHashStoreTransaction> BeginTransaction(
-      std::unique_ptr<HashStoreContents> storage) override;
+      HashStoreContents* storage) override;
+  std::string ComputeMac(const std::string& path,
+                         const base::Value* new_value) override;
+  std::unique_ptr<base::DictionaryValue> ComputeSplitMacs(
+      const std::string& path,
+      const base::DictionaryValue* split_values) override;
 
  private:
   // A MockPrefHashStoreTransaction is handed to the caller on
@@ -175,6 +183,7 @@ class MockPrefHashStore : public PrefHashStore {
     }
 
     // PrefHashStoreTransaction implementation.
+    std::string GetStoreUMASuffix() const override;
     PrefHashStoreTransaction::ValueState CheckValue(
         const std::string& path,
         const base::Value* value) const override;
@@ -209,6 +218,9 @@ class MockPrefHashStore : public PrefHashStore {
                        const base::Value* new_value,
                        PrefHashFilter::PrefTrackingStrategy strategy);
 
+  // Records a call to this mock's ClearHash methods.
+  void RecordClearHash(const std::string& path);
+
   std::map<std::string, PrefHashStoreTransaction::ValueState> check_results_;
   std::map<std::string, std::vector<std::string>> invalid_keys_results_;
 
@@ -217,6 +229,7 @@ class MockPrefHashStore : public PrefHashStore {
 
   std::map<std::string, ValuePtrStrategyPair> checked_values_;
   std::map<std::string, ValuePtrStrategyPair> stored_values_;
+  std::vector<std::string> cleared_values_;
 
   // Number of transactions that were performed via this MockPrefHashStore.
   size_t transactions_performed_;
@@ -247,12 +260,30 @@ 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));
 }
 
+std::string MockPrefHashStore::ComputeMac(const std::string& path,
+                                          const base::Value* new_value) {
+  return "atomic mac for: " + path;
+};
+
+std::unique_ptr<base::DictionaryValue> MockPrefHashStore::ComputeSplitMacs(
+    const std::string& path,
+    const base::DictionaryValue* split_values) {
+  std::unique_ptr<base::DictionaryValue> macs_dict(new base::DictionaryValue);
+  for (base::DictionaryValue::Iterator it(*split_values); !it.IsAtEnd();
+       it.Advance()) {
+    macs_dict->SetStringWithoutPathExpansion(
+        it.key(), "split mac for: " + path + "/" + it.key());
+  }
+
+  return macs_dict;
+};
+
 PrefHashStoreTransaction::ValueState MockPrefHashStore::RecordCheckValue(
     const std::string& path,
     const base::Value* value,
@@ -279,6 +310,18 @@ void MockPrefHashStore::RecordStoreHash(
           .second);
 }
 
+void MockPrefHashStore::RecordClearHash(const std::string& path) {
+  // Don't expect the same pref to be cleared more than once.
+  EXPECT_EQ(cleared_values_.end(),
+            std::find(cleared_values_.begin(), cleared_values_.end(), path));

[gab, 2016/08/08 04:37:46]

cleared_values.find(path) instead of std::find

[proberge, 2016/08/31 17:30:17]

'find': is not a member of 'std::vector<std::string,std::allocator<_Ty>>'

Changed cleared_values_ to a set instead.

+  cleared_values_.push_back(path);
+}
+
+std::string MockPrefHashStore::MockPrefHashStoreTransaction::GetStoreUMASuffix()
+    const {
+  return "unused";
+}
+
 PrefHashStoreTransaction::ValueState
 MockPrefHashStore::MockPrefHashStoreTransaction::CheckValue(
     const std::string& path,
@@ -334,8 +377,7 @@ void MockPrefHashStore::MockPrefHashStoreTransaction::ImportHash(
 
 void MockPrefHashStore::MockPrefHashStoreTransaction::ClearHash(
     const std::string& path) {
-  // Allow this to be called by PrefHashFilter's deprecated tracked prefs
-  // cleanup tasks.
+  outer_->RecordClearHash(path);
 }
 
 bool MockPrefHashStore::MockPrefHashStoreTransaction::IsSuperMACValid() const {
@@ -359,6 +401,165 @@ std::vector<PrefHashFilter::TrackedPreferenceMetadata> GetConfiguration(
   return configuration;
 }
 
+class MockHashStoreContents : public HashStoreContents {
+ public:
+  MockHashStoreContents(){};
+
+  // Returns the number of hashes stored.
+  size_t stored_hashes_count() const { return dictionary_.size(); }
+
+  // Returns the pointer value that was passed to |StoreMac| for
+  // |path|. The returned pointer could since have been freed and is thus not
+  // safe to dereference.

[gab, 2016/08/08 04:37:46]

This class appears to be storing real data, not pointer values, so this isn't
true?

And the only use case of this immediately deferences the pointer to compare
strings... so why use pointers at all? I'm skeptical of the paradigm used here.

[proberge, 2016/08/31 17:30:17]

Done.

+  const std::string* stored_mac(const std::string& path) const {

[gab, 2016/08/08 04:37:46]

I realize that you copied this from MockPrefHashStore, but these methods names
should be using PascalCasing per not being trivial inline getters/setters (i.e.
snake_case is fine for stored_hashes_count() only here).

[proberge, 2016/08/31 17:30:17]

Done.

+    const base::Value* out_value;
+    if (dictionary_.GetWithoutPathExpansion(path, &out_value)) {
+      const base::StringValue* value_as_string;
+      bool is_string = out_value->GetAsString(&value_as_string);
+      DCHECK(is_string);
+
+      return &value_as_string->GetString();
+    }
+
+    return nullptr;
+  }
+

[gab, 2016/08/08 04:37:46]

PS: Will wait for this to be split in its own CL w/ above comments addressed to
review further this test further.

[proberge, 2016/08/31 17:30:17]

Acknowledged.

+  const std::string* stored_split_mac(const std::string& path,
+                                      const std::string& split_path) const {
+    const base::Value* out_value;
+    if (dictionary_.GetWithoutPathExpansion(path, &out_value)) {
+      const base::DictionaryValue* value_as_dict;
+      bool is_dict = out_value->GetAsDictionary(&value_as_dict);
+      DCHECK(is_dict);
+
+      if (value_as_dict->GetWithoutPathExpansion(split_path, &out_value)) {
+        const base::StringValue* value_as_string;
+        bool is_string = out_value->GetAsString(&value_as_string);
+        DCHECK(is_string);
+
+        return &value_as_string->GetString();
+      }
+    }
+
+    return nullptr;
+  }
+
+  // HashStoreContents implementation.
+  std::unique_ptr<HashStoreContents> MakeCopy() const override;
+  std::string GetUMASuffix() const override;
+  void Reset() override;
+  bool GetMac(const std::string& path, std::string* out_value) override;
+  bool GetSplitMacs(const std::string& path,
+                    std::map<std::string, std::string>* split_macs) override;
+  void SetMac(const std::string& path, const std::string& value) override;
+  void SetSplitMac(const std::string& path,
+                   const std::string& split_path,
+                   const std::string& value) override;
+  void ImportEntry(const std::string& path,
+                   const base::Value* in_value) override;
+  bool RemoveEntry(const std::string& path) override;
+  const base::DictionaryValue* GetContents() const override;
+  std::string GetSuperMac() const override;
+  void SetSuperMac(const std::string& super_mac) override;
+
+ private:
+  MockHashStoreContents(MockHashStoreContents* origin_mock);
+
+  // Records calls to this mock's SetMac/SetSplitMac methods.
+  void RecordSetMac(const std::string& path, const std::string& mac) {
+    dictionary_.SetStringWithoutPathExpansion(path, mac);
+  }
+  void RecordSetSplitMac(const std::string& path,
+                         const std::string& split_path,
+                         const std::string& mac) {
+    std::unique_ptr<base::DictionaryValue> inner_value(
+        new base::DictionaryValue);
+    inner_value->SetStringWithoutPathExpansion(split_path, mac);
+    dictionary_.SetWithoutPathExpansion(path, std::move(inner_value));
+  }
+
+  base::DictionaryValue dictionary_;
+
+  // The code being tested copies its HashStoreContents for use in a callback
+  // which can be executed during shutdown. To be able to capture the behavior
+  // of the copy, we make it forward calls to the mock it was created from.
+  MockHashStoreContents* origin_mock_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockHashStoreContents);
+};
+
+MockHashStoreContents::MockHashStoreContents(MockHashStoreContents* origin_mock)
+    : origin_mock_(origin_mock) {}
+
+std::unique_ptr<HashStoreContents> MockHashStoreContents::MakeCopy() const {
+  // Return a new MockHashStoreContents which forwards all requests to this
+  // mock instance.
+  return std::unique_ptr<HashStoreContents>(
+      new MockHashStoreContents(const_cast<MockHashStoreContents*>(this)));
+}
+
+std::string MockHashStoreContents::GetUMASuffix() const {
+  return "Unused";
+}
+
+void MockHashStoreContents::Reset() {
+  ADD_FAILURE() << "Unexpected call.";
+}
+
+bool MockHashStoreContents::GetMac(const std::string& path,
+                                   std::string* out_value) {
+  ADD_FAILURE() << "Unexpected call.";
+  return false;
+}
+
+bool MockHashStoreContents::GetSplitMacs(
+    const std::string& path,
+    std::map<std::string, std::string>* split_macs) {
+  ADD_FAILURE() << "Unexpected call.";
+  return false;
+}
+
+void MockHashStoreContents::SetMac(const std::string& path,
+                                   const std::string& value) {
+  if (origin_mock_)
+    origin_mock_->RecordSetMac(path, value);
+  else
+    RecordSetMac(path, value);
+}
+
+void MockHashStoreContents::SetSplitMac(const std::string& path,
+                                        const std::string& split_path,
+                                        const std::string& value) {
+  if (origin_mock_)
+    origin_mock_->RecordSetSplitMac(path, split_path, value);
+  else
+    RecordSetSplitMac(path, split_path, value);
+}
+
+void MockHashStoreContents::ImportEntry(const std::string& path,
+                                        const base::Value* in_value) {
+  ADD_FAILURE() << "Unexpected call.";
+}
+
+bool MockHashStoreContents::RemoveEntry(const std::string& path) {
+  ADD_FAILURE() << "Unexpected call.";
+  return false;
+}
+
+const base::DictionaryValue* MockHashStoreContents::GetContents() const {
+  ADD_FAILURE() << "Unexpected call.";
+  return nullptr;
+}
+
+std::string MockHashStoreContents::GetSuperMac() const {
+  ADD_FAILURE() << "Unexpected call.";
+  return NULL;
+}
+
+void MockHashStoreContents::SetSuperMac(const std::string& super_mac) {
+  ADD_FAILURE() << "Unexpected call.";
+}
+
 class PrefHashFilterTest
     : public testing::TestWithParam<PrefHashFilter::EnforcementLevel> {
  public:
@@ -386,9 +587,21 @@ class PrefHashFilterTest
       PrefHashFilter::TrackedPreferenceMetadata>& configuration) {
     std::unique_ptr<MockPrefHashStore> temp_mock_pref_hash_store(
         new MockPrefHashStore);
+    std::unique_ptr<MockPrefHashStore>
+        temp_mock_external_validation_pref_hash_store(new MockPrefHashStore);
+    std::unique_ptr<MockHashStoreContents>
+        temp_mock_external_validation_hash_store_contents(
+            new MockHashStoreContents);
     mock_pref_hash_store_ = temp_mock_pref_hash_store.get();
+    mock_external_validation_pref_hash_store_ =
+        temp_mock_external_validation_pref_hash_store.get();
+    mock_external_validation_hash_store_contents_ =
+        temp_mock_external_validation_hash_store_contents.get();
     pref_hash_filter_.reset(new PrefHashFilter(
-        std::move(temp_mock_pref_hash_store), configuration,
+        std::move(temp_mock_pref_hash_store),
+        std::move(temp_mock_external_validation_pref_hash_store),
+        std::move(temp_mock_external_validation_hash_store_contents),
+        configuration,
         base::Bind(&PrefHashFilterTest::RecordReset, base::Unretained(this)),
         &mock_validation_delegate_, arraysize(kTestTrackedPrefs), true));
   }
@@ -414,6 +627,8 @@ class PrefHashFilterTest
   }
 
   MockPrefHashStore* mock_pref_hash_store_;
+  MockPrefHashStore* mock_external_validation_pref_hash_store_;
+  MockHashStoreContents* mock_external_validation_hash_store_contents_;
   std::unique_ptr<base::DictionaryValue> pref_store_contents_;
   MockValidationDelegate mock_validation_delegate_;
   std::unique_ptr<PrefHashFilter> pref_hash_filter_;
@@ -1035,6 +1250,115 @@ TEST_P(PrefHashFilterTest, DontResetReportOnly) {
   }
 }
 
+TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallback) {
+  base::DictionaryValue root_dict;
+  // Ownership of the following values is transfered to |root_dict|.
+  base::Value* int_value1 = new base::FundamentalValue(1);
+  base::Value* int_value2 = new base::FundamentalValue(2);
+  base::DictionaryValue* dict_value = new base::DictionaryValue;
+  dict_value->Set("a", new base::FundamentalValue(true));
+  root_dict.Set(kAtomicPref, int_value1);
+  root_dict.Set(kAtomicPref2, int_value2);
+  root_dict.Set(kSplitPref, dict_value);
+
+  // Skip updating kAtomicPref2.
+  pref_hash_filter_->FilterUpdate(kAtomicPref);
+  pref_hash_filter_->FilterUpdate(kSplitPref);
+
+  base::Callback<void(bool)> callback =
+      pref_hash_filter_->FilterSerializeData(&root_dict);
+
+  ASSERT_FALSE(callback.is_null());
+
+  // The two prefs should have been cleared from the external validation store.
+  ASSERT_EQ(2u,
+            mock_external_validation_pref_hash_store_->cleared_paths_count());
+
+  // No pref write should occur before we run |callback|.
+  ASSERT_EQ(0u,
+            mock_external_validation_pref_hash_store_->stored_paths_count());
+  ASSERT_EQ(
+      0u, mock_external_validation_hash_store_contents_->stored_hashes_count());
+
+  callback.Run(true);
+
+  // Note that the callback writes directly to the contents without going
+  // through a pref hash store.
+  ASSERT_EQ(0u,
+            mock_external_validation_pref_hash_store_->stored_paths_count());
+  ASSERT_EQ(
+      2u, mock_external_validation_hash_store_contents_->stored_hashes_count());
+  ASSERT_EQ(
+      "atomic mac for: atomic_pref",
+      *mock_external_validation_hash_store_contents_->stored_mac(kAtomicPref));
+  ASSERT_EQ("split mac for: split_pref/a",
+            *mock_external_validation_hash_store_contents_->stored_split_mac(
+                kSplitPref, "a"));
+}
+
+TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbackWithFalse) {
+  base::DictionaryValue root_dict;
+  // Ownership of the following values is transfered to |root_dict|.
+  base::Value* int_value1 = new base::FundamentalValue(1);
+  root_dict.Set(kAtomicPref, int_value1);
+
+  // Skip updating kAtomicPref2.
+  pref_hash_filter_->FilterUpdate(kAtomicPref);
+
+  base::Callback<void(bool)> callback =
+      pref_hash_filter_->FilterSerializeData(&root_dict);
+
+  ASSERT_FALSE(callback.is_null());
+
+  // The pref should have been cleared from the external validation store.
+  ASSERT_EQ(1u,
+            mock_external_validation_pref_hash_store_->cleared_paths_count());
+
+  callback.Run(false);
+
+  // Expect no writes to the external validation hash store contents.
+  ASSERT_EQ(0u,
+            mock_external_validation_pref_hash_store_->stored_paths_count());
+  ASSERT_EQ(
+      0u, mock_external_validation_hash_store_contents_->stored_hashes_count());
+}
+
+TEST_P(PrefHashFilterTest, ExternalValidationValueChanged) {
+  // Ownership of this value is transfered to |pref_store_contents_|.
+  base::Value* int_value = new base::FundamentalValue(1234);
+  pref_store_contents_->Set(kAtomicPref, int_value);
+
+  base::DictionaryValue* dict_value = new base::DictionaryValue;
+  dict_value->SetString("a", "foo");
+  dict_value->SetInteger("b", 1234);
+  dict_value->SetInteger("c", 56);
+  dict_value->SetBoolean("d", false);
+  pref_store_contents_->Set(kSplitPref, dict_value);
+
+  mock_external_validation_pref_hash_store_->SetCheckResult(
+      kAtomicPref, PrefHashStoreTransaction::CHANGED);
+  mock_external_validation_pref_hash_store_->SetCheckResult(
+      kSplitPref, PrefHashStoreTransaction::CHANGED);
+
+  std::vector<std::string> mock_invalid_keys;
+  mock_invalid_keys.push_back("a");
+  mock_invalid_keys.push_back("c");
+  mock_external_validation_pref_hash_store_->SetInvalidKeysResult(
+      kSplitPref, mock_invalid_keys);
+
+  DoFilterOnLoad(false);
+
+  ASSERT_EQ(arraysize(kTestTrackedPrefs),
+            mock_external_validation_pref_hash_store_->checked_paths_count());
+  ASSERT_EQ(2u,
+            mock_external_validation_pref_hash_store_->stored_paths_count());
+  ASSERT_EQ(
+      1u, mock_external_validation_pref_hash_store_->transactions_performed());
+
+  // TODO(proberge): query mock_validation_state_ for number of CHANGED
+  // and UNCHANGED preferences once the class supports external validation.
+}
+
 INSTANTIATE_TEST_CASE_P(PrefHashFilterTestInstance,
                         PrefHashFilterTest,
                         testing::Values(PrefHashFilter::NO_ENFORCEMENT,