Fix a race condition in preference metric reporting.

chrome/browser/prefs/hashed_pref_store.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PREFS_HASHED_PREF_STORE_H_
+#define CHROME_BROWSER_PREFS_HASHED_PREF_STORE_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
+#include "base/prefs/persistent_pref_store.h"
+#include "base/prefs/pref_store.h"
+
+class PrefHashStore;
+
+namespace base {
+class Value;
+}  // namespace base
+
+// This interface is complementary to the PrefStore interface, declaring
+// additional functionality that adds support for setting values and persisting
+// the data to some backing store.
+class HashedPrefStore : public PersistentPrefStore {
+ public:
+  HashedPrefStore(const scoped_refptr<PersistentPrefStore>& pref_store,
+                  scoped_ptr<PrefHashStore> pref_hash_store);
+
+  virtual bool GetMutableValue(const std::string& key,

[gab, 2013/11/29 04:05:51]

Having to override all of these methods just to forward the calls to the
wrapped_pref_store_ feels to me like its calling out that this is the wrong
design (and I'm sorry I didn't realize that when we discussed earlier today..).
The possibility of someone extending this "helper" class to do much more than
simply forward these calls scares me. It is too big and strong for what it needs
to do.

I'm leaning more and more towards a VerifierDelegate design where a
JsonPrefStore could be given a VerifierDelegate which, if set, gets to verify
(and change if necessary) the state of the pref store BEFORE pref changes start
generating notifications to observers.

I don't feel this would be intrusive in the JsonPrefStore code as it only
requires:
1) a member/setter for the delegate
2) a call to the delegate that it should do its thing (right before calling
OnInitializationComplete())
3) a member variable in the pref store to know not to notify observers of
changes done by the delegate

Then we are guaranteed that when the store is handed off to the world and its
real observers, it is done in a verified state.

And the same delegate is also informed of all further changes.

This is akin to being the first observer as in your first design, but is much
more explicit about it and also has state to avoid notifying other observers
when doing the initial verification-based updates.

The store is handed ownership of its delegate.

The only downside to this I see is that it might be slightly harder to force
future implementations of PersistentPrefStore to do the same... here are
solutions I can think of:

1) It might already be taken care of simply by the fact that if someone wants to
trade a JsonPrefStore object for a FooPrefStore object, the FooPrefStore object
will have to handle set_verifier_delegate() or the calling code won't compile
and I don't think anyone sane (and their reviewers) would just blindly remove
the call without understanding what it does...

2) We could make PersistentPrefStore implement handling the delegate (I don't
know whether we care that it remains pure-virtual or if it being virtual is good
enough... actually it has a destructor defined so it is not pure virtual,
right?); then the delegate itself would simply have to DCHECK that it's being
told about initialization before any other pref change observation event.

3) Just add a pure-virtual SetVerifierDelegate() method to PersistentPrefStore
and assume new implementations will do the right thing.
At the end of the day, there are always things that new implementations are
implicitly expected to do which are only enforced by common sense (e.g.,
notifying observers added via AddObserver() on modification).

[gab, 2013/11/29 04:17:10]

Actually, even simpler than a delegate would be to hand a Callback over to the
pref store, to be called once: right before calling other observers (and
disabling observer notifications about change for the duration of the callback).

Than we can just be a regular observer for the remainder of the store's lifetime
(perhaps an owned observer, otherwise I'm not sure who would own it).

+                               base::Value** result) OVERRIDE;
+  virtual void ReportValueChanged(const std::string& key) OVERRIDE;
+  virtual void SetValue(const std::string& key, base::Value* value) OVERRIDE;
+  virtual void SetValueSilently(const std::string& key, base::Value* value)
+      OVERRIDE;
+  virtual void RemoveValue(const std::string& key) OVERRIDE;
+  virtual void MarkNeedsEmptyValue(const std::string& key) OVERRIDE;
+  virtual bool ReadOnly() const OVERRIDE;
+  virtual PrefReadError GetReadError() const OVERRIDE;
+  virtual PrefReadError ReadPrefs() OVERRIDE;
+  virtual void ReadPrefsAsync(
+      PersistentPrefStore::ReadErrorDelegate* error_delegate) OVERRIDE;
+  virtual void CommitPendingWrite() OVERRIDE;
+  virtual void AddObserver(PrefStore::Observer* observer) OVERRIDE;
+  virtual void RemoveObserver(PrefStore::Observer* observer) OVERRIDE;
+  virtual bool HasObservers() const OVERRIDE;
+  virtual bool IsInitializationComplete() const OVERRIDE;
+  virtual bool GetValue(const std::string& key,
+                        const base::Value** result) const OVERRIDE;
+
+ protected:
+  virtual ~HashedPrefStore() {}
+
+ private:
+  class PrefStoreObserver : public PrefStore::Observer {
+   public:
+    PrefStoreObserver(
+        const scoped_refptr<PersistentPrefStore>& wrapped_pref_store,
+        scoped_ptr<PrefHashStore> pref_hash_store);
+    virtual ~PrefStoreObserver();
+
+    // PrefStore::Observer implementation.
+    virtual void OnPrefValueChanged(const std::string& key) OVERRIDE;
+    virtual void OnInitializationCompleted(bool succeeded) OVERRIDE;
+
+    PersistentPrefStore* wrapped_pref_store() {
+      return wrapped_pref_store_;
+    }
+    const PersistentPrefStore* wrapped_pref_store() const {
+      return wrapped_pref_store_;
+    }
+    ObserverList<PrefStore::Observer, true>& observers() {

[gab, 2013/11/29 04:05:51]

I don't think we allow return by non-const ref in Chromium

+      return observers_;
+    }
+    const ObserverList<PrefStore::Observer, true>& observers() const {
+      return observers_;
+    }
+
+   private:
+    bool initialized_;
+
+    scoped_refptr<PersistentPrefStore> wrapped_pref_store_;
+
+    ObserverList<PrefStore::Observer, true> observers_;
+    scoped_ptr<PrefHashStore> pref_hash_store_;
+
+    DISALLOW_COPY_AND_ASSIGN(PrefStoreObserver);
+  };
+
+  PrefStoreObserver internal_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(HashedPrefStore);
+};
+
+#endif  // CHROME_BROWSER_PREFS_HASHED_PREF_STORE_H_