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

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
Committed again: https://src.chromium.org/viewvc/chrome?view=rev&revision=269415
Reverted again: https://src.chromium.org/viewvc/chrome?view=rev&revision=269438

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269735

[gab, 2014-04-29 03:44:36]

Bernhard PTAL.

I'd like to get your opinion on the overall structure of this framework
overnight before I finish up a TODO in tracked_preferences_migration.cc and
write/adapt some tests tomorrow EDT.

See CL description for details and motivation.

Thanks!
Gab

[Bernhard Bauer, 2014-04-29 15:13:03]

Generally, looks good!

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:318: default:
Could we get rid of the default statement?

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:364: const bool had_filter_modification =
Nit: we don't usually mark local variables as const.

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:370: if (error_delegate_.get() && read_error_ !=
PREF_READ_ERROR_NONE)
You could probably leave out the .get().

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:117: // potentially been intercepted and modified
by on_file_read_interceptor_.
Nit: Put |on_file_read_interceptor_| between pipes?

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:134: scoped_ptr<OnFileReadInterceptor>
on_file_read_interceptor_;
Why do you need a scoped_ptr to a Callback? Callbacks are (cheaply) copyable and
can be NULL.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:55: static void
MigratePrefsFromOldToNewStore(
I think this could live outside of the class if it's private and static anyway.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:81: JsonPrefStore*
unprotected_pref_store,
Nit: Unindent by two spaces.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:83: :
unprotected_pref_names_(unprotected_pref_names),
Unindent this line and the following ones too, by two more spaces.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:86: // The
callbacks binded below will own this TrackedPreferencesMigrator by
Nit: "bound"

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:160: const
base::Value* tmp = NULL;
Could you come up with a better name for this variable than |tmp|? |old_value|
maybe?

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:164: if
(old_store->Get(pref_name, &tmp)) {
You could probably invert the conditions here and `continue` instead of nesting
if-statements.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.h:22: void
SetupTrackedPrefererencesMigration(
There's one "re" too much in there :)

[gab, 2014-04-29 15:46:43]

Applied/replied to your comments; I'll ping again once I'm done with the TODO in
tracked_preferences_migration.cc and with the tests.

Thanks!
Gab

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:318: default:
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Could we get rid of the default statement?

Yes, if we provide a case for PREF_READ_ERROR_MAX_ENUM; which is kind of ugly,
but better than a default IMO as it will at least ensure that anyone adding to
this enum gets an error when compiling this switch statement in clang.

Done.

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:364: const bool had_filter_modification =
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Nit: we don't usually mark local variables as const.

Sure, I like putting const as much as possible whenever it's not expected to
change -- I prefer being explicit; but I don't really mind in this case.

Done.

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.cc:370: if (error_delegate_.get() && read_error_ !=
PREF_READ_ERROR_NONE)
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> You could probably leave out the .get().

Done.

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:117: // potentially been intercepted and modified
by on_file_read_interceptor_.
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Nit: Put |on_file_read_interceptor_| between pipes?

Done.

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:134: scoped_ptr<OnFileReadInterceptor>
on_file_read_interceptor_;
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Why do you need a scoped_ptr to a Callback? Callbacks are (cheaply) copyable
and
> can be NULL.

I wasn't sure how to best express that this Callback will only be called once
and then deleted.

scoped_ptr::reset() felt easy and intuitive.

I guess I could make it a class member and do 
on_file_read_callback_ = OnFileReadCallback();
instead of a reset(), but that didn't feel as clean from my POV.

WDYT?

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:55: static void
MigratePrefsFromOldToNewStore(
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> I think this could live outside of the class if it's private and static
anyway.

I debated both and went this way because it was slightly more encapsulated
functionality and it kept this method close to its only caller (making it
slightly more readable IMO).

Otherwise I'd have to put in the anonymous namespace above and a reader would
have to bounce back-and-forth -- our style guide also says that type
declarations (i.e. classes) should come before methods IIRC which means that I
would technically have to put it at the bottom of this namespace, but then I'd
have to forward-declare it for it to be usable by this class...

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:81: JsonPrefStore*
unprotected_pref_store,
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Nit: Unindent by two spaces.

Done.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:83: :
unprotected_pref_names_(unprotected_pref_names),
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Unindent this line and the following ones too, by two more spaces.

I thought we wanted an extra 4 spaces here, but git cl format seems to disagree
with me and put it flush with the params.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:86: // The
callbacks binded below will own this TrackedPreferencesMigrator by
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Nit: "bound"

Done.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:160: const
base::Value* tmp = NULL;
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> Could you come up with a better name for this variable than |tmp|? |old_value|
> maybe?

Done.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:164: if
(old_store->Get(pref_name, &tmp)) {
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> You could probably invert the conditions here and `continue` instead of
nesting
> if-statements.

This nesting is required to set |old_store_needs_cleanup|.

It is technically possible that the values would end up in both stores following
a crash (as highlighted by the comment below) -- in which case we would still
need to cleanup |old_store| despite |new_store| already having a value.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.h:22: void
SetupTrackedPrefererencesMigration(
On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> There's one "re" too much in there :)

Haha, good catch, introducing some rimes in the tracked preferences ;)!

[Bernhard Bauer, 2014-04-29 16:38:53]

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:134: scoped_ptr<OnFileReadInterceptor>
on_file_read_interceptor_;
On 2014/04/29 15:46:44, gab wrote:
> On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > Why do you need a scoped_ptr to a Callback? Callbacks are (cheaply) copyable
> and
> > can be NULL.
> 
> I wasn't sure how to best express that this Callback will only be called once
> and then deleted.
> 
> scoped_ptr::reset() felt easy and intuitive.
> 
> I guess I could make it a class member and do 
> on_file_read_callback_ = OnFileReadCallback();
> instead of a reset(), but that didn't feel as clean from my POV.
> 
> WDYT?

Callback also has a Reset() (capital R) method.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:20: enum
PrefStoreID {
This could be private, right?

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:36:
~TrackedPreferencesMigrator();
Destructors should be private for refcounted classes. No one else should be able
to delete a refcounted object.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:55: static void
MigratePrefsFromOldToNewStore(
On 2014/04/29 15:46:44, gab wrote:
> On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > I think this could live outside of the class if it's private and static
> anyway.
> 
> I debated both and went this way because it was slightly more encapsulated
> functionality and it kept this method close to its only caller (making it
> slightly more readable IMO).
> 
> Otherwise I'd have to put in the anonymous namespace above and a reader would
> have to bounce back-and-forth -- our style guide also says that type
> declarations (i.e. classes) should come before methods IIRC which means that I
> would technically have to put it at the bottom of this namespace, but then I'd
> have to forward-declare it for it to be usable by this class...

Hm, but class declarations don't need to be grouped with their implementations
-- usually the former is in the header and the latter in the .cc file anyway.
You have a similar issue if you have two classes (in an anonymous namespace)
that reference each other: the declarations for both classes need to come first,
and then the implementations. In fact, being able to reference a class before
it's implemented is the reason why I usually don't just fully inline classes in
anonymous namespaces.

But anyway, at this point it's probably just a matter of personal preference. It
should be possible to move the method out of the class and still adhere to the
style guide, so that should not keep you, but if you prefer to do it some other
way, I won't object either.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:164: if
(old_store->Get(pref_name, &tmp)) {
On 2014/04/29 15:46:44, gab wrote:
> On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > You could probably invert the conditions here and `continue` instead of
> nesting
> > if-statements.
> 
> This nesting is required to set |old_store_needs_cleanup|.
> 
> It is technically possible that the values would end up in both stores
following
> a crash (as highlighted by the comment below) -- in which case we would still
> need to cleanup |old_store| despite |new_store| already having a value.

Hm, wouldn't the following work?

  if (!old_store->Get(pref_name, &tmp))
    continue;

  *old_store_needs_cleanup = true;
  if (new_store->Get(pref_name, &tmp))
    continue;
  
  [...]

It saves you two levels of indenting.

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:106: // Note, this method is used with asynchronous
file reading, so class exposes
Nit: "this class"

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:107: // it only for the internal needs (read: do
not call it manually).
Wait, now I'm confused. Who calls this method, and why does it need to be
public?

[gab, 2014-04-29 20:12:13]

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/20001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:134: scoped_ptr<OnFileReadInterceptor>
on_file_read_interceptor_;
On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> On 2014/04/29 15:46:44, gab wrote:
> > On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > > Why do you need a scoped_ptr to a Callback? Callbacks are (cheaply)
copyable
> > and
> > > can be NULL.
> > 
> > I wasn't sure how to best express that this Callback will only be called
once
> > and then deleted.
> > 
> > scoped_ptr::reset() felt easy and intuitive.
> > 
> > I guess I could make it a class member and do 
> > on_file_read_callback_ = OnFileReadCallback();
> > instead of a reset(), but that didn't feel as clean from my POV.
> > 
> > WDYT?
> 
> Callback also has a Reset() (capital R) method.

Oh awesome :)! (I'd looked for that in callback.h, but apparently it's in
callback_internal.h which is exposed from callback.h....?!)

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:20: enum
PrefStoreID {
On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> This could be private, right?

Done.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:36:
~TrackedPreferencesMigrator();
On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> Destructors should be private for refcounted classes. No one else should be
able
> to delete a refcounted object.

That's what I wanted to do, but it doesn't work because it's the callbacks that
end up owning this class (and thus deleting it); technically those callbacks
will run in this class so you're right that the delete will happen in this
class. However the compiler can't figure  this out when a reference is handed in
base::Bind below and won't compile unless this destructor is public :(.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:55: static void
MigratePrefsFromOldToNewStore(
On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> On 2014/04/29 15:46:44, gab wrote:
> > On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > > I think this could live outside of the class if it's private and static
> > anyway.
> > 
> > I debated both and went this way because it was slightly more encapsulated
> > functionality and it kept this method close to its only caller (making it
> > slightly more readable IMO).
> > 
> > Otherwise I'd have to put in the anonymous namespace above and a reader
would
> > have to bounce back-and-forth -- our style guide also says that type
> > declarations (i.e. classes) should come before methods IIRC which means that
I
> > would technically have to put it at the bottom of this namespace, but then
I'd
> > have to forward-declare it for it to be usable by this class...
> 
> Hm, but class declarations don't need to be grouped with their implementations
> -- usually the former is in the header and the latter in the .cc file anyway.
> You have a similar issue if you have two classes (in an anonymous namespace)
> that reference each other: the declarations for both classes need to come
first,
> and then the implementations. In fact, being able to reference a class before
> it's implemented is the reason why I usually don't just fully inline classes
in
> anonymous namespaces.
> 
> But anyway, at this point it's probably just a matter of personal preference.
It
> should be possible to move the method out of the class and still adhere to the
> style guide, so that should not keep you, but if you prefer to do it some
other
> way, I won't object either.

Ok, done. I put the free-method between TrackedPreferencesMigrator's declaration
and definition. That feels a bit weird to me, but I don't mind if you feel
that's better.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:164: if
(old_store->Get(pref_name, &tmp)) {
On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> On 2014/04/29 15:46:44, gab wrote:
> > On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > > You could probably invert the conditions here and `continue` instead of
> > nesting
> > > if-statements.
> > 
> > This nesting is required to set |old_store_needs_cleanup|.
> > 
> > It is technically possible that the values would end up in both stores
> following
> > a crash (as highlighted by the comment below) -- in which case we would
still
> > need to cleanup |old_store| despite |new_store| already having a value.
> 
> Hm, wouldn't the following work?
> 
>   if (!old_store->Get(pref_name, &tmp))
>     continue;
> 
>   *old_store_needs_cleanup = true;
>   if (new_store->Get(pref_name, &tmp))
>     continue;
>   
>   [...]
> 
> It saves you two levels of indenting.

Not sure which one I prefer; I've heard of people trying to minimize return
statements in a simple method to make for nicer assembly (reducing stack popping
complexity) and I guess the same applies to continue.

Otherwise I do agree that it's slightly easier to read with the continues.

Done.

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:106: // Note, this method is used with asynchronous
file reading, so class exposes
On 2014/04/29 16:38:54, Bernhard Bauer wrote:
> Nit: "this class"

Done.

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:107: // it only for the internal needs (read: do
not call it manually).
On 2014/04/29 16:38:54, Bernhard Bauer wrote:
> Wait, now I'm confused. Who calls this method, and why does it need to be
> public?

This is just putting back a comment that was on this method before and I'd
accidently removed in patch set 1.

It's called by the internal FileThreadDeserializer class in json_pref_store.cc

I was thinking of changing how this works to use callbacks instead of giving the
FileThreadDeserializer a JsonPrefStore*, but I didn't bother doing it in this CL
as I'm trying to keep it single-purpose -- I'm happy to add TODO if you agree
this should be done.

[Bernhard Bauer, 2014-04-29 21:20:27]

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:36:
~TrackedPreferencesMigrator();
On 2014/04/29 20:12:14, gab wrote:
> On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> > Destructors should be private for refcounted classes. No one else should be
> able
> > to delete a refcounted object.
> 
> That's what I wanted to do, but it doesn't work because it's the callbacks
that
> end up owning this class (and thus deleting it); technically those callbacks
> will run in this class so you're right that the delete will happen in this
> class. However the compiler can't figure  this out when a reference is handed
in
> base::Bind below and won't compile unless this destructor is public :(.

You'll need to friend the base class, i.e.
base::RefCounted<TrackedPreferencesMigrator>. I admit that it's a bit awkward to
write, but that is definitely the common style. I also think there's a presubmit
check / Clang plugin that will complain about public destructors for refcounted
classes.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:164: if
(old_store->Get(pref_name, &tmp)) {
On 2014/04/29 20:12:14, gab wrote:
> On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> > On 2014/04/29 15:46:44, gab wrote:
> > > On 2014/04/29 15:13:04, Bernhard Bauer wrote:
> > > > You could probably invert the conditions here and `continue` instead of
> > > nesting
> > > > if-statements.
> > > 
> > > This nesting is required to set |old_store_needs_cleanup|.
> > > 
> > > It is technically possible that the values would end up in both stores
> > following
> > > a crash (as highlighted by the comment below) -- in which case we would
> still
> > > need to cleanup |old_store| despite |new_store| already having a value.
> > 
> > Hm, wouldn't the following work?
> > 
> >   if (!old_store->Get(pref_name, &tmp))
> >     continue;
> > 
> >   *old_store_needs_cleanup = true;
> >   if (new_store->Get(pref_name, &tmp))
> >     continue;
> >   
> >   [...]
> > 
> > It saves you two levels of indenting.
> 
> Not sure which one I prefer; I've heard of people trying to minimize return
> statements in a simple method to make for nicer assembly (reducing stack
popping
> complexity) and I guess the same applies to continue.

Huh. I would have assumed that the compiler would produce the same machine code,
as both versions are functionally equivalent. 

> Otherwise I do agree that it's slightly easier to read with the continues.
> 
> Done.

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:107: // it only for the internal needs (read: do
not call it manually).
On 2014/04/29 20:12:14, gab wrote:
> On 2014/04/29 16:38:54, Bernhard Bauer wrote:
> > Wait, now I'm confused. Who calls this method, and why does it need to be
> > public?
> 
> This is just putting back a comment that was on this method before and I'd
> accidently removed in patch set 1.
> 
> It's called by the internal FileThreadDeserializer class in json_pref_store.cc
> 
> I was thinking of changing how this works to use callbacks instead of giving
the
> FileThreadDeserializer a JsonPrefStore*, but I didn't bother doing it in this
CL
> as I'm trying to keep it single-purpose -- I'm happy to add TODO if you agree
> this should be done.

Oh, OK. If you want to update it, that would be great, but it definitely doesn't
have to happen in this CL.

[gab, 2014-04-29 22:36:04]

Thanks for the initial comments, more changes to follow by tomorrow.

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:36:
~TrackedPreferencesMigrator();
On 2014/04/29 21:20:28, Bernhard Bauer wrote:
> On 2014/04/29 20:12:14, gab wrote:
> > On 2014/04/29 16:38:53, Bernhard Bauer wrote:
> > > Destructors should be private for refcounted classes. No one else should
be
> > able
> > > to delete a refcounted object.
> > 
> > That's what I wanted to do, but it doesn't work because it's the callbacks
> that
> > end up owning this class (and thus deleting it); technically those callbacks
> > will run in this class so you're right that the delete will happen in this
> > class. However the compiler can't figure  this out when a reference is
handed
> in
> > base::Bind below and won't compile unless this destructor is public :(.
> 
> You'll need to friend the base class, i.e.
> base::RefCounted<TrackedPreferencesMigrator>. I admit that it's a bit awkward
to
> write, but that is definitely the common style. I also think there's a
presubmit
> check / Clang plugin that will complain about public destructors for
refcounted
> classes.

Ah I see, done, thanks!

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/60001/base/prefs/json_pref_sto...
base/prefs/json_pref_store.h:107: // it only for the internal needs (read: do
not call it manually).
On 2014/04/29 21:20:28, Bernhard Bauer wrote:
> On 2014/04/29 20:12:14, gab wrote:
> > On 2014/04/29 16:38:54, Bernhard Bauer wrote:
> > > Wait, now I'm confused. Who calls this method, and why does it need to be
> > > public?
> > 
> > This is just putting back a comment that was on this method before and I'd
> > accidently removed in patch set 1.
> > 
> > It's called by the internal FileThreadDeserializer class in
json_pref_store.cc
> > 
> > I was thinking of changing how this works to use callbacks instead of giving
> the
> > FileThreadDeserializer a JsonPrefStore*, but I didn't bother doing it in
this
> CL
> > as I'm trying to keep it single-purpose -- I'm happy to add TODO if you
agree
> > this should be done.
> 
> Oh, OK. If you want to update it, that would be great, but it definitely
doesn't
> have to happen in this CL.

SG, added TODO.

[gab, 2014-04-30 04:24:59]

PTAL, more logic added to do the cleanup of migrated values' source store after
their destination store was successfully written to disk.

Tested manually as detailed in TEST section of CL description; will add
new/expand existing tests tomorrow.

Cheers!
Gab

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure.h
File base/critical_closure.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure.h...
base/critical_closure.h:27: CallbackType MakeCriticalClosure(const CallbackType&
closure);
Had to make this more generic to support use-case below; happy to rename
file/method if this is otherwise fine with you and owners of this file.

[Bernhard Bauer, 2014-04-30 09:31:04]

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
File base/critical_closure_ios.mm (right):

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:24: void Run() {
Hm, this class only implements an argumentless Run() method, so it will only
work with argumentless callbacks, right? I think that shouldn't be a problem
though, because SFINAE.

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:33: base::ios::ScopedCriticalAction
criticial_action_;
If you want to clean this up a bit, it should be named |critical_action_| :)

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:34: scoped_ptr<CallbackType> closure_;
Oh, and this could probably also directly own the closure.

https://codereview.chromium.org/257003007/diff/140001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/files/important_fi...
base/files/important_file_writer.h:109: // If |result| is true and
|on_succesful_write_| is set, invokes
Nit: The member is |on_next_successful_write_| (includes "next", and
"successful" has a double-s).

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:370: return weak_factory_.GetWeakPtr();;
Nit: one semicolon too much.

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.h:173: base::WeakPtrFactory<JsonPrefStore>
weak_factory_;
You now have a WeakPtrFactory in a refcounted object. That, as well as the fact
that you are wielding weak pointers to this class outside of it should be a red
flag. See also below...

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:199:
base::WeakPtr<JsonPrefStore> unprotected_pref_store =
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.

[Bernhard Bauer, 2014-04-30 09:31:04]

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
File base/critical_closure_ios.mm (right):

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:24: void Run() {
Hm, this class only implements an argumentless Run() method, so it will only
work with argumentless callbacks, right? I think that shouldn't be a problem
though, because SFINAE.

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:33: base::ios::ScopedCriticalAction
criticial_action_;
If you want to clean this up a bit, it should be named |critical_action_| :)

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:34: scoped_ptr<CallbackType> closure_;
Oh, and this could probably also directly own the closure.

https://codereview.chromium.org/257003007/diff/140001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/files/important_fi...
base/files/important_file_writer.h:109: // If |result| is true and
|on_succesful_write_| is set, invokes
Nit: The member is |on_next_successful_write_| (includes "next", and
"successful" has a double-s).

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:370: return weak_factory_.GetWeakPtr();;
Nit: one semicolon too much.

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.h:173: base::WeakPtrFactory<JsonPrefStore>
weak_factory_;
You now have a WeakPtrFactory in a refcounted object. That, as well as the fact
that you are wielding weak pointers to this class outside of it should be a red
flag. See also below...

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:199:
base::WeakPtr<JsonPrefStore> unprotected_pref_store =
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]

Thanks for the review, see new patch set and replies below.

Cheers,
Gab

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
File base/critical_closure_ios.mm (right):

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:24: void Run() {
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> Hm, this class only implements an argumentless Run() method, so it will only
> work with argumentless callbacks, right? I think that shouldn't be a problem
> though, because SFINAE.

Ah good point, I actually made the template more specific to only accept
Callback<R(void)> types; which actually makes more sense (it doesn't really make
sense to have a partially bound critical task IMO).

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:33: base::ios::ScopedCriticalAction
criticial_action_;
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> If you want to clean this up a bit, it should be named |critical_action_| :)

Done.

https://codereview.chromium.org/257003007/diff/140001/base/critical_closure_i...
base/critical_closure_ios.mm:34: scoped_ptr<CallbackType> closure_;
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> Oh, and this could probably also directly own the closure.

Done, and also removed refcounting (whose only purpose was to have the Callback
own the CriticalClosure); using base::Owned() instead.

https://codereview.chromium.org/257003007/diff/140001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/files/important_fi...
base/files/important_file_writer.h:109: // If |result| is true and
|on_succesful_write_| is set, invokes
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> Nit: The member is |on_next_successful_write_| (includes "next", and
> "successful" has a double-s).

Done.

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:370: return weak_factory_.GetWeakPtr();;
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> Nit: one semicolon too much.

Done.

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.h:173: base::WeakPtrFactory<JsonPrefStore>
weak_factory_;
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> You now have a WeakPtrFactory in a refcounted object. That, as well as the
fact
> that you are wielding weak pointers to this class outside of it should be a
red
> flag. See also below...

I don't see why this is a red flag. Ref-counting is an ownership model; weak
pointers are a reference model. I don't see why the two can't be used together.

As weak_ptr.h says:
// Weak pointers are useful when an object needs to be accessed safely by one
// or more objects other than its owner, and those callers can cope with the
// object vanishing and e.g. tasks posted to it being silently dropped.
// Reference-counting such an object would complicate the ownership graph and
// make it harder to reason about the object's lifetime.

(this comment only mentions that it's better to use WeakPtr than RefCounting
whenever possible, but I'd argue it's simlarly better to use WeakPtrs for
already ref-counted objects than handing more references for similar reasons).

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:199:
base::WeakPtr<JsonPrefStore> unprotected_pref_store =
On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> 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.

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.

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:206:
DCHECK(protected_pref_store);
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:08]

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/140001/base/prefs/json_pref_st...
base/prefs/json_pref_store.h:173: base::WeakPtrFactory<JsonPrefStore>
weak_factory_;
On 2014/04/30 14:32:53, gab wrote:
> On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> > You now have a WeakPtrFactory in a refcounted object. That, as well as the
> fact
> > that you are wielding weak pointers to this class outside of it should be a
> red
> > flag. See also below...
> 
> I don't see why this is a red flag. Ref-counting is an ownership model; weak
> pointers are a reference model. I don't see why the two can't be used
together.
> 
> As weak_ptr.h says:
> // Weak pointers are useful when an object needs to be accessed safely by one
> // or more objects other than its owner, and those callers can cope with the
> // object vanishing and e.g. tasks posted to it being silently dropped.
> // Reference-counting such an object would complicate the ownership graph and
> // make it harder to reason about the object's lifetime.
> 
> (this comment only mentions that it's better to use WeakPtr than RefCounting
> whenever possible, but I'd argue it's simlarly better to use WeakPtrs for
> already ref-counted objects than handing more references for similar reasons).

Hm, I read that comment more as, "if you have an object and want to safely
access it at a later point, prefer using weak pointers than starting to refcount
it". But anyway, what I meant by red flag is not that it's a no-go, just that
you should take a close look at what you're doing. I have so far never
encountered a situation where refcounting + weak pointers turned out to be the
best solution.

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:199:
base::WeakPtr<JsonPrefStore> unprotected_pref_store =
On 2014/04/30 14:32:53, gab wrote:
> On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:206:
DCHECK(protected_pref_store);
On 2014/04/30 14:32:53, gab wrote:
> 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>.

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.

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

Why is that a problem? Like you said, weak pointers are for referencing
something, refcounting is for (shared) ownership.

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

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?

https://codereview.chromium.org/257003007/diff/270001/base/files/important_fi...
File base/files/important_file_writer_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/270001/base/files/important_fi...
base/files/important_file_writer_unittest.cc:135: // Confirm that writting again
without re-installing the observer doesn't
Nit: "writing"

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:66: // PREF_READ_ERROR_NONE when there actually
was an error).
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]

Thanks again for the review, replies below (I hope I don't sound pushy in my
replies, I really want to come up with the cleanest design for this and I much
appreciate your feedback; I did analyze a lot of the possibilities and I still
believe this is the best one, apologies ahead of time for the lengthy replies).

Cheers,
Gab

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:199:
base::WeakPtr<JsonPrefStore> unprotected_pref_store =
On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> On 2014/04/30 14:32:53, gab wrote:
> > On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:206:
DCHECK(protected_pref_store);
On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> On 2014/04/30 14:32:53, gab wrote:
> > 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>.
> 
> 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.
> 
> >  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...).
> 
> Why is that a problem? Like you said, weak pointers are for referencing
> something, refcounting is for (shared) ownership.
> 
> > 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.
> 
> 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?


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

https://codereview.chromium.org/257003007/diff/270001/base/files/important_fi...
File base/files/important_file_writer_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/270001/base/files/important_fi...
base/files/important_file_writer_unittest.cc:135: // Confirm that writting again
without re-installing the observer doesn't
On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> Nit: "writing"

Done.

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:66: // PREF_READ_ERROR_NONE when there actually
was an error).
On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> Huh, why is that? Doing PostTask should guarantee that these things happen
> sequentially, no?
> 
> (We could probably use PostTaskAndReply here, though).

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

[gab, 2014-05-01 18:04:53]

Oh and one more thing I'm realizing now that I'm writing the migration tests.
TrackedPreferencesMigrator is easy to unittest as-is. I suspect that would be
harder if the PrefFilter was in charge of the migration.

This strengthens my belief that this is the right approach (multiple single
purpose and easy to test components).

Cheers,
Gab

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

Thanks for your patience with me :) Just in case it's not clear, I'm pushing
back because I want to keep the base/prefs interface simple, not to make things
difficult for you.

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:199:
base::WeakPtr<JsonPrefStore> unprotected_pref_store =
On 2014/05/01 15:34:54, gab wrote:
> On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> > On 2014/04/30 14:32:53, gab wrote:
> > > On 2014/04/30 09:31:05, Bernhard Bauer wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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).

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

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

https://codereview.chromium.org/257003007/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:206:
DCHECK(protected_pref_store);
On 2014/05/01 15:34:54, gab wrote:
> On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> > On 2014/04/30 14:32:53, gab wrote:
> > > 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>.
> > 
> > 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.
> > 
> > >  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...).
> > 
> > Why is that a problem? Like you said, weak pointers are for referencing
> > something, refcounting is for (shared) ownership.
> > 
> > > 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.
> > 
> > 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?
> 
> 
> (Apologies for the wall of text...)

Heh, I'll respond in kind :)

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

I don't think that's particularly weird; pref loading is inherently asynchronous
anyway, and right now the read interceptor is asynchronous too. 

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

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

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

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

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.

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

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

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:66: // PREF_READ_ERROR_NONE when there actually
was an error).
On 2014/05/01 15:34:54, gab wrote:
> On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> > Huh, why is that? Doing PostTask should guarantee that these things happen
> > sequentially, no?
> > 
> > (We could probably use PostTaskAndReply here, though).
> 
> 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...

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:00]

I've been thinking about this some more. Here's a modified proposal based on the
current one:

1) Declare PrefFilter::FilterOnLoad to be asynchronous (and augment it to be
handed ownership of the prefs + the FinalizeFileReadCallback to hand them back
when it's done).
2) Remove the |on_file_read_interceptor_| from JsonPrefStore.
3) Hook the TrackedPreferencesMigrator on both PrefHashFilters to intercept
FilterOnLoads (similar to how it was currently hooked on the JsonPrefStore with
ownership through callbacks).
4) Instead of giving a WeakPtr<JsonPrefStore> for cleanups, Callbacks bound to
WeakPtr<JsonPrefStore>s could be handed out for
JsonPrefStore::RemoveValueSilently() and
JsonPrefStore::RegisterOnNextSuccessfulWriteCallback() when calling
PrefHashFilter::FilterOnLoad().

Essentially having a diamond shaped ownership graph with the root being the
SegregatedPrefStore, splitting into the 2 JsonPrefStores which each own their
PrefHashFilter which together own the same TrackedPreferencesMigrator through
callbacks.

1+2+3 remove the double filter interfaces from JsonPrefStore, bringing it back
down to only PrefFilter. The downside is that it increases the complexity of the
PrefFilter API (which now takes a FinalizeFileReadCallback and maybe more
callbacks, i.e. for (4)).

4 removes the WeakPtr<JsonPrefStore> return you didn't like -- we could also do
this in the current model (if you don't like 1+2+3).

This proposal would make TrackedPreferencesMigrator agnostic to the
JsonPrefStore (only talking to it through callbacks).
It would also make the JsonPrefStore agnostic to migration -- from it's POV, the
only difference is that FilterOnLoad is now asynchronous and that it now
supports WeakPtrs.
PrefHashFilter would then take most of the heat, piping things around like:

JsonPrefStore::OnFileRead() =>(direct call) PrefHashFilter::FilterOnLoad()
=>(via callback) TrackedPreferencesMigrator::OnPrefsLoaded() =>(via callback x2
the second time it's called) PrefHashFilter::FinalizeFilterOnLoad() =>(via
callback x2) JsonPrefStore::FinalizeFileRead()

And TrackedPreferencesMigrator::OnPrefsLoaded() would have used weakly bound
callbacks to bind the cleanup tasks (without otherwise knowing they are
JsonPrefStore related).

WDYT?

Cheers!
Gab

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
base/prefs/json_pref_store.cc:66: // PREF_READ_ERROR_NONE when there actually
was an error).
On 2014/05/01 20:16:00, Bernhard Bauer wrote:
> On 2014/05/01 15:34:54, gab wrote:
> > On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> > > Huh, why is that? Doing PostTask should guarantee that these things happen
> > > sequentially, no?
> > > 
> > > (We could probably use PostTaskAndReply here, though).
> > 
> > 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...
> 
> 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.)

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)

[Bernhard Bauer, 2014-05-02 08:23:08]

On 2014/05/02 04:15:00, gab wrote:
> I've been thinking about this some more. Here's a modified proposal based on
the
> current one:
> 
> 1) Declare PrefFilter::FilterOnLoad to be asynchronous (and augment it to be
> handed ownership of the prefs + the FinalizeFileReadCallback to hand them back
> when it's done).
> 2) Remove the |on_file_read_interceptor_| from JsonPrefStore.
> 3) Hook the TrackedPreferencesMigrator on both PrefHashFilters to intercept
> FilterOnLoads (similar to how it was currently hooked on the JsonPrefStore
with
> ownership through callbacks).
> 4) Instead of giving a WeakPtr<JsonPrefStore> for cleanups, Callbacks bound to
> WeakPtr<JsonPrefStore>s could be handed out for
> JsonPrefStore::RemoveValueSilently() and
> JsonPrefStore::RegisterOnNextSuccessfulWriteCallback() when calling
> PrefHashFilter::FilterOnLoad().
> 
> Essentially having a diamond shaped ownership graph with the root being the
> SegregatedPrefStore, splitting into the 2 JsonPrefStores which each own their
> PrefHashFilter which together own the same TrackedPreferencesMigrator through
> callbacks.
> 
> 1+2+3 remove the double filter interfaces from JsonPrefStore, bringing it back
> down to only PrefFilter. The downside is that it increases the complexity of
the
> PrefFilter API (which now takes a FinalizeFileReadCallback and maybe more
> callbacks, i.e. for (4)).
> 
> 4 removes the WeakPtr<JsonPrefStore> return you didn't like -- we could also
do
> this in the current model (if you don't like 1+2+3).
> 
> This proposal would make TrackedPreferencesMigrator agnostic to the
> JsonPrefStore (only talking to it through callbacks).
> It would also make the JsonPrefStore agnostic to migration -- from it's POV,
the
> only difference is that FilterOnLoad is now asynchronous and that it now
> supports WeakPtrs.
> PrefHashFilter would then take most of the heat, piping things around like:
> 
> JsonPrefStore::OnFileRead() =>(direct call) PrefHashFilter::FilterOnLoad()
> =>(via callback) TrackedPreferencesMigrator::OnPrefsLoaded() =>(via callback
x2
> the second time it's called) PrefHashFilter::FinalizeFilterOnLoad() =>(via
> callback x2) JsonPrefStore::FinalizeFileRead()
> 
> And TrackedPreferencesMigrator::OnPrefsLoaded() would have used weakly bound
> callbacks to bind the cleanup tasks (without otherwise knowing they are
> JsonPrefStore related).
> 
> WDYT?

That sounds great. Thanks a lot for your flexibility here!

> Cheers!
> Gab
> 
>
https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
> File base/prefs/json_pref_store.cc (right):
> 
>
https://codereview.chromium.org/257003007/diff/270001/base/prefs/json_pref_st...
> base/prefs/json_pref_store.cc:66: // PREF_READ_ERROR_NONE when there actually
> was an error).
> On 2014/05/01 20:16:00, Bernhard Bauer wrote:
> > On 2014/05/01 15:34:54, gab wrote:
> > > On 2014/05/01 12:37:09, Bernhard Bauer wrote:
> > > > Huh, why is that? Doing PostTask should guarantee that these things
happen
> > > > sequentially, no?
> > > > 
> > > > (We could probably use PostTaskAndReply here, though).
> > > 
> > > 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...
> > 
> > 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.)
> 
> 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)

Yeah, this is one of those rare cases where something unexpectedly works,
instead of the opposite :)

[gab, 2014-05-05 13:53:46]

Bernhard PTAL, new design as per prior discussion and as drawn up @
https://chromium.googlecode.com/issues/attachment?aid=3657690004001&name=Trac...

Migration tests still to come.

(Also dropped some comments for myself of the nits I've spotted already, but
can't fix due to RDP problems this morning).

Cheers,
Gab

https://codereview.chromium.org/257003007/diff/330001/base/files/important_fi...
File base/files/important_file_writer_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/330001/base/files/important_fi...
base/files/important_file_writer_unittest.cc:111:
TEST_F(ImportantFileWriterTest, BasicWithSucessfulWriteObserver) {
s/Sucess/Success

https://codereview.chromium.org/257003007/diff/330001/base/prefs/json_pref_st...
File base/prefs/json_pref_store_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/330001/base/prefs/json_pref_st...
base/prefs/json_pref_store_unittest.cc:51: void OnFileRead(const
PostFilterOnLoadCallback& finalize_prefs_read,
unused method.

https://codereview.chromium.org/257003007/diff/330001/base/prefs/json_pref_st...
base/prefs/json_pref_store_unittest.cc:426: // EXPECT_CALL(*mock_error_delegate,
Fix these; for some reason the mock gtest framework was complaining about
|mock_error_delegate| being leaked at this point... it's not, it's owned by the
|pref_store| which simply hasn't been deleted yet. Not sure why it cares..

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/segregated_pref_store.cc:128: //
TrackedPreferencesMigrator and complete asynchronously.
s/asynchronously/synchronously

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:39:
PROTECTED_PREF_hash_FILTER
Fix case.

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:89: //
|destination_pref_store| they were migrated to was successfully written to
Update this comment to match new arguments.

[gab, 2014-05-05 17:30:35]

FYI, self-nits addressed.

https://codereview.chromium.org/257003007/diff/330001/base/files/important_fi...
File base/files/important_file_writer_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/330001/base/files/important_fi...
base/files/important_file_writer_unittest.cc:111:
TEST_F(ImportantFileWriterTest, BasicWithSucessfulWriteObserver) {
On 2014/05/05 13:53:46, gab wrote:
> s/Sucess/Success

Done.

https://codereview.chromium.org/257003007/diff/330001/base/prefs/json_pref_st...
File base/prefs/json_pref_store_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/330001/base/prefs/json_pref_st...
base/prefs/json_pref_store_unittest.cc:51: void OnFileRead(const
PostFilterOnLoadCallback& finalize_prefs_read,
On 2014/05/05 13:53:46, gab wrote:
> unused method.

Done.

https://codereview.chromium.org/257003007/diff/330001/base/prefs/json_pref_st...
base/prefs/json_pref_store_unittest.cc:426: // EXPECT_CALL(*mock_error_delegate,
On 2014/05/05 13:53:46, gab wrote:
> Fix these; for some reason the mock gtest framework was complaining about
> |mock_error_delegate| being leaked at this point... it's not, it's owned by
the
> |pref_store| which simply hasn't been deleted yet. Not sure why it cares..

Done.

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/segregated_pref_store.cc:128: //
TrackedPreferencesMigrator and complete asynchronously.
On 2014/05/05 13:53:46, gab wrote:
> s/asynchronously/synchronously

Done.

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right):

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:39:
PROTECTED_PREF_hash_FILTER
On 2014/05/05 13:53:46, gab wrote:
> Fix case.

Done.

https://codereview.chromium.org/257003007/diff/330001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_preferences_migration.cc:89: //
|destination_pref_store| they were migrated to was successfully written to
On 2014/05/05 13:53:46, gab wrote:
> Update this comment to match new arguments.

Done.

[gab, 2014-05-05 23:43:51]

@Bernhard, PTAL, all tests have been written and uploaded as of patch set 24.

@Nico PTAL for base/critical_closure.h and base/files/important_file_writer*
changes.

critical_closure.h changes are to expand MakeCriticalClosure's API to support
not only Closures, but Callbacks with a return value which still have zero
params -- needed in this CL) -- hints on why the iOS build is unhappy would be
nice (I don't see why it would have compiled before, but no longer does...).

Thanks!
Gab

[Nico, 2014-05-06 03:00:09]

https://codereview.chromium.org/257003007/diff/470001/base/critical_closure.h
File base/critical_closure.h (right):

https://codereview.chromium.org/257003007/diff/470001/base/critical_closure.h...
base/critical_closure.h:11: #import <UIKit/UIKit.h>
wot, why do you need UIKit in base?

That's why this doesn't compile btw: UIKit.h contains ObjC code, but you include
this header from a cc file (sequenced_worker_pool.cc). Only .mm files are
compiled as ObjC++.

[Bernhard Bauer, 2014-05-06 12:46:59]

Looks pretty good overall!

https://codereview.chromium.org/257003007/diff/470001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/470001/base/prefs/json_pref_st...
base/prefs/json_pref_store.h:70: // See details in pref_filter.h.
Hmm... could we add a new PrefReadError value for "initialization not finished
yet" and have this method return that? That way, if someone uses an asynchronous
PrefFilter but expects ReadPrefs() to finish synchronously, they would at least
get an error code.

https://codereview.chromium.org/257003007/diff/470001/base/prefs/persistent_p...
File base/prefs/persistent_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/470001/base/prefs/persistent_p...
base/prefs/persistent_pref_store.h:31: // PREF_READ_ERROR_OTHER = 8,  //
Deprecated: used to be the default error
I'm wondering whether we even need to mention this value here. In the long run,
explaining all the stupid stuff we _used_ to do might not scale very well... ;-)
We could just say "value 8 has been removed" or something.

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:371:
scoped_ptr<base::DictionaryValue> prefs,
Nit: Align arguments

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/segregated_pref_store.cc:128: //
TrackedPreferencesMigrator and complete synchronously.
If we had a return value that indicates that the call is asynchronous, we could
verify that here :)

[gab, 2014-05-06 14:40:34]

Thanks Nico, PTAL (see reply below).

https://codereview.chromium.org/257003007/diff/470001/base/critical_closure.h
File base/critical_closure.h (right):

https://codereview.chromium.org/257003007/diff/470001/base/critical_closure.h...
base/critical_closure.h:11: #import <UIKit/UIKit.h>
On 2014/05/06 03:00:10, Nico wrote:
> wot, why do you need UIKit in base?
> 
> That's why this doesn't compile btw: UIKit.h contains ObjC code, but you
include
> this header from a cc file (sequenced_worker_pool.cc). Only .mm files are
> compiled as ObjC++.

Oh of course, moved that as-is from the .mm file; not sure why it was initially
added there? Potentially for the DCHECK([[UIDevice currentDevice]
isMultitaskingSupported]); ??

I had to remove that DCHECK because it also couldn't build in C++ context.

If you think this DCHECK is required then we could add a helper with a .mm
specific impl; WDYT?

[gab, 2014-05-06 15:57:42]

Bernhard PTAL, comments addressed.

Thanks!
Gab

https://codereview.chromium.org/257003007/diff/470001/base/prefs/json_pref_st...
File base/prefs/json_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/470001/base/prefs/json_pref_st...
base/prefs/json_pref_store.h:70: // See details in pref_filter.h.
On 2014/05/06 12:47:00, Bernhard Bauer wrote:
> Hmm... could we add a new PrefReadError value for "initialization not finished
> yet" and have this method return that? That way, if someone uses an
asynchronous
> PrefFilter but expects ReadPrefs() to finish synchronously, they would at
least
> get an error code.

Done.

https://codereview.chromium.org/257003007/diff/470001/base/prefs/persistent_p...
File base/prefs/persistent_pref_store.h (right):

https://codereview.chromium.org/257003007/diff/470001/base/prefs/persistent_p...
base/prefs/persistent_pref_store.h:31: // PREF_READ_ERROR_OTHER = 8,  //
Deprecated: used to be the default error
On 2014/05/06 12:47:00, Bernhard Bauer wrote:
> I'm wondering whether we even need to mention this value here. In the long
run,
> explaining all the stupid stuff we _used_ to do might not scale very well...
;-)
> We could just say "value 8 has been removed" or something.

How about 
// PREF_READ_ERROR_OTHER = 8,  // Deprecated.
? Leaving the previous name there can be useful I guess if it was ever reported
by old histograms.

Plus just putting:
// Value 8 was removed.
looks like a comment about the next enum value...

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:371:
scoped_ptr<base::DictionaryValue> prefs,
On 2014/05/06 12:47:00, Bernhard Bauer wrote:
> Nit: Align arguments

Done.

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/segregated_pref_store.cc (right):

https://codereview.chromium.org/257003007/diff/470001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/segregated_pref_store.cc:128: //
TrackedPreferencesMigrator and complete synchronously.
On 2014/05/06 12:47:00, Bernhard Bauer wrote:
> If we had a return value that indicates that the call is asynchronous, we
could
> verify that here :)

That required making PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE have priority
over other error codes (otherwise the added DCHECKs here don't make sense as
they aren't guaranteed).

Let me know if this is what you had in mind.

[Bernhard Bauer, 2014-05-06 18:41:12]

LGTM!

[gab, 2014-05-06 19:40:27]

Thanks,

@Nico (see below).

Cheers,
Gab



On 2014/05/06 14:40:34, gab wrote:
> Thanks Nico, PTAL (see reply below).
> 
> https://codereview.chromium.org/257003007/diff/470001/base/critical_closure.h
> File base/critical_closure.h (right):
> 
>
https://codereview.chromium.org/257003007/diff/470001/base/critical_closure.h...
> base/critical_closure.h:11: #import <UIKit/UIKit.h>
> On 2014/05/06 03:00:10, Nico wrote:
> > wot, why do you need UIKit in base?
> > 
> > That's why this doesn't compile btw: UIKit.h contains ObjC code, but you
> include
> > this header from a cc file (sequenced_worker_pool.cc). Only .mm files are
> > compiled as ObjC++.
> 
> Oh of course, moved that as-is from the .mm file; not sure why it was
initially
> added there? Potentially for the DCHECK([[UIDevice currentDevice]
> isMultitaskingSupported]); ??
> 
> I had to remove that DCHECK because it also couldn't build in C++ context.
> 
> If you think this DCHECK is required then we could add a helper with a .mm
> specific impl; WDYT?

[Nico, 2014-05-06 21:14:50]

Except for the iOS bits, the non-prefs bits of this CL look fine.

https://codereview.chromium.org/257003007/diff/620001/base/critical_closure.h
File base/critical_closure.h (right):

https://codereview.chromium.org/257003007/diff/620001/base/critical_closure.h...
base/critical_closure.h:43: }  // namespace internal
Why is this getting inlined? This seems worse than what we had before (since
you're removing that DCHECK, which I imagine someone added for a reason.)

(Side remark: This CL does more than one thing, it would've been good to split
it up more.)

https://codereview.chromium.org/257003007/diff/620001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/257003007/diff/620001/base/files/important_fi...
base/files/important_file_writer.h:94: // successful write event. Only one
callback should be set at once, if this
"…can be set at once." (remove comment after that)

[gab, 2014-05-07 03:59:08]

Thanks Nico, PTAL.

Cheers,
Gab

https://codereview.chromium.org/257003007/diff/620001/base/critical_closure.h
File base/critical_closure.h (right):

https://codereview.chromium.org/257003007/diff/620001/base/critical_closure.h...
base/critical_closure.h:43: }  // namespace internal
On 2014/05/06 21:14:50, Nico wrote:
> Why is this getting inlined? This seems worse than what we had before (since
> you're removing that DCHECK, which I imagine someone added for a reason.)

Because it's now templated and the callers must have all the code to be able to
compile+link (if we keep this in the .mm file, the templates don't get
instantiated in the library and aren't found at link time -- maybe we could
force instantiation for the two specific types we care about for now
Callback<void(void)> and Callback<bool(void)> in the .mm file though if you
prefer that?).

Another option is to leave the templates in the header, but bring back the
DCHECK via a C++ helper, e.g., IsMultiTaskingSupported() which could be
implemented in ObjC in critical_closure_internal_ios.mm.
I did that for now, let me know what you think.

> 
> (Side remark: This CL does more than one thing, it would've been good to split
> it up more.)

I agree it's a big CL, but the new APIs only make sense in this change
(otherwise they're introduced yet no one uses them) -- given they don't make the
core logic of this CL harder I didn't think it was worth splitting them
(especially given the timing of this CL landing is tight).

https://codereview.chromium.org/257003007/diff/620001/base/files/important_fi...
File base/files/important_file_writer.h (right):

https://codereview.chromium.org/257003007/diff/620001/base/files/important_fi...
base/files/important_file_writer.h:94: // successful write event. Only one
callback should be set at once, if this
On 2014/05/06 21:14:50, Nico wrote:
> "…can be set at once." (remove comment after that)

Done.

[Nico, 2014-05-07 21:50:47]

lgtm I suppose, but changing MakeCriticalClosure to no longer return a close
seems pretty weird.

> I agree it's a big CL, but the new APIs only make sense in this change
> (otherwise they're introduced yet no one uses them)

That's fine. You can say "this will be used by CL $link, see file $foo" or
something like that.

> (especially given the timing of this CL landing is tight).

This is a reason for sending it out in pieces. In my experience, smaller changes
are much easier to review.

(+stuart ios bit fyi)

[Nico, 2014-05-07 21:50:55]

lgtm I suppose, but changing MakeCriticalClosure to no longer return a closure
seems pretty weird.

> I agree it's a big CL, but the new APIs only make sense in this change
> (otherwise they're introduced yet no one uses them)

That's fine. You can say "this will be used by CL $link, see file $foo" or
something like that.

> (especially given the timing of this CL landing is tight).

This is a reason for sending it out in pieces. In my experience, smaller changes
are much easier to review.

(+stuart ios bit fyi)

[stuartmorgan, 2014-05-07 22:47:32]

On 2014/05/07 21:50:55, Nico wrote:
> (+stuart ios bit fyi)

I'm totally perplexed. The CL description is that you are changing the prefs
system, and you are as a totally unexplained side effect changing a core piece
of iOS infrastructure to have a different signature?

This is not LGTM as far as I'm concerned. This piece should be in a separate CL
that explains why it's needed, and someone familiar with iOS should review it,
instead of it just being slipped in as part of a largely unrelated mega-CL.

[stuartmorgan, 2014-05-07 22:49:07]

And to be clear, this is going to make for a bad merge into the iOS tree since
we have other uses of the critical closure system, which is why having this go
in without any input from the iOS side is a significant problem.

[gab, 2014-05-07 23:49:59]

On 2014/05/07 22:47:32, stuartmorgan wrote:
> On 2014/05/07 21:50:55, Nico wrote:
> > (+stuart ios bit fyi)
> 
> I'm totally perplexed. The CL description is that you are changing the prefs
> system, and you are as a totally unexplained side effect changing a core piece
> of iOS infrastructure to have a different signature?

The new signature is only required by this CL hence why I introduced it with it.
I'm happy to split it off if this makes the review process easier.

> 
> This is not LGTM as far as I'm concerned. This piece should be in a separate
CL
> that explains why it's needed, and someone familiar with iOS should review it,
> instead of it just being slipped in as part of a largely unrelated mega-CL.

Split off into its own CL (https://codereview.chromium.org/274773003/), details
explained there.

It shouldn't affect the iOS build as the previous signature was for Closures
(i.e. Callback<void(void)>) and now it's for Callback<R(void)) which is a
superset of Callback<void(void)> and existing callers shouldn't be affected).

I'm happy to have a long design discussion later on the best solution here, but
in the mean time this is blocking a critical Proteus feature (this CL) which
must land ASAP for M36.
So unless this is completely broken in some way, if the concerns are only about
how to structure this code, can we have this discussion after landing this?

(and please unblock this CL now that I've moved the part about iOS to a separate
CL -- which still needs to land first)

Thanks!
Gab

[Nico, 2014-05-07 23:59:33]

On Wed, May 7, 2014 at 4:50 PM, <gab@chromium.org> wrote:

> On 2014/05/07 22:47:32, stuartmorgan wrote:
>
>> On 2014/05/07 21:50:55, Nico wrote:
>> > (+stuart ios bit fyi)
>>
>
>  I'm totally perplexed. The CL description is that you are changing the
>> prefs
>> system, and you are as a totally unexplained side effect changing a core
>> piece
>> of iOS infrastructure to have a different signature?
>>
>
> The new signature is only required by this CL hence why I introduced it
> with it.
> I'm happy to split it off if this makes the review process easier.
>
>
>
>  This is not LGTM as far as I'm concerned. This piece should be in a
>> separate
>>
> CL
>
>> that explains why it's needed, and someone familiar with iOS should
>> review it,
>> instead of it just being slipped in as part of a largely unrelated
>> mega-CL.
>>
>
> Split off into its own CL (https://codereview.chromium.org/274773003/),
> details
> explained there.
>
> It shouldn't affect the iOS build as the previous signature was for
> Closures
> (i.e. Callback<void(void)>) and now it's for Callback<R(void)) which is a
> superset of Callback<void(void)> and existing callers shouldn't be
> affected).
>
> I'm happy to have a long design discussion later on the best solution
> here, but
> in the mean time this is blocking a critical Proteus feature (this CL)
> which
> must land ASAP for M36.
>

We don't generally do "must land ASAP" as a justification for code (as I'm
sure you know).


> So unless this is completely broken in some way, if the concerns are only
> about
> how to structure this code, can we have this discussion after landing this?
>
> (and please unblock this CL now that I've moved the part about iOS to a
> separate
> CL -- which still needs to land first)
>
> Thanks!
> Gab
>
> https://codereview.chromium.org/257003007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2014-05-08 02:38:14]

On May 7, 2014 7:59 PM, "Nico Weber" <thakis@chromium.org> wrote:
>
> On Wed, May 7, 2014 at 4:50 PM, <gab@chromium.org> wrote:
>>
>> On 2014/05/07 22:47:32, stuartmorgan wrote:
>>>
>>> On 2014/05/07 21:50:55, Nico wrote:
>>> > (+stuart ios bit fyi)
>>
>>
>>> I'm totally perplexed. The CL description is that you are changing the
prefs
>>> system, and you are as a totally unexplained side effect changing a
core piece
>>> of iOS infrastructure to have a different signature?
>>
>>
>> The new signature is only required by this CL hence why I introduced it
with it.
>> I'm happy to split it off if this makes the review process easier.
>>
>>
>>
>>> This is not LGTM as far as I'm concerned. This piece should be in a
separate
>>
>> CL
>>>
>>> that explains why it's needed, and someone familiar with iOS should
review it,
>>> instead of it just being slipped in as part of a largely unrelated
mega-CL.
>>
>>
>> Split off into its own CL (https://codereview.chromium.org/274773003/),
details
>> explained there.
>>
>> It shouldn't affect the iOS build as the previous signature was for
Closures
>> (i.e. Callback<void(void)>) and now it's for Callback<R(void)) which is a
>> superset of Callback<void(void)> and existing callers shouldn't be
affected).
>>
>> I'm happy to have a long design discussion later on the best solution
here, but
>> in the mean time this is blocking a critical Proteus feature (this CL)
which
>> must land ASAP for M36.
>
>
> We don't generally do "must land ASAP" as a justification for code (as
I'm sure you know).

Yes of course, and I'm one of the strong advocates of high code quality,
but I think exceptions can be made for critical CLs when we're down to
interface arguments that won't otherwise affect the binary and can thus be
adjusted in a follow-up CL, after branch point.

Either way, I think we're almost there and I mostly wanted to stress the
urgency (as we've missed branch points before because reviewers were not
aware of the urgency of a CL and we we've now been specifically mandated by
Proteus leadership to make this clear to reviewers in such cases).

And I don't think temporarily compromising the quality of the critical
closure API will even be required here anyways.

Thanks,
Gab

>
>>
>> So unless this is completely broken in some way, if the concerns are
only about
>> how to structure this code, can we have this discussion after landing
this?
>>
>> (and please unblock this CL now that I've moved the part about iOS to a
separate
>> CL -- which still needs to land first)
>>
>> Thanks!
>> Gab
>>
>> https://codereview.chromium.org/257003007/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-05-08 02:42:34]

On Wed, May 7, 2014 at 7:38 PM, Gabriel Charette <gab@chromium.org> wrote:

>
> On May 7, 2014 7:59 PM, "Nico Weber" <thakis@chromium.org> wrote:
> >
> > On Wed, May 7, 2014 at 4:50 PM, <gab@chromium.org> wrote:
> >>
> >> On 2014/05/07 22:47:32, stuartmorgan wrote:
> >>>
> >>> On 2014/05/07 21:50:55, Nico wrote:
> >>> > (+stuart ios bit fyi)
> >>
> >>
> >>> I'm totally perplexed. The CL description is that you are changing the
> prefs
> >>> system, and you are as a totally unexplained side effect changing a
> core piece
> >>> of iOS infrastructure to have a different signature?
> >>
> >>
> >> The new signature is only required by this CL hence why I introduced it
> with it.
> >> I'm happy to split it off if this makes the review process easier.
> >>
> >>
> >>
> >>> This is not LGTM as far as I'm concerned. This piece should be in a
> separate
> >>
> >> CL
> >>>
> >>> that explains why it's needed, and someone familiar with iOS should
> review it,
> >>> instead of it just being slipped in as part of a largely unrelated
> mega-CL.
> >>
> >>
> >> Split off into its own CL (https://codereview.chromium.org/274773003/),
> details
> >> explained there.
> >>
> >> It shouldn't affect the iOS build as the previous signature was for
> Closures
> >> (i.e. Callback<void(void)>) and now it's for Callback<R(void)) which is
> a
> >> superset of Callback<void(void)> and existing callers shouldn't be
> affected).
> >>
> >> I'm happy to have a long design discussion later on the best solution
> here, but
> >> in the mean time this is blocking a critical Proteus feature (this CL)
> which
> >> must land ASAP for M36.
> >
> >
> > We don't generally do "must land ASAP" as a justification for code (as
> I'm sure you know).
>
> Yes of course, and I'm one of the strong advocates of high code quality,
> but I think exceptions can be made for critical CLs when we're down to
> interface arguments that won't otherwise affect the binary and can thus be
> adjusted in a follow-up CL, after branch point.
>
I agree that this CL is solid, just felt compelled to point out that "omg
MXXX" shouldn't be used to dismissed reviewer comments.

Pinging reviewers and pointing out urgency is of course fair game (when
used in moderation, but the project is pretty good about that part).

> Either way, I think we're almost there and I mostly wanted to stress the
> urgency (as we've missed branch points before because reviewers were not
> aware of the urgency of a CL and we we've now been specifically mandated by
> Proteus leadership to make this clear to reviewers in such cases).
>
> And I don't think temporarily compromising the quality of the critical
> closure API will even be required here anyways.
>

Thanks,
> Gab
>
> >
> >>
> >> So unless this is completely broken in some way, if the concerns are
> only about
> >> how to structure this code, can we have this discussion after landing
> this?
> >>
> >> (and please unblock this CL now that I've moved the part about iOS to a
> separate
> >> CL -- which still needs to land first)
> >>
> >> Thanks!
> >> Gab
> >>
> >> https://codereview.chromium.org/257003007/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stuartmorgan, 2014-05-08 17:02:07]

LGTM to undo my veto; thanks for splitting it out.

[gab, 2014-05-09 11:59:51]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-09 12:06:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/790001

[gab, 2014-05-09 17:08:32]

Committed patchset #37 manually as r269346 (presubmit successful).

[gab, 2014-05-09 18:36:30]

FYI this was reverted in r269367 for introducing a memory leak [1].

From my analysis @ http://pastebin.com/QTrYGLPs this would be caused by a
pre-existing leak in post_task_and_reply_impl.cc [2] which is now stressed by
this CL.

[1]
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pos...

The leak occurs if shutdown occurs before a PostTaskAndReply's reply gets to
run; somehow this was never caught before?!?! This is an issue way beyond the
scope of this CL (and I doubt this CL makes it worse).

[gab, 2014-05-09 19:05:06]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-09 19:09:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/890001

[gab, 2014-05-09 20:32:12]

Robert, PTAL at the added memory suppressions.

[robertshield, 2014-05-09 20:34:30]

suppressions LGTM

[gab, 2014-05-09 21:33:29]

Committed patchset #40 manually as r269415 (presubmit successful).

[gab, 2014-05-09 21:35:40]

On 2014/05/09 21:33:29, gab wrote:
> Committed patchset #40 manually as r269415 (presubmit successful).

FYI, this is the same as patch set 37 (which is green across the board and was
originally landed) -- the only difference is the addition of memory suppressions
to workaround an existing issue  highlighted by this CL
(http://crbug.com/371974).

Please ping me directly if you see any problems with it.

Thanks!
Gab

[gab, 2014-05-10 02:41:38]

Robert, PTAL at patch set 43; diff it against patch set 38.

Essentially it's removing usage of PostTaskAndReply for now because of the
massive leak reports caused by it (http://crbug.com/371974). I'll look into
putting it back ASAP, but it in the mean time we could commit the meat of this
CL.

(note that this will make it such that we never cleanup, but since this code is
resilient to failed cleanups it should be okay (i.e., cleanup affected users
eventually in a build with the mini fix))

Cheers!
Gab

[robertshield, 2014-05-10 02:46:05]

important file writer changes lgtm

[gab, 2014-05-10 02:46:45]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-10 02:48:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/970001

[gab, 2014-05-10 03:06:01]

The CQ bit was unchecked by gab@chromium.org

[gab, 2014-05-10 03:06:16]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-10 03:09:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/970001

[gab, 2014-05-10 03:17:52]

The CQ bit was unchecked by gab@chromium.org

[gab, 2014-05-10 03:22:34]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-10 03:25:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/970001

[commit-bot: I haz the power, 2014-05-10 05:10:52]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-10 05:17:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 05:17:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[robertshield, 2014-05-10 13:18:06]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2014-05-10 13:20:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/970001

[gab, 2014-05-10 14:43:53]

The CQ bit was unchecked by gab@chromium.org

[gab, 2014-05-11 04:39:10]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-11 04:39:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/1030001

[gab, 2014-05-11 04:50:23]

FYI, if anyone is following along, this CL was reverted for breaking the LSAN
(Leak Sanitizer) bots.

It turns out PostTaskAndReply is leaking (http://crbug.com/371974) on shutdown
which now affects many tests that use ImportantFileWriter.
I've suppressed all those leak reports for now (I think; hopefully the list
isn't much longer than those that have already surfaced) as it doesn't otherwise
affect Chrome (see bug for details).
I'll fix the issue and remove the suppressions in a follow up CL.

There was another leak reported in ProfilePrefStoreManagerTests. That one was a
true product bug -- a single line lost in the latest API change I'd done to this
CL which made it such that the interceptor callback was no longer Reset() after
being called; thus keeping the migration's chain alive. 

Cheers!
Gab

[commit-bot: I haz the power, 2014-05-11 06:12:51]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-11 06:16:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-11 06:16:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[gab, 2014-05-12 00:28:58]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-12 00:29:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/1130001

[gab, 2014-05-12 00:31:20]

@Nico: FYI, slightly modified the implementation in ImportantFileWriter to
minimize the scope of the leaks caused by using PostTaskAndReply.

(happy to land a follow-up CL if you have comments on the updated code)

Cheers,
Gab

[robertshield, 2014-05-12 00:39:12]

lgtm

https://codereview.chromium.org/257003007/diff/1130001/base/files/important_f...
File base/files/important_file_writer.cc (right):

https://codereview.chromium.org/257003007/diff/1130001/base/files/important_f...
base/files/important_file_writer.cc:174: // suppressing all of those is
irrealistic hence we avoid most of them by
nit: s/irrealistic/unrealistic/

[commit-bot: I haz the power, 2014-05-12 00:52:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-12 00:54:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-12 00:54:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[gab, 2014-05-12 02:11:20]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-12 02:11:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/1130001

[commit-bot: I haz the power, 2014-05-12 02:14:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-12 02:17:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-12 02:17:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[gab, 2014-05-12 02:20:59]

Thanks Robert, addressed comment nit.

https://codereview.chromium.org/257003007/diff/1130001/base/files/important_f...
File base/files/important_file_writer.cc (right):

https://codereview.chromium.org/257003007/diff/1130001/base/files/important_f...
base/files/important_file_writer.cc:174: // suppressing all of those is
irrealistic hence we avoid most of them by
On 2014/05/12 00:39:13, robertshield wrote:
> nit: s/irrealistic/unrealistic/

Done.

[gab, 2014-05-12 02:22:08]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-05-12 02:22:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/257003007/1160001

[gab, 2014-05-12 02:24:19]

Committed patchset #52 manually as r269735 (presubmit successful).

[gab, 2014-05-12 02:25:18]

On 2014/05/12 02:24:19, gab wrote:
> Committed patchset #52 manually as r269735 (presubmit successful).

CQ was about to land patch set 50, but a minor merge conflict was landed before
it did; merged and dcommitted.