Introduce a new framework for back-and-forth tracked/protected preferences migration.

base/prefs/json_pref_store.cc

Index: base/prefs/json_pref_store.cc
diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc
index b0a16cc11431e01fd8661949388c11cdc972f156..60823c3b38aeed3b67dde150d8afae1df2a485e1 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -59,7 +59,12 @@ class FileThreadDeserializer
   // Reports deserialization result on the origin thread.
   void ReportOnOriginThread() {
     DCHECK(origin_loop_proxy_->BelongsToCurrentThread());
-    delegate_->OnFileRead(value_.release(), error_, no_dir_);
+    // TODO(gab): There is a race condition in error reporting here as |error_|
+    // is initialized on the origin thread, set on the task runner's thread in
+    // HandleErrors(), and then read here on the origin thread again with no
+    // synchronization barriers (which means we potentially report
+    // PREF_READ_ERROR_NONE when there actually was an error).

[Bernhard Bauer, 2014/05/01 12:37:09]

Huh, why is that? Doing PostTask should guarantee that these things happen
sequentially, no?

(We could probably use PostTaskAndReply here, though).

[gab, 2014/05/01 15:34:54]

Right, not a race condition in the sense that they won't happen sequentially in
time; but a race condition in the sense that:
1) |error_| is initialized to 1 on thread A.
(at which point both thread A/B see 1 as the value)
2) |error_| is written as 2 to on thread B.
(at which point thread B sees 2 as the value, but thread A might still see 1
until the caches are synchronized if it doesn't run under the same cache as
thread B)
3) |error_| is read on thread A (potentially erroneously reading 1).

Yes, the fix is to use PostTaskAndReplyWithResult() from task_runner_util.h

I'm not 100% sure how it avoids the same issue, it probably has to do with the
return value being bound in the callbacks which forces copies rather than using
the same reference on both threads... not sure... but I'm pretty sure it somehow
does it right or Chrome has a bigger problem...

[Bernhard Bauer, 2014/05/01 20:16:00]

I think it's already correct as it currently is, otherwise we would have a
bigger problem. I think this was discussed recently on chromium-dev
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/o05a009KHsQ/t1Ek_...),
and the takeaway was that posting tasks will use synchronization primitives with
memory barriers, which should ensure that everything is written out to RAM.

There are still advantages to using PostTaskAndReply, namely that destruction of
tasks and refcounted objects will happen on the originating thread, but the
member access should be okay.

(For PostTaskAndReplyWithResult() you would need to use a struct with the three
values you want to pass back though... Sigh.)

[gab, 2014/05/02 04:15:01]

Oh I see, this is very well hidden side-effect of posting tasks, but definitely
an interesting one.

So indeed there is no race here since the task is posted back to the origin
thread after the write to error_ has occurred; and since PostTask uses a lock,
memory is synchronized.

Thanks for the pointer!

(changed TODO to mention to use PostTaskAndReplyWithResult instead)

+    delegate_->OnFileRead(value_.Pass(), error_, no_dir_);
   }
 
   static base::Value* DoReading(const base::FilePath& path,
@@ -161,7 +166,8 @@ JsonPrefStore::JsonPrefStore(const base::FilePath& filename,
       writer_(filename, sequenced_task_runner),
       pref_filter_(pref_filter.Pass()),
       initialized_(false),
-      read_error_(PREF_READ_ERROR_OTHER) {}
+      read_error_(PREF_READ_ERROR_NONE),
+      weak_factory_(this) {}
 
 bool JsonPrefStore::GetValue(const std::string& key,
                              const base::Value** result) const {
@@ -224,6 +230,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 +246,25 @@ 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);
+  scoped_ptr<base::Value> value(
+      FileThreadDeserializer::DoReading(path_, &error, &no_dir));
+  OnFileRead(value.Pass(), 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(NULL, PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
+    OnFileRead(
+        scoped_ptr<base::Value>(), PREF_READ_ERROR_FILE_NOT_SPECIFIED, false);
     return;
   }
 
@@ -276,53 +290,65 @@ 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::InterceptNextFileRead(
+    const OnFileReadInterceptor& on_file_read_interceptor) {
+  DCHECK(on_file_read_interceptor_.is_null());
+  on_file_read_interceptor_ = on_file_read_interceptor;
+}
+
+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> unprocessed_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 (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());
+        unprocessed_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_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 (on_file_read_interceptor_.is_null()) {
+    FinalizeFileRead(initialization_successful, unprocessed_prefs.Pass(),
+                     false);
+  } else {
+    const FinalizePrefsReadCallback finalize_file_read(
+        base::Bind(&JsonPrefStore::FinalizeFileRead, this,
+                   initialization_successful));
+    on_file_read_interceptor_.Run(unprocessed_prefs.Pass(),
+                                  finalize_file_read);
+    on_file_read_interceptor_.Reset();
+  }
 }
 
 JsonPrefStore::~JsonPrefStore() {
@@ -337,3 +363,34 @@ bool JsonPrefStore::SerializeData(std::string* output) {
   serializer.set_pretty_print(true);
   return serializer.Serialize(*prefs_);
 }
+
+base::WeakPtr<JsonPrefStore> JsonPrefStore::FinalizeFileRead(
+    bool initialization_successful,
+    scoped_ptr<base::DictionaryValue> prefs,
+    bool schedule_write) {
+  if (!initialization_successful) {
+    FOR_EACH_OBSERVER(PrefStore::Observer,
+                      observers_,
+                      OnInitializationCompleted(false));
+    return weak_factory_.GetWeakPtr();
+  }
+
+  prefs_ = prefs.Pass();
+
+  initialized_ = true;
+
+  bool had_filter_modification =
+      pref_filter_ && pref_filter_->FilterOnLoad(prefs_.get());
+
+  if ((schedule_write || had_filter_modification) && !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 weak_factory_.GetWeakPtr();
+}