Separate storage for protected preferences into Protected Preferences file.

chrome/browser/prefs/profile_pref_store_manager.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prefs/profile_pref_store_manager.h"
 
 #include "base/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/json_pref_store.h"
 #include "base/prefs/persistent_pref_store.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "chrome/browser/prefs/pref_hash_store_impl.h"
 #include "chrome/browser/prefs/tracked/pref_service_hash_store_contents.h"
+#include "chrome/browser/prefs/tracked/protected_pref_store.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 
 namespace {
 
+// An adaptor that allows a PrefHashStoreImpl to access a preference store
+// directly as a dictionary. Uses an equivalent layout to
+// PrefStoreHashStoreContents.
+class DictionaryHashStoreContents : public HashStoreContents {
+ public:
+  // Instantiates a HashStoreContents that is a copy of |to_copy|. The copy is
+  // mutable but does not affect the original, nor is it persisted to disk in
+  // any other way.
+  explicit DictionaryHashStoreContents(const HashStoreContents& to_copy)
+      : hash_store_id_(to_copy.hash_store_id()),
+        super_mac_(to_copy.GetSuperMac()),
+        dictionary_(to_copy.IsInitialized()
+                        ? to_copy.GetContents()->DeepCopy()
+                        : static_cast<base::DictionaryValue*>(NULL)) {

[Bernhard Bauer, 2014/03/26 15:05:50]

Is the static_cast because of the ?: operator?

I would probably initialize |dictionary_| to NULL here in any case, and in the
constructor body conditionally assign the other value to it.

[erikwright (departed), 2014/03/26 21:08:12]

Done.

+    int version = 0;
+    if (to_copy.GetVersion(&version))
+      version_.reset(new int(version));
+  }
+
+  // HashStoreContents implementation
+  virtual std::string hash_store_id() const OVERRIDE { return hash_store_id_; }
+
+  virtual void Reset() OVERRIDE {
+    dictionary_.reset();
+    super_mac_ = "";

[Bernhard Bauer, 2014/03/26 15:05:50]

In general, using std::string() instead of "" saves a handful of instructions.
In this case, you can just call .clear().

[erikwright (departed), 2014/03/26 21:08:12]

Done.

+    version_.reset();
+  }
+
+  virtual bool IsInitialized() const OVERRIDE {
+    return dictionary_;

[Bernhard Bauer, 2014/03/26 15:05:50]

I would be a bit happier if we had an explicit coercion to bool here (for
example, by negating it twice, i.e. `!!dictionary_`). I think I remember a bug
where a value was truncated to a char (because there is no type that has only
one bit), which meant that a non-zero value where the least-significant byte was
zero would end up being interpreted as false.

[erikwright (departed), 2014/03/26 18:37:15]

This is explicitly supported by scoped_ptr, and covered by its unit tests. I
don't mind following your suggestion if you feel strongly about it, but since
other reviewers have specifically asked me to use implicit coercion to boolean
in the past I suspect it will be unlikely to catch on.

Of course, it would be a problem if this were safe with scoped_ptr but not with
raw pointers, in the chance that a future CL changed the type of dictionary_. I
read up a bit about it, and couldn't find any reference to the potential for
harm here. I did find the following (search for "Boolean conversions"):

http://en.cppreference.com/w/cpp/language/implicit_cast

It explicitly defines the behaviour when a pointer is cast to bool.

[Bernhard Bauer, 2014/03/27 15:10:45]

Oh, right, it's a scoped_ptr. Yeah, those are safe.
What I was thinking of was http://bugs.mysql.com/bug.php?id=64884 (which made it
to some tech news sites at the time). Maybe that bug is because of C though and
C++ has sane implicit conversion to bool.

[erikwright (departed), 2014/03/27 19:26:21]

Ah, right. Yeah, that would be an improvement due to a native bool type with
explicit conversion semantics.

Thanks for sharing that!

+  }
+
+  virtual const base::DictionaryValue* GetContents() const OVERRIDE{
+    return dictionary_.get();
+  }
+
+  virtual scoped_ptr<MutableDictionary> GetMutableContents() OVERRIDE {
+    return scoped_ptr<MutableDictionary>(
+        new SimpleMutableDictionary(this));
+  }
+
+  virtual std::string GetSuperMac() const OVERRIDE { return super_mac_; }
+
+  virtual void SetSuperMac(const std::string& super_mac) OVERRIDE {
+    super_mac_ = super_mac;
+  }
+
+  virtual bool GetVersion(int* version) const OVERRIDE {
+    if (!version_)
+      return false;
+    *version = *version_;
+    return true;
+  }
+
+  virtual void SetVersion(int version) OVERRIDE {
+    version_.reset(new int(version));
+  }
+
+ private:
+  class SimpleMutableDictionary
+      : public HashStoreContents::MutableDictionary {
+   public:
+    explicit SimpleMutableDictionary(DictionaryHashStoreContents* outer)
+        : outer_(outer) {}
+
+    virtual ~SimpleMutableDictionary() {}
+
+    // MutableDictionary implementation
+    virtual base::DictionaryValue* operator->() OVERRIDE {
+      if (!outer_->dictionary_)
+        outer_->dictionary_.reset(new base::DictionaryValue);
+      return outer_->dictionary_.get();
+    }
+
+   private:
+    DictionaryHashStoreContents* outer_;
+    DISALLOW_COPY_AND_ASSIGN(SimpleMutableDictionary);
+  };
+
+  const std::string hash_store_id_;
+  std::string super_mac_;
+  scoped_ptr<int> version_;
+  scoped_ptr<base::DictionaryValue> dictionary_;
+
+  DISALLOW_COPY_AND_ASSIGN(DictionaryHashStoreContents);
+};
+
 // An in-memory PrefStore backed by an immutable DictionaryValue.
 class DictionaryPrefStore : public PrefStore {
  public:
   explicit DictionaryPrefStore(const base::DictionaryValue* dictionary)
       : dictionary_(dictionary) {}
 
   virtual bool GetValue(const std::string& key,
                         const base::Value** result) const OVERRIDE {
     const base::Value* tmp = NULL;
     if (!dictionary_->Get(key, &tmp))
@@ skipping 10 matching lines @@
   const base::DictionaryValue* dictionary_;
 
   DISALLOW_COPY_AND_ASSIGN(DictionaryPrefStore);
 };
 
 }  // namespace
 
 // TODO(erikwright): Enable this on Chrome OS and Android once MACs are moved
 // out of Local State. This will resolve a race condition on Android and a
 // privacy issue on ChromeOS. http://crbug.com/349158
 const bool ProfilePrefStoreManager::kPlatformSupportsPreferenceTracking =

[Bernhard Bauer, 2014/03/26 15:05:50]

Hm, couldn't you make this a #define and then #ifdef out all the code that's
unnecessary if the platform doesn't support preference tracking?

[erikwright (departed), 2014/03/26 21:08:12]

It's also referenced in chrome_pref_service_factory.cc, so you would need to
expose that #define in the header and reference it there too (or use both a
#define and a boolean).

(1) This will be universally supported shortly (the follow up to this CL will
resolve the issues blocking ChromeOS and Android).
(2) I don't think it will have any measurable impact on performance or code
size. I'm pretty sure 80% of the stuff that would be hidden would still be
referenceable.

[Bernhard Bauer, 2014/03/27 15:10:45]

Oh, ok. If it will be removed soon anyway, it's fine.

 #if defined(OS_ANDROID) || defined(OS_CHROMEOS)
     false;
 #else
     true;
 #endif
 
 // Waits for a PrefStore to be initialized and then initializes the
 // corresponding PrefHashStore.
 // The observer deletes itself when its work is completed.
 class ProfilePrefStoreManager::InitializeHashStoreObserver
@@ skipping 104 matching lines @@
 }
 
 void ProfilePrefStoreManager::ResetPrefHashStore() {
   if (kPlatformSupportsPreferenceTracking)
     GetPrefHashStoreImpl()->Reset();
 }
 
 PersistentPrefStore* ProfilePrefStoreManager::CreateProfilePrefStore(
     const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
   scoped_ptr<PrefFilter> pref_filter;
-  if (kPlatformSupportsPreferenceTracking) {
-    pref_filter.reset(
-        new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
-                           tracking_configuration_,
-                           reporting_ids_count_));
+  if (!kPlatformSupportsPreferenceTracking) {
+    return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                             io_task_runner,
+                             scoped_ptr<PrefFilter>());
   }
-  return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
-                           io_task_runner,
-                           pref_filter.Pass());
+
+  std::vector<PrefHashFilter::TrackedPreferenceMetadata>
+      unprotected_configuration;
+  std::vector<PrefHashFilter::TrackedPreferenceMetadata>
+      protected_configuration;
+  std::set<std::string> protected_pref_names;
+  for (size_t i = 0; i < tracking_configuration_.size(); ++i) {
+    if (tracking_configuration_[i].enforcement_level >
+        PrefHashFilter::NO_ENFORCEMENT) {
+      protected_configuration.push_back(tracking_configuration_[i]);
+      protected_pref_names.insert(tracking_configuration_[i].name);
+    } else {
+      unprotected_configuration.push_back(tracking_configuration_[i]);
+    }
+  }
+
+  scoped_ptr<PrefHashFilter> unprotected_pref_hash_filter(
+      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                         unprotected_configuration,
+                         reporting_ids_count_));
+  scoped_ptr<PrefHashFilter> protected_pref_hash_filter(
+      new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                         protected_configuration,
+                         reporting_ids_count_));
+
+  scoped_refptr<PersistentPrefStore> unprotected_pref_store(
+      new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                        io_task_runner,
+                        unprotected_pref_hash_filter.PassAs<PrefFilter>()));
+  scoped_refptr<PersistentPrefStore> protected_pref_store(new JsonPrefStore(
+      profile_path_.Append(chrome::kProtectedPreferencesFilename),
+      io_task_runner,
+      protected_pref_hash_filter.PassAs<PrefFilter>()));
+
+  return new ProtectedPrefStore(
+      unprotected_pref_store,
+      protected_pref_store,
+      protected_pref_names,
+      base::Bind(&PrefHashFilter::MigrateValues,
+                 base::Owned(new PrefHashFilter(
+                     CopyPrefHashStore(),
+                     protected_configuration,
+                     reporting_ids_count_)),
+                 unprotected_pref_store,
+                 protected_pref_store));
 }
 
 void ProfilePrefStoreManager::UpdateProfileHashStoreIfRequired(
     const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
   if (!kPlatformSupportsPreferenceTracking)
     return;
   scoped_ptr<PrefHashStoreImpl> pref_hash_store_impl(GetPrefHashStoreImpl());
   const PrefHashStoreImpl::StoreVersion current_version =
       pref_hash_store_impl->GetCurrentVersion();
   UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferencesAlternateStoreVersion",
@@ skipping 15 matching lines @@
   }
 }
 
 bool ProfilePrefStoreManager::InitializePrefsFromMasterPrefs(
     const base::DictionaryValue& master_prefs) {
   // Create the profile directory if it doesn't exist yet (very possible on
   // first run).
   if (!base::CreateDirectory(profile_path_))
     return false;
 
+  // This will write out to a single combined file which will be immediately
+  // migrated to two files on load.
   JSONFileValueSerializer serializer(
       GetPrefFilePathFromProfilePath(profile_path_));
 
   // Call Serialize (which does IO) on the main thread, which would _normally_
   // be verboten. In this case however, we require this IO to synchronously
   // complete before Chrome can start (as master preferences seed the Local
   // State and Preferences files). This won't trip ThreadIORestrictions as they
   // won't have kicked in yet on the main thread.
   bool success = serializer.Serialize(master_prefs);
 
   if (success && kPlatformSupportsPreferenceTracking) {
     scoped_refptr<const PrefStore> pref_store(
         new DictionaryPrefStore(&master_prefs));
     PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
                    tracking_configuration_,
                    reporting_ids_count_).Initialize(*pref_store);
   }
 
   UMA_HISTOGRAM_BOOLEAN("Settings.InitializedFromMasterPrefs", success);
   return success;
 }
 
+PersistentPrefStore*
+ProfilePrefStoreManager::CreateDeprecatedCombinedProfilePrefStore(
+    const scoped_refptr<base::SequencedTaskRunner>& io_task_runner) {
+  scoped_ptr<PrefFilter> pref_filter;
+  if (kPlatformSupportsPreferenceTracking) {
+    pref_filter.reset(
+        new PrefHashFilter(GetPrefHashStoreImpl().PassAs<PrefHashStore>(),
+                           tracking_configuration_,
+                           reporting_ids_count_));
+  }
+  return new JsonPrefStore(GetPrefFilePathFromProfilePath(profile_path_),
+                           io_task_runner,
+                           pref_filter.Pass());
+}
+
 scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl() {
   DCHECK(kPlatformSupportsPreferenceTracking);
 
   return make_scoped_ptr(new PrefHashStoreImpl(
       seed_,
       device_id_,
       scoped_ptr<HashStoreContents>(new PrefServiceHashStoreContents(
           profile_path_.AsUTF8Unsafe(), local_state_))));
 }
+
+scoped_ptr<PrefHashStore> ProfilePrefStoreManager::CopyPrefHashStore() {
+  DCHECK(kPlatformSupportsPreferenceTracking);
+
+  PrefServiceHashStoreContents real_contents(profile_path_.AsUTF8Unsafe(),
+                                             local_state_);
+  return scoped_ptr<PrefHashStore>(new PrefHashStoreImpl(
+      seed_,
+      device_id_,
+      scoped_ptr<HashStoreContents>(
+          new DictionaryHashStoreContents(real_contents))));
+}