Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
324493002:
Move preference MACs to the protected preference stores. (Closed)
|
Created:
6 years, 6 months ago by erikwright (departed) Modified:
6 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Cait (Slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMove preference MACs to the protected preference stores.
(1) 1-time migration of MACs from local state to Preferences/Protected Preferences.
(2) Migrate MACs between Preferences/Protected Preferences according to configuration changes.
Proposed follow-up tasks are:
(1) Introduce TrackedPreferencesMigrationDelegate
(2) Introduce protections to prevent unintended stamping of the super MAC.
(3) Expanded test coverage of PrefHashFilter
(4) Expanded test coverage for legacy migration in TrackedPreferencesMigrationTest
BUG=372547, 368480
R=asvitkine@chromium.org, gab@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278164
Patch Set 1 #Patch Set 2 : Pull some stuff from Persistent to WriteablePrefStore. #
Total comments: 47
Patch Set 3 : Make unit tests pass. #Patch Set 4 : Rebase. #Patch Set 5 : Respond to CR comments. #
Total comments: 7
Patch Set 6 : Extract some pre-CLs. #Patch Set 7 : New approach. #
Total comments: 1
Patch Set 8 : Correct errant keystroke. #Patch Set 9 : Remove some now unnecessary changes. #
Total comments: 52
Patch Set 10 : Existing tests pass. #Patch Set 11 : Review comments. #Patch Set 12 : Self-review. #
Total comments: 34
Patch Set 13 : Rebase. #Patch Set 14 : Rebase. #Patch Set 15 : Fix test compile failure. #Patch Set 16 : Add some test coverage. #Patch Set 17 : Gab comments. #Patch Set 18 : Add tracking for MAC migrations from local state. #
Total comments: 22
Patch Set 19 : Add a test for newly tracked prefs when first enabling protection. #
Total comments: 2
Patch Set 20 : Final review comments? #Patch Set 21 : Final final...? #
Total comments: 1
Patch Set 22 : Rebase. #
Total comments: 1
Patch Set 23 : Histograms nit. #Patch Set 24 : Comment typo. #Messages
Total messages: 31 (0 generated)
PTAL. The one remaining change I want to make is to move GetMutableValue from PersistentPrefStore to WritablePrefStore. Right now I have an unnecessary copy of the hash store when updating it, simply to support a DictionaryPrefStore for initialization from master prefs.
First responding to the points in the CL description inline: (1) Make JsonPrefStore store its loaded value before invoking its filter. I think this is incorrect, see this comment and others for details: https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... (2) Move GetMutableValue and ReportValueChanged from PersistentPrefStore to WriteablePrefStore. (3) Remove the concept of hash store version. (4) Remove support for initializing/migrating hash stores for unloaded profiles. The above 3 points can be done in precursor CLs (as you've previously stated offline). I originally said we may want to keep everything in a single CL for ease of merging, but after seeing what the strippable parts are, I'd definitely take at least 3+4 out (removals look benign in standalone CLs, but they otherwise make this CL look unnecessarily bigger -- I'm fine with a single removal CL for both 3 and 4). (5) Fix a bug wherein a super MAC would only be stamped if any protected/tracked value changes. Good catch! This was previously unintentionally WAI as the extension.settings pref happens to always be updated on startup, but when moving to a model where the MACs are in their respective store this will indeed be broken for the store not in charge of extensions. (6) 1-time migration of MACs from local state to Preferences/Protected Preferences. This isn't really a 1-time migration IMO; it's more an "if necessary" migration (i.e., if it fails to commit migrated MACs to disk the first time, it will do migration again in the next run). (7) Migrate MACs between Preferences/Protected Preferences according to configuration changes. Yay :-)! Overall, as described in my comments above and below, I still think that my afore-proposed approach is better. I also don't think it's more complicated than the current approach. I in fact think it's simpler as it doesn't have a cyclic dependency and allows for a simpler ownership model as described in my comments below. I'm happy to have someone else take a look if you disagree with my point of view (I think Bernhard would be best as he was the reviewer on the addition of TrackedPreferencesMigrator). Cheers! Gab https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... base/prefs/json_pref_store.cc:345: &JsonPrefStore::FinalizeFileRead, this, initialization_successful)); This callback was using ref-counting simply to avoid changing the old behaviour where the reader (FileThreadDeserializer) would own a ref to this JsonPrefStore all the way until the end of OnFileRead() -- which would guarantee that observers are always notified of a read (success or fail) after calling ReadPrefsAsync(). We (me and Bernhard) thought this was a side-effect of the current implementation and ideally wanted to eventually remove that (i.e., have the read task be weakly bound to this JsonPrefStore, allowing the store to go away if it needs to before the read comes back), but kept it as-is for now for the sake of not introducing more changes into the big change that was TrackedPreferencesMigrator et al. By making FilterOnLoad take a raw pointer to this store's prefs rather than own them until it's ready to give them back to us you are making the above callback HAVE to use ref-counting or the following could happen: 1) JsonPrefStore 1 hands a raw pointer to its prefs post-read. 2) JsonPrefStore 1 goes away. 2) JsonPrefStore 2 hands a raw pointer to its prefs post-read; which triggers migration, which will use the invalid pointer to 1's prefs. At the very least a comment should be added to this effect above. Overall however I think this is the wrong approach. This strengthens my belief in the other approach I'd suggested earlier: Let the TrackedPreferencesMigrator be aware of where to find the MACs in the pref dictionaries (or even better: wrap the dictionary in a HashStoreContents that already knows how to do this to avoid duplicating this logic). The TrackedPreferencesMigrator also needs to be given a HashStoreContents wrapping local_state. Then the TrackedPreferencesMigrator does ALL the migration work while owning the prefs and hands them back to their respective stores when everything is done. Also, in my proposal, PrefHashStores don't have a reference to the PrefStore storing the MACs so there is no cyclic dependency like there is in the model you're proposing (JsonPrefStore => PrefHashFilter => PrefHashStore -> JsonPrefStore). Instead PrefHashFilters are always handed a DictionaryValue* in operations where they're meant to do work on the prefs and that DictionaryValue* is passed to PrefHashStore::BeginTransaction() -- most likely as a DictionaryHashStoreContents. PrefHashStores essentially now only own the state of whether the hash store was initially trusted (this state would be initialized in FilterOnLoad) -- in fact PrefHashStore can probably go away altogether and we can let that boolean state live in PrefHashFilter. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_filter.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_filter.cc:160: pref_hash_store_->CommitPendingWrite(); Remove this call (and PrefHashStore::has_pending_write_/CommitPendingWrite(), but I'm okay with leaving the calls behind and removing it in a follow-up CL to keep things small in this one). https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:56: initial_hashes_dictionary_trusted_); This histogram should also move. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:61: void PrefHashStoreImpl::SetHashStoreContents( Call this method Init()? And explicitly state that it MUST be called before using this PrefHashStoreImpl (as the rest of the class assumes its set). Note: In my proposed model this method is not required as there is no cyclic dependency; I find that much cleaner. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:173: const base::Value* hash) { I think |hash| should be passed as a std::string. The fact that it's stored as a Value* is not relevant to this API. Passing it by pointer could perhaps save us a few string copies, but in this case it doesn't even help with that either since you have to DeepCopy() the Value* below anyways. So I say pass it by std::string and use SetString below (this class is well aware that hashes are stored as strings anyways). https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:180: super_mac_dirty_ || outer_->initial_hashes_dictionary_trusted_; It's incorrect to re-write the super MAC here if the super MAC of the source store is not trusted. I think it's possible in the current model for an attacker to completely delete this hash store, put a single hash for an irrelevant pref in the other store, have that hash be migrated and generate a new super MAC... and then we happily TrustInitialize every other pref... :(. The way I see super MACs working in the model I proposed is as follows: TrackedPreferencesMigrator can easily keep track of which stores it pushed new MACs into. It can thus re-generate super MACs for those stores if and only if the super MAC for both the source store(s) and the destination store were previously valid. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_store_transaction.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_transaction.h:57: virtual bool StampSuperMac() = 0; Add those methods at the bottom of this file (keeping CheckValue/StoreHash and CheckSplitValue/StoreSplitHash together makes more sense than adding this in between). And add comments for them. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:59: FOR_EACH_OBSERVER(Observer, observers_, OnPrefValueChanged(key)); Call ReportValueChanged() here and below instead of making the call explicitly (and thus expanding the macro in multiple places)? https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:75: return true; Just like JsonPrefStore::GetMutableValue(), I think return dictionary_->Get(key, result); works just fine here. (and I think it also works fine for GetValue()). https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:93: MigrationDelegateImpl(const base::WeakPtr<JsonPrefStore>& pref_store, Although this delegate is potentially a nice clean up (it would need to check weak pointers properly below though), I don't think it's necessary in order to get working code so I'd vote against it in this CL since you may want to request merge. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:304: } In my proposed model this logic can remain as it used to be. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/pref_store_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/pref_store_hash_store_contents.h:16: class PrefStoreHashStoreContents : public HashStoreContents { To avoid a lot of new code (trying to keep things small), as part of my proposed model, I suggest we re-use the same logic for both PrefServiceHashStoreContents and DictionaryHashStoreContents; the only difference being how the hash store dictionary is obtained. So we could have a HashStoreContents with most of the logic which takes in a delegate or has a single virtual method to be able to get the actual DictionaryValue from either a PrefService or a DictionaryValue. FWICT, the only other differences are that (1) the hash_store_id is empty (we can explicitly pass in an empty string) and (2) we ideally want to name the pref "super_mac" rather than "hash_of_hashes", we can either make that a parameter to be removed later or just not care... https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:47: void CleanupPrefStoreAndMaybeLocalState( s/LocalState/LegacyHashStore ? here and below (since the API never really mentions that the legacy hash store has to be Local State). https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:80: // Invokes |store_cleaner| for every |keys_to_clean|. Expand method comment. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:92: if (!--pending_commits->data) s/!--pending_commits->data/--pending_commits->data == 0 is less obscure IMO https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:93: this->legacy_pref_hash_store_->Reset(); Instead of using the ref-counted |pending_commits| logic, I would prefer to put legacy_hash_store_transaction->ClearHash(*it); in the loop above and simply have logic in PrefHashStoreTransactionImpl::ClearHash() to trigger a Reset() when the last hash is cleared. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:111: this, One of the reason this was previously bound to a free function was so that the reference to the JsonPrefStores held by the finalize_(un)protected_filter_on_load_ callbacks would be released post migration. Now the TrackedPreferencesMigrator and its members will live until the cleanup tasks are complete which is wrong (I realize this wasn't obvious, apologies for the lack of comments on this). The model is that the TrackedPreferencesMigrator holds on to the prefs of one store while the other store is being read; once both stores are read, it migrates and then immediately frees everything (scheduling optimistic tasks to maybe clean up later, through weak pointers, if possible). Either way, I think this is another argument for not introducing the delegate interface in this CL. Smaller changes FTW. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:153: if (!new_store->Get(pref_name, &value_in_new_store)) { Remove the |value_in_new_store| variable, you can pass NULL into this Get() call. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:255: if (!pending_commits->data) { Above I suggested getting rid of |pending_commits|. Here this would simply turn into: if (!unprotected_prefs_altered && !protected_prefs_altered) { // If neither store was altered in this run, it is safe to immediately reset the legacy hash store. legacy_pref_hash_store_->Reset(); } https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:257: } else { Remove this else conditional, the code below needs to run unconditionally (i.e., it's possible that neither store was altered yet they both need cleanup -- since cleanup may have failed in the previous run). https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.h:20: class TrackedPreferencesMigrationDelegate { As mentioned before, I agree that this delegate may be a nice addition to reduce the number of parameters to SetupTrackedPreferencesMigration. But since it only has one caller, I don't think it's a necessary cleanup as part of this CL.
https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:173: const base::Value* hash) { On 2014/06/06 21:54:59, gab wrote: > I think |hash| should be passed as a std::string. The fact that it's stored as a > Value* is not relevant to this API. Passing it by pointer could perhaps save us > a few string copies, but in this case it doesn't even help with that either > since you have to DeepCopy() the Value* below anyways. > > So I say pass it by std::string and use SetString below (this class is well > aware that hashes are stored as strings anyways). That requires separate pairs of methods for atomic and split prefs. The benefit of using a Value is that it works for either a dictionary or a string. I could also introduce a purpose-built type (PreferenceMac, say) that would hold either a string or a dictionary. Returning anything other than a Value requires me to copy everything that comes out. Probably not a performance issue, but it will be extra code. Let me know what you think, given that. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:180: super_mac_dirty_ || outer_->initial_hashes_dictionary_trusted_; On 2014/06/06 21:54:59, gab wrote: > It's incorrect to re-write the super MAC here if the super MAC of the source > store is not trusted. > > I think it's possible in the current model for an attacker to completely delete > this hash store, put a single hash for an irrelevant pref in the other store, > have that hash be migrated and generate a new super MAC... and then we happily > TrustInitialize every other pref... :(. > > > The way I see super MACs working in the model I proposed is as follows: > TrackedPreferencesMigrator can easily keep track of which stores it pushed new > MACs into. It can thus re-generate super MACs for those stores if and only if > the super MAC for both the source store(s) and the destination store were > previously valid. We talked about this offline. It's correct to keep the super MAC validity if and only if the target store has a valid super MAC.
https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:173: const base::Value* hash) { On 2014/06/09 18:34:50, erikwright wrote: > On 2014/06/06 21:54:59, gab wrote: > > I think |hash| should be passed as a std::string. The fact that it's stored as > a > > Value* is not relevant to this API. Passing it by pointer could perhaps save > us > > a few string copies, but in this case it doesn't even help with that either > > since you have to DeepCopy() the Value* below anyways. > > > > So I say pass it by std::string and use SetString below (this class is well > > aware that hashes are stored as strings anyways). > > That requires separate pairs of methods for atomic and split prefs. The benefit > of using a Value is that it works for either a dictionary or a string. > > I could also introduce a purpose-built type (PreferenceMac, say) that would hold > either a string or a dictionary. > > Returning anything other than a Value requires me to copy everything that comes > out. Probably not a performance issue, but it will be extra code. > > Let me know what you think, given that. Duh, of course, passing it via a Value object is fine to me then. Perhaps renaming the parameter and method from "hash" to something more generic like "hash_content" would help here; otherwise a dictionary is not really a hash per se. How about: ImportHashContent(const std::string& path, const base::Value* hash_content);
On 2014/06/09 20:24:33, gab wrote: > https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... > File chrome/browser/prefs/pref_hash_store_impl.cc (right): > > https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... > chrome/browser/prefs/pref_hash_store_impl.cc:173: const base::Value* hash) { > On 2014/06/09 18:34:50, erikwright wrote: > > On 2014/06/06 21:54:59, gab wrote: > > > I think |hash| should be passed as a std::string. The fact that it's stored > as > > a > > > Value* is not relevant to this API. Passing it by pointer could perhaps save > > us > > > a few string copies, but in this case it doesn't even help with that either > > > since you have to DeepCopy() the Value* below anyways. > > > > > > So I say pass it by std::string and use SetString below (this class is well > > > aware that hashes are stored as strings anyways). > > > > That requires separate pairs of methods for atomic and split prefs. The > benefit > > of using a Value is that it works for either a dictionary or a string. > > > > I could also introduce a purpose-built type (PreferenceMac, say) that would > hold > > either a string or a dictionary. > > > > Returning anything other than a Value requires me to copy everything that > comes > > out. Probably not a performance issue, but it will be extra code. > > > > Let me know what you think, given that. > > Duh, of course, passing it via a Value object is fine to me then. Don't feel too bad. I had to run it in the debugger to realize my mistake (I originally had it return a string as you suggest, resulting in failures for split prefs). > > Perhaps renaming the parameter and method from "hash" to something more generic > like "hash_content" would help here; otherwise a dictionary is not really a hash > per se. > > How about: > > ImportHashContent(const std::string& path, const base::Value* hash_content);
Some follow-up comments. I'll upload some more patchsets shortly. https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... base/prefs/json_pref_store.cc:345: &JsonPrefStore::FinalizeFileRead, this, initialization_successful)); On 2014/06/06 21:54:58, gab wrote: > This callback was using ref-counting simply to avoid changing the old behaviour > where the reader (FileThreadDeserializer) would own a ref to this JsonPrefStore > all the way until the end of OnFileRead() -- which would guarantee that > observers are always notified of a read (success or fail) after calling > ReadPrefsAsync(). > > We (me and Bernhard) thought this was a side-effect of the current > implementation and ideally wanted to eventually remove that (i.e., have the read > task be weakly bound to this JsonPrefStore, allowing the store to go away if it > needs to before the read comes back), but kept it as-is for now for the sake of > not introducing more changes into the big change that was > TrackedPreferencesMigrator et al. > > By making FilterOnLoad take a raw pointer to this store's prefs rather than own > them until it's ready to give them back to us you are making the above callback > HAVE to use ref-counting or the following could happen: > > 1) JsonPrefStore 1 hands a raw pointer to its prefs post-read. > 2) JsonPrefStore 1 goes away. > 2) JsonPrefStore 2 hands a raw pointer to its prefs post-read; which triggers > migration, which will use the invalid pointer to 1's prefs. > This can be mitigated in other ways. For example, you can have the post_filter_on_load_callback own the DictionaryValue (or share with this store, via refcounting, an object that owns the Dictionary). So I don't think that passing the raw pointer to the dictionary really compromises the other choices you could make. Really, if there is an asynchronous process ongoing, something is going to be kept alive. It might be a DictionaryValue, it might be a JsonPrefStore, or it might be something in between. The only reason to really care what it is is if there are other side-effects to the destructor of the JsonPrefStore. And if there are, you can find lots of ways to ensure those side-effects have the semantics you expect. > At the very least a comment should be added to this effect above. I don't mind adding a comment - would you mind supplying your preferred wording? > Overall however I think this is the wrong approach. This strengthens my belief > in the other approach I'd suggested earlier: > > Let the TrackedPreferencesMigrator be aware of where to find the MACs in the > pref dictionaries (or even better: wrap the dictionary in a HashStoreContents > that already knows how to do this to avoid duplicating this logic). HashStoreContents and/or PrefHashStore are appropriate layers of abstraction and can provide whatever APIs we need to move things with the semantics we want. There is no need to break encapsulation here. > The TrackedPreferencesMigrator also needs to be given a HashStoreContents > wrapping local_state. As it is in this patchset. > Then the TrackedPreferencesMigrator does ALL the migration work while owning the > prefs and hands them back to their respective stores when everything is done. I believe that's what this patchset does. Is there a distinction you are making here, other than the part about "owns"? All that is injected is the implementation of PrefHashStore (and HashStoreContents) which only wraps the storage model for the hashes. IIUC the primary difference you want to see is that the PrefStore* or DictionaryValue* is passed into the PrefHashStore rather than being embedded within it. > Also, in my proposal, PrefHashStores don't have a reference to the PrefStore > storing the MACs so there is no cyclic dependency like there is in the model > you're proposing (JsonPrefStore => PrefHashFilter => PrefHashStore -> > JsonPrefStore). Instead PrefHashFilters are always handed a DictionaryValue* in > operations where they're meant to do work on the prefs and that DictionaryValue* > is passed to PrefHashStore::BeginTransaction() -- most likely as a > DictionaryHashStoreContents. I agree that we should break the circular dependency. > PrefHashStores essentially now only own the state of whether the hash store was > initially trusted (this state would be initialized in FilterOnLoad) -- in fact > PrefHashStore can probably go away altogether and we can let that boolean state > live in PrefHashFilter. PrefHashStore implements: (1) the process for calculating a MAC from a value and storing it in a Dictionary. (2) the process of retrieving a MAC from a dictionary, comparing it to a value, and returning a result. (3) the protocol for maintaing the super MAC and determining whether newly protected values should be trusted. Since (3) is intimately tied to the import process it is essential that it be implemented by a component that participates in the import process. The PrefHashFilter does not. I believe that the above demonstrates why it is appropriate for the PrefHashStore to be injected as a dependency into the migration process, even if it expects to be handed a DictionaryValue to work on.
https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... base/prefs/json_pref_store.cc:345: &JsonPrefStore::FinalizeFileRead, this, initialization_successful)); On 2014/06/10 14:25:36, erikwright wrote: > On 2014/06/06 21:54:58, gab wrote: > > This callback was using ref-counting simply to avoid changing the old > behaviour > > where the reader (FileThreadDeserializer) would own a ref to this > JsonPrefStore > > all the way until the end of OnFileRead() -- which would guarantee that > > observers are always notified of a read (success or fail) after calling > > ReadPrefsAsync(). > > > > We (me and Bernhard) thought this was a side-effect of the current > > implementation and ideally wanted to eventually remove that (i.e., have the > read > > task be weakly bound to this JsonPrefStore, allowing the store to go away if > it > > needs to before the read comes back), but kept it as-is for now for the sake > of > > not introducing more changes into the big change that was > > TrackedPreferencesMigrator et al. > > > > By making FilterOnLoad take a raw pointer to this store's prefs rather than > own > > them until it's ready to give them back to us you are making the above > callback > > HAVE to use ref-counting or the following could happen: > > > > 1) JsonPrefStore 1 hands a raw pointer to its prefs post-read. > > 2) JsonPrefStore 1 goes away. > > 2) JsonPrefStore 2 hands a raw pointer to its prefs post-read; which triggers > > migration, which will use the invalid pointer to 1's prefs. > > > > This can be mitigated in other ways. For example, you can have the > post_filter_on_load_callback own the DictionaryValue (or share with this store, > via refcounting, an object that owns the Dictionary). So I don't think that > passing the raw pointer to the dictionary really compromises the other choices > you could make. Really, if there is an asynchronous process ongoing, something > is going to be kept alive. It might be a DictionaryValue, it might be a > JsonPrefStore, or it might be something in between. The only reason to really > care what it is is if there are other side-effects to the destructor of the > JsonPrefStore. And if there are, you can find lots of ways to ensure those > side-effects have the semantics you expect. > > > At the very least a comment should be added to this effect above. > > I don't mind adding a comment - would you mind supplying your preferred wording? > > > Overall however I think this is the wrong approach. This strengthens my belief > > in the other approach I'd suggested earlier: > > > > Let the TrackedPreferencesMigrator be aware of where to find the MACs in the > > pref dictionaries (or even better: wrap the dictionary in a HashStoreContents > > that already knows how to do this to avoid duplicating this logic). > > HashStoreContents and/or PrefHashStore are appropriate layers of abstraction and > can provide whatever APIs we need to move things with the semantics we want. > There is no need to break encapsulation here. > > > The TrackedPreferencesMigrator also needs to be given a HashStoreContents > > wrapping local_state. > > As it is in this patchset. > > > Then the TrackedPreferencesMigrator does ALL the migration work while owning > the > > prefs and hands them back to their respective stores when everything is done. > > I believe that's what this patchset does. Is there a distinction you are making > here, other than the part about "owns"? All that is injected is the > implementation of PrefHashStore (and HashStoreContents) which only wraps the > storage model for the hashes. > > IIUC the primary difference you want to see is that the PrefStore* or > DictionaryValue* is passed into the PrefHashStore rather than being embedded > within it. > > > Also, in my proposal, PrefHashStores don't have a reference to the PrefStore > > storing the MACs so there is no cyclic dependency like there is in the model > > you're proposing (JsonPrefStore => PrefHashFilter => PrefHashStore -> > > JsonPrefStore). Instead PrefHashFilters are always handed a DictionaryValue* > in > > operations where they're meant to do work on the prefs and that > DictionaryValue* > > is passed to PrefHashStore::BeginTransaction() -- most likely as a > > DictionaryHashStoreContents. > > I agree that we should break the circular dependency. > > > PrefHashStores essentially now only own the state of whether the hash store > was > > initially trusted (this state would be initialized in FilterOnLoad) -- in fact > > PrefHashStore can probably go away altogether and we can let that boolean > state > > live in PrefHashFilter. > > PrefHashStore implements: > > (1) the process for calculating a MAC from a value and storing it in a > Dictionary. > (2) the process of retrieving a MAC from a dictionary, comparing it to a value, > and returning a result. > (3) the protocol for maintaing the super MAC and determining whether newly > protected values should be trusted. > > Since (3) is intimately tied to the import process it is essential that it be > implemented by a component that participates in the import process. The > PrefHashFilter does not. > > I believe that the above demonstrates why it is appropriate for the > PrefHashStore to be injected as a dependency into the migration process, even if > it expects to be handed a DictionaryValue to work on. For the comment to be added above, how about: // This callback must own a reference to this store since it depends on |prefs_| being alive when it runs. // TODO(gab): Remove this temporary requirement by handing |prefs_| to FilterOnLoad() which will hand them back to this JsonPrefStore via FinalizeFileRead, as was the case prior to https://codereview.chromium.org/324493002. As for your point in (3) though, I'm not convinced that PrefHashStore has to be involved in migration. I think HashStoreContents could be used for everything since we want to move the MACs as-is (no knowledge of how to store/check them is required). PrefHashStore would still be used by the PrefHashFilter for MAC computing/storing/checking logic, but I see it becoming an implementation detail of PrefHashFilter (and this is what I meant by "it can probably go away altogether"; not that we would just inline all its logic into PrefHashFilter). There would be no local_state backed PrefHashStore, the only local_state backed object would be the PrefServiceHashStoreContents handed to TrackedPreferencesMigrator. Similarly the TrackedPreferencesMigrator would build DictionaryHashStoreContents directly from the prefs dictionaries it receives (no need to inject the dictionaries into pre-existing objects). The PrefHashStore owned by the PrefHashFilter would build a DictionaryHashStoreContents from the raw DictionaryValue for every PrefHashStoreTransaction. Lastly, my first comment was describing my complete vision, it wasn't trying to imply that none of the benefits it provides were present in the current proposal (apologies if it felt that way); some things are indeed already covered by this proposal as you mentioned above.
Responded to some of your comments. I plan to start splitting this into smaller CLs now. https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/324493002/diff/40001/base/prefs/json_pref_sto... base/prefs/json_pref_store.cc:345: &JsonPrefStore::FinalizeFileRead, this, initialization_successful)); On 2014/06/10 16:54:33, gab wrote: > On 2014/06/10 14:25:36, erikwright wrote: > > On 2014/06/06 21:54:58, gab wrote: > > > This callback was using ref-counting simply to avoid changing the old > > behaviour > > > where the reader (FileThreadDeserializer) would own a ref to this > > JsonPrefStore > > > all the way until the end of OnFileRead() -- which would guarantee that > > > observers are always notified of a read (success or fail) after calling > > > ReadPrefsAsync(). > > > > > > We (me and Bernhard) thought this was a side-effect of the current > > > implementation and ideally wanted to eventually remove that (i.e., have the > > read > > > task be weakly bound to this JsonPrefStore, allowing the store to go away if > > it > > > needs to before the read comes back), but kept it as-is for now for the sake > > of > > > not introducing more changes into the big change that was > > > TrackedPreferencesMigrator et al. > > > > > > By making FilterOnLoad take a raw pointer to this store's prefs rather than > > own > > > them until it's ready to give them back to us you are making the above > > callback > > > HAVE to use ref-counting or the following could happen: > > > > > > 1) JsonPrefStore 1 hands a raw pointer to its prefs post-read. > > > 2) JsonPrefStore 1 goes away. > > > 2) JsonPrefStore 2 hands a raw pointer to its prefs post-read; which > triggers > > > migration, which will use the invalid pointer to 1's prefs. > > > > > > > This can be mitigated in other ways. For example, you can have the > > post_filter_on_load_callback own the DictionaryValue (or share with this > store, > > via refcounting, an object that owns the Dictionary). So I don't think that > > passing the raw pointer to the dictionary really compromises the other choices > > you could make. Really, if there is an asynchronous process ongoing, something > > is going to be kept alive. It might be a DictionaryValue, it might be a > > JsonPrefStore, or it might be something in between. The only reason to really > > care what it is is if there are other side-effects to the destructor of the > > JsonPrefStore. And if there are, you can find lots of ways to ensure those > > side-effects have the semantics you expect. > > > > > At the very least a comment should be added to this effect above. > > > > I don't mind adding a comment - would you mind supplying your preferred > wording? > > > > > Overall however I think this is the wrong approach. This strengthens my > belief > > > in the other approach I'd suggested earlier: > > > > > > Let the TrackedPreferencesMigrator be aware of where to find the MACs in the > > > pref dictionaries (or even better: wrap the dictionary in a > HashStoreContents > > > that already knows how to do this to avoid duplicating this logic). > > > > HashStoreContents and/or PrefHashStore are appropriate layers of abstraction > and > > can provide whatever APIs we need to move things with the semantics we want. > > There is no need to break encapsulation here. > > > > > The TrackedPreferencesMigrator also needs to be given a HashStoreContents > > > wrapping local_state. > > > > As it is in this patchset. > > > > > Then the TrackedPreferencesMigrator does ALL the migration work while owning > > the > > > prefs and hands them back to their respective stores when everything is > done. > > > > I believe that's what this patchset does. Is there a distinction you are > making > > here, other than the part about "owns"? All that is injected is the > > implementation of PrefHashStore (and HashStoreContents) which only wraps the > > storage model for the hashes. > > > > IIUC the primary difference you want to see is that the PrefStore* or > > DictionaryValue* is passed into the PrefHashStore rather than being embedded > > within it. > > > > > Also, in my proposal, PrefHashStores don't have a reference to the PrefStore > > > storing the MACs so there is no cyclic dependency like there is in the model > > > you're proposing (JsonPrefStore => PrefHashFilter => PrefHashStore -> > > > JsonPrefStore). Instead PrefHashFilters are always handed a DictionaryValue* > > in > > > operations where they're meant to do work on the prefs and that > > DictionaryValue* > > > is passed to PrefHashStore::BeginTransaction() -- most likely as a > > > DictionaryHashStoreContents. > > > > I agree that we should break the circular dependency. > > > > > PrefHashStores essentially now only own the state of whether the hash store > > was > > > initially trusted (this state would be initialized in FilterOnLoad) -- in > fact > > > PrefHashStore can probably go away altogether and we can let that boolean > > state > > > live in PrefHashFilter. > > > > PrefHashStore implements: > > > > (1) the process for calculating a MAC from a value and storing it in a > > Dictionary. > > (2) the process of retrieving a MAC from a dictionary, comparing it to a > value, > > and returning a result. > > (3) the protocol for maintaing the super MAC and determining whether newly > > protected values should be trusted. > > > > Since (3) is intimately tied to the import process it is essential that it be > > implemented by a component that participates in the import process. The > > PrefHashFilter does not. > > > > I believe that the above demonstrates why it is appropriate for the > > PrefHashStore to be injected as a dependency into the migration process, even > if > > it expects to be handed a DictionaryValue to work on. > > For the comment to be added above, how about: > > // This callback must own a reference to this store since it depends on |prefs_| > being alive when it runs. > // TODO(gab): Remove this temporary requirement by handing |prefs_| to > FilterOnLoad() which will hand them back to this JsonPrefStore via > FinalizeFileRead, as was the case prior to > https://codereview.chromium.org/324493002. > > > As for your point in (3) though, I'm not convinced that PrefHashStore has to be > involved in migration. I think HashStoreContents could be used for everything > since we want to move the MACs as-is (no knowledge of how to store/check them is > required). PrefHashStore would still be used by the PrefHashFilter for MAC > computing/storing/checking logic, but I see it becoming an implementation detail > of PrefHashFilter (and this is what I meant by "it can probably go away > altogether"; not that we would just inline all its logic into PrefHashFilter). > There would be no local_state backed PrefHashStore, the only local_state backed > object would be the PrefServiceHashStoreContents handed to > TrackedPreferencesMigrator. I agree that we don't need a Local State backed PrefHashStore. > Similarly the TrackedPreferencesMigrator would build DictionaryHashStoreContents > directly from the prefs dictionaries it receives (no need to inject the > dictionaries into pre-existing objects). > The PrefHashStore owned by the PrefHashFilter would build a > DictionaryHashStoreContents from the raw DictionaryValue for every > PrefHashStoreTransaction. The thing that you're missing is that the PrefHashStore validates and calculates (or not) the super MAC for the stores it is migrating to/from. And in order to do that it needs the seed, device ID, and to know whether/not to actually do this. While the "super MAC for Protected, not for regular Preferences" part is not that big a deal, actually validating/updating the super MAC for the Protected store using the seed/device ID is not practical without being handed a non-trivial part of a PrefHashStore. In particular, you probably want something very similar to the existing "Transaction" semantics that only update the super MAC once when the import (or clean) is complete. > > > Lastly, my first comment was describing my complete vision, it wasn't trying to > imply that none of the benefits it provides were present in the current proposal > (apologies if it felt that way); some things are indeed already covered by this > proposal as you mentioned above. No hurt feelings, just wanted to make sure I had understood all of your points. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:56: initial_hashes_dictionary_trusted_); On 2014/06/06 21:54:59, gab wrote: > This histogram should also move. Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_impl.cc:173: const base::Value* hash) { On 2014/06/09 20:24:33, gab wrote: > On 2014/06/09 18:34:50, erikwright wrote: > > On 2014/06/06 21:54:59, gab wrote: > > > I think |hash| should be passed as a std::string. The fact that it's stored > as > > a > > > Value* is not relevant to this API. Passing it by pointer could perhaps save > > us > > > a few string copies, but in this case it doesn't even help with that either > > > since you have to DeepCopy() the Value* below anyways. > > > > > > So I say pass it by std::string and use SetString below (this class is well > > > aware that hashes are stored as strings anyways). > > > > That requires separate pairs of methods for atomic and split prefs. The > benefit > > of using a Value is that it works for either a dictionary or a string. > > > > I could also introduce a purpose-built type (PreferenceMac, say) that would > hold > > either a string or a dictionary. > > > > Returning anything other than a Value requires me to copy everything that > comes > > out. Probably not a performance issue, but it will be extra code. > > > > Let me know what you think, given that. > > Duh, of course, passing it via a Value object is fine to me then. > > Perhaps renaming the parameter and method from "hash" to something more generic > like "hash_content" would help here; otherwise a dictionary is not really a hash > per se. > > How about: > > ImportHashContent(const std::string& path, const base::Value* hash_content); I thought about that, but it's consistent with StoreSplitHash. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_hash_store_transaction.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_hash_store_transaction.h:57: virtual bool StampSuperMac() = 0; On 2014/06/06 21:54:59, gab wrote: > Add those methods at the bottom of this file (keeping CheckValue/StoreHash and > CheckSplitValue/StoreSplitHash together makes more sense than adding this in > between). > > And add comments for them. Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:59: FOR_EACH_OBSERVER(Observer, observers_, OnPrefValueChanged(key)); On 2014/06/06 21:54:59, gab wrote: > Call ReportValueChanged() here and below instead of making the call explicitly > (and thus expanding the macro in multiple places)? Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:75: return true; On 2014/06/06 21:54:59, gab wrote: > Just like JsonPrefStore::GetMutableValue(), I think > return dictionary_->Get(key, result); > works just fine here. > > (and I think it also works fine for GetValue()). Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:304: } On 2014/06/06 21:54:59, gab wrote: > In my proposed model this logic can remain as it used to be. Hmm. I don't see how. Previously we wrote out the values without MACs and then (in a second step) wrote the MACs to local state (in memory). Now we need to augment the dictionary with MACs before writing it out. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/pref_store_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/pref_store_hash_store_contents.h:16: class PrefStoreHashStoreContents : public HashStoreContents { On 2014/06/06 21:54:59, gab wrote: > To avoid a lot of new code (trying to keep things small), as part of my proposed > model, I suggest we re-use the same logic for both PrefServiceHashStoreContents > and DictionaryHashStoreContents; the only difference being how the hash store > dictionary is obtained. I think the other methods like "Initialized", "Reset", where the super MAC is stored, also all differ due to either tracking a single hash store or multiple hash stores in the underlying data store. > So we could have a HashStoreContents with most of the logic which takes in a > delegate or has a single virtual method to be able to get the actual > DictionaryValue from either a PrefService or a DictionaryValue. > > FWICT, the only other differences are that (1) the hash_store_id is empty (we > can explicitly pass in an empty string) and (2) we ideally want to name the pref > "super_mac" rather than "hash_of_hashes", we can either make that a parameter to > be removed later or just not care... In the production code it may be sufficient just to have a class that is only aware of how to read (export) MACs from local state. You will probably still need something able to create a legacy store (for testing purposes) but that could be something test-specific. At which point, yes, there would be only one HashStoreContents implementation. But I think that the thing you end up with will look like this (new) class so I would go ahead and add the new class and plan to remove/pare down the old one. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:47: void CleanupPrefStoreAndMaybeLocalState( On 2014/06/06 21:54:59, gab wrote: > s/LocalState/LegacyHashStore ? > > here and below (since the API never really mentions that the legacy hash store > has to be Local State). Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:80: // Invokes |store_cleaner| for every |keys_to_clean|. On 2014/06/06 21:54:59, gab wrote: > Expand method comment. Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:92: if (!--pending_commits->data) On 2014/06/06 21:54:59, gab wrote: > s/!--pending_commits->data/--pending_commits->data == 0 > > is less obscure IMO Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:93: this->legacy_pref_hash_store_->Reset(); On 2014/06/06 21:54:59, gab wrote: > Instead of using the ref-counted |pending_commits| logic, I would prefer to put > legacy_hash_store_transaction->ClearHash(*it); > in the loop above and simply have logic in > PrefHashStoreTransactionImpl::ClearHash() to trigger a Reset() when the last > hash is cleared. Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:111: this, On 2014/06/06 21:54:59, gab wrote: > One of the reason this was previously bound to a free function was so that the > reference to the JsonPrefStores held by the > finalize_(un)protected_filter_on_load_ callbacks would be released post > migration. > > Now the TrackedPreferencesMigrator and its members will live until the cleanup > tasks are complete which is wrong (I realize this wasn't obvious, apologies for > the lack of comments on this). > > The model is that the TrackedPreferencesMigrator holds on to the prefs of one > store while the other store is being read; once both stores are read, it > migrates and then immediately frees everything (scheduling optimistic tasks to > maybe clean up later, through weak pointers, if possible). > > Either way, I think this is another argument for not introducing the delegate > interface in this CL. Smaller changes FTW. The delegate impl is bound to the pref stores by weak pointers, so it will not keep them alive. I added comments and some missing checks to make this clear and enforce it. The only other state that is referenced (and potentially held alive) is the PrefHashStore backed by Local State. I don't believe there is any possibility of this code executing after Local State is gone, so I don't believe we are introducing any new references. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:153: if (!new_store->Get(pref_name, &value_in_new_store)) { On 2014/06/06 21:55:00, gab wrote: > Remove the |value_in_new_store| variable, you can pass NULL into this Get() > call. Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:255: if (!pending_commits->data) { On 2014/06/06 21:54:59, gab wrote: > Above I suggested getting rid of |pending_commits|. > > Here this would simply turn into: > > if (!unprotected_prefs_altered && !protected_prefs_altered) { > // If neither store was altered in this run, it is safe to immediately reset > the legacy hash store. > legacy_pref_hash_store_->Reset(); > } Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:257: } else { On 2014/06/06 21:54:59, gab wrote: > Remove this else conditional, the code below needs to run unconditionally (i.e., > it's possible that neither store was altered yet they both need cleanup -- since > cleanup may have failed in the previous run). Done. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.h:20: class TrackedPreferencesMigrationDelegate { On 2014/06/06 21:55:00, gab wrote: > As mentioned before, I agree that this delegate may be a nice addition to reduce > the number of parameters to SetupTrackedPreferencesMigration. > > But since it only has one caller, I don't think it's a necessary cleanup as part > of this CL. I found it preferable to adding an additional two parameters to the public method (and many of the private methods).
Response to your comments and latest patch set below; I'll be waiting for your split up CLs now. Please upload them as soon as you can (it's okay to add tests later). Thanks! Gab https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/pro... chrome/browser/prefs/profile_pref_store_manager.cc:304: } On 2014/06/10 20:27:47, erikwright wrote: > On 2014/06/06 21:54:59, gab wrote: > > In my proposed model this logic can remain as it used to be. > > Hmm. I don't see how. Previously we wrote out the values without MACs and then > (in a second step) wrote the MACs to local state (in memory). Now we need to > augment the dictionary with MACs before writing it out. This new code is required because of the circular dependency. In my proposal, where FilterOnLoad temporarily owns the DictionaryValue* and is in charge of building a DictionaryHashStoreContents to initiate a PrefHashStoreTransaction from: all of the subtleties happens in FilterOnLoad, the initialization code here doesn't need to be aware of any of that. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/pref_store_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/pref_store_hash_store_contents.h:16: class PrefStoreHashStoreContents : public HashStoreContents { On 2014/06/10 20:27:48, erikwright wrote: > On 2014/06/06 21:54:59, gab wrote: > > To avoid a lot of new code (trying to keep things small), as part of my > proposed > > model, I suggest we re-use the same logic for both > PrefServiceHashStoreContents > > and DictionaryHashStoreContents; the only difference being how the hash store > > dictionary is obtained. > > I think the other methods like "Initialized", "Reset", where the super MAC is > stored, also all differ due to either tracking a single hash store or multiple > hash stores in the underlying data store. > > > So we could have a HashStoreContents with most of the logic which takes in a > > delegate or has a single virtual method to be able to get the actual > > DictionaryValue from either a PrefService or a DictionaryValue. > > > > FWICT, the only other differences are that (1) the hash_store_id is empty (we > > can explicitly pass in an empty string) and (2) we ideally want to name the > pref > > "super_mac" rather than "hash_of_hashes", we can either make that a parameter > to > > be removed later or just not care... > > In the production code it may be sufficient just to have a class that is only > aware of how to read (export) MACs from local state. > > You will probably still need something able to create a legacy store (for > testing purposes) but that could be something test-specific. At which point, > yes, there would be only one HashStoreContents implementation. But I think that > the thing you end up with will look like this (new) class so I would go ahead > and add the new class and plan to remove/pare down the old one. Okay, keeping them as two separate classes with slight code duplication SGTM given the former will go away. Please add a comment on PrefServiceHashStoreContents stating that it is deprecated and a TODO to remove it in M30+ once this migration step becomes irrelevant. https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:111: this, On 2014/06/10 20:27:48, erikwright wrote: > On 2014/06/06 21:54:59, gab wrote: > > One of the reason this was previously bound to a free function was so that the > > reference to the JsonPrefStores held by the > > finalize_(un)protected_filter_on_load_ callbacks would be released post > > migration. > > > > Now the TrackedPreferencesMigrator and its members will live until the cleanup > > tasks are complete which is wrong (I realize this wasn't obvious, apologies > for > > the lack of comments on this). > > > > The model is that the TrackedPreferencesMigrator holds on to the prefs of one > > store while the other store is being read; once both stores are read, it > > migrates and then immediately frees everything (scheduling optimistic tasks to > > maybe clean up later, through weak pointers, if possible). > > > > Either way, I think this is another argument for not introducing the delegate > > interface in this CL. Smaller changes FTW. > > The delegate impl is bound to the pref stores by weak pointers, so it will not > keep them alive. I added comments and some missing checks to make this clear and > enforce it. > > The only other state that is referenced (and potentially held alive) is the > PrefHashStore backed by Local State. I don't believe there is any possibility of > this code executing after Local State is gone, so I don't believe we are > introducing any new references. This is not what I meant: By binding the above callback to "this" you keep the TrackedPreferencesMigrator alive. The TrackedPreferencesMigrator itself owns the (un)protected_intercept_completion_callback_ which hold references to their specific JsonPrefStores. Before, the above callback was bound to a free function; allowing TrackedPreferencesMigrator to go away; releasing its references on the JsonPrefStore held via the callbacks it owns. Once again, I'm not saying we can't make this work, but please move the addition of the TrackedPreferencesMigrationDelegate to a follow-up CL; it is non-trivial and hinders our ability to commit the core logic ASAP. (I realized after writing this comment that you kind of need the delegate in your model since otherwise you also need PrefHashStore to support weak pointers... :( -- one way to make the above work with delegates would be to keep using a free function and pass the delegates into the cleanup callbacks rather than keeping the TrackedPreferencesMigrator around for them to be available) https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right): https://codereview.chromium.org/324493002/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/tracked_preferences_migration.h:20: class TrackedPreferencesMigrationDelegate { On 2014/06/10 20:27:48, erikwright wrote: > On 2014/06/06 21:55:00, gab wrote: > > As mentioned before, I agree that this delegate may be a nice addition to > reduce > > the number of parameters to SetupTrackedPreferencesMigration. > > > > But since it only has one caller, I don't think it's a necessary cleanup as > part > > of this CL. > > I found it preferable to adding an additional two parameters to the public > method (and many of the private methods). Agreed, except that for the sake of simplicity on the core change, I think this should be done in a follow-up CL. https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:103: TrackedPreferencesMigrator::ScheduleSourcePrefStoreCleanup( This fits on the previous line I think. https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:126: // to true if any value was copied to |new_store|. Update this comment. https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:151: !destination_transaction->GetHash(pref_name); Inline this in conditional on line 172; no need to save this boolean up here early. https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:176: if (old_hash || !destination_hash_missing) { I don't think this conditional is necessary. You are essentially only skipping the case where |old_hash| is NULL and there is no destination hash which means |migrated_value| is true and there is no benefit to not running the code below -- since ImportHash will no-op and |new_store_altered| is already set to true (sure it's useless to run this code, but it's also harmless; this conditional makes things look more complex for no added benefit IMO). https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:231: if (!protected_delegate_->IsValid() || !unprotected_delegate_->IsValid()) { This is not possible since the read tasks [1] and the (un)protected_intercept_completion_callback_'s own the JsonPrefStores [2] so they (and migration) must complete before these stores can be released after a ReadPrefs(Async) call. As mentioned in my first comment on this CL, we would like to eventually make it such that read tasks are weakly bound, but this is not yet the case (and this CL makes this harder by passing a raw pointer to the internal |prefs_| object of the JsonPrefStore rather than having a clear ownership passing model like it does now). [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr... [2] https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr... Remove this code. https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc (left): https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:318: EXPECT_FALSE(HasPrefs(MOCK_UNPROTECTED_PREF_STORE)); These HasPrefs() tests were important to confirm the appropriate flow of calls to TrackedPreferencesMigrator::MigrateIfReady(). In your model you'll have to find another way to confirm that the migrator received the data it needs for either store (even if it doesn't receive ownership of that data). https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc (right): https://codereview.chromium.org/324493002/diff/100001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:541: } Please also add tests for MAC migration from Local State.
Extracted a lot of the setup for this CL into separate CLs. https://codereview.chromium.org/332473002/ https://codereview.chromium.org/329093003/ https://codereview.chromium.org/334433002/ https://codereview.chromium.org/331433002/ https://codereview.chromium.org/329173002/ https://codereview.chromium.org/326403003/
Also, in this CL you should comment out the pref_hash_store_->CommitPendingWrite() call @ https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... with a TODO to remove it and the CommitPendingWrite() hooks added for the purpose only in a follow-up CL.
I believe this CL addresses all of the structural issues previously raised by Gab. I realize I have not necessarily addressed the various nits. In addition to some manual testing I need to revisit the existing unit tests and add additional test coverage. But I would appreciate an affirmative nod from Gab's direction. https://codereview.chromium.org/324493002/diff/150001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (left): https://codereview.chromium.org/324493002/diff/150001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:74: UMA_HISTOGRAM_BOOLEAN("Settings.HashesDictionaryTrusted", We need to rethink where this goes. May need an explicit check in FilterOnLoad or something.
WooooooooooooooooooOOOOooooooooooooooooooooohoooooooooooooooo :-)!!! Me right now: http://www.infinitelooper.com/?v=SkNRC0Wj3eM&p=n I'm so very much happier with this model! Thanks a lot for switching gears. Two top-level comments: 1) You were right that we have to keep PrefHashStore around as it still holds the configuration used to store hashes (I originally thought it would become a logic only object which could have been replaced by a bunch of free functions, but I'd forgotten about the configuration (seed, device_id, etc. it holds)) -- maybe we can do better long term (I find we have a few too many layers of abstractions to express this model). 2) I think we should rename PrefHashStore since it's no longer really a store, it's nothing more than a configurable object that hands out PrefHashStoreTransactions... So perhaps PrefHashTransactionManager? (to be done as a follow-up as it only affects readability long term, not code correctness short term; I'm happy to do it as I plan to clean up this code when we're done) Comments below as I did a top-level pass over the design and a detailed pass over some sections of the migration algorithm; previous comments still apply in some cases so have a pass over those as well (as you said you would I know, but just reiterating!). Ping me when you have a cleaned up patch set of this design; can't wait to send this to trunk! Cheers and thanks! Gab https://codereview.chromium.org/324493002/diff/190001/base/prefs/pref_filter.h File base/prefs/pref_filter.h (right): https://codereview.chromium.org/324493002/diff/190001/base/prefs/pref_filter.... base/prefs/pref_filter.h:48: // contained in |pref_store_contents| to a string. Add an explicit comment that this call is allowed to modify |pref_store_contents| before it is serialized. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.h:84: // |pref_store|. Update this comment. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:17: // PrefHashStoreTransactions. Update this comment to state that this PrefHashStore may also hold state between transactions (otherwise its essentially just a logic object with from the description). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:23: // of checks/transformations on the hash store. Modify this comment to state that all transformations performed on the PrefHashStoreTransaction returned will be based on |storage|. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:25: base::DictionaryValue* storage) = 0; I think it would make sense for this to take a HashStoreContents* instead of directly taking a base::DictionaryValue* (it internally immediately stores this pointer in a DictionaryHashStoreContents anyways and it would make it such that only PrefHashFilter and TrackedPreferencesMigrator have to be intimately aware that the hashes are stored in the prefs dictionary). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (left): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:73: UMA_HISTOGRAM_BOOLEAN("Settings.HashesDictionaryTrusted", Regarding what to do with this histogram: I think we should expose an IsSuperMacTrusted() method on the PrefHashStoreTransaction and let FilterOnLoad() report it. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:80: super_mac_valid_ = So if a previous transaction stamped a super MAC, all follow-up transactions will trust the store, this *may* be okay since this state only matters in the initial FilterOnLoad(), but this is not the way this used to work (where the initial trust was the only thing that mattered throughout the lifetime of the PrefHashStore). Something to keep in mind as we refine this CL. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:91: // modified in this transaction. Remove the comment above, the new variable name makes this explicit :-)! https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:152: if (!hash) Start if/else block with the positive condition when possible (so I was told to do by my C++ readability reviewer). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:156: super_mac_dirty_ = super_mac_dirty_ || super_mac_valid_; I find: if (super_mac_valid_) super_mac_dirty_ = true; to be more readable here (and I suspect it may even generate slightly more efficient assembly code since it doesn't always force a write). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:274: reporting_ids_count_).Initialize(copy.get()); s/copy.get()/to_serialize https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:293: scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl( Rename this method to GetPrefHashStore() and return a scoped_ptr<PrefHashStore>, no needs the Impl and it will avoid having to PassAs<PrefHashStore>() everywhere above. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:29: SimpleMutableDictionary::SimpleMutableDictionary(const std::string& key, This is only used for the kProfilePreferenceHashes pref. I think it's overkill to make this generic here. I suggest removing the |key_| member and hardcoding kProfilePreferenceHashes instead below. (and renaming this class to e.g., MutablePreferencesHashesDictionary) https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:46: const base::DictionaryValue* storage) { Since this method needs access to the only member of DictionaryHashStoreContents, how about simply making it a private method? https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:66: prefs::kProfilePreferenceHashes, In fact, how about using a new pref altogether, e.g. kPreferenceMACs? It's our one-time opportunity at renaming everything (renaming the variables and classes is easily done later, but renaming on-disk state, not so much). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:25: // |kSuperMacPref| I'd add brackets here to make this slightly more readable, how about: // A super MAC (used to validate the hash store contents when determining // whether an unknown value is to be trusted) is stored as a string under // |kSuperMacPref|. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/pref_service_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/pref_service_hash_store_contents.h:48: virtual void CommitPendingWrite(); This can be removed from here as well IMO. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:131: old_hash_store.GetContents(); Merge the above 2 statements into 1: const base::DictionaryValue* old_hash_store_contents = DictionaryHashStoreContents(old_store).GetContents(); https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:160: scoped_ptr<base::Value> value_copy(value_in_old_store->DeepCopy()); inline this below as it was before https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:193: nit: rm empty line https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:278: nit: rm 2/3 empty lines https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:279: if (!unprotected_prefs_altered && !protected_prefs_altered) { So you will always cleanup the hashes in the next run following a successful migration rather than try to do it in the same run? I agree that this may be simpler. FTR, the reason we attempt to cleanup values in the same run is that leaving old values behind is problematic in some scenarios, e.g.: - Run #1 (unprotected to protected) => values are copied (new values are saved to protected). - Run #2 (protected to unprotected) => old unprotected pref applies without migration if its still around :( and we want to minimize the number of users affected by this as much as possible (we can't make it zero because clean up is still a best effort operation that can only occur when we have confirmation of the destination's store's write (so that the number of users losing their settings is zero) -- i.e., we prefer some users getting their old settings back in failed cleanup scenarios to some users losing their settings in failed migration write scenarios). Whereas the way your MAC migration work I think this is okay. As long as the value itself was cleaned up from the old store, the MAC will be migrated back with its value, overwriting the MAC left behind, should its value's destination store be flipped back to the old store. The MACs left behind are essentially never used and it's okay to wait until the next run to clean them up since it makes code here simpler. I just wanted to make sure that this was intentional and that we're on the same page as to why it's done this way. Please add a comment here explaining that this code is intended to run a cleanup step in a run following a successful migration and why this is okay and simpler. Tests for the scenarios described above will be necessary; I believe there already exists cleanup tests for the values, you'll have to augment them to support these scenarios in a world with MACs being added to the picture. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:286: legacy_pref_hash_store_->Reset(); One impact of doing this is that we will lose our accumulated super MACs in Local State and since we will most likely first run this code in NO_ENFORCEMENT mode, the MACs will be migrated to Preferences without a super MAC. We will thus be re-seeding super MACs from scratch when we enable enforcement... This is probably okay as it would only prevent us from adding protection to existing prefs in the short term (new prefs can be protected right away as they'll be NULL when our framework first loads with them). But we should bring this up in a discussion tomorrow to make sure everyone is aware of this fact. One way to fix this could be to generate a super MAC for unprotected preferences if and only if no preferences are protected and have some sort of migration for the super MAC, but that doesn't sound trivial to implement. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:318: nit: rm empty line
I've fixed existing tests. I will now begin responding to Gab's comments (none of which seem major) before adding new test coverage.
test updates lg, waiting for more updates :-)! Thanks! Gab
PTAL. I have responded to all CR comments and also done my own self-review (including writing/updating comments, etc.). I will now rebase, launch try jobs, and proceed with expanding test coverage. https://codereview.chromium.org/324493002/diff/190001/base/prefs/pref_filter.h File base/prefs/pref_filter.h (right): https://codereview.chromium.org/324493002/diff/190001/base/prefs/pref_filter.... base/prefs/pref_filter.h:48: // contained in |pref_store_contents| to a string. On 2014/06/13 01:57:43, gab wrote: > Add an explicit comment that this call is allowed to modify > |pref_store_contents| before it is serialized. Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter.h:84: // |pref_store|. On 2014/06/13 01:57:43, gab wrote: > Update this comment. Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:17: // PrefHashStoreTransactions. On 2014/06/13 01:57:43, gab wrote: > Update this comment to state that this PrefHashStore may also hold state between > transactions (otherwise its essentially just a logic object with from the > description). Corrected it to refer specifically to this being a configuration/implementation object. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:23: // of checks/transformations on the hash store. On 2014/06/13 01:57:43, gab wrote: > Modify this comment to state that all transformations performed on the > PrefHashStoreTransaction returned will be based on |storage|. In tests we may ignore |storage| in order to reproduce a legacy hash store in Local State. So I updated the comment but explicitly allowed for alternatives. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:25: base::DictionaryValue* storage) = 0; On 2014/06/13 01:57:43, gab wrote: > I think it would make sense for this to take a HashStoreContents* instead of > directly taking a base::DictionaryValue* (it internally immediately stores this > pointer in a DictionaryHashStoreContents anyways and it would make it such that > only PrefHashFilter and TrackedPreferencesMigrator have to be intimately aware > that the hashes are stored in the prefs dictionary). Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:80: super_mac_valid_ = On 2014/06/13 01:57:43, gab wrote: > So if a previous transaction stamped a super MAC, all follow-up transactions > will trust the store, this *may* be okay since this state only matters in the > initial FilterOnLoad(), but this is not the way this used to work (where the > initial trust was the only thing that mattered throughout the lifetime of the > PrefHashStore). > > Something to keep in mind as we refine this CL. Yeah, there is kind of limited choice. I don't think it's appropriate to leave state in the PrefHashStore instance since it is no longer bound to a single underlying storage (even if, in principle, it will only be used with one storage). I had considered introducing PrefHashStore::BeginFiltering which would essentially be BeginTransaction but with the side-effect of updating the super MAC. But I don't think it's that useful unless it provides the _only_ way to perform the operations associated with filtering (Check(Split)Value/Store(Split)Hash). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:91: // modified in this transaction. On 2014/06/13 01:57:43, gab wrote: > Remove the comment above, the new variable name makes this explicit :-)! Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:152: if (!hash) On 2014/06/13 01:57:43, gab wrote: > Start if/else block with the positive condition when possible (so I was told to > do by my C++ readability reviewer). Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:156: super_mac_dirty_ = super_mac_dirty_ || super_mac_valid_; On 2014/06/13 01:57:43, gab wrote: > I find: > > if (super_mac_valid_) > super_mac_dirty_ = true; > > to be more readable here (and I suspect it may even generate slightly more > efficient assembly code since it doesn't always force a write). Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:274: reporting_ids_count_).Initialize(copy.get()); On 2014/06/13 01:57:43, gab wrote: > s/copy.get()/to_serialize Can't. |copy| is non-const, whereas |to_serialize| is const (for compatibility with |master_prefs|). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.cc:293: scoped_ptr<PrefHashStoreImpl> ProfilePrefStoreManager::GetPrefHashStoreImpl( On 2014/06/13 01:57:43, gab wrote: > Rename this method to GetPrefHashStore() and return a scoped_ptr<PrefHashStore>, > no needs the Impl and it will avoid having to PassAs<PrefHashStore>() everywhere > above. Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:29: SimpleMutableDictionary::SimpleMutableDictionary(const std::string& key, On 2014/06/13 01:57:43, gab wrote: > This is only used for the kProfilePreferenceHashes pref. I think it's overkill > to make this generic here. > > I suggest removing the |key_| member and hardcoding kProfilePreferenceHashes > instead below. > > (and renaming this class to e.g., MutablePreferencesHashesDictionary) The flexibility was a remnant from a previous incarnation. Thanks. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:46: const base::DictionaryValue* storage) { On 2014/06/13 01:57:43, gab wrote: > Since this method needs access to the only member of > DictionaryHashStoreContents, how about simply making it a private method? Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:66: prefs::kProfilePreferenceHashes, On 2014/06/13 01:57:43, gab wrote: > In fact, how about using a new pref altogether, e.g. kPreferenceMACs? > > It's our one-time opportunity at renaming everything (renaming the variables and > classes is easily done later, but renaming on-disk state, not so much). Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:25: // |kSuperMacPref| On 2014/06/13 01:57:43, gab wrote: > I'd add brackets here to make this slightly more readable, how about: > > // A super MAC (used to validate the hash store contents when determining > // whether an unknown value is to be trusted) is stored as a string under > // |kSuperMacPref|. Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/pref_service_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/pref_service_hash_store_contents.h:48: virtual void CommitPendingWrite(); On 2014/06/13 01:57:43, gab wrote: > This can be removed from here as well IMO. Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:131: old_hash_store.GetContents(); On 2014/06/13 01:57:44, gab wrote: > Merge the above 2 statements into 1: > > const base::DictionaryValue* old_hash_store_contents = > DictionaryHashStoreContents(old_store).GetContents(); Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:160: scoped_ptr<base::Value> value_copy(value_in_old_store->DeepCopy()); On 2014/06/13 01:57:44, gab wrote: > inline this below as it was before Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:193: On 2014/06/13 01:57:44, gab wrote: > nit: rm empty line Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:278: On 2014/06/13 01:57:44, gab wrote: > nit: rm 2/3 empty lines Done. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:279: if (!unprotected_prefs_altered && !protected_prefs_altered) { On 2014/06/13 01:57:44, gab wrote: > So you will always cleanup the hashes in the next run following a successful > migration rather than try to do it in the same run? > > I agree that this may be simpler. > > FTR, the reason we attempt to cleanup values in the same run is that leaving old > values behind is problematic in some scenarios, e.g.: > - Run #1 (unprotected to protected) => values are copied (new values are saved > to protected). > - Run #2 (protected to unprotected) => old unprotected pref applies without > migration if its still around :( > and we want to minimize the number of users affected by this as much as possible > (we can't make it zero because clean up is still a best effort operation that > can only occur when we have confirmation of the destination's store's write (so > that the number of users losing their settings is zero) -- i.e., we prefer some > users getting their old settings back in failed cleanup scenarios to some users > losing their settings in failed migration write scenarios). > > Whereas the way your MAC migration work I think this is okay. As long as the > value itself was cleaned up from the old store, the MAC will be migrated back > with its value, overwriting the MAC left behind, should its value's destination > store be flipped back to the old store. > The MACs left behind are essentially never used and it's okay to wait until the > next run to clean them up since it makes code here simpler. > > I just wanted to make sure that this was intentional and that we're on the same > page as to why it's done this way. > > Please add a comment here explaining that this code is intended to run a cleanup > step in a run following a successful migration and why this is okay and simpler. > > Tests for the scenarios described above will be necessary; I believe there > already exists cleanup tests for the values, you'll have to augment them to > support these scenarios in a world with MACs being added to the picture. Yes it was intentional, and your understanding of why it's safe and how it works is the same as mine. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:286: legacy_pref_hash_store_->Reset(); On 2014/06/13 01:57:44, gab wrote: > One impact of doing this is that we will lose our accumulated super MACs in > Local State and since we will most likely first run this code in NO_ENFORCEMENT > mode, the MACs will be migrated to Preferences without a super MAC. We will thus > be re-seeding super MACs from scratch when we enable enforcement... I believe that one of the side effects of this CL is that we now write a super MAC even if the Protected store is empty. That may be a bad idea, as that super MAC could be used at any time in the future to blindly TrustedInitialize all values. An attacker could store it before protection is on or (if they are able, even briefly, to trick Finch), generate it in the future. > This is probably okay as it would only prevent us from adding protection to > existing prefs in the short term (new prefs can be protected right away as > they'll be NULL when our framework first loads with them). But we should bring > this up in a discussion tomorrow to make sure everyone is aware of this fact. Did we already discuss this? I don't think so; if not, we definitely should. > One way to fix this could be to generate a super MAC for unprotected preferences > if and only if no preferences are protected and have some sort of migration for > the super MAC, but that doesn't sound trivial to implement. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:318: On 2014/06/13 01:57:44, gab wrote: > nit: rm empty line Done.
lvvvvvvvvvvg! This CL is awesome now. I love the structure of our framework which allows me to be very confident in this CL's correctness, good work (and thanks for bearing with me!)! Did a full pass and only got some nits and suggestions below (apart from smiles and goose-bumps from the awesomeness of this framework -- yea I'm weird, sexy code does that to me..!). Will wait to see tests covering the new code (I doubt anything bad will pop up in tests and I could even be convinced to leave new tests for a follow-up CL if it looks like they'll take a while to write). Cheers! Gab https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:80: super_mac_valid_ = On 2014/06/16 20:51:26, erikwright wrote: > On 2014/06/13 01:57:43, gab wrote: > > So if a previous transaction stamped a super MAC, all follow-up transactions > > will trust the store, this *may* be okay since this state only matters in the > > initial FilterOnLoad(), but this is not the way this used to work (where the > > initial trust was the only thing that mattered throughout the lifetime of the > > PrefHashStore). > > > > Something to keep in mind as we refine this CL. > > Yeah, there is kind of limited choice. I don't think it's appropriate to leave > state in the PrefHashStore instance since it is no longer bound to a single > underlying storage (even if, in principle, it will only be used with one > storage). > > I had considered introducing PrefHashStore::BeginFiltering which would > essentially be BeginTransaction but with the side-effect of updating the super > MAC. But I don't think it's that useful unless it provides the _only_ way to > perform the operations associated with filtering > (Check(Split)Value/Store(Split)Hash). How about adding a PrefHashFilter::filter_on_load_performed_ member to PrefHashFilter and DCHECK'ing that FilterOnLoad() only happens once? This depends on that fact (and much of the framework kind of assumes this as well). https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:286: legacy_pref_hash_store_->Reset(); On 2014/06/16 20:51:27, erikwright wrote: > On 2014/06/13 01:57:44, gab wrote: > > One impact of doing this is that we will lose our accumulated super MACs in > > Local State and since we will most likely first run this code in > NO_ENFORCEMENT > > mode, the MACs will be migrated to Preferences without a super MAC. We will > thus > > be re-seeding super MACs from scratch when we enable enforcement... > > I believe that one of the side effects of this CL is that we now write a super > MAC even if the Protected store is empty. That may be a bad idea, as that super > MAC could be used at any time in the future to blindly TrustedInitialize all > values. An attacker could store it before protection is on or (if they are able, > even briefly, to trick Finch), generate it in the future. This doesn't matter, an uninitialized store is also untrusted (even if the super MAC is valid); we should probably also not trust an initialized store with no MACs in it; WDYT? > > This is probably okay as it would only prevent us from adding protection to > > existing prefs in the short term (new prefs can be protected right away as > > they'll be NULL when our framework first loads with them). But we should bring > > this up in a discussion tomorrow to make sure everyone is aware of this fact. > > Did we already discuss this? I don't think so; if not, we definitely should. Yep, please ping us tomorrow for a chat. > > > One way to fix this could be to generate a super MAC for unprotected > preferences > > if and only if no preferences are protected and have some sort of migration > for > > the super MAC, but that doesn't sound trivial to implement. > https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:15: class PrefHashStore { Add a TODO as such: // TODO(gab): Rename this class to PrefHashTransactionManager as it is no longer even used as a store. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:102: PrefHashCalculator::VALID; I find this indent weird, I'd prefer wrapping "super_mac) ==" down so that the RHS of the check is not alone on this line indented the same as the params above (which makes it feel like it's a param at first sight). https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:281: const base::DictionaryValue* hashed_prefs = contents()->GetContents(); s/hashed_prefs/hashes_dict/ for consistency with other methods in this file. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl_unittest.cc:74: transaction.reset(); Remove the last 3 lines as well (the reason to reset early was test the commit, now that it was removed, it's sufficient to let the scope take care of it). Same in similar removals below. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.h:103: // (and, by extension, accept newly protected preferences as s/accept newly protected preferences/accept non-null newly protected preferences/ https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:16: const char kPreferenceMACs[] = "macs"; Put both "macs" and "super_mac" under the same JSON node, i.e.: "protection.macs" "protection.super_mac" or "mac_store_contents.macs" "mac_store_contents.super_mac" https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:18: // static Remove "// static" here and above. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:31: DISALLOW_COPY_AND_ASSIGN(MutablePreferenceMacDictionary); I think the Chromium style is to have an empty line before DISALLOW_COPY_AND_ASSIGN, but I won't fight if you feel otherwise ;-) https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:26: // Constructs a DictionaryHashStoreContents that reads and writes to s/reads and writes to/reads from and writes to/ https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:28: explicit DictionaryHashStoreContents(base::DictionaryValue* storage); The fact that |storage| must outlive this DictionaryHashStoreContents seems implicit to me, but just making sure you feel the same way and are making a conscious decision not to mention this above. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/pref_service_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/pref_service_hash_store_contents.h:20: class PrefServiceHashStoreContents : public HashStoreContents { Add a comment that this is a deprecated class; kept around to perform migration, with a TODO to delete it in, e.g, M30. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:11: #include "base/memory/scoped_ptr.h" Remove this include which is now in the header. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:128: HashStoreContents* legacy_hash_store) { Personal preference: put this param between |new_hash_store| and |old_store_needs_cleanup|. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:171: legacy_hash_store_contents->Get(pref_name, &old_hash); I would like to log an UMA stat if we migrate any MACs from Local State in this method (to get a graph of the migration panning out). i.e. set a bool here if |old_hash != NULL| and report a "Settings.MACsMigratedFromLegacyStore" with histogram type "enum=BooleanMigrated" at the end of this method. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:172: if (old_hash || !destination_hash_missing) { I think this would be less subtle (and more readable) as: if (old_hash) { new_hash_store_transaction->ImportHash(pref_name, old_hash); *new_store_altered = true; } else if (!destination_hash_missing) { new_hash_store_transaction->ClearHash(pref_name); *new_store_altered = true; } Furthermore, this makes it such that ImportHash() can explicitly not support NULL hashes, via contract and DCHECK (simplifying that method as well). https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:180: void CleanupMigratedHashes(const std::set<std::string>& migrated_pref_names, Move this method just below ScheduleSourcePrefStoreCleanup() to keep cleanup related methods together. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.h:14: #include "chrome/browser/prefs/tracked/hash_store_contents.h" Fwd-decl instead of including these here.
FYI, I checked some boxes for a bunch of Xsan bots just in case. On Mon, Jun 16, 2014 at 10:00 PM, <gab@chromium.org> wrote: > lvvvvvvvvvvg! > > This CL is awesome now. I love the structure of our framework which allows > me to > be very confident in this CL's correctness, good work (and thanks for > bearing > with me!)! > > Did a full pass and only got some nits and suggestions below (apart from > smiles > and goose-bumps from the awesomeness of this framework -- yea I'm weird, > sexy > code does that to me..!). > > Will wait to see tests covering the new code (I doubt anything bad will > pop up > in tests and I could even be convinced to leave new tests for a follow-up > CL if > it looks like they'll take a while to write). > > Cheers! > Gab > > > > https://codereview.chromium.org/324493002/diff/190001/ > chrome/browser/prefs/pref_hash_store_impl.cc > File chrome/browser/prefs/pref_hash_store_impl.cc (right): > > https://codereview.chromium.org/324493002/diff/190001/ > chrome/browser/prefs/pref_hash_store_impl.cc#newcode80 > chrome/browser/prefs/pref_hash_store_impl.cc:80: super_mac_valid_ = > On 2014/06/16 20:51:26, erikwright wrote: > >> On 2014/06/13 01:57:43, gab wrote: >> > So if a previous transaction stamped a super MAC, all follow-up >> > transactions > >> > will trust the store, this *may* be okay since this state only >> > matters in the > >> > initial FilterOnLoad(), but this is not the way this used to work >> > (where the > >> > initial trust was the only thing that mattered throughout the >> > lifetime of the > >> > PrefHashStore). >> > >> > Something to keep in mind as we refine this CL. >> > > Yeah, there is kind of limited choice. I don't think it's appropriate >> > to leave > >> state in the PrefHashStore instance since it is no longer bound to a >> > single > >> underlying storage (even if, in principle, it will only be used with >> > one > >> storage). >> > > I had considered introducing PrefHashStore::BeginFiltering which would >> essentially be BeginTransaction but with the side-effect of updating >> > the super > >> MAC. But I don't think it's that useful unless it provides the _only_ >> > way to > >> perform the operations associated with filtering >> (Check(Split)Value/Store(Split)Hash). >> > > How about adding a PrefHashFilter::filter_on_load_performed_ member to > PrefHashFilter and DCHECK'ing that FilterOnLoad() only happens once? > > This depends on that fact (and much of the framework kind of assumes > this as well). > > > https://codereview.chromium.org/324493002/diff/190001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc > File chrome/browser/prefs/tracked/tracked_preferences_migration.cc > (right): > > https://codereview.chromium.org/324493002/diff/190001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc#newcode286 > chrome/browser/prefs/tracked/tracked_preferences_migration.cc:286: > legacy_pref_hash_store_->Reset(); > On 2014/06/16 20:51:27, erikwright wrote: > >> On 2014/06/13 01:57:44, gab wrote: >> > One impact of doing this is that we will lose our accumulated super >> > MACs in > >> > Local State and since we will most likely first run this code in >> NO_ENFORCEMENT >> > mode, the MACs will be migrated to Preferences without a super MAC. >> > We will > >> thus >> > be re-seeding super MACs from scratch when we enable enforcement... >> > > I believe that one of the side effects of this CL is that we now write >> > a super > >> MAC even if the Protected store is empty. That may be a bad idea, as >> > that super > >> MAC could be used at any time in the future to blindly >> > TrustedInitialize all > >> values. An attacker could store it before protection is on or (if they >> > are able, > >> even briefly, to trick Finch), generate it in the future. >> > > This doesn't matter, an uninitialized store is also untrusted (even if > the super MAC is valid); we should probably also not trust an > initialized store with no MACs in it; WDYT? > > > > This is probably okay as it would only prevent us from adding >> > protection to > >> > existing prefs in the short term (new prefs can be protected right >> > away as > >> > they'll be NULL when our framework first loads with them). But we >> > should bring > >> > this up in a discussion tomorrow to make sure everyone is aware of >> > this fact. > > Did we already discuss this? I don't think so; if not, we definitely >> > should. > > Yep, please ping us tomorrow for a chat. > > > > > One way to fix this could be to generate a super MAC for unprotected >> preferences >> > if and only if no preferences are protected and have some sort of >> > migration > >> for >> > the super MAC, but that doesn't sound trivial to implement. >> > > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store.h > File chrome/browser/prefs/pref_hash_store.h (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store.h#newcode15 > chrome/browser/prefs/pref_hash_store.h:15: class PrefHashStore { > Add a TODO as such: > > // TODO(gab): Rename this class to PrefHashTransactionManager as it is > no longer even used as a store. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store_impl.cc > File chrome/browser/prefs/pref_hash_store_impl.cc (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store_impl.cc#newcode102 > chrome/browser/prefs/pref_hash_store_impl.cc:102: > PrefHashCalculator::VALID; > I find this indent weird, I'd prefer wrapping "super_mac) ==" down so > that the RHS of the check is not alone on this line indented the same as > the params above (which makes it feel like it's a param at first sight). > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store_impl.cc#newcode281 > chrome/browser/prefs/pref_hash_store_impl.cc:281: const > base::DictionaryValue* hashed_prefs = contents()->GetContents(); > s/hashed_prefs/hashes_dict/ > > for consistency with other methods in this file. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store_impl_unittest.cc > File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/pref_hash_store_impl_unittest.cc#newcode74 > chrome/browser/prefs/pref_hash_store_impl_unittest.cc:74: > transaction.reset(); > Remove the last 3 lines as well (the reason to reset early was test the > commit, now that it was removed, it's sufficient to let the scope take > care of it). > > Same in similar removals below. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/profile_pref_store_manager.h > File chrome/browser/prefs/profile_pref_store_manager.h (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/profile_pref_store_manager.h#newcode103 > chrome/browser/prefs/profile_pref_store_manager.h:103: // (and, by > extension, accept newly protected preferences as > s/accept newly protected preferences/accept non-null newly protected > preferences/ > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc > File chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc > (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc#newcode16 > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:16: const > char kPreferenceMACs[] = "macs"; > Put both "macs" and "super_mac" under the same JSON node, i.e.: > > "protection.macs" > "protection.super_mac" > > or > > "mac_store_contents.macs" > "mac_store_contents.super_mac" > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc#newcode18 > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:18: // > static > Remove "// static" here and above. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc#newcode31 > chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:31: > DISALLOW_COPY_AND_ASSIGN(MutablePreferenceMacDictionary); > I think the Chromium style is to have an empty line before > DISALLOW_COPY_AND_ASSIGN, but I won't fight if you feel otherwise ;-) > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.h > File chrome/browser/prefs/tracked/dictionary_hash_store_contents.h > (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.h#newcode26 > chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:26: // > Constructs a DictionaryHashStoreContents that reads and writes to > s/reads and writes to/reads from and writes to/ > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/dictionary_hash_store_contents.h#newcode28 > chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:28: > explicit DictionaryHashStoreContents(base::DictionaryValue* storage); > The fact that |storage| must outlive this DictionaryHashStoreContents > seems implicit to me, but just making sure you feel the same way and are > making a conscious decision not to mention this above. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/pref_service_hash_store_contents.h > File chrome/browser/prefs/tracked/pref_service_hash_store_contents.h > (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/pref_service_hash_store_contents.h#newcode20 > chrome/browser/prefs/tracked/pref_service_hash_store_contents.h:20: > class PrefServiceHashStoreContents : public HashStoreContents { > Add a comment that this is a deprecated class; kept around to perform > migration, with a TODO to delete it in, e.g, M30. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc > File chrome/browser/prefs/tracked/tracked_preferences_migration.cc > (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc#newcode11 > chrome/browser/prefs/tracked/tracked_preferences_migration.cc:11: > #include "base/memory/scoped_ptr.h" > Remove this include which is now in the header. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc#newcode128 > chrome/browser/prefs/tracked/tracked_preferences_migration.cc:128: > HashStoreContents* legacy_hash_store) { > Personal preference: put this param between |new_hash_store| and > |old_store_needs_cleanup|. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc#newcode171 > chrome/browser/prefs/tracked/tracked_preferences_migration.cc:171: > legacy_hash_store_contents->Get(pref_name, &old_hash); > I would like to log an UMA stat if we migrate any MACs from Local State > in this method (to get a graph of the migration panning out). > > i.e. set a bool here if |old_hash != NULL| and report a > "Settings.MACsMigratedFromLegacyStore" with histogram type > "enum=BooleanMigrated" at the end of this method. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc#newcode172 > chrome/browser/prefs/tracked/tracked_preferences_migration.cc:172: if > (old_hash || !destination_hash_missing) { > I think this would be less subtle (and more readable) as: > > if (old_hash) { > new_hash_store_transaction->ImportHash(pref_name, old_hash); > *new_store_altered = true; > } else if (!destination_hash_missing) { > new_hash_store_transaction->ClearHash(pref_name); > *new_store_altered = true; > } > > Furthermore, this makes it such that ImportHash() can explicitly not > support NULL hashes, via contract and DCHECK (simplifying that method as > well). > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.cc#newcode180 > chrome/browser/prefs/tracked/tracked_preferences_migration.cc:180: void > CleanupMigratedHashes(const std::set<std::string>& migrated_pref_names, > Move this method just below ScheduleSourcePrefStoreCleanup() to keep > cleanup related methods together. > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.h > File chrome/browser/prefs/tracked/tracked_preferences_migration.h > (right): > > https://codereview.chromium.org/324493002/diff/250001/ > chrome/browser/prefs/tracked/tracked_preferences_migration.h#newcode14 > chrome/browser/prefs/tracked/tracked_preferences_migration.h:14: > #include "chrome/browser/prefs/tracked/hash_store_contents.h" > Fwd-decl instead of including these here. > > https://codereview.chromium.org/324493002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/324493002/diff/250001/base/prefs/pref_filter.h File base/prefs/pref_filter.h (left): https://codereview.chromium.org/324493002/diff/250001/base/prefs/pref_filter.... base/prefs/pref_filter.h:50: const base::DictionaryValue* pref_store_contents) = 0; this requires a corresponding change to InterceptingPrefFilter in json_pref_filter_unittest.cc
gab: PTAL for test coverage. I could probably add more, but this is a start. I'd like you to validate the approach. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:80: super_mac_valid_ = On 2014/06/17 02:00:05, gab wrote: > On 2014/06/16 20:51:26, erikwright wrote: > > On 2014/06/13 01:57:43, gab wrote: > > > So if a previous transaction stamped a super MAC, all follow-up transactions > > > will trust the store, this *may* be okay since this state only matters in > the > > > initial FilterOnLoad(), but this is not the way this used to work (where the > > > initial trust was the only thing that mattered throughout the lifetime of > the > > > PrefHashStore). > > > > > > Something to keep in mind as we refine this CL. > > > > Yeah, there is kind of limited choice. I don't think it's appropriate to leave > > state in the PrefHashStore instance since it is no longer bound to a single > > underlying storage (even if, in principle, it will only be used with one > > storage). > > > > I had considered introducing PrefHashStore::BeginFiltering which would > > essentially be BeginTransaction but with the side-effect of updating the super > > MAC. But I don't think it's that useful unless it provides the _only_ way to > > perform the operations associated with filtering > > (Check(Split)Value/Store(Split)Hash). > > How about adding a PrefHashFilter::filter_on_load_performed_ member to > PrefHashFilter and DCHECK'ing that FilterOnLoad() only happens once? > > This depends on that fact (and much of the framework kind of assumes this as > well). Personally, I'm not worried about a single instance of PrefHashFilter calling filter_on_load twice. The real risk is that something in the migration code ends up triggering an unintended super MAC update, given that that component has a completely distinct instance of PrefHashStore. To protect against this, I would change the object model for a PrefHashStore to guarantee that it can only be used once for load-time enforcement. For example, forcing you to give up your only reference to the object at the end of enforcement. I don't think it's critical at the moment. Probably gold-plating, especially if there are regression tests to catch it. But I definitely think the boolean "did this once" flag is at best superficial and a false sense of security. https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:286: legacy_pref_hash_store_->Reset(); On 2014/06/17 02:00:05, gab wrote: > On 2014/06/16 20:51:27, erikwright wrote: > > On 2014/06/13 01:57:44, gab wrote: > > > One impact of doing this is that we will lose our accumulated super MACs in > > > Local State and since we will most likely first run this code in > > NO_ENFORCEMENT > > > mode, the MACs will be migrated to Preferences without a super MAC. We will > > thus > > > be re-seeding super MACs from scratch when we enable enforcement... > > > > I believe that one of the side effects of this CL is that we now write a super > > MAC even if the Protected store is empty. That may be a bad idea, as that > super > > MAC could be used at any time in the future to blindly TrustedInitialize all > > values. An attacker could store it before protection is on or (if they are > able, > > even briefly, to trick Finch), generate it in the future. > > This doesn't matter, an uninitialized store is also untrusted (even if the > super MAC is valid); we should probably also not trust an initialized store with > no MACs in it; WDYT? Sure, but then probably we should not bother writing a super MAC that we wouldn't trust anyways. > > > > This is probably okay as it would only prevent us from adding protection to > > > existing prefs in the short term (new prefs can be protected right away as > > > they'll be NULL when our framework first loads with them). But we should > bring > > > this up in a discussion tomorrow to make sure everyone is aware of this > fact. > > > > Did we already discuss this? I don't think so; if not, we definitely should. > > Yep, please ping us tomorrow for a chat. > > > > > > One way to fix this could be to generate a super MAC for unprotected > > preferences > > > if and only if no preferences are protected and have some sort of migration > > for > > > the super MAC, but that doesn't sound trivial to implement. > > >
https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/190001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:286: legacy_pref_hash_store_->Reset(); On 2014/06/17 17:54:05, erikwright wrote: > On 2014/06/17 02:00:05, gab wrote: > > On 2014/06/16 20:51:27, erikwright wrote: > > > On 2014/06/13 01:57:44, gab wrote: > > > > One impact of doing this is that we will lose our accumulated super MACs > in > > > > Local State and since we will most likely first run this code in > > > NO_ENFORCEMENT > > > > mode, the MACs will be migrated to Preferences without a super MAC. We > will > > > thus > > > > be re-seeding super MACs from scratch when we enable enforcement... > > > > > > I believe that one of the side effects of this CL is that we now write a > super > > > MAC even if the Protected store is empty. That may be a bad idea, as that > > super > > > MAC could be used at any time in the future to blindly TrustedInitialize all > > > values. An attacker could store it before protection is on or (if they are > > able, > > > even briefly, to trick Finch), generate it in the future. > > > > This doesn't matter, an uninitialized store is also untrusted (even if the > > super MAC is valid); we should probably also not trust an initialized store > with > > no MACs in it; WDYT? > > Sure, but then probably we should not bother writing a super MAC that we > wouldn't trust anyways. > > > > > > > This is probably okay as it would only prevent us from adding protection > to > > > > existing prefs in the short term (new prefs can be protected right away as > > > > they'll be NULL when our framework first loads with them). But we should > > bring > > > > this up in a discussion tomorrow to make sure everyone is aware of this > > fact. > > > > > > Did we already discuss this? I don't think so; if not, we definitely should. > > > > Yep, please ping us tomorrow for a chat. > > > > > > > > > One way to fix this could be to generate a super MAC for unprotected > > > preferences > > > > if and only if no preferences are protected and have some sort of > migration > > > for > > > > the super MAC, but that doesn't sound trivial to implement. > > > > > > True, but not trusting it in these scenarios no matter what is easier IMO.
Still need to add an UMA metric. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store.h:15: class PrefHashStore { On 2014/06/17 02:00:05, gab wrote: > Add a TODO as such: > > // TODO(gab): Rename this class to PrefHashTransactionManager as it is no longer > even used as a store. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:102: PrefHashCalculator::VALID; On 2014/06/17 02:00:05, gab wrote: > I find this indent weird, I'd prefer wrapping "super_mac) ==" down so that the > RHS of the check is not alone on this line indented the same as the params above > (which makes it feel like it's a param at first sight). Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl.cc:281: const base::DictionaryValue* hashed_prefs = contents()->GetContents(); On 2014/06/17 02:00:05, gab wrote: > s/hashed_prefs/hashes_dict/ > > for consistency with other methods in this file. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl_unittest.cc:74: transaction.reset(); On 2014/06/17 02:00:05, gab wrote: > Remove the last 3 lines as well (the reason to reset early was test the commit, > now that it was removed, it's sufficient to let the scope take care of it). > > Same in similar removals below. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.h:103: // (and, by extension, accept newly protected preferences as On 2014/06/17 02:00:05, gab wrote: > s/accept newly protected preferences/accept non-null newly protected > preferences/ Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:16: const char kPreferenceMACs[] = "macs"; On 2014/06/17 02:00:06, gab wrote: > Put both "macs" and "super_mac" under the same JSON node, i.e.: > > "protection.macs" > "protection.super_mac" > > or > > "mac_store_contents.macs" > "mac_store_contents.super_mac" Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:18: // static On 2014/06/17 02:00:06, gab wrote: > Remove "// static" here and above. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.cc:31: DISALLOW_COPY_AND_ASSIGN(MutablePreferenceMacDictionary); On 2014/06/17 02:00:05, gab wrote: > I think the Chromium style is to have an empty line before > DISALLOW_COPY_AND_ASSIGN, but I won't fight if you feel otherwise ;-) Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/dictionary_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:26: // Constructs a DictionaryHashStoreContents that reads and writes to On 2014/06/17 02:00:06, gab wrote: > s/reads and writes to/reads from and writes to/ Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/dictionary_hash_store_contents.h:28: explicit DictionaryHashStoreContents(base::DictionaryValue* storage); On 2014/06/17 02:00:06, gab wrote: > The fact that |storage| must outlive this DictionaryHashStoreContents seems > implicit to me, but just making sure you feel the same way and are making a > conscious decision not to mention this above. I believe it is implicit in the world of scoped_ptr etc. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/pref_service_hash_store_contents.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/pref_service_hash_store_contents.h:20: class PrefServiceHashStoreContents : public HashStoreContents { On 2014/06/17 02:00:06, gab wrote: > Add a comment that this is a deprecated class; kept around to perform migration, > with a TODO to delete it in, e.g, M30. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:11: #include "base/memory/scoped_ptr.h" On 2014/06/17 02:00:06, gab wrote: > Remove this include which is now in the header. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:128: HashStoreContents* legacy_hash_store) { On 2014/06/17 02:00:06, gab wrote: > Personal preference: put this param between |new_hash_store| and > |old_store_needs_cleanup|. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:172: if (old_hash || !destination_hash_missing) { On 2014/06/17 02:00:06, gab wrote: > I think this would be less subtle (and more readable) as: > > if (old_hash) { > new_hash_store_transaction->ImportHash(pref_name, old_hash); > *new_store_altered = true; > } else if (!destination_hash_missing) { > new_hash_store_transaction->ClearHash(pref_name); > *new_store_altered = true; > } > > Furthermore, this makes it such that ImportHash() can explicitly not support > NULL hashes, via contract and DCHECK (simplifying that method as well). Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:180: void CleanupMigratedHashes(const std::set<std::string>& migrated_pref_names, On 2014/06/17 02:00:06, gab wrote: > Move this method just below ScheduleSourcePrefStoreCleanup() to keep cleanup > related methods together. Done. https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.h (right): https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.h:14: #include "chrome/browser/prefs/tracked/hash_store_contents.h" On 2014/06/17 02:00:06, gab wrote: > Fwd-decl instead of including these here. Done.
Added an additional regression test for the case where a newly-tracked-and-protected value is added at the same time protection is enabled.
Almost there; happy to do a review tonight (and CQ if it lg) if you have time to pump out another patch set before then. Cheers! Gab https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter_unittest.cc:325: NOTIMPLEMENTED(); IMO these NOTIMPLEMENTED()'s should be ADD_FAILURE()'s instead with a comment stating that they are not expected to be called from PrefHashFilter and that, if that ever changes, these should be augmented to test that? https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl_unittest.cc:334: } Add tests to exercise the basics of the new methods in pref_hash_store_transaction.h https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:339: // In a subsequent launch, the local state hash store would be reset. s/would/should https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:287: UMA_HISTOGRAM_BOOLEAN("Settings.MigratedHashesFromLocalState", Needs to be added to histograms.xml as well (and to grt's doc of histograms we care about). https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc (left): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:115: VerifyValuesStored(MOCK_PROTECTED_PREF_STORE, no_prefs_stored); Move those back into SetUp(), right after the Reset() call. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:66: // A test fixture designed to like this: s/designed to like this/designed to be used like this/ (I think I wrote this, sorry about that!) https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:101: migration_modified_protected_store_ = false; s/protected/unprotected https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:168: scoped_ptr<HashStoreContents>( This fits on the previous line. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:227: expected_pref_in_hash_store, static_cast<std::string*>(NULL)); Uh? Do you really need this static_cast?! https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:383: expected_protected_hashes.push_back(kProtectedPref); 5 lines above are unused; remove them. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:647: } Add a test where MACs start in |local_state_| exercising the legacy migration flow. https://codereview.chromium.org/324493002/diff/380001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/324493002/diff/380001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:323: ASSERT_TRUE(local_state_.GetUserPrefValue( Tests appear to be choking on this; as if there was an issue with the legacy_hash_store_contents logic, but I don't see anything obvious.
gab: PTAL. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter_unittest.cc:325: NOTIMPLEMENTED(); On 2014/06/17 22:08:21, gab wrote: > IMO these NOTIMPLEMENTED()'s should be ADD_FAILURE()'s instead with a comment > stating that they are not expected to be called from PrefHashFilter and that, if > that ever changes, these should be augmented to test that? Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_store_impl_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_store_impl_unittest.cc:334: } On 2014/06/17 22:08:21, gab wrote: > Add tests to exercise the basics of the new methods in > pref_hash_store_transaction.h Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:339: // In a subsequent launch, the local state hash store would be reset. On 2014/06/17 22:08:21, gab wrote: > s/would/should Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration.cc:287: UMA_HISTOGRAM_BOOLEAN("Settings.MigratedHashesFromLocalState", On 2014/06/17 22:08:22, gab wrote: > Needs to be added to histograms.xml as well (and to grt's doc of histograms we > care about). Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc (left): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:115: VerifyValuesStored(MOCK_PROTECTED_PREF_STORE, no_prefs_stored); On 2014/06/17 22:08:22, gab wrote: > Move those back into SetUp(), right after the Reset() call. They are actually misleading no-ops, because VerifyValuesStored does not do an exclusive verification, but merely an inclusive verification. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc (right): https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:66: // A test fixture designed to like this: On 2014/06/17 22:08:22, gab wrote: > s/designed to like this/designed to be used like this/ > > (I think I wrote this, sorry about that!) Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:101: migration_modified_protected_store_ = false; On 2014/06/17 22:08:22, gab wrote: > s/protected/unprotected Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:168: scoped_ptr<HashStoreContents>( On 2014/06/17 22:08:22, gab wrote: > This fits on the previous line. Depends on how you want to wrap operators. The new version seems to follow the greatest number of style guidelines. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:227: expected_pref_in_hash_store, static_cast<std::string*>(NULL)); On 2014/06/17 22:08:22, gab wrote: > Uh? Do you really need this static_cast?! Yes. There are multiple overloads taking string16, string, etc. So you need to pick one explicitly. I could use Get() but by using GetString I am also doing a type check. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:383: expected_protected_hashes.push_back(kProtectedPref); On 2014/06/17 22:08:22, gab wrote: > 5 lines above are unused; remove them. Done. https://codereview.chromium.org/324493002/diff/360001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc:647: } On 2014/06/17 22:08:22, gab wrote: > Add a test where MACs start in |local_state_| exercising the legacy migration > flow. This is actually covered in the profile_pref_store_manager_unittest.cc. I will be happy to add one here too for completeness but Robert has asked that non-essential test additions be deferred to a follow-up. https://codereview.chromium.org/324493002/diff/380001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right): https://codereview.chromium.org/324493002/diff/380001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager_unittest.cc:323: ASSERT_TRUE(local_state_.GetUserPrefValue( On 2014/06/17 22:08:22, gab wrote: > Tests appear to be choking on this; as if there was an issue with the > legacy_hash_store_contents logic, but I don't see anything obvious. Done.
asvitkine: PTAL for histograms.xml
histograms lgtm % nit https://codereview.chromium.org/324493002/diff/440001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/324493002/diff/440001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25298: +<histogram name="Settings.MigratedHashesFromLocalState" enum="Boolean"> Nit: enum="BooleanMigrated"
LGTM++ w/ typo fix below =D!!! Can you explicitly state in this CL's description the follow-up CLs that should follow this, I forget what they all were, but I remember a few "to be done as a follow-up" comments throughout this review. Cheers! Gab https://codereview.chromium.org/324493002/diff/420001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/324493002/diff/420001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_filter_unittest.cc:328: // |post_filter_on_load_callback| invocatin. s/invocatin/invocation/
The CQ bit was checked by erikwright@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/324493002/480001
Message was sent while issue was closed.
Committed patchset #24 manually as r278164 (presubmit successful). |
