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

BUG=365769
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, stuartmorgan@chromium.org, thakis@chromium.org

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

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

3 files changed, 115 insertions, 10 deletions

diff --git a/base/files/important_file_writer.cc b/base/files/important_file_writer.cc
index 9cbbd73..1250824 100644
--- a/base/files/important_file_writer.cc
+++ b/base/files/important_file_writer.cc
@@ -23,6 +23,7 @@
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 
@@ -93,13 +94,13 @@ bool ImportantFileWriter::WriteFileAtomically(const FilePath& path,
   return true;
 }
 
-ImportantFileWriter::ImportantFileWriter(
-    const FilePath& path, base::SequencedTaskRunner* task_runner)
-        : path_(path),
-          task_runner_(task_runner),
-          serializer_(NULL),
-          commit_interval_(TimeDelta::FromMilliseconds(
-              kDefaultCommitIntervalMs)) {
+ImportantFileWriter::ImportantFileWriter(const FilePath& path,
+                                         base::SequencedTaskRunner* task_runner)
+    : path_(path),
+      task_runner_(task_runner),
+      serializer_(NULL),
+      commit_interval_(TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)),
+      weak_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(task_runner_.get());
 }
@@ -126,11 +127,13 @@ void ImportantFileWriter::WriteNow(const std::string& data) {
   if (HasPendingWrite())
     timer_.Stop();
 
-  if (!task_runner_->PostTask(
+  if (!base::PostTaskAndReplyWithResult(
+          task_runner_,
           FROM_HERE,
           MakeCriticalClosure(
-              Bind(IgnoreResult(&ImportantFileWriter::WriteFileAtomically),
-                   path_, data)))) {
+              Bind(&ImportantFileWriter::WriteFileAtomically, path_, data)),
+          Bind(&ImportantFileWriter::ForwardSuccessfulWrite,
+               weak_factory_.GetWeakPtr()))) {
     // Posting the task to background message loop is not expected
     // to fail, but if it does, avoid losing data and just hit the disk
     // on the current thread.
@@ -164,4 +167,18 @@ void ImportantFileWriter::DoScheduledWrite() {
   serializer_ = NULL;
 }
 
+void ImportantFileWriter::RegisterOnNextSuccessfulWriteCallback(
+    const base::Closure& on_next_successful_write) {
+  DCHECK(on_next_successful_write_.is_null());
+  on_next_successful_write_ = on_next_successful_write;
+}
+
+void ImportantFileWriter::ForwardSuccessfulWrite(bool result) {
+  DCHECK(CalledOnValidThread());
+  if (result && !on_next_successful_write_.is_null()) {
+    on_next_successful_write_.Run();
+    on_next_successful_write_.Reset();
+  }
+}
+
 }  // namespace base
diff --git a/base/files/important_file_writer.h b/base/files/important_file_writer.h
index ba1c745..bdc6ba1 100644
--- a/base/files/important_file_writer.h
+++ b/base/files/important_file_writer.h
@@ -9,6 +9,7 @@
 
 #include "base/base_export.h"
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/non_thread_safe.h"
@@ -89,6 +90,11 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe {
   // Serialize data pending to be saved and execute write on backend thread.
   void DoScheduledWrite();
 
+  // Registers |on_next_successful_write| to be called once, on the next
+  // successful write event. Only one callback can be set at once.
+  void RegisterOnNextSuccessfulWriteCallback(
+      const base::Closure& on_next_successful_write);
+
   TimeDelta commit_interval() const {
     return commit_interval_;
   }
@@ -98,6 +104,13 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe {
   }
 
  private:
+  // If |result| is true and |on_next_successful_write_| is set, invokes
+  // |on_successful_write_| and then resets it; no-ops otherwise.
+  void ForwardSuccessfulWrite(bool result);
+
+  // Invoked once and then reset on the next successful write event.
+  base::Closure on_next_successful_write_;
+
   // Path being written to.
   const FilePath path_;
 
@@ -113,6 +126,8 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe {
   // Time delta after which scheduled data will be written to disk.
   TimeDelta commit_interval_;
 
+  WeakPtrFactory<ImportantFileWriter> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ImportantFileWriter);
 };
 
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) {