Revert 269415 "Introduce a new framework for back-and-forth trac..."

Reverting because this appears to have caused Linux TSAN redness.


> 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

TBR=gab@chromium.org

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

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

5 files changed, 77 insertions, 348 deletions

diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc
index e99d64f..b0a16cc 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -39,8 +39,6 @@ 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,
@@ -61,7 +59,7 @@ class FileThreadDeserializer
   // Reports deserialization result on the origin thread.
   void ReportOnOriginThread() {
     DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
-    delegate_->OnFileRead(value_.Pass(), error_, no_dir_);
+    delegate_->OnFileRead(value_.release(), error_, no_dir_);
   }
 
   static base::Value* DoReading(const base::FilePath& path,
@@ -163,9 +161,7 @@ JsonPrefStore::JsonPrefStore(const base::FilePath& filename,
       writer_(filename, sequenced_task_runner),
       pref_filter_(pref_filter.Pass()),
       initialized_(false),
-      filtering_in_progress_(false),
-      read_error_(PREF_READ_ERROR_NONE) {
-}
+      read_error_(PREF_READ_ERROR_OTHER) {}
 
 bool JsonPrefStore::GetValue(const std::string& key,
                              const base::Value** result) const {
@@ -228,12 +224,6 @@ 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_;
 }
@@ -244,26 +234,23 @@ PersistentPrefStore::PrefReadError JsonPrefStore::GetReadError() const {
 
 PersistentPrefStore::PrefReadError JsonPrefStore::ReadPrefs() {
   if (path_.empty()) {
-    OnFileRead(
-        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return PREF_READ_ERROR_FILE_NOT_SPECIFIED;
   }
 
   PrefReadError error;
   bool no_dir;
-  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;
+  base::Value* value =
+      FileThreadDeserializer::DoReading(path_, &error, &no_dir);
+  OnFileRead(value, error, no_dir);
+  return error;
 }
 
-void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) {
+void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate *error_delegate) {
   initialized_ = false;
   error_delegate_.reset(error_delegate);
   if (path_.empty()) {
-    OnFileRead(
-        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return;
   }
 
@@ -289,63 +276,53 @@ void JsonPrefStore::ReportValueChanged(const std::string& key) {
     writer_.ScheduleWrite(this);
 }
 
-void JsonPrefStore::RegisterOnNextSuccessfulWriteCallback(
-    const base::Closure& on_next_successful_write) {
-  writer_.RegisterOnNextSuccessfulWriteCallback(on_next_successful_write);
-}
-
-void JsonPrefStore::OnFileRead(scoped_ptr<base::Value> value,
+void JsonPrefStore::OnFileRead(base::Value* value_owned,
                                PersistentPrefStore::PrefReadError error,
                                bool no_dir) {
-  scoped_ptr<base::DictionaryValue> unfiltered_prefs(new base::DictionaryValue);
-
+  scoped_ptr<base::Value> value(value_owned);
   read_error_ = error;
 
-  bool initialization_successful = !no_dir;
-
-  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 (no_dir) {
+    FOR_EACH_OBSERVER(PrefStore::Observer,
+                      observers_,
+                      OnInitializationCompleted(false));
+    return;
   }
 
-  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);
+  initialized_ = true;
+
+  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 (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));
 }
 
 JsonPrefStore::~JsonPrefStore() {
@@ -360,32 +337,3 @@ 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 49e74ee..ad13feb 100644
--- a/base/prefs/json_pref_store.h
+++ b/base/prefs/json_pref_store.h
@@ -9,12 +9,10 @@
 #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"
@@ -34,8 +32,7 @@ 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::SupportsWeakPtr<JsonPrefStore> {
+      public base::ImportantFileWriter::DataSerializer {
  public:
   // Returns instance of SequencedTaskRunner which guarantees that file
   // operations on the same file will be executed in sequenced order.
@@ -66,37 +63,16 @@ 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;
 
-  // 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);
+  // 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);
 
  private:
   virtual ~JsonPrefStore();
@@ -104,17 +80,6 @@ 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_;
 
@@ -131,7 +96,6 @@ 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 4c9c847..f4e1e51 100644
--- a/base/prefs/json_pref_store_unittest.cc
+++ b/base/prefs/json_pref_store_unittest.cc
@@ -4,12 +4,10 @@
 
 #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"
 #include "base/memory/scoped_ptr.h"
-#include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_filter.h"
 #include "base/run_loop.h"
@@ -27,50 +25,6 @@ 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&));
@@ -94,13 +48,6 @@ class JsonPrefStoreTest : public testing::Test {
     ASSERT_TRUE(PathExists(data_dir_));
   }
 
-  virtual void TearDown() OVERRIDE {
-    // Make sure all pending tasks have been processed (e.g., deleting the
-    // JsonPrefStore may post write tasks).
-    message_loop_.PostTask(FROM_HERE, MessageLoop::QuitWhenIdleClosure());
-    message_loop_.Run();
-  }
-
   // The path to temporary directory used to contain the test operations.
   base::ScopedTempDir temp_dir_;
   // The path to the directory where the test data is stored.
@@ -210,7 +157,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");
@@ -220,8 +167,7 @@ TEST_F(JsonPrefStoreTest, Basic) {
       message_loop_.message_loop_proxy().get(),
       scoped_ptr<PrefFilter>());
   ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs());
-  EXPECT_FALSE(pref_store->ReadOnly());
-  EXPECT_TRUE(pref_store->IsInitializationComplete());
+  ASSERT_FALSE(pref_store->ReadOnly());
 
   // The JSON file looks like this:
   // {
@@ -239,7 +185,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");
@@ -262,8 +208,7 @@ TEST_F(JsonPrefStoreTest, BasicAsync) {
     RunLoop().RunUntilIdle();
     pref_store->RemoveObserver(&mock_observer);
 
-    EXPECT_FALSE(pref_store->ReadOnly());
-    EXPECT_TRUE(pref_store->IsInitializationComplete());
+    ASSERT_FALSE(pref_store->ReadOnly());
   }
 
   // The JSON file looks like this:
@@ -356,117 +301,4 @@ 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 177d860..44f3442 100644
--- a/base/prefs/persistent_pref_store.h
+++ b/base/prefs/persistent_pref_store.h
@@ -17,22 +17,19 @@ 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 explicit values of the enums as it will change the
-  // server's meaning of the histogram.
+  // NOTE: Don't change the order here as it will change the server's meaning
+  // of the histogram.
   enum PrefReadError {
     PREF_READ_ERROR_NONE = 0,
-    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_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_MAX_ENUM
   };
 
diff --git a/base/prefs/pref_filter.h b/base/prefs/pref_filter.h
index 1020029..ce07f92 100644
--- a/base/prefs/pref_filter.h
+++ b/base/prefs/pref_filter.h
@@ -7,8 +7,6 @@
 
 #include <string>
 
-#include "base/callback_forward.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/prefs/base_prefs_export.h"
 
 namespace base {
@@ -20,25 +18,15 @@ 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() {}
 
-  // 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 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;
 
   // Receives notification when a pref store value is changed, before Observers
   // are notified.