Introduce a new framework for back-and-forth tracked preference migration

between Protected Preferences and unprotected Preferences.

Migration from unprotected Preferences to Protected Preferences was previously
done after both stores had been initialized. This was inherently incorrect as
some operations (PrefHashFilter::FilterOnLoad) would occur before the values
had been moved to the proper store. It also introduced a weird method in
PrefHashFilter::MigrateValues which required an independent PrefHashFilter
(backed by a copy of the real PrefHashStore). This after-the-fact migration
caused Settings.TrackedPreferenceCleared spikes when changing a value from
being enforced to not being enforced (as we'd have a MAC, but no value yet in
this store when running FilterOnLoad()) and more importantly it also caused
issue 365769 -- both of these issues highlight the incorrectness of the
current approach.

The migration back from Protected Preferences to unprotected Preferences when
enforcement was disabled was using yet another mechanism which would only kick
in when a given pref was written to (ref. old non-const
SegregatedPrefStore::StoreForKey()).

The new framework intercepts PrefFilter::FilterOnLoad() events for both stores
and does the back-and-forth migration in place before it even hands them back
to the PrefFilter::FinalizeFilterOnLoad() which then hands it back to the
JsonPrefStores (so that they are agnostic to the migration; from their point
of view their values were always in their store as they received it).
Furthermore, this new framework will easily allow us to later move MACs out of
Local State into their respective stores (which is a task on our radar which
we currently have no easy way to accomplish).

The new framework also handles read errors better. For example, it was
previously possible for the unprotected->protected migration to result in data
loss if the protected store was somehow read-only from a read error while the
unprotected store wasn't -- resulting in an in-memory migration only flushed
to disk in the store from which the value was deleted... The new framework
handles those cases, preferring temporary data duplication over potential data
loss (duplicated data is cleaned up once confirmation is obtained that the new
authority for this data has been successfully written to disk -- it will even
try again in following Chrome runs if it doesn't succeed in this one).

Note: This CL helped LSAN discover an existing leak in post_task_and_reply_impl.cc, see issue 371974 for details.

BUG=365769, 371974
TEST=
A) Make sure all kTrackedPrefs consistently report
   Settings.TrackedPreferenceUnchanged across changes from various enforcement
   levels (using --force-fieldtrials).
B) Make sure the prefs are properly migrated to their new store (and
   subsequently cleaned up from their old store) when changing the
   enforcement_level across multiple runs.
C) Make sure prefs are properly migrated in a quick startup/shutdown with a
   new enforcement_level and that their old value is properly cleaned up in a
   subsequent startup at the same enforcement_level (or re-migrated at another
   enforcement_level).

R=bauerb@chromium.org, robertshield@chromium.org, stuartmorgan@chromium.org, thakis@chromium.org

Initially Committed in: https://src.chromium.org/viewvc/chrome?view=rev&revision=269346
Reverted in: https://src.chromium.org/viewvc/chrome?view=rev&revision=269367

Review URL: https://codereview.chromium.org/257003007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269415 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 73 insertions, 0 deletions

diff --git a/base/files/important_file_writer_unittest.cc b/base/files/important_file_writer_unittest.cc
index 02a5f76..3f62fe4 100644
--- a/base/files/important_file_writer_unittest.cc
+++ b/base/files/important_file_writer_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "base/files/important_file_writer.h"
 
+#include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
@@ -41,6 +42,41 @@ class DataSerializer : public ImportantFileWriter::DataSerializer {
   const std::string data_;
 };
 
+class SuccessfulWriteObserver {
+ public:
+  SuccessfulWriteObserver() : successful_write_observed_(false) {}
+
+  // Register on_successful_write() to be called on the next successful write
+  // of |writer|.
+  void ObserveNextSuccessfulWrite(ImportantFileWriter* writer);
+
+  // Returns true if a successful write was observed via on_successful_write()
+  // and resets the observation state to false regardless.
+  bool GetAndResetObservationState();
+
+ private:
+  void on_successful_write() {
+    EXPECT_FALSE(successful_write_observed_);
+    successful_write_observed_ = true;
+  }
+
+  bool successful_write_observed_;
+
+  DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteObserver);
+};
+
+void SuccessfulWriteObserver::ObserveNextSuccessfulWrite(
+    ImportantFileWriter* writer) {
+  writer->RegisterOnNextSuccessfulWriteCallback(base::Bind(
+      &SuccessfulWriteObserver::on_successful_write, base::Unretained(this)));
+}
+
+bool SuccessfulWriteObserver::GetAndResetObservationState() {
+  bool was_successful_write_observed = successful_write_observed_;
+  successful_write_observed_ = false;
+  return was_successful_write_observed;
+}
+
 }  // namespace
 
 class ImportantFileWriterTest : public testing::Test {
@@ -52,6 +88,7 @@ class ImportantFileWriterTest : public testing::Test {
   }
 
  protected:
+  SuccessfulWriteObserver successful_write_observer_;
   FilePath file_;
   MessageLoop loop_;
 
@@ -62,11 +99,47 @@ class ImportantFileWriterTest : public testing::Test {
 TEST_F(ImportantFileWriterTest, Basic) {
   ImportantFileWriter writer(file_, MessageLoopProxy::current().get());
   EXPECT_FALSE(PathExists(writer.path()));
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  writer.WriteNow("foo");
+  RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("foo", GetFileContent(writer.path()));
+}
+
+TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
+  ImportantFileWriter writer(file_, MessageLoopProxy::current().get());
+  EXPECT_FALSE(PathExists(writer.path()));
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
   writer.WriteNow("foo");
   RunLoop().RunUntilIdle();
 
+  // Confirm that the observer is invoked.
+  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
+
+  // Confirm that re-installing the observer works for another write.
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  writer.WriteNow("bar");
+  RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("bar", GetFileContent(writer.path()));
+
+  // Confirm that writing again without re-installing the observer doesn't
+  // result in a notification.
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  writer.WriteNow("baz");
+  RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
 
 TEST_F(ImportantFileWriterTest, ScheduleWrite) {