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

chrome/browser/prefs/tracked/tracked_preferences_migration.cc

Index: chrome/browser/prefs/tracked/tracked_preferences_migration.cc
diff --git a/chrome/browser/prefs/tracked/tracked_preferences_migration.cc b/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
new file mode 100644
index 0000000000000000000000000000000000000000..552a089712ed1eb473f6ff8ab4cfc0ecb74d85aa
--- /dev/null
+++ b/chrome/browser/prefs/tracked/tracked_preferences_migration.cc
@@ -0,0 +1,239 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/prefs/tracked/tracked_preferences_migration.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/prefs/json_pref_store.h"
+#include "base/values.h"
+
+namespace {
+
+class TrackedPreferencesMigrator
+    : public base::RefCounted<TrackedPreferencesMigrator> {
+ public:
+  // Constructs a TrackedPreferencesMigrator which sets itself up as the
+  // OnFileReadInterceptor for both |unprotected_pref_store| and
+  // |protected_pref_store|. It guarantees that the most recent pref value
+  // for |unprotected_pref_names| and |protected_pref_names| are in the
+  // appropriate JsonPrefStores before those stores even sees those values.
+  TrackedPreferencesMigrator(
+      const std::set<std::string>& unprotected_pref_names,
+      const std::set<std::string>& protected_pref_names,
+      JsonPrefStore* unprotected_pref_store,
+      JsonPrefStore* protected_pref_store);
+
+ private:
+  friend class base::RefCounted<TrackedPreferencesMigrator>;
+
+  enum PrefStoreID {
+    UNPROTECTED_PREFERENCE_STORE,
+    PROTECTED_PREFERENCE_STORE
+  };
+
+  ~TrackedPreferencesMigrator();
+
+  // Stores the data coming in for the store identified by |id| into this class
+  // and then calls MigrateIfReady();
+  void OnFileRead(
+      PrefStoreID id,
+      scoped_ptr<base::DictionaryValue> prefs,
+      const JsonPrefStore::FinalizePrefsReadCallback& finalize_prefs_read);
+
+  // Proceeds with migration if both |unprotected_prefs_| and |protected_prefs_|
+  // have been set.
+  void MigrateIfReady();
+
+  const std::set<std::string> unprotected_pref_names_;
+  const std::set<std::string> protected_pref_names_;
+
+  scoped_ptr<base::DictionaryValue> unprotected_prefs_;
+  scoped_ptr<base::DictionaryValue> protected_prefs_;
+
+  JsonPrefStore::FinalizePrefsReadCallback finalize_unprotected_prefs_read_;
+  JsonPrefStore::FinalizePrefsReadCallback finalize_protected_prefs_read_;
+
+  DISALLOW_COPY_AND_ASSIGN(TrackedPreferencesMigrator);
+};
+
+// Silently removes all |keys_to_clean| from |pref_store| if the weak pointer is
+// still valid.
+void CleanupPrefStore(const base::WeakPtr<JsonPrefStore>& pref_store,
+                      const std::set<std::string>& keys_to_clean) {
+  if (pref_store) {
+    for (std::set<std::string>::const_iterator it = keys_to_clean.begin();
+         it != keys_to_clean.end(); ++it) {
+      // Silently clean the values as they have already been copied to their
+      // new authoritative store and we don't want to alert PrefStore observers
+      // about the irrelevant removal of this value which no one should be
+      // reading anymore.
+      pref_store->RemoveValueSilently(*it);
+    }
+  }
+}
+
+// If |wait_for_commit_to_destination_store|: schedules a cleanup of the
+// |keys_to_clean| from the |source_pref_store| once the
+// |destination_pref_store| they were migrated to was successfully written to
+// disk. Otherwise, executes the cleanup right away.
+void ScheduleSourcePrefStoreCleanup(
+    const base::WeakPtr<JsonPrefStore>& source_pref_store,
+    const std::set<std::string>& keys_to_clean,
+    bool wait_for_commit_to_destination_store,
+    JsonPrefStore* destination_pref_store) {
+  DCHECK(source_pref_store);
+  DCHECK(destination_pref_store);
+  DCHECK(!keys_to_clean.empty());
+  if (wait_for_commit_to_destination_store) {
+    destination_pref_store->RegisterOnNextSuccessfulWriteCallback(
+        base::Bind(&CleanupPrefStore, source_pref_store, keys_to_clean));
+  } else {
+    CleanupPrefStore(source_pref_store, keys_to_clean);
+  }
+}
+
+// Copies the value of each pref in |pref_names| which is set in |old_store|,
+// but not in |new_store| into |new_store|. Sets |old_store_needs_cleanup| to
+// true if any old duplicates remain in |old_store| and sets |new_store_altered|
+// to true if any value was copied to |new_store|.
+void MigratePrefsFromOldToNewStore(
+    const std::set<std::string>& pref_names,
+    const base::DictionaryValue* old_store,
+    base::DictionaryValue* new_store,
+    bool* old_store_needs_cleanup,
+    bool* new_store_altered) {
+  for (std::set<std::string>::const_iterator it = pref_names.begin();
+       it != pref_names.end(); ++it) {
+    const std::string& pref_name = *it;
+
+    const base::Value* value_in_old_store = NULL;
+    if (!old_store->Get(pref_name, &value_in_old_store))
+      continue;
+
+    // Whether this value ends up being copied below or was left behind by a
+    // previous incomplete migration, it should be cleaned up.
+    *old_store_needs_cleanup = true;
+
+    if (new_store->Get(pref_name, NULL))
+      continue;
+
+    // Copy the value from |old_store| to |new_store| rather than moving it to
+    // avoid data loss should |old_store| be flushed to disk without |new_store|
+    // having equivalently been successfully flushed to disk (e.g., on crash or
+    // in cases where |new_store| is read-only following a read error on
+    // startup).
+    new_store->Set(pref_name, value_in_old_store->DeepCopy());
+    *new_store_altered = true;
+  }
+}
+
+TrackedPreferencesMigrator::TrackedPreferencesMigrator(
+    const std::set<std::string>& unprotected_pref_names,
+    const std::set<std::string>& protected_pref_names,
+    JsonPrefStore* unprotected_pref_store,
+    JsonPrefStore* protected_pref_store)
+    : unprotected_pref_names_(unprotected_pref_names),
+      protected_pref_names_(protected_pref_names) {
+  // The callbacks bound below will own this TrackedPreferencesMigrator by
+  // reference.
+  unprotected_pref_store->InterceptNextFileRead(
+      base::Bind(&TrackedPreferencesMigrator::OnFileRead,
+                 this,
+                 UNPROTECTED_PREFERENCE_STORE));
+  protected_pref_store->InterceptNextFileRead(
+      base::Bind(&TrackedPreferencesMigrator::OnFileRead,
+                 this,
+                 PROTECTED_PREFERENCE_STORE));
+}
+
+TrackedPreferencesMigrator::~TrackedPreferencesMigrator() {}
+
+void TrackedPreferencesMigrator::OnFileRead(
+    PrefStoreID id,
+    scoped_ptr<base::DictionaryValue> prefs,
+    const JsonPrefStore::FinalizePrefsReadCallback& finalize_prefs_read) {
+  switch (id) {
+    case UNPROTECTED_PREFERENCE_STORE:
+      unprotected_prefs_ = prefs.Pass();
+      finalize_unprotected_prefs_read_ = finalize_prefs_read;
+      break;
+    case PROTECTED_PREFERENCE_STORE:
+      protected_prefs_ = prefs.Pass();
+      finalize_protected_prefs_read_ = finalize_prefs_read;
+      break;
+  }
+
+  MigrateIfReady();
+}
+
+void TrackedPreferencesMigrator::MigrateIfReady() {
+  // Wait for both stores to have been read before proceeding.
+  if (!protected_prefs_ || !unprotected_prefs_)
+    return;
+
+  bool protected_prefs_need_cleanup = false;
+  bool unprotected_prefs_altered = false;
+  MigratePrefsFromOldToNewStore(unprotected_pref_names_,
+                                protected_prefs_.get(),
+                                unprotected_prefs_.get(),
+                                &protected_prefs_need_cleanup,
+                                &unprotected_prefs_altered);
+  bool unprotected_prefs_need_cleanup = false;
+  bool protected_prefs_altered = false;
+  MigratePrefsFromOldToNewStore(protected_pref_names_,
+                                unprotected_prefs_.get(),
+                                protected_prefs_.get(),
+                                &unprotected_prefs_need_cleanup,
+                                &protected_prefs_altered);
+
+  // Hand the processed prefs back to their respective JsonPrefStore and keep
+  // weak pointers to those stores to make a best effort attempt at cleaning up
+  // duplicate values once those values' destination store was successfully
+  // written to disk.
+  base::WeakPtr<JsonPrefStore> unprotected_pref_store =

[Bernhard Bauer, 2014/04/30 09:31:05]

Hm, this is weird. First of all, why do we need to get the weak pointer only
now? The point of using a weak pointer is that you can get it early, and if the
object is destroyed between then and the time you unreference the pointer, you
will get NULL back. So presumably if the pref store is handing out weak pointers
to itself anyway, it could do that earlier on, which would at least make the
callbacks somewhat less awkward.

In addition, in JsonPrefStore::FinalizeFileRead(), you create a new weak pointer
from the factory, so if that method runs, you are guaranteed that the pointer is
still valid, so you wouldn't need a weak pointer anyway...

And if you think about it, in order to run a callback, you need to be sure that
its callee is still alive by the time you run it, which 
is achieved in this case by refcounting! (The base::Bind() call in
JsonPrefStore::OnFileRead() does not use a wrapper for its first argument, so it
will keep a reference to it). So, what that means is that by keeping these
callbacks around, you keep the PrefStore alive anyway, so calling something on a
PrefStore that has been destroyed is not really an issue. Now, whether you
*want* to keep the PrefStore alive when it would otherwise be destroyed, I'm not
sure about...

TL;DR: You have a reference cycle that keeps your objects alive (which you may
or may not want), but in any case you could get the same behavior without weak
pointers.

[gab, 2014/04/30 14:32:53]

I don't think I have a reference cycle. The weak pointers especially break this
cycle.

The idea is that:
JsonPrefStore depends on and owns TrackedPreferencesMigrator (through the
on_file_read_interceptor_).

The migration later depends on the JsonPrefStore to complete its cleanup; it
does so by handing weak pointers to callbacks bound to CleanupPrefStore().

At which point the migration is complete (TrackedPreferencesMigrator is deleted
as both callbacks referring to it have completed and been reset).

There are no dependencies left behind other than weak pointers bound to free
functions which will be used to later clean up if the store it's meant to
cleanup is still alive when it kicks in (but not otherwise being involved in its
lifetime) -- using strong references rather than weak pointer here would mean
that the cleanup tasks would own the JsonPrefStore which would definitely be
wrong IMO.

More below.

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

You're right, it's not a reference cycle. You are keeping one JsonPrefStore
around longer than it might otherwise be though, via the callback it hands to
OnFileRead(). What happens is the following:

1. Both pref stores (let's say pref stores A and B) own the
TrackedPreferencesMigrator via the callback that is their read interceptor.
2. WLOG, A finishes reading its prefs. 
3. JsonPrefStore::OnFileRead() creates a callback bound to
JsonPrefStore::FinalizeFileRead() on A and passes that to the interceptor
callback.
4. TrackedPreferencesMigrator::OnFileRead() stores that callback, which means
that TrackedPreferencesMigrator now references A. A then resets its read
interceptor afterwards, which means that A no longer references the
TrackedPreferencesMigrator. So basically, the reference edge turns around here,
and B references the TrackedPreferencesMigrator, which references A now.

Now, in practice that will probably not turn out to be a problem, because both A
and B are referenced by the same SegregatedPrefStore, so they will have the same
lifetime anyway, but of course knowing this requires knowing how these
JsonPrefStores are used, which to some extent defeats the layering here.

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

True, we could also bound the FinalizeFileReadCallback to a weak pointer to the
JsonPrefStore rather than a strong reference.

As you said, in practice it doesn't matter, but it could be clearer from
json_pref_store.cc's point of view (not handing extra references to ourselves.

The reason I did it this way is that previously FileThreadDeserializer would own
its JsonPrefStore until the end of OnFileRead and I figured in the new model it
would make sense to  force the JsonPrefStore to stay up until the end of
FinalizeFileRead through the FinalizeFileReadCallback -- although if the
callback holds the only reference left to the JsonPrefStore, it's kind of
pointless to even do FinalizeFileRead (the only benefit is to ensure that
|error_delegate_| gets notified of read errors and that observers get notified
of OnInitializationComplete() -- which, again, was previously the case since
JsonPrefStore could never be released in the middle of a read as it was owned by
the FileThreadDeserializer).

Let me know what you prefer, I have a slight preference for keeping the strong
reference in FinalizeFileReadCallback to keep the same behaviour as before (of
being guaranteed, after a ReadPrefs call, to at least live until the read result
is reported to error_delegate_ and observers).

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

That's true, JsonPrefStore does already survive longer due to the
FileThreadDeserializer... I think that's something I'd ultimately like to change
too. IMO, when whoever owns the JsonPrefStore (i.e. the PrefStoreKeeper in the
pre-SegregatedPrefStore world, or the latter now) is destroyed, the
JsonPrefStore should be destroyed too. All of this JsonPrefStore stuff predated
base::Callback and widespread use of weak pointers, and I think PrefStore being
refcounted is an artifact of that.

But I think I'm digressing; In terms of object lifetimes, I'm okay with it now
(although I might still at some point in the future make PrefStore
non-refcounted).

+      finalize_unprotected_prefs_read_.Run(unprotected_prefs_.Pass(),
+                                           unprotected_prefs_altered);
+  base::WeakPtr<JsonPrefStore> protected_pref_store =
+      finalize_protected_prefs_read_.Run(protected_prefs_.Pass(),
+                                         protected_prefs_altered);
+  DCHECK(unprotected_pref_store);
+  DCHECK(protected_pref_store);

[gab, 2014/04/30 14:32:53]

I agree that it's somewhat weird to get the weak pointers and then ASSERT that
they are valid. The reason we need weak pointers is not to call
JsonPrefStore::RegisterOnNextSuccessfulWriteCallback() (see
ScheduleSourcePrefStoreCleanup()), but rather to Bind that callback to a weak
pointer of the store that should be cleaned up when that kicks in.

I guess we could get a raw pointer instead and build the weak pointers ourselves
when binding in ScheduleSourcePrefStoreCleanup(), but I opted against that as
that would mean:
 1) JsonPrefStore has to implement SupportsWeakPtr<JsonPrefStore>.
 2) Returning a raw pointer from FinalizeFileRead() is even weirder than
returning a weak pointer IMO (at least with returning a weak pointer you kind of
assume from the API that the caller won't start ref-counting you (a caller
ref-counting the raw pointer of a weak pointer would be a red flag IMO); whereas
if you return a raw pointer it's much more okay for the caller to start
refcounting you... which is absolutely not the purpose of this API).

Another option to avoid having FinalizeFileRead() returning a WeakPtr would be
to obtain one from the JsonPrefStore much earlier and pipe it through
SetupTrackedPreferencesMigration, storing them in TrackedPreferencesMigrator
before handing them off to the cleanup tasks.
I also opted against that as it felt weird for TrackedPreferencesMigrator to
both store weak pointers to the JsonPrefStore while being called back by it and
temporarily owning some of its state (effectively giving it the
TrackedPreferencesMigrator two ways of modifying the dictionaries in
MigrateIfReady() -- either directly of via the WeakPtr<JsonPrefStore>'s...).

One last option would be to keep TrackedPreferencesMigrator alive after the
migration and keep the raw pointers to the base::DictionaryValue's; but then the
ownership is completely broken, modifying the dictionary we gave back to the
JsonPrefStore after the fact feels horribly wrong (hence why we need some way to
communicate back with the JsonPrefStore via its public API to do the delayed
cleanup).

Overall I like the current model:
 - JsonPrefStore owns TrackedPreferencesMigrator implicitly through the
OnFileReadInterceptor.
 - When the migration occurs it schedules source store cleanup tasks to happen
after the next successful destination store write; these tasks use weak pointers
to avoid being part of the dependency graph.
 - Migration is complete and TrackedPreferencesMigrator is deleted.
 - The only thing left behind are the cleanup tasks bound to a free function and
using a weak pointer param.
 - The dependency/ownership graph for migration is thus easy to reason about as
the cleanup tasks are not part of the dependency graph in any way, it's just a
best-effort task that will do it's thing if everything is still up when it kicks
in and won't otherwise affect any other component.

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

I would not have a problem with that. The usual objection to SupportsWeakPtr is
about handing out weak pointers to an object to other classes, but that is
exactly what we're doing here anyway. If the class supports weak pointers (in
the sense that it allows other classes to have weak pointers to it), we might as
well be honest about it.
Why is that a problem? Like you said, weak pointers are for referencing
something, refcounting is for (shared) ownership.
Stepping back a bit and putting on my base/prefs/OWNERS hat, here's what's
bothering me:

The signature of InterceptNextFileRead() is... not pretty. To completely grok
it, you need to look up two typedefs, with nested templates in each (we set a
callback that will get called with another callback which will return a weak
pointer to the original object we're calling the method on). The method also
ties getting the weak pointer to intercepting the read, which seems a bit
arbitrary if you don't know about the TrackedPreferencesMigrator use case (and
JsonPrefStore isn't supposed to know about it). In addition, we already have a
mechanism to modify the preferences as they are loaded from disk before the rest
of the system gets to see them, with the PrefFilter. So, maybe we could do the
following things:

* Make read interception part of the pref filter. Then JsonPrefStore would have
only a single mechanism to manipulate its prefs; the only difference to now
would be that the pref filter would work asynchronously.
* Decouple getting weak pointers (or doing cleanup work that doesn't require the
PrefStore to stay around) from the rest. We could make the PrefStore hand out
weak pointers directly (either via SupportsWeakPtr(), like mentioned above, or a
method that will return a weak pointer from the factory), or maybe just have a
method that will return an uncurried cleanup callback (i.e. one bound to
RemoveValueSilently, but without arguments), so we don't have to hand out the
weak pointers to other classes.

Does that sound reasonable?

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

(Apologies for the wall of text...)



I had initially thought the same thing (having two types of early filters is
kind of weird) and looked into doing it with the PrefFilter.

The problem with doing it with the PrefFilter is that it's currently uniquely
owned by its JsonPrefStore. For it to be able to handle migration, it would have
to become ref-counted and have the migration logic as part of FilterOnLoad.
There would then be a single PrefHashFilter which would know about both store's
configurations and have to handle FilterUpdate, FilterSerializeData, etc.
independently for both stores. Also, as you mentioned this would make
FilterOnLoad asynchronous (which is somewhat weird IMO). This would much
increase the complexity of PrefHashFilter solely to support the migration case
(while can be handled on its own as it is in the current proposed model).

I agree that the current proposed model isn't simple, but at least every piece
is single-purposed.

As for when the weak pointers are handed out. I wouldn't mind handing them off
earlier. It just felt cleaner to me to hand them out when the store is
initialized then to have the TrackedPreferencesMigrator start off with them
(since it never actually needs them other than to hand them off to the cleanup
jobs).
While I agree that this is not JsonPrefStore's problem, I don't think the API of
JsonPrefStore::FinalizePrefsReadCallback including

  // Returns a weak pointer to this JsonPrefStore which can be used by the
  // caller for future cleanup operations while this JsonPrefStore is still up.

is terribly ugly. As a reader this doesn't actually affect the behaviour of
JsonPrefStore, nor does it expand what the reader has to grok to understand
JsonPrefStore, all the reader is essentially told is that some
OnFileReadInterceptor out there may be given a weak pointer to this
JsonPrefStore, but since weak pointers aren't part of the ownership graph it
essentially doesn't matter (it's not worse than returning a bool or whatever
IMO).


*** Another option I just thought about while writing the above:
We could make the TrackedPreferencesMigrator a PrefFilter and have it own both
PrefHashFilters as they exist today. PrefFilters would then have to be
refcounted and owned by both JsonPrefStores. The filter could then handle
migration and later act as some kind of SegregatedPrefHashFilter (forwarding
other filter operations down to the appropriate PrefHashFilter).
This feels more intrusive though; e.g. to know which PrefHashFilter to forward
FilterSerializeData() to, the calling store would have to pass in some kind of
PrefStoreID (i.e. more state that the JsonPrefStore needs to be aware of) -- or
maybe the TrackedPreferencesMigrator could hold a std::map<const JsonPrefStore*,
PrefHashFilter> and JsonPrefStores would then only need to add "this" to the
Filter*() calls; but IMO passing pointers to yourself to other objects you don't
own is kind of weird as you have to assume the call won't somehow grab ownership
of yourself (which perhaps can be addressed with const-ness, but const-ness is
defined by the call's API unless you explicitly const_cast "this" before handing
it out...).

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

Heh, I'll respond in kind :)
I don't think that's particularly weird; pref loading is inherently asynchronous
anyway, and right now the read interceptor is asynchronous too. 
Well, PrefFilter is an abstract interface that you could implement however you
want. Presumably you could have two PrefFilter implementations that are each
owned by their JsonPrefStore and that forward to TrackedPreferencesMigrator when
they need to synchronize, and otherwise do their thing on their own.

Admittedly, it might be a bit weird for JsonPrefStore to own an object which
then has to... keep a reference to TrackedPreferencesMigrator? A raw pointer?
But that's of course an artifact of the JsonPrefStore API that requires
ownership of its PrefFilter. Remember, this was added back when there was no
SegregatedPrefStore, so the JsonPrefStore was owned by some completely unrelated
code, and there was no good owner for the PrefFilter. Now, the only object that
directly knows about a JsonPrefStore is the SegregatedPrefStore, so it would be
much less of a problem for JsonPrefStore to only keep a raw pointer to its
PrefFilter, with it being its owner's responsibility that the PrefFilter will
outlive the JsonPrefStore. This is closer to the classical Chromium model of Foo
and FooClient (in an embedding layer above Foo), where Foo would not own
FooClient, but the embedding layer would own Foo instead.
I think this is the fundamental problem: You are designing the JsonPrefStore API
specifically for this use case, where we only need a weak pointer after we have
called the finalization callback, whereas my goal with my base/prefs/OWNERS hat
on[*] is to keep its API general and agnostic of the client.

[*] And judging from your G+ picture, I think you are a man who appreciates a
good hat. :)
I just felt it's weird that this is the only way to get a weak pointer to the
JsonPrefStore. We are mixing up two things just because they happen to be called
at the same time in the client.
This is probably an aside at this point, but passing a raw pointer by default
never implies transfer of ownership in Chrome. That only happens with a
scoped_ptr or by adding a comment to that effect (callee takes ownership of
argument, caller takes ownership of return value).

+
+  if (unprotected_prefs_need_cleanup) {
+    // Cleanup the |protected_pref_names_| from the |unprotected_pref_store|
+    // once they were successfully written to the |protected_pref_store|.
+    ScheduleSourcePrefStoreCleanup(unprotected_pref_store,
+                                   protected_pref_names_,
+                                   protected_prefs_altered,
+                                   protected_pref_store.get());
+  }
+
+  if (protected_prefs_need_cleanup) {
+    // Cleanup the |unprotected_pref_names_| from the |protected_pref_store|
+    // once they were successfully written to the |unprotected_pref_store|.
+    ScheduleSourcePrefStoreCleanup(protected_pref_store,
+                                   unprotected_pref_names_,
+                                   unprotected_prefs_altered,
+                                   unprotected_pref_store.get());
+  }
+}
+
+}  // namespace
+
+void SetupTrackedPreferencesMigration(
+    const std::set<std::string>& unprotected_pref_names,
+    const std::set<std::string>& protected_pref_names,
+    JsonPrefStore* unprotected_pref_store,
+    JsonPrefStore* protected_pref_store) {
+  scoped_refptr<TrackedPreferencesMigrator> prefs_migrator(
+      new TrackedPreferencesMigrator(unprotected_pref_names,
+                                     protected_pref_names,
+                                     unprotected_pref_store,
+                                     protected_pref_store));
+}