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 16c76f1d7ac50dbd9e65c64b230e185215e62f78..83fb1096c8cdc5fe7d2d1f3e287c1378b36ded77 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.
@@ -215,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_;
 
@@ -223,6 +229,7 @@ class MockPrefHashStore : public PrefHashStore {
 
   std::map<std::string, ValuePtrStrategyPair> checked_values_;
   std::map<std::string, ValuePtrStrategyPair> stored_values_;
+  std::set<std::string> cleared_values_;
 
   // Number of transactions that were performed via this MockPrefHashStore.
   size_t transactions_performed_;
@@ -302,6 +309,12 @@ 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(), cleared_values_.find(path));
+  cleared_values_.insert(path);
+}
+
 base::StringPiece
 MockPrefHashStore::MockPrefHashStoreTransaction::GetStoreUMASuffix() const {
   return "unused";
@@ -362,8 +375,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 {
@@ -387,6 +399,172 @@ 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 stored MAC for an Atomic preference.
+  std::string GetStoredMac(const std::string& path) const;
+  // Returns the stored MAC for a Split preference.
+  std::string GetStoredSplitMac(const std::string& path,
+                                const std::string& split_path) const;
+
+  // HashStoreContents implementation.
+  bool IsCopyable() const override;
+  std::unique_ptr<HashStoreContents> MakeCopy() const override;
+  base::StringPiece 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);

[gab, 2016/09/16 19:47:32]

Won't unconditionally creating a new dict overwrite other MACs in the same split
pref?

[proberge, 2016/09/20 21:35:45]

I *think* that setting a dictionary to an entry with an existing dictionary just
merges the entries.

[gab, 2016/09/21 17:55:30]

I don't think this is true, every pref is a node when you set a pref which
happens to be a dictionary, you replace that node with the new dictionary, no
merge is performed AFAIK.

[proberge, 2016/09/21 21:09:24]

You're right. Added some logic to not clobber the dictionary.

+    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.
+  // Once set, |origin_mock_| must outlive this instance.
+  MockHashStoreContents* origin_mock_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockHashStoreContents);
+};
+
+std::string MockHashStoreContents::GetStoredMac(const std::string& path) const {
+  const base::Value* out_value;
+  if (dictionary_.GetWithoutPathExpansion(path, &out_value)) {

[gab, 2016/09/16 19:47:33]

Assuming you're not using GetStringWithoutPathExpansion() so that you can verify
type below? If so that makes sense to me :-)

[proberge, 2016/09/20 21:35:45]

Done.

+    const base::StringValue* value_as_string;
+    EXPECT_TRUE(out_value->GetAsString(&value_as_string));
+
+    return value_as_string->GetString();
+  }
+
+  return NULL;
+}
+
+std::string MockHashStoreContents::GetStoredSplitMac(
+    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;
+    EXPECT_TRUE(out_value->GetAsDictionary(&value_as_dict));
+
+    if (value_as_dict->GetWithoutPathExpansion(split_path, &out_value)) {
+      const base::StringValue* value_as_string;
+      EXPECT_TRUE(out_value->GetAsString(&value_as_string));
+
+      return value_as_string->GetString();
+    }
+  }
+
+  return NULL;
+}
+
+MockHashStoreContents::MockHashStoreContents(MockHashStoreContents* origin_mock)
+    : origin_mock_(origin_mock) {}
+
+bool MockHashStoreContents::IsCopyable() const {
+  return true;
+}
+
+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)));
+}
+
+base::StringPiece 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:
@@ -414,9 +592,22 @@ 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),
+        new PrefHashFilter::StoreContentsPair(

[gab, 2016/09/16 19:47:32]

Note: you should question yourself anytime you see a naked "new" call, it's
almost always a leak (or a poor design -- i.e. a comment that says "takes
ownership" instead of explicitly using unique_ptr)

In this case this is addressed by an aforementioned API change but just for the
record as a teaching point.

[proberge, 2016/09/20 21:35:45]

Done.

+            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));
   }
@@ -442,6 +633,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_;
@@ -1063,6 +1256,115 @@ TEST_P(PrefHashFilterTest, DontResetReportOnly) {
   }
 }
 
+TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallback) {
+  base::DictionaryValue root_dict;
+  // Ownership of the following values is transfered to |root_dict|.

[gab, 2016/09/16 19:47:33]

DictionaryValue::Set() now supports unique_ptr so we no longer need to do this
bad style temporary raw pointer (i.e. this is an example of an old bad API that
required raw pointers and took ownership... in the process of being fixed in
Chromium). Though I guess this is okay for now as it makes it consistent with
surrounding style, so let's keep it as-is in this CL.

[proberge, 2016/09/20 21:35:45]

Acknowledged.

+  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|.

[gab, 2016/09/16 19:47:33]

s/before we run X/before X is run/ (avoid pronouns)

[proberge, 2016/09/20 21:35:45]

Done.

+  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_->GetStoredMac(kAtomicPref));
+  ASSERT_EQ("split mac for: split_pref/a",
+            mock_external_validation_hash_store_contents_->GetStoredSplitMac(
+                kSplitPref, "a"));
+}
+
+TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbackWithFalse) {

[gab, 2016/09/16 19:47:33]

s/WithFalse/WithFailure/

[proberge, 2016/09/20 21:35:45]

Done.

+  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.

[gab, 2016/09/16 19:47:32]

// Only update kAtomicPref.

[proberge, 2016/09/20 21:35:45]

Done.

+  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.

[gab, 2016/09/16 19:47:32]

Can be done now?

[proberge, 2016/09/20 21:35:45]

Not yet. See https://codereview.chromium.org/2345723002/

+}
+
 INSTANTIATE_TEST_CASE_P(PrefHashFilterTestInstance,
                         PrefHashFilterTest,
                         testing::Values(PrefHashFilter::NO_ENFORCEMENT,