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

5 files changed, 340 insertions, 77 deletions

diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc
index b0a16cc..e99d64f 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -39,6 +39,8 @@ class FileThreadDeserializer
 
   void Start(const base::FilePath& path) {
     DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
+    // TODO(gab): This should use PostTaskAndReplyWithResult instead of using
+    // the |error_| member to pass data across tasks.
     sequenced_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&FileThreadDeserializer::ReadFileAndReport,
@@ -59,7 +61,7 @@ class FileThreadDeserializer
   // Reports deserialization result on the origin thread.
   void ReportOnOriginThread() {
     DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
-    delegate_->OnFileRead(value_.release(), error_, no_dir_);
+    delegate_->OnFileRead(value_.Pass(), error_, no_dir_);
   }
 
   static base::Value* DoReading(const base::FilePath& path,
@@ -161,7 +163,9 @@ JsonPrefStore::JsonPrefStore(const base::FilePath& filename,
       writer_(filename, sequenced_task_runner),
       pref_filter_(pref_filter.Pass()),
       initialized_(false),
-      read_error_(PREF_READ_ERROR_OTHER) {}
+      filtering_in_progress_(false),
+      read_error_(PREF_READ_ERROR_NONE) {
+}
 
 bool JsonPrefStore::GetValue(const std::string& key,
                              const base::Value** result) const {
@@ -224,6 +228,12 @@ void JsonPrefStore::RemoveValue(const std::string& key) {
     ReportValueChanged(key);
 }
 
+void JsonPrefStore::RemoveValueSilently(const std::string& key) {
+  prefs_->RemovePath(key, NULL);
+  if (!read_only_)
+    writer_.ScheduleWrite(this);
+}
+
 bool JsonPrefStore::ReadOnly() const {
   return read_only_;
 }
@@ -234,23 +244,26 @@ PersistentPrefStore::PrefReadError JsonPrefStore::GetReadError() const {
 
 PersistentPrefStore::PrefReadError JsonPrefStore::ReadPrefs() {
   if (path_.empty()) {
-    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(
+        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return PREF_READ_ERROR_FILE_NOT_SPECIFIED;
   }
 
   PrefReadError error;
   bool no_dir;
-  base::Value* value =
-      FileThreadDeserializer::DoReading(path_, &error, &no_dir);
-  OnFileRead(value, error, no_dir);
-  return error;
+  scoped_ptr<base::Value> value(
+      FileThreadDeserializer::DoReading(path_, &error, &no_dir));
+  OnFileRead(value.Pass(), error, no_dir);
+  return filtering_in_progress_ ? PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE :
+                                  error;
 }
 
-void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate *error_delegate) {
+void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
   initialized_ = false;
   error_delegate_.reset(error_delegate);
   if (path_.empty()) {
-    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(
+        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return;
   }
 
@@ -276,53 +289,63 @@ void JsonPrefStore::ReportValueChanged(const std::string& key) {
     writer_.ScheduleWrite(this);
 }
 
-void JsonPrefStore::OnFileRead(base::Value* value_owned,
+void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
+    const base::Closure& on_next_successful_write) {
+  writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
+}
+
+void JsonPrefStore::OnFileRead(scoped_ptr<base::Value> value,
                                PersistentPrefStore::PrefReadError error,
                                bool no_dir) {
-  scoped_ptr<base::Value> value(value_owned);
-  read_error_ = error;
+  scoped_ptr<base::DictionaryValue> unfiltered_prefs(new base::DictionaryValue);
 
-  if (no_dir) {
-    FOR_EACH_OBSERVER(PrefStore::Observer,
-                      observers_,
-                      OnInitializationCompleted(false));
-    return;
-  }
+  read_error_ = error;
 
-  initialized_ = true;
+  bool initialization_successful = !no_dir;
 
-  switch (error) {
-    case PREF_READ_ERROR_ACCESS_DENIED:
-    case PREF_READ_ERROR_FILE_OTHER:
-    case PREF_READ_ERROR_FILE_LOCKED:
-    case PREF_READ_ERROR_JSON_TYPE:
-    case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
-      read_only_ = true;
-      break;
-    case PREF_READ_ERROR_NONE:
-      DCHECK(value.get());
-      prefs_.reset(static_cast<base::DictionaryValue*>(value.release()));
-      break;
-    case PREF_READ_ERROR_NO_FILE:
-      // If the file just doesn't exist, maybe this is first run.  In any case
-      // there's no harm in writing out default prefs in this case.
-      break;
-    case PREF_READ_ERROR_JSON_PARSE:
-    case PREF_READ_ERROR_JSON_REPEAT:
-      break;
-    default:
-      NOTREACHED() << "Unknown error: " << error;
+  if (initialization_successful) {
+    switch (read_error_) {
+      case PREF_READ_ERROR_ACCESS_DENIED:
+      case PREF_READ_ERROR_FILE_OTHER:
+      case PREF_READ_ERROR_FILE_LOCKED:
+      case PREF_READ_ERROR_JSON_TYPE:
+      case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
+        read_only_ = true;
+        break;
+      case PREF_READ_ERROR_NONE:
+        DCHECK(value.get());
+        unfiltered_prefs.reset(
+            static_cast<base::DictionaryValue*>(value.release()));
+        break;
+      case PREF_READ_ERROR_NO_FILE:
+        // If the file just doesn't exist, maybe this is first run.  In any case
+        // there's no harm in writing out default prefs in this case.
+        break;
+      case PREF_READ_ERROR_JSON_PARSE:
+      case PREF_READ_ERROR_JSON_REPEAT:
+        break;
+      case PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE:
+        // This is a special error code to be returned by ReadPrefs when it
+        // can't complete synchronously, it should never be returned by the read
+        // operation itself.
+        NOTREACHED();
+        break;
+      case PREF_READ_ERROR_MAX_ENUM:
+        NOTREACHED();
+        break;
+    }
   }
 
-  if (pref_filter_ && pref_filter_->FilterOnLoad(prefs_.get()))
-    writer_.ScheduleWrite(this);
-
-  if (error_delegate_.get() && error != PREF_READ_ERROR_NONE)
-    error_delegate_->OnError(error);
-
-  FOR_EACH_OBSERVER(PrefStore::Observer,
-                    observers_,
-                    OnInitializationCompleted(true));
+  if (pref_filter_) {
+    filtering_in_progress_ = true;
+    const PrefFilter::PostFilterOnLoadCallback post_filter_on_load_callback(
+        base::Bind(
+            &JsonPrefStore::FinalizeFileRead, this, initialization_successful));
+    pref_filter_->FilterOnLoad(post_filter_on_load_callback,
+                               unfiltered_prefs.Pass());
+  } else {
+    FinalizeFileRead(initialization_successful, unfiltered_prefs.Pass(), false);
+  }
 }
 
 JsonPrefStore::~JsonPrefStore() {
@@ -337,3 +360,32 @@ bool JsonPrefStore::SerializeData(std::string* output) {
   serializer.set_pretty_print(true);
   return serializer.Serialize(*prefs_);
 }
+
+void JsonPrefStore::FinalizeFileRead(bool initialization_successful,
+                                     scoped_ptr<base::DictionaryValue> prefs,
+                                     bool schedule_write) {
+  filtering_in_progress_ = false;
+
+  if (!initialization_successful) {
+    FOR_EACH_OBSERVER(PrefStore::Observer,
+                      observers_,
+                      OnInitializationCompleted(false));
+    return;
+  }
+
+  prefs_ = prefs.Pass();
+
+  initialized_ = true;
+
+  if (schedule_write && !read_only_)
+    writer_.ScheduleWrite(this);
+
+  if (error_delegate_ && read_error_ != PREF_READ_ERROR_NONE)
+    error_delegate_->OnError(read_error_);
+
+  FOR_EACH_OBSERVER(PrefStore::Observer,
+                    observers_,
+                    OnInitializationCompleted(true));
+
+  return;
+}
diff --git a/base/prefs/json_pref_store.h b/base/prefs/json_pref_store.h
index ad13feb..49e74ee 100644
--- a/base/prefs/json_pref_store.h
+++ b/base/prefs/json_pref_store.h
@@ -9,10 +9,12 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/important_file_writer.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/observer_list.h"
 #include "base/prefs/base_prefs_export.h"
@@ -32,7 +34,8 @@ class Value;
 // A writable PrefStore implementation that is used for user preferences.
 class BASE_PREFS_EXPORT JsonPrefStore
     : public PersistentPrefStore,
-      public base::ImportantFileWriter::DataSerializer {
+      public base::ImportantFileWriter::DataSerializer,
+      public base::SupportsWeakPtr<JsonPrefStore> {
  public:
   // Returns instance of SequencedTaskRunner which guarantees that file
   // operations on the same file will be executed in sequenced order.
@@ -63,16 +66,37 @@ class BASE_PREFS_EXPORT JsonPrefStore
   virtual void RemoveValue(const std::string& key) OVERRIDE;
   virtual bool ReadOnly() const OVERRIDE;
   virtual PrefReadError GetReadError() const OVERRIDE;
+  // Note this method may be asynchronous if this instance has a |pref_filter_|
+  // in which case it will return PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE.
+  // See details in pref_filter.h.
   virtual PrefReadError ReadPrefs() OVERRIDE;
   virtual void ReadPrefsAsync(ReadErrorDelegate* error_delegate) OVERRIDE;
   virtual void CommitPendingWrite() OVERRIDE;
   virtual void ReportValueChanged(const std::string& key) OVERRIDE;
 
-  // This method is called after JSON file has been read. Method takes
-  // ownership of the |value| pointer. Note, this method is used with
-  // asynchronous file reading, so class exposes it only for the internal needs.
-  // (read: do not call it manually).
-  void OnFileRead(base::Value* value_owned, PrefReadError error, bool no_dir);
+  // Just like RemoveValue(), but doesn't notify observers. Used when doing some
+  // cleanup that shouldn't otherwise alert observers.
+  void RemoveValueSilently(const std::string& key);
+
+  // Registers |on_next_successful_write| to be called once, on the next
+  // successful write event of |writer_|.
+  void RegisterOnNextSuccessfulWriteCallback(
+      const base::Closure& on_next_successful_write);
+
+  // This method is called after the JSON file has been read.  It then hands
+  // |value| (or an empty dictionary in some read error cases) to the
+  // |pref_filter| if one is set. It also gives a callback pointing at
+  // FinalizeFileRead() to that |pref_filter_| which is then responsible for
+  // invoking it when done. If there is no |pref_filter_|, FinalizeFileRead()
+  // is invoked directly.
+  // Note, this method is used with asynchronous file reading, so this class
+  // exposes it only for the internal needs (read: do not call it manually).
+  // TODO(gab): Move this method to the private section and hand a callback to
+  // it to FileThreadDeserializer rather than exposing this public method and
+  // giving a JsonPrefStore* to FileThreadDeserializer.
+  void OnFileRead(scoped_ptr<base::Value> value,
+                  PrefReadError error,
+                  bool no_dir);
 
  private:
   virtual ~JsonPrefStore();
@@ -80,6 +104,17 @@ class BASE_PREFS_EXPORT JsonPrefStore
   // ImportantFileWriter::DataSerializer overrides:
   virtual bool SerializeData(std::string* output) OVERRIDE;
 
+  // This method is called after the JSON file has been read and the result has
+  // potentially been intercepted and modified by |pref_filter_|.
+  // |initialization_successful| is pre-determined by OnFileRead() and should
+  // be used when reporting OnInitializationCompleted().
+  // |schedule_write| indicates whether a write should be immediately scheduled
+  // (typically because the |pref_filter_| has already altered the |prefs|) --
+  // this will be ignored if this store is read-only.
+  void FinalizeFileRead(bool initialization_successful,
+                        scoped_ptr<base::DictionaryValue> prefs,
+                        bool schedule_write);
+
   base::FilePath path_;
   const scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
 
@@ -96,6 +131,7 @@ class BASE_PREFS_EXPORT JsonPrefStore
   scoped_ptr<ReadErrorDelegate> error_delegate_;
 
   bool initialized_;
+  bool filtering_in_progress_;
   PrefReadError read_error_;
 
   std::set<std::string> keys_need_empty_value_;
diff --git a/base/prefs/json_pref_store_unittest.cc b/base/prefs/json_pref_store_unittest.cc
index f4e1e51..9e273c7 100644
--- a/base/prefs/json_pref_store_unittest.cc
+++ b/base/prefs/json_pref_store_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "base/prefs/json_pref_store.h"
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted.h"
@@ -25,6 +26,50 @@ namespace {
 
 const char kHomePage[] = "homepage";
 
+// A PrefFilter that will intercept all calls to FilterOnLoad() and hold on
+// to the |prefs| until explicitly asked to release them.
+class InterceptingPrefFilter : public PrefFilter {
+ public:
+  InterceptingPrefFilter();
+  virtual ~InterceptingPrefFilter();
+
+  // PrefFilter implementation:
+  virtual void FilterOnLoad(
+      const PostFilterOnLoadCallback& post_filter_on_load_callback,
+      scoped_ptr<base::DictionaryValue> pref_store_contents) OVERRIDE;
+  virtual void FilterUpdate(const std::string& path) OVERRIDE {}
+  virtual void FilterSerializeData(
+      const base::DictionaryValue* pref_store_contents) OVERRIDE {}
+
+  bool has_intercepted_prefs() const { return intercepted_prefs_ != NULL; }
+
+  // Finalize an intercepted read, handing |intercept_prefs_| back to its
+  // JsonPrefStore.
+  void ReleasePrefs();
+
+ private:
+  PostFilterOnLoadCallback post_filter_on_load_callback_;
+  scoped_ptr<base::DictionaryValue> intercepted_prefs_;
+
+  DISALLOW_COPY_AND_ASSIGN(InterceptingPrefFilter);
+};
+
+InterceptingPrefFilter::InterceptingPrefFilter() {}
+InterceptingPrefFilter::~InterceptingPrefFilter() {}
+
+void InterceptingPrefFilter::FilterOnLoad(
+    const PostFilterOnLoadCallback& post_filter_on_load_callback,
+    scoped_ptr<base::DictionaryValue> pref_store_contents) {
+  post_filter_on_load_callback_ = post_filter_on_load_callback;
+  intercepted_prefs_ = pref_store_contents.Pass();
+}
+
+void InterceptingPrefFilter::ReleasePrefs() {
+  EXPECT_FALSE(post_filter_on_load_callback_.is_null());
+  post_filter_on_load_callback_.Run(intercepted_prefs_.Pass(), false);
+  post_filter_on_load_callback_.Reset();
+}
+
 class MockPrefStoreObserver : public PrefStore::Observer {
  public:
   MOCK_METHOD1(OnPrefValueChanged, void (const std::string&));
@@ -157,7 +202,7 @@ void RunBasicJsonPrefStoreTest(JsonPrefStore* pref_store,
 
 TEST_F(JsonPrefStoreTest, Basic) {
   ASSERT_TRUE(base::CopyFile(data_dir_.AppendASCII("read.json"),
-                                  temp_dir_.path().AppendASCII("write.json")));
+                             temp_dir_.path().AppendASCII("write.json")));
 
   // Test that the persistent value can be loaded.
   base::FilePath input_file = temp_dir_.path().AppendASCII("write.json");
@@ -167,7 +212,8 @@ TEST_F(JsonPrefStoreTest, Basic) {
       message_loop_.message_loop_proxy().get(),
       scoped_ptr<PrefFilter>());
   ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs());
-  ASSERT_FALSE(pref_store->ReadOnly());
+  EXPECT_FALSE(pref_store->ReadOnly());
+  EXPECT_TRUE(pref_store->IsInitializationComplete());
 
   // The JSON file looks like this:
   // {
@@ -185,7 +231,7 @@ TEST_F(JsonPrefStoreTest, Basic) {
 
 TEST_F(JsonPrefStoreTest, BasicAsync) {
   ASSERT_TRUE(base::CopyFile(data_dir_.AppendASCII("read.json"),
-                                  temp_dir_.path().AppendASCII("write.json")));
+                             temp_dir_.path().AppendASCII("write.json")));
 
   // Test that the persistent value can be loaded.
   base::FilePath input_file = temp_dir_.path().AppendASCII("write.json");
@@ -208,7 +254,8 @@ TEST_F(JsonPrefStoreTest, BasicAsync) {
     RunLoop().RunUntilIdle();
     pref_store->RemoveObserver(&mock_observer);
 
-    ASSERT_FALSE(pref_store->ReadOnly());
+    EXPECT_FALSE(pref_store->ReadOnly());
+    EXPECT_TRUE(pref_store->IsInitializationComplete());
   }
 
   // The JSON file looks like this:
@@ -301,4 +348,117 @@ TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) {
   EXPECT_FALSE(pref_store->ReadOnly());
 }
 
+TEST_F(JsonPrefStoreTest, ReadWithInterceptor) {
+  ASSERT_TRUE(base::CopyFile(data_dir_.AppendASCII("read.json"),
+                             temp_dir_.path().AppendASCII("write.json")));
+
+  // Test that the persistent value can be loaded.
+  base::FilePath input_file = temp_dir_.path().AppendASCII("write.json");
+  ASSERT_TRUE(PathExists(input_file));
+
+  scoped_ptr<InterceptingPrefFilter> intercepting_pref_filter(
+      new InterceptingPrefFilter());
+  InterceptingPrefFilter* raw_intercepting_pref_filter_ =
+      intercepting_pref_filter.get();
+  scoped_refptr<JsonPrefStore> pref_store =
+      new JsonPrefStore(input_file,
+                        message_loop_.message_loop_proxy().get(),
+                        intercepting_pref_filter.PassAs<PrefFilter>());
+
+  ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE,
+            pref_store->ReadPrefs());
+  EXPECT_FALSE(pref_store->ReadOnly());
+
+  // The store shouldn't be considered initialized until the interceptor
+  // returns.
+  EXPECT_TRUE(raw_intercepting_pref_filter_->has_intercepted_prefs());
+  EXPECT_FALSE(pref_store->IsInitializationComplete());
+  EXPECT_FALSE(pref_store->GetValue(kHomePage, NULL));
+
+  raw_intercepting_pref_filter_->ReleasePrefs();
+
+  EXPECT_FALSE(raw_intercepting_pref_filter_->has_intercepted_prefs());
+  EXPECT_TRUE(pref_store->IsInitializationComplete());
+  EXPECT_TRUE(pref_store->GetValue(kHomePage, NULL));
+
+  // The JSON file looks like this:
+  // {
+  //   "homepage": "http://www.cnn.com",
+  //   "some_directory": "/usr/local/",
+  //   "tabs": {
+  //     "new_windows_in_tabs": true,
+  //     "max_tabs": 20
+  //   }
+  // }
+
+  RunBasicJsonPrefStoreTest(
+      pref_store.get(), input_file, data_dir_.AppendASCII("write.golden.json"));
+}
+
+TEST_F(JsonPrefStoreTest, ReadAsyncWithInterceptor) {
+  ASSERT_TRUE(base::CopyFile(data_dir_.AppendASCII("read.json"),
+                             temp_dir_.path().AppendASCII("write.json")));
+
+  // Test that the persistent value can be loaded.
+  base::FilePath input_file = temp_dir_.path().AppendASCII("write.json");
+  ASSERT_TRUE(PathExists(input_file));
+
+  scoped_ptr<InterceptingPrefFilter> intercepting_pref_filter(
+      new InterceptingPrefFilter());
+  InterceptingPrefFilter* raw_intercepting_pref_filter_ =
+      intercepting_pref_filter.get();
+  scoped_refptr<JsonPrefStore> pref_store =
+      new JsonPrefStore(input_file,
+                        message_loop_.message_loop_proxy().get(),
+                        intercepting_pref_filter.PassAs<PrefFilter>());
+
+  MockPrefStoreObserver mock_observer;
+  pref_store->AddObserver(&mock_observer);
+
+  // Ownership of the |mock_error_delegate| is handed to the |pref_store| below.
+  MockReadErrorDelegate* mock_error_delegate = new MockReadErrorDelegate;
+
+  {
+    pref_store->ReadPrefsAsync(mock_error_delegate);
+
+    EXPECT_CALL(mock_observer, OnInitializationCompleted(true)).Times(0);
+    // EXPECT_CALL(*mock_error_delegate,
+    //             OnError(PersistentPrefStore::PREF_READ_ERROR_NONE)).Times(0);
+    RunLoop().RunUntilIdle();
+
+    EXPECT_FALSE(pref_store->ReadOnly());
+    EXPECT_TRUE(raw_intercepting_pref_filter_->has_intercepted_prefs());
+    EXPECT_FALSE(pref_store->IsInitializationComplete());
+    EXPECT_FALSE(pref_store->GetValue(kHomePage, NULL));
+  }
+
+  {
+    EXPECT_CALL(mock_observer, OnInitializationCompleted(true)).Times(1);
+    // EXPECT_CALL(*mock_error_delegate,
+    //             OnError(PersistentPrefStore::PREF_READ_ERROR_NONE)).Times(0);
+
+    raw_intercepting_pref_filter_->ReleasePrefs();
+
+    EXPECT_FALSE(pref_store->ReadOnly());
+    EXPECT_FALSE(raw_intercepting_pref_filter_->has_intercepted_prefs());
+    EXPECT_TRUE(pref_store->IsInitializationComplete());
+    EXPECT_TRUE(pref_store->GetValue(kHomePage, NULL));
+  }
+
+  pref_store->RemoveObserver(&mock_observer);
+
+  // The JSON file looks like this:
+  // {
+  //   "homepage": "http://www.cnn.com",
+  //   "some_directory": "/usr/local/",
+  //   "tabs": {
+  //     "new_windows_in_tabs": true,
+  //     "max_tabs": 20
+  //   }
+  // }
+
+  RunBasicJsonPrefStoreTest(
+      pref_store.get(), input_file, data_dir_.AppendASCII("write.golden.json"));
+}
+
 }  // namespace base
diff --git a/base/prefs/persistent_pref_store.h b/base/prefs/persistent_pref_store.h
index 44f3442..177d860 100644
--- a/base/prefs/persistent_pref_store.h
+++ b/base/prefs/persistent_pref_store.h
@@ -17,19 +17,22 @@ class BASE_PREFS_EXPORT PersistentPrefStore : public WriteablePrefStore {
  public:
   // Unique integer code for each type of error so we can report them
   // distinctly in a histogram.
-  // NOTE: Don't change the order here as it will change the server's meaning
-  // of the histogram.
+  // NOTE: Don't change the explicit values of the enums as it will change the
+  // server's meaning of the histogram.
   enum PrefReadError {
     PREF_READ_ERROR_NONE = 0,
-    PREF_READ_ERROR_JSON_PARSE,
-    PREF_READ_ERROR_JSON_TYPE,
-    PREF_READ_ERROR_ACCESS_DENIED,
-    PREF_READ_ERROR_FILE_OTHER,
-    PREF_READ_ERROR_FILE_LOCKED,
-    PREF_READ_ERROR_NO_FILE,
-    PREF_READ_ERROR_JSON_REPEAT,
-    PREF_READ_ERROR_OTHER,
-    PREF_READ_ERROR_FILE_NOT_SPECIFIED,
+    PREF_READ_ERROR_JSON_PARSE = 1,
+    PREF_READ_ERROR_JSON_TYPE = 2,
+    PREF_READ_ERROR_ACCESS_DENIED = 3,
+    PREF_READ_ERROR_FILE_OTHER = 4,
+    PREF_READ_ERROR_FILE_LOCKED = 5,
+    PREF_READ_ERROR_NO_FILE = 6,
+    PREF_READ_ERROR_JSON_REPEAT = 7,
+    // PREF_READ_ERROR_OTHER = 8,  // Deprecated.
+    PREF_READ_ERROR_FILE_NOT_SPECIFIED = 9,
+    // Indicates that ReadPrefs() couldn't complete synchronously and is waiting
+    // for an asynchronous task to complete first.
+    PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE = 10,
     PREF_READ_ERROR_MAX_ENUM
   };
 
diff --git a/base/prefs/pref_filter.h b/base/prefs/pref_filter.h
index ce07f92..1020029 100644
--- a/base/prefs/pref_filter.h
+++ b/base/prefs/pref_filter.h
@@ -7,6 +7,8 @@
 
 #include <string>
 
+#include "base/callback_forward.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/prefs/base_prefs_export.h"
 
 namespace base {
@@ -18,15 +20,25 @@ class Value;
 // Currently supported only by JsonPrefStore.
 class BASE_PREFS_EXPORT PrefFilter {
  public:
+  // A callback to be invoked when |prefs| have been read (and possibly
+  // pre-modified) and are now ready to be handed back to this callback's
+  // builder. |schedule_write| indicates whether a write should be immediately
+  // scheduled (typically because the |prefs| were pre-modified).
+  typedef base::Callback<void(scoped_ptr<base::DictionaryValue> prefs,
+                              bool schedule_write)> PostFilterOnLoadCallback;
+
   virtual ~PrefFilter() {}
 
-  // Receives notification when the pref store data has been loaded but before
-  // Observers are notified.
-  // Changes made by a PrefFilter during FilterOnLoad do not result in
-  // notifications to |PrefStore::Observer|s.
-  // Implementations should return true if they modify the dictionary, to allow
-  // the changes to be persisted.
-  virtual bool FilterOnLoad(base::DictionaryValue* pref_store_contents) = 0;
+  // This method is given ownership of the |pref_store_contents| read from disk
+  // before the underlying PersistentPrefStore gets to use them. It must hand
+  // them back via |post_filter_on_load_callback|, but may modify them first.
+  // Note: This method is asynchronous, which may make calls like
+  // PersistentPrefStore::ReadPrefs() asynchronous. The owner of filtered
+  // PersistentPrefStores should handle this to make the reads look synchronous
+  // to external users (see SegregatedPrefStore::ReadPrefs() for an example).
+  virtual void FilterOnLoad(
+      const PostFilterOnLoadCallback& post_filter_on_load_callback,
+      scoped_ptr<base::DictionaryValue> pref_store_contents) = 0;
 
   // Receives notification when a pref store value is changed, before Observers
   // are notified.