Integrate registry_hash_store_contents with the rest of tracked prefs.

components/user_prefs/tracked/pref_hash_filter.cc

Index: components/user_prefs/tracked/pref_hash_filter.cc
diff --git a/components/user_prefs/tracked/pref_hash_filter.cc b/components/user_prefs/tracked/pref_hash_filter.cc
index 42ce04accebcd9d121d14909dbbb4693b6325b5b..06e71d6dbe00570884665924cb9d27152a4267c1 100644
--- a/components/user_prefs/tracked/pref_hash_filter.cc
+++ b/components/user_prefs/tracked/pref_hash_filter.cc
@@ -8,8 +8,10 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
@@ -51,12 +53,18 @@ void CleanupDeprecatedTrackedPreferences(
 
 PrefHashFilter::PrefHashFilter(
     std::unique_ptr<PrefHashStore> pref_hash_store,
+    StoreContentsPair* external_validation_hash_store_pair,
     const std::vector<TrackedPreferenceMetadata>& tracked_preferences,
     const base::Closure& on_reset_on_load,
     TrackedPreferenceValidationDelegate* delegate,
     size_t reporting_ids_count,
     bool report_super_mac_validity)
     : pref_hash_store_(std::move(pref_hash_store)),
+      external_validation_hash_store_pair_(
+          external_validation_hash_store_pair

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

To support this now that it will come in by value you can check one of its
members for null and use nullopt if so (and DCHECK below that if the Optional
member is set, both items are non-null).

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

Done.

+              ? base::make_optional(
+                    std::move(*external_validation_hash_store_pair))
+              : base::nullopt),
       on_reset_on_load_(on_reset_on_load),
       report_super_mac_validity_(report_super_mac_validity) {
   DCHECK(pref_hash_store_);
@@ -139,7 +147,7 @@ void PrefHashFilter::Initialize(base::DictionaryValue* pref_store_contents) {
        it != tracked_paths_.end(); ++it) {
     const std::string& initialized_path = it->first;
     const TrackedPreference* initialized_preference = it->second;
-    const base::Value* value = NULL;
+    const base::Value* value = nullptr;
     pref_store_contents->Get(initialized_path, &value);
     initialized_preference->OnNewValue(value, hash_store_transaction.get());
   }
@@ -156,30 +164,47 @@ void PrefHashFilter::FilterUpdate(const std::string& path) {
 // Updates the stored hashes for |changed_paths_| before serializing data to
 // disk. This is required as storing the hash everytime a pref's value changes
 // is too expensive (see perf regression @ http://crbug.com/331273).
-void PrefHashFilter::FilterSerializeData(
+base::Callback<void(bool)> PrefHashFilter::FilterSerializeData(
     base::DictionaryValue* pref_store_contents) {
+  // Generate the post-write callback before clearing |changed_paths_|.
+  base::Callback<void(bool)> callback =
+      GetOnWriteSynchronousCallback(pref_store_contents);
+
   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(&dictionary_contents));
+
+      std::unique_ptr<PrefHashStoreTransaction>
+          external_validation_hash_store_transaction;
+      if (external_validation_hash_store_pair_) {
+        external_validation_hash_store_transaction =
+            external_validation_hash_store_pair_->first->BeginTransaction(
+                external_validation_hash_store_pair_->second.get());
+      }
+
       for (ChangedPathsMap::const_iterator it = changed_paths_.begin();
            it != changed_paths_.end(); ++it) {
         const std::string& changed_path = it->first;
         const TrackedPreference* changed_preference = it->second;
-        const base::Value* value = NULL;
+        const base::Value* value = nullptr;
         pref_store_contents->Get(changed_path, &value);
         changed_preference->OnNewValue(value, hash_store_transaction.get());
+
+        // Clear MACs for modified prefs in the external store. They will be
+        // repopulated when |callback| is run.

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

Add: "This avoids false positives in the event of a failure to complete the
write."

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

Done.

+        if (external_validation_hash_store_transaction)
+          external_validation_hash_store_transaction->ClearHash(changed_path);

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

I just realized : it's possible with the current way ImportantFileWriter works
for this to run in parallel with FlushToExternalStore() because
ImportantFileWriter doesn't currently wait until its last write is done before
scheduling a new one... This was fine before the introduction of a synchronous
callback post-write...

We could add locks here, but even then it wouldn't be correct because:
 1) The old FlushToExternalStore() could still run after the latest write's
ClearHash(), resulting in potential false positives as well.
 2) We'd have to enforce in the HashStoreContents API that copyable ones do not
do any expensive operations (e.g. I/O) in Set calls which seems out of place.

I think the solution is to address this at the ImportantFileWriter level, i.e.
we could cancel a write if it didn't occur yet by the time another one is fired
(and/or postpone the new write until after the current one completes if it's
underway). Happy to discuss how this could work in person (and I'm sorry to see
you hit yet another corner case with this CL!).

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

IIUC you're worried about:
1. Pref write #1 is requested. Pref A's registry entry is cleared.
2. Pref write #1 does WriteNow(), but isn't done writing to disk yet.
3. Pref write #2 is requested. Pref A's registry entry is cleared.
4. Pref write #1's WriteNow finished. Pref A's registry entry is updated with
pref write #1's data.
5. Pref write #2 does WriteNow(). Two cases are possible:
a) Writing to disk succeeds and we update the registry.
b) Writing to disk fails and we don't update the registry.

In either case, there's consistency (#1 file with #1 registry or #2 file with #2
registry). Are you thinking of another case?

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

Not quite, what I mean is:

1. Pref write #1 is requested. Pref A's registry entry is cleared.
2. Pref write #2 is requested. Pref A's registry entry is cleared.
3. Pref write #1's write happens. Pref A's registry entry is updated with
pref write #1's data.
4. Pref write #2 write happens. Process crashes before registry flush.

We now have #2 file with #1 registry.

Also, this assumes locks, without them 2 & 3 can occur in parallel and result in
even less clear behavior (what's a Clear mixed with a parallel Write of multiple
entries?!).

The solution to this is probably to add the ability to cancel writes (or at
least ignore the notification that the write occurred). I'm okay landing this CL
without this if you're willing to live with the potential false positives caused
by it temporarily (add a TODO if so).

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

I don't think cancelling the first write would work. If the write task has
already started on the IFW's sequenced thread runner, how would we safely cancel
it?

I can think of three options:

1. Prevent posting write tasks if there's an unfinished write. 

This negatively impacts prefs performance even when the platform doesn't support
registry validation, so I don't think it's a good idea.

2. Make the synchronous callback no-op if a more recent write was scheduled.

This would increase the number of blank registry entries on startup (registry
entries wouldn't get written if the second write was for different changed
preferences) but that's better than false positives. The main pain point is
working around the requirement that it should work during Chrome shutdown. From
your example, if Chrome starts to shut down before step #3, we wouldn't be able
to rely on the JsonPrefStore as a synchronization mechanism to know that write
#1's callback shouldn't write prefs to the registry. In that case we would need
another mechanism such as writing a number in the registry when clearing the
hashes, and reading that number to see if we're the last callback to have
cleared the hashes.

3. Move the registry clears to a synchronous pre-write callback.

This would require adding another callback (RegisterOnNextBeforeWriteCallback())
past a base/ reviewer's watchful eyes, but I think it would be the simplest to
implement. Since the IFW uses a sequenced thread pool, the [Clear registry
hashes, Write file to disk, Write registry hashes] operations are isolated from
other write's.

wdyt?

[gab, 2016/09/22 18:38:27]

Good points, I like 3 most. I think instead of registering two independent
callbacks though it'd be better to register an
ImportantFileWriter::WriteObserver which can override and handle either
notification at will.

[proberge, 2016/09/22 19:10:14]

sgtm. I'll send a quick e-mail to @dcheng to verify the approach and start
another pre-work CL.

       }
       changed_paths_.clear();
     }
-    // TODO(gab): Remove this histogram by Feb 21 2014; after sufficient timing
-    // data has been gathered from the wild to be confident this doesn't
-    // significantly affect performance on the UI thread.
     UMA_HISTOGRAM_TIMES("Settings.FilterSerializeDataTime",
                         base::TimeTicks::Now() - checkpoint);
   }
+
+  return callback;
 }
 
 void PrefHashFilter::FinalizeFilterOnLoad(
@@ -195,6 +220,14 @@ void PrefHashFilter::FinalizeFilterOnLoad(
     std::unique_ptr<PrefHashStoreTransaction> hash_store_transaction(
         pref_hash_store_->BeginTransaction(&dictionary_contents));
 
+    std::unique_ptr<PrefHashStoreTransaction>
+        external_validation_hash_store_transaction;
+    if (external_validation_hash_store_pair_) {
+      external_validation_hash_store_transaction =
+          external_validation_hash_store_pair_->first->BeginTransaction(
+              external_validation_hash_store_pair_->second.get());
+    }
+
     CleanupDeprecatedTrackedPreferences(
         pref_store_contents.get(), hash_store_transaction.get());
 
@@ -205,8 +238,9 @@ void PrefHashFilter::FinalizeFilterOnLoad(
 
     for (TrackedPreferencesMap::const_iterator it = tracked_paths_.begin();
          it != tracked_paths_.end(); ++it) {
-      if (it->second->EnforceAndReport(pref_store_contents.get(),
-                                       hash_store_transaction.get())) {
+      if (it->second->EnforceAndReport(
+              pref_store_contents.get(), hash_store_transaction.get(),
+              external_validation_hash_store_transaction.get())) {
         did_reset = true;
         prefs_altered = true;
       }
@@ -225,12 +259,90 @@ void PrefHashFilter::FinalizeFilterOnLoad(
       on_reset_on_load_.Run();
   }
 
-  // TODO(gab): Remove this histogram by Feb 21 2014; after sufficient timing
-  // data has been gathered from the wild to be confident this doesn't
-  // significantly affect startup.
   UMA_HISTOGRAM_TIMES("Settings.FilterOnLoadTime",
                       base::TimeTicks::Now() - checkpoint);
 
   post_filter_on_load_callback.Run(std::move(pref_store_contents),
                                    prefs_altered);
 }
+
+// static
+void PrefHashFilter::FlushToExternalStore(
+    std::unique_ptr<HashStoreContents> external_validation_hash_store_contents,
+    std::unique_ptr<base::DictionaryValue> changed_paths_and_macs,
+    bool write_success) {
+  DCHECK(!changed_paths_and_macs->empty());
+  DCHECK(external_validation_hash_store_contents);
+  if (!write_success)
+    return;
+
+  for (base::DictionaryValue::Iterator it(*changed_paths_and_macs);
+       !it.IsAtEnd(); it.Advance()) {
+    const std::string& changed_path = it.key();
+
+    const base::DictionaryValue* split_values = nullptr;
+    if (it.value().GetAsDictionary(&split_values)) {
+      for (base::DictionaryValue::Iterator inner_it(*split_values);
+           !inner_it.IsAtEnd(); inner_it.Advance()) {
+        std::string mac;
+        bool is_string = inner_it.value().GetAsString(&mac);
+        DCHECK(is_string);
+
+        external_validation_hash_store_contents->SetSplitMac(
+            changed_path, inner_it.key(), mac);
+      }
+    } else {
+      const base::StringValue* value_as_string;
+      bool is_string = it.value().GetAsString(&value_as_string);
+      DCHECK(is_string);
+
+      external_validation_hash_store_contents->SetMac(
+          changed_path, value_as_string->GetString());
+    }
+  }
+}
+
+base::Callback<void(bool)> PrefHashFilter::GetOnWriteSynchronousCallback(
+    base::DictionaryValue* pref_store_contents) {
+  if (changed_paths_.empty() || !external_validation_hash_store_pair_) {
+    return base::Callback<void(bool)>();
+  }
+
+  std::unique_ptr<base::DictionaryValue> changed_paths_macs(
+      new base::DictionaryValue);

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

Use MakeUnique

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

Done.

+
+  for (ChangedPathsMap::const_iterator it = changed_paths_.begin();
+       it != changed_paths_.end(); ++it) {
+    const std::string& changed_path = it->first;
+    const TrackedPreference* changed_preference = it->second;
+    const base::Value* new_value = nullptr;
+    pref_store_contents->Get(changed_path, &new_value);

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

Move this into the switch so that we can GetDictionary directly and avoid the
extra hoop.

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

It's used by both ATOMIC and SPLIT branches. I don't understand what's requested
here.

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

In the ATOMIC branch use Get directly and in the SPLIT branch use GetDictionary
directly.

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

Done.

+
+    switch (changed_preference->GetType()) {
+      case TrackedPreferenceType::ATOMIC:
+        changed_paths_macs->SetStringWithoutPathExpansion(
+            changed_path,
+            external_validation_hash_store_pair_->first->ComputeMac(
+                changed_path, new_value));
+        break;
+      case TrackedPreferenceType::SPLIT:
+        const base::DictionaryValue* dict_value = nullptr;
+        if (new_value && new_value->GetAsDictionary(&dict_value)) {

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

When moving the Get() not that it's fine for |new_value| to be null so I think
this if is incorrect (i.e. we do need to update the MACs to represent a null
pref.

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

Acknowledged.

+          changed_paths_macs->SetWithoutPathExpansion(
+              changed_path,
+              external_validation_hash_store_pair_->first->ComputeSplitMacs(
+                  changed_path, dict_value));
+        }
+        break;
+    }
+  }
+
+  DCHECK(external_validation_hash_store_pair_->second->IsCopyable())
+      << "External HashStoreContents must be copyable as it needs to be used "
+         "off-thread";
+
+  return base::Bind(
+      &FlushToExternalStore,
+      base::Passed(external_validation_hash_store_pair_->second->MakeCopy()),
+      base::Passed(&changed_paths_macs));
+}