Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2204943002:
Integrate registry_hash_store_contents with the rest of tracked prefs. (Closed)
DescriptionIntegrate registry_hash_store_contents with the rest of tracked prefs.
This change adds Windows-only logic to the PrefHashFilter such that it
verifies preferences against MACs stored in the registry. Unlike the
current tracked preference logic, this extra check does NOT reset
settings.
To avoid inconsistent state with the MACs in secure_preferences, we
clear the registry MACs before writing secure_preferences, and write
the registry MACs after the file is successfully written.
BUG=624858
Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c
Committed: https://crrev.com/269fd091f46bbe9615024efe1ab6cfd44ca2547a
Cr-Original-Commit-Position: refs/heads/master@{#422240}
Cr-Commit-Position: refs/heads/master@{#422957}
Patch Set 1 #Patch Set 2 : Some cleanups #Patch Set 3 : Added some comments #
Total comments: 3
Patch Set 4 : Experiment with giving two transactions to EnforceAndReport #
Total comments: 79
Patch Set 5 : Address comments on #4 #Patch Set 6 : Add unit and browser tests. GetType => GetUMASuffix #
Total comments: 2
Patch Set 7 : Remove a lost include statement #
Total comments: 106
Patch Set 8 : Address comments on #7 #Patch Set 9 : Apply fixes from 2324663003, fix bad merge #Patch Set 10 : Rebased and added important_file_writer CL as dependent patchset #
Total comments: 70
Patch Set 11 : Apply fixes from #10 #
Total comments: 16
Patch Set 12 : Address comments on #11 #
Total comments: 4
Patch Set 13 : upload post dependent-patchset commit #
Total comments: 26
Patch Set 14 : Apply comments on #12 #
Total comments: 7
Patch Set 15 : Address comments on #14 #Patch Set 16 : Address comments on #15 #Patch Set 17 : Merge in 2372663003 and update returned callbacks accordingly #Patch Set 18 : Fix bad merge #
Total comments: 6
Patch Set 19 : Make ClearFromExternalStore take raw pointers #
Total comments: 2
Patch Set 20 : Add const qualifier to ClearFromExternalStore's DictionaryValue argument #
Total comments: 2
Patch Set 21 : Changes to make non-win browsertests compile, as they handle file paths and base::string16 differe… #Patch Set 22 : Revert changes in patchset 21 and ifdef the variable instead #Patch Set 23 : Put GetRegistryPathForTestProfile in the ifdef as well #
Total comments: 2
Patch Set 24 : Inline registry path string into GetRegistryPathForTestProfile #Patch Set 25 : Empty patchset #Patch Set 26 : Remove dcheck, add comment #
Total comments: 2
Patch Set 27 : Simplify has_pending_write_reply/callback booleans and logic #
Total comments: 7
Patch Set 28 : Apply comments from patch set 27 #Messages
Total messages: 70 (21 generated)
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. BUG=624858 ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry hashes before writing secure_preferences, and write the registry hashes after the file is successfully written. BUG=624858 ==========
proberge@chromium.org changed reviewers: + gab@chromium.org
https://codereview.chromium.org/2204943002/diff/40001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:330: registry_hash_store_transaction.get()); Calling ValidateAndReport after EnforceAndReport is a bit strange. In the case that we reset setting Foo in EnforceAndReport, the check against the registry will consider Foo to be Cleared and kPreferenceResetTime to be Changed. A solution would be to make EnforceAndReport take the registry_hash_store_transaction as a parameter and do all MACs checks before restoring settings.
https://codereview.chromium.org/2204943002/diff/40001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:330: registry_hash_store_transaction.get()); On 2016/08/02 21:26:07, proberge wrote: > Calling ValidateAndReport after EnforceAndReport is a bit strange. In the case > that we reset setting Foo in EnforceAndReport, the check against the registry > will consider Foo to be Cleared and kPreferenceResetTime to be Changed. > > A solution would be to make EnforceAndReport take the > registry_hash_store_transaction as a parameter and do all MACs checks before > restoring settings. Tried the above in PatchSet 4. LMK what you think.
First pass, approach looks good overall. Please update CL description to only mention "MACs" (not "hashes") and try to wrap it to ~72 characters (column length of git commits -- some Chromium reviewers are harsh on this, I don't care much besides trying to keep a reasonable wrapping length...). https://codereview.chromium.org/2204943002/diff/40001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:330: registry_hash_store_transaction.get()); On 2016/08/02 22:09:33, proberge wrote: > On 2016/08/02 21:26:07, proberge wrote: > > Calling ValidateAndReport after EnforceAndReport is a bit strange. In the case > > that we reset setting Foo in EnforceAndReport, the check against the registry > > will consider Foo to be Cleared and kPreferenceResetTime to be Changed. > > > > A solution would be to make EnforceAndReport take the > > registry_hash_store_transaction as a parameter and do all MACs checks before > > restoring settings. > > Tried the above in PatchSet 4. LMK what you think. Yes that SGTM https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.cc:179: blocking_on_next_successful_write_.Reset(); This won't respect the contract (it won't stay around if the first write is a failure -- to do so you'd have to only Reset() it in the ForwardSuccessfulWrite() on success). But maybe that's not the contract you want? Maybe what you want is more: // Registers |on_next_write_callback| to be synchronously invoked from // WriteFileAtomically() on its next write (i.e. from |task_runner_|), with // |success| indicating whether it succeeded or not. void RegisterOnWriteSynchronousCallback( const Callback<void(bool success)>& on_next_write_callback); Then you can put the old registry entries back on failure? Or you may decide to not do anything on failure (if it's too complicated to implement a rollback for a use case that will pretty much never happen) -- i.e. just report failure with UMA at first to see if it even matters to handle it. But I'm pretty sure you don't want the API to let the Callback stick around until the next successful write (as that wouldn't match the MACs you saved anyways so dumping them to the registry doesn't make sense -- it does make sense for the existing use case which is merely waiting for "a" successful write to complete the migration). https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... File base/files/important_file_writer.h (right): https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:100: void RegisterOnNextSuccessfulWriteCallback( Similar to comment below, suggest updating this to: // Registers |on_next_successful_write_reply| to be invoked on the sequence // owning this ImportantFileWriter in reply to the next successful write. void RegisterOnNextSuccessfulWriteReply( const Closure& on_next_successful_write_reply); https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:104: // successful write event. Multiple callbacks can be set at once. Your implementation only allows a single callback, not multiple. https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:105: void RegisterOnNextSuccessfulWriteBlockingCallback( The ImportantFileWriter doesn't have a notion of the "BlockingPool", it just so happens that |task_runner_| is bound to it in the JsonPrefStore's use case (and even the JsonPrefStore doesn't know that either, it comes as a generic SequencedTaskRunner from the factory in chrome). Suggest: // Registers |on_next_successful_write_callback| to be synchronously invoked // from WriteFileAtomically() on its next successful write (i.e. from // |task_runner_|). void RegisterOnNextSuccessfulWriteSynchronousCallback( const Closure& on_next_successful_write_callback); https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:121: Closure on_next_successful_write_; on_next_successful_write_reply_ https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:125: Closure blocking_on_next_successful_write_; // Invoked synchronously on the next successful write and reset on this ImportantFileWriter's owning sequence in the reply to such a write. Closure on_next_successful_write_callback_; https://codereview.chromium.org/2204943002/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/2204943002/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.h:104: std::unique_ptr<PrefHashStore> MaybeGetRegistryPrefHashStore(); // Returns a PrefHashStore that can be used for extra out-of-band // verifications on this platform (or null if none for this platform). std::unique_ptr<PrefHashStore> GetExternalVerificationPrefHashStore(); https://codereview.chromium.org/2204943002/diff/60001/components/prefs/json_p... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/prefs/json_p... components/prefs/json_pref_store.h:117: const base::Closure& on_next_successful_write); Adapt API to match changes to ImportantFileWriter's API https://codereview.chromium.org/2204943002/diff/60001/components/prefs/pref_f... File components/prefs/pref_filter.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/prefs/pref_f... components/prefs/pref_filter.h:57: base::DictionaryValue* pref_store_contents) = 0; Instead of forcing the caller to call this in a particular order w.r.t. FilterSerializeData, make the API: // Receives notification when the pref store is about to serialize data // contained in |pref_store_contents| to a string. Modifications to // |pref_store_contents| will be persisted to disk and also affect the // in-memory state. If the returned Callback is non-null, it will be // registered to be invoked synchronously after the next write (from the I/O // TaskRunner so it must not be bound to thread-unsafe member state). virtual base::Callback<void(bool success)> FilterSerializeData( base::DictionaryValue* pref_store_contents) = 0; then it's up to the implementation to do things in the order that pleases it. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (left): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:179: // significantly affect performance on the UI thread. Remove comment but let's leave the stat in place (and monitor it to make sure it doesn't regress now that we will compute GetOnNextWriteSynchronousCallback() in FilterSerializeData). https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:230: // significantly affect startup. Same here, remove comment but leave stat. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:217: base::Closure PrefHashFilter::GetPostSerializeCallback( This will become private with PrefFilter API change suggestion. Suggest renaming to "GetOnNextWriteSynchronousCallback" https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:223: base::DictionaryValue* changed_paths_macs = new base::DictionaryValue; Use a std::unique_ptr here and use base::Passed instead of base::Owned in Bind(). https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:228: const base::Value* value = NULL; s/NULL/nullptr/ (here and elsewhere in this CL -- changed as of C++11) https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:232: if (value && value->GetAsDictionary(&dict_value)) { Dictionary doesn't mean it's a split mac (it's okay to store dictionaries as atomic as well, split macs is only when someone wants to split the values in a dictionary -- i.e. extensions.settings) An alternative might be to use: const TrackedPreference* changed_preference = it->second; changed_preference->GetType() (and introduce TrackedPreference:GetType() => ATOMIC/SPLIT) https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:233: // Don't break 'extensions.settings' into two DictionaryValues. 'extensions.settings' is too specific for a comment here. And I don't think we need this comment anyway, it's logical to use "WithoutPathExpansion" when the input is used as a "key" rather than a "path". https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:238: // Don't break 'session.restore_on_startup' into two DictionaryValues. rm comment https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:261: MaybeGetRegistryHashStoreTransaction()); std::unique_ptr<PrefHashStoreTransaction> external_hash_store_transaction = external_validation_hash_store_contents ? WrapUnique(new PrefHashStoreTransaction(external_validation_hash_store_contents)) : nullptr; https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:79: std::unique_ptr<PrefHashStore> registry_pref_hash_store, I think this can all be made generic, i.e.: |external_validation_hash_store| // |external_validation_hash_store| will be used (if non-null) to perform extra validations without triggering resets. (see other suggestions below) https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:81: const base::FilePath& profile_path, "profiles" are a foreign concept to components/. It was a mistake of mine to not call this out on the interface of RegistryHashStoreContentsWin. Please change its interface to call that string something else (e.g. |store_key|), it just happens to users in chrome/ use the |profile_name| as a |store_key| but the components/user_prefs/ layers shouldn't be aware of that. Also, why do we need the key again here? Shouldn't it already be embedded in |external_validation_hash_store|? Oh I see it's because the former is the store, whereas you're using it to get the HashStoreContents at run-time. In this case I think the |external_validation_hash_store_contents| could also be passed in as it doesn't need to be built with live data (like DictionaryHashStoreContents does) and then the rest of the code can truly be generic (merely checking for nullptr). https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:121: MaybeGetRegistryHashStoreTransaction(); Check |if (external_validation_hash_store)| instead of depending on Windows. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:125: static void PrefHashFilter::FlushToRegistry( FlushToExternalStore() -- no-ops (or simply not called) |if (!external_validation_hash_store)| Nothing else is Windows specific as it uses the HashStoreContents API :-) https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store.h:32: virtual std::string ComputeMac(const std::string& path, Document these new methods. Something like // Computes the MAC to be associated with |path| and |value| in this store. PrefHashStoreTransaction typically uses this internally but it's also exposed for users that want to compute MACs ahead of time for asynchronous operations. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store.h:33: const base::Value* new_value) = 0; s/new_value/value/ (this API doesn't care that it's "new") https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:79: base::DictionaryValue* split_macs = new base::DictionaryValue; std::unique_ptr here instead of only wrapping it at the end https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:87: split_macs->SetString(it.key(), ComputeMac(keyed_path, &it.value())); SetStringWithoutPathExpansion https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:93: std::string PrefHashStoreImpl::ComputeMac(const std::string& path, Same order as in header (this method before ComputeSplitMacs()) https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:250: bool is_string = it.value().GetAsString(&mac); Too bad that we now have an extra string copy, I guess this is the fault of values.h for not exposing something like GetAsStringPiece() for users not caring for an actual copy... oh well, fine for now (assuming we don't see a regressions in timing stats). https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:118: } What's the change detected on this line?! https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:43: PrefHashStoreTransaction::ValueState registry_value_state = Unused outside of if scope below, move it in there. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:48: registry_transaction->GetStoreType()); Instead of adding "StoreType", I'd prefer to make this more generic, i.e. GetUMAPrefix() -- then TrackedPreferenceHelper doesn't need to all of a sudden depend on HashStoreContents (and worse, depend on its impl types. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:75: (value_state != PrefHashStoreTransaction::UNCHANGED || Here I think what you want to check is |was_reset|. i.e. if you have an external_validation_transaction and either the pref was reset or your external MAC was invalid, you want to store a new MAC? https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_atomic_preference.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.h:38: PrefHashStoreTransaction* registry_transaction) const override; |external_valiation_transaction| https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_preference_helper.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.cc:69: std::string histogram_name; const char* histogram_name; to avoid an unnecessary string copy of the constants. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.cc:105: histogram_name_prefix.append(histogram_name), 1, reporting_ids_count_, Add a '.' between prefix and base histogram name https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_preference_helper.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.h:45: // Reports |value_state| via UMA under |reporting_id_|. // Reports |value_state| via UMA under |reporting_id_|. |uma_prefix| will be prepended to all metrics if non-empty. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.h:47: HashStoreContentsType validation_type) const; As mentioned above, I think this could just be const std::string& uma_prefix https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_split_preference.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_split_preference.cc:105: (value_state != PrefHashStoreTransaction::UNCHANGED || ditto: |was_reset|
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry hashes before writing secure_preferences, and write the registry hashes after the file is successfully written. BUG=624858 ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 ==========
Also updated CL description https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.cc:179: blocking_on_next_successful_write_.Reset(); On 2016/08/03 18:19:34, gab wrote: > This won't respect the contract (it won't stay around if the first write is a > failure -- to do so you'd have to only Reset() it in the > ForwardSuccessfulWrite() on success). > > But maybe that's not the contract you want? > > Maybe what you want is more: > > // Registers |on_next_write_callback| to be synchronously invoked from > // WriteFileAtomically() on its next write (i.e. from |task_runner_|), with > // |success| indicating whether it succeeded or not. > void RegisterOnWriteSynchronousCallback( > const Callback<void(bool success)>& on_next_write_callback); > > Then you can put the old registry entries back on failure? Or you may decide to > not do anything on failure (if it's too complicated to implement a rollback for > a use case that will pretty much never happen) -- i.e. just report failure with > UMA at first to see if it even matters to handle it. > But I'm pretty sure you don't want the API to let the Callback stick around > until the next successful write (as that wouldn't match the MACs you saved > anyways so dumping them to the registry doesn't make sense -- it does make sense > for the existing use case which is merely waiting for "a" successful write to > complete the migration). Right, unlike the migration case we care about a specific write, not any successful write. I updated the contract to take in a bool callback. https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... File base/files/important_file_writer.h (right): https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:100: void RegisterOnNextSuccessfulWriteCallback( On 2016/08/03 18:19:34, gab wrote: > Similar to comment below, suggest updating this to: > > // Registers |on_next_successful_write_reply| to be invoked on the sequence > // owning this ImportantFileWriter in reply to the next successful write. > void RegisterOnNextSuccessfulWriteReply( > const Closure& on_next_successful_write_reply); Done. https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:104: // successful write event. Multiple callbacks can be set at once. On 2016/08/03 18:19:34, gab wrote: > Your implementation only allows a single callback, not multiple. Done. https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:105: void RegisterOnNextSuccessfulWriteBlockingCallback( On 2016/08/03 18:19:34, gab wrote: > The ImportantFileWriter doesn't have a notion of the "BlockingPool", it just so > happens that |task_runner_| is bound to it in the JsonPrefStore's use case (and > even the JsonPrefStore doesn't know that either, it comes as a generic > SequencedTaskRunner from the factory in chrome). > > Suggest: > > // Registers |on_next_successful_write_callback| to be synchronously invoked > // from WriteFileAtomically() on its next successful write (i.e. from > // |task_runner_|). > void RegisterOnNextSuccessfulWriteSynchronousCallback( > const Closure& on_next_successful_write_callback); Done. https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:121: Closure on_next_successful_write_; On 2016/08/03 18:19:34, gab wrote: > on_next_successful_write_reply_ Done. https://codereview.chromium.org/2204943002/diff/60001/base/files/important_fi... base/files/important_file_writer.h:125: Closure blocking_on_next_successful_write_; On 2016/08/03 18:19:34, gab wrote: > // Invoked synchronously on the next successful write and reset on this > ImportantFileWriter's owning sequence in the reply to such a write. > Closure on_next_successful_write_callback_; Done. https://codereview.chromium.org/2204943002/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/2204943002/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/profile_pref_store_manager.h:104: std::unique_ptr<PrefHashStore> MaybeGetRegistryPrefHashStore(); On 2016/08/03 18:19:34, gab wrote: > // Returns a PrefHashStore that can be used for extra out-of-band > // verifications on this platform (or null if none for this platform). > std::unique_ptr<PrefHashStore> GetExternalVerificationPrefHashStore(); Done. https://codereview.chromium.org/2204943002/diff/60001/components/prefs/json_p... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/prefs/json_p... components/prefs/json_pref_store.h:117: const base::Closure& on_next_successful_write); On 2016/08/03 18:19:34, gab wrote: > Adapt API to match changes to ImportantFileWriter's API Done. https://codereview.chromium.org/2204943002/diff/60001/components/prefs/pref_f... File components/prefs/pref_filter.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/prefs/pref_f... components/prefs/pref_filter.h:57: base::DictionaryValue* pref_store_contents) = 0; On 2016/08/03 18:19:34, gab wrote: > Instead of forcing the caller to call this in a particular order w.r.t. > FilterSerializeData, make the API: > > // Receives notification when the pref store is about to serialize data > // contained in |pref_store_contents| to a string. Modifications to > // |pref_store_contents| will be persisted to disk and also affect the > // in-memory state. If the returned Callback is non-null, it will be > // registered to be invoked synchronously after the next write (from the I/O > // TaskRunner so it must not be bound to thread-unsafe member state). > virtual base::Callback<void(bool success)> FilterSerializeData( > base::DictionaryValue* pref_store_contents) = 0; > > then it's up to the implementation to do things in the order that pleases it. Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (left): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:179: // significantly affect performance on the UI thread. On 2016/08/03 18:19:35, gab wrote: > Remove comment but let's leave the stat in place (and monitor it to make sure it > doesn't regress now that we will compute GetOnNextWriteSynchronousCallback() in > FilterSerializeData). Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:230: // significantly affect startup. On 2016/08/03 18:19:35, gab wrote: > Same here, remove comment but leave stat. Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:217: base::Closure PrefHashFilter::GetPostSerializeCallback( On 2016/08/03 18:19:35, gab wrote: > This will become private with PrefFilter API change suggestion. > > Suggest renaming to "GetOnNextWriteSynchronousCallback" Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:223: base::DictionaryValue* changed_paths_macs = new base::DictionaryValue; On 2016/08/03 18:19:35, gab wrote: > Use a std::unique_ptr here and use base::Passed instead of base::Owned in > Bind(). Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:228: const base::Value* value = NULL; On 2016/08/03 18:19:34, gab wrote: > s/NULL/nullptr/ (here and elsewhere in this CL -- changed as of C++11) Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:232: if (value && value->GetAsDictionary(&dict_value)) { On 2016/08/03 18:19:35, gab wrote: > Dictionary doesn't mean it's a split mac (it's okay to store dictionaries as > atomic as well, split macs is only when someone wants to split the values in a > dictionary -- i.e. extensions.settings) > > An alternative might be to use: > > const TrackedPreference* changed_preference = it->second; > changed_preference->GetType() > > (and introduce TrackedPreference:GetType() => ATOMIC/SPLIT) Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:233: // Don't break 'extensions.settings' into two DictionaryValues. On 2016/08/03 18:19:35, gab wrote: > 'extensions.settings' is too specific for a comment here. > > And I don't think we need this comment anyway, it's logical to use > "WithoutPathExpansion" when the input is used as a "key" rather than a "path". Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:238: // Don't break 'session.restore_on_startup' into two DictionaryValues. On 2016/08/03 18:19:35, gab wrote: > rm comment Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.cc:261: MaybeGetRegistryHashStoreTransaction()); On 2016/08/03 18:19:34, gab wrote: > std::unique_ptr<PrefHashStoreTransaction> external_hash_store_transaction = > external_validation_hash_store_contents > ? WrapUnique(new > PrefHashStoreTransaction(external_validation_hash_store_contents)) > : nullptr; Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_filter.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:79: std::unique_ptr<PrefHashStore> registry_pref_hash_store, On 2016/08/03 18:19:35, gab wrote: > I think this can all be made generic, i.e.: > > |external_validation_hash_store| > > // |external_validation_hash_store| will be used (if non-null) to perform extra > validations without triggering resets. > > (see other suggestions below) Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:81: const base::FilePath& profile_path, On 2016/08/03 18:19:35, gab wrote: > "profiles" are a foreign concept to components/. > > It was a mistake of mine to not call this out on the interface of > RegistryHashStoreContentsWin. Please change its interface to call that string > something else (e.g. |store_key|), it just happens to users in chrome/ use the > |profile_name| as a |store_key| but the components/user_prefs/ layers shouldn't > be aware of that. > > Also, why do we need the key again here? Shouldn't it already be embedded in > |external_validation_hash_store|? Oh I see it's because the former is the store, > whereas you're using it to get the HashStoreContents at run-time. In this case I > think the |external_validation_hash_store_contents| could also be passed in as > it doesn't need to be built with live data (like DictionaryHashStoreContents > does) and then the rest of the code can truly be generic (merely checking for > nullptr). Done. Note that I chose to add a MakeCopy method to HashStoreContents. The alternative, making it a RefCountedThreadSafe, was driving me crazy due to all the existing uses of HashStoreContents which don't need to be managed by a scoped_refptr. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:121: MaybeGetRegistryHashStoreTransaction(); On 2016/08/03 18:19:35, gab wrote: > Check |if (external_validation_hash_store)| instead of depending on Windows. Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_filter.h:125: static void PrefHashFilter::FlushToRegistry( On 2016/08/03 18:19:35, gab wrote: > FlushToExternalStore() -- no-ops (or simply not called) |if > (!external_validation_hash_store)| > > Nothing else is Windows specific as it uses the HashStoreContents API :-) Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store.h:32: virtual std::string ComputeMac(const std::string& path, On 2016/08/03 18:19:35, gab wrote: > Document these new methods. Something like > > // Computes the MAC to be associated with |path| and |value| in this store. > PrefHashStoreTransaction typically uses this internally but it's also exposed > for users that want to compute MACs ahead of time for asynchronous operations. Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store.h:33: const base::Value* new_value) = 0; On 2016/08/03 18:19:35, gab wrote: > s/new_value/value/ (this API doesn't care that it's "new") Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:79: base::DictionaryValue* split_macs = new base::DictionaryValue; On 2016/08/03 18:19:36, gab wrote: > std::unique_ptr here instead of only wrapping it at the end Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:87: split_macs->SetString(it.key(), ComputeMac(keyed_path, &it.value())); On 2016/08/03 18:19:35, gab wrote: > SetStringWithoutPathExpansion Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:93: std::string PrefHashStoreImpl::ComputeMac(const std::string& path, On 2016/08/03 18:19:35, gab wrote: > Same order as in header (this method before ComputeSplitMacs()) Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:250: bool is_string = it.value().GetAsString(&mac); On 2016/08/03 18:19:35, gab wrote: > Too bad that we now have an extra string copy, I guess this is the fault of > values.h for not exposing something like GetAsStringPiece() for users not caring > for an actual copy... oh well, fine for now (assuming we don't see a regressions > in timing stats). Tried some StringValue shenanigans. From my reading this ends up not copying the string, but I'm not 100% sure. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:118: } On 2016/08/03 18:19:36, gab wrote: > What's the change detected on this line?! Left had no line break at eof https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:43: PrefHashStoreTransaction::ValueState registry_value_state = On 2016/08/03 18:19:36, gab wrote: > Unused outside of if scope below, move it in there. Used on line 76 to decide whether to update the registry. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:48: registry_transaction->GetStoreType()); On 2016/08/03 18:19:36, gab wrote: > Instead of adding "StoreType", I'd prefer to make this more generic, i.e. > GetUMAPrefix() -- then TrackedPreferenceHelper doesn't need to all of a sudden The issue with GetUMAPrefix is that it's odd for DictionaryHashStoreContents to return an empty string (to avoid changing the existing metrics). > depend on HashStoreContents (and worse, depend on its impl types. The dependency is on an enum, not the impl type. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:75: (value_state != PrefHashStoreTransaction::UNCHANGED || On 2016/08/03 18:19:36, gab wrote: > Here I think what you want to check is |was_reset|. > > i.e. if you have an external_validation_transaction and either the pref was > reset or your external MAC was invalid, you want to store a new MAC? Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_atomic_preference.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.h:38: PrefHashStoreTransaction* registry_transaction) const override; On 2016/08/03 18:19:36, gab wrote: > |external_valiation_transaction| Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_preference_helper.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.cc:69: std::string histogram_name; On 2016/08/03 18:19:36, gab wrote: > const char* histogram_name; > > to avoid an unnecessary string copy of the constants. Done. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.cc:105: histogram_name_prefix.append(histogram_name), 1, reporting_ids_count_, On 2016/08/03 18:19:36, gab wrote: > Add a '.' between prefix and base histogram name Done. Any more efficient way than doing two appends? https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_preference_helper.h (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.h:45: // Reports |value_state| via UMA under |reporting_id_|. On 2016/08/03 18:19:36, gab wrote: > // Reports |value_state| via UMA under |reporting_id_|. |uma_prefix| will be > prepended to all metrics if non-empty. Done, but keeping |validation_type| for now. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.h:47: HashStoreContentsType validation_type) const; On 2016/08/03 18:19:36, gab wrote: > As mentioned above, I think this could just be > > const std::string& uma_prefix Ack, see above reply https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_split_preference.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_split_preference.cc:105: (value_state != PrefHashStoreTransaction::UNCHANGED || On 2016/08/03 18:19:36, gab wrote: > ditto: |was_reset| Done.
Replying to comments from phone, will do full review before Monday. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_atomic_preference.cc:48: registry_transaction->GetStoreType()); On 2016/08/04 00:13:46, proberge wrote: > On 2016/08/03 18:19:36, gab wrote: > > Instead of adding "StoreType", I'd prefer to make this more generic, i.e. > > GetUMAPrefix() -- then TrackedPreferenceHelper doesn't need to all of a sudden > > The issue with GetUMAPrefix is that it's odd for DictionaryHashStoreContents to > return an empty string (to avoid changing the existing metrics). I think that's fine, there can be a comment on it explaining why. > > > depend on HashStoreContents (and worse, depend on its impl types. > > The dependency is on an enum, not the impl type. But the enum has to be aligned with the impl types, exposing implementation details in the interface https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/tracked_preference_helper.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/tracked_preference_helper.cc:105: histogram_name_prefix.append(histogram_name), 1, reporting_ids_count_, On 2016/08/04 00:13:46, proberge wrote: > On 2016/08/03 18:19:36, gab wrote: > > Add a '.' between prefix and base histogram name > > Done. Any more efficient way than doing two appends? In C++ strings are mutable so two appends is efficient (unlike Java where each string modifications results in a new string)
On 2016/08/05 19:59:25, gab wrote: > Replying to comments from phone, will do full review before Monday. > > https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... > File components/user_prefs/tracked/tracked_atomic_preference.cc (right): > > https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... > components/user_prefs/tracked/tracked_atomic_preference.cc:48: > registry_transaction->GetStoreType()); > On 2016/08/04 00:13:46, proberge wrote: > > On 2016/08/03 18:19:36, gab wrote: > > > Instead of adding "StoreType", I'd prefer to make this more generic, i.e. > > > GetUMAPrefix() -- then TrackedPreferenceHelper doesn't need to all of a > sudden > > > > The issue with GetUMAPrefix is that it's odd for DictionaryHashStoreContents > to > > return an empty string (to avoid changing the existing metrics). > > I think that's fine, there can be a comment on it explaining why. > > > > > > > depend on HashStoreContents (and worse, depend on its impl types. > > > > The dependency is on an enum, not the impl type. > > But the enum has to be aligned with the impl types, exposing implementation > details in the interface > > https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... > File components/user_prefs/tracked/tracked_preference_helper.cc (right): > > https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... > components/user_prefs/tracked/tracked_preference_helper.cc:105: > histogram_name_prefix.append(histogram_name), 1, reporting_ids_count_, > On 2016/08/04 00:13:46, proberge wrote: > > On 2016/08/03 18:19:36, gab wrote: > > > Add a '.' between prefix and base histogram name > > > > Done. Any more efficient way than doing two appends? > > In C++ strings are mutable so two appends is efficient (unlike Java where each > string modifications results in a new string) Done and Ack. Added the tests: CL size intensifies.
https://codereview.chromium.org/2204943002/diff/100001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/100001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:277: chrome_prefs::SetPreferenceValidationRegistryPathForTesting(registry_key); Getting the registry to work during the browser test was fun. We want registry state to be conserved between the PRE_ test and the real test. We can't use the OverrideRegistry like most tests, since that destroys the key when the PRE_ test is over. Also, all the PRE_ tests run, then the real tests run so we can't rely on test ordering. https://codereview.chromium.org/2204943002/diff/100001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/100001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:496: // mock instance. PrefHashFilter creates a copy of the HashStoreContents when giving it to the post-write callback. We want to know what happens to the copy, which is problematic when mocking. This giant hack allows us to know what operations are done on the mock.
Another pass, overall approach lg now :-) Please split up this CL however, 1200+ lines is to be avoided as much as possible, I found at least 4 things highlighted below that can easily be done in their own precursor CLs. https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:250: bool is_string = it.value().GetAsString(&mac); On 2016/08/04 00:13:46, proberge wrote: > On 2016/08/03 18:19:35, gab wrote: > > Too bad that we now have an extra string copy, I guess this is the fault of > > values.h for not exposing something like GetAsStringPiece() for users not > caring > > for an actual copy... oh well, fine for now (assuming we don't see a > regressions > > in timing stats). > > Tried some StringValue shenanigans. From my reading this ends up not copying the > string, but I'm not 100% sure. Yes, your method of getting the underlying StringValue* and then using its GetString method to get a const& out of it is the way to avoid the copy, yay :-) https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... File base/files/important_file_writer.h (right): https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:99: // owning this ImportantFileWriter in reply to the next successful write. + "Only one such callback can be set at once." here and below https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:100: void RegisterOnNextSuccessfulWriteReply( Need to update important_file_writer_unittest.cc https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:106: void RegisterOnNextWriteSynchronousCallback( Update CL description to explain why this is required (for the eventual base owner reviewing this' sake) https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:107: const Callback<void(bool success)>& on_next_write_callback); Add test in important_file_writer_unittest.cc https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:45: base::string16 g_preference_validation_registry_path_for_testing; This will result in an undesired static initializer, global variables have to be POD types, given this is always used from the same thread you can make it a raw pointer initialized to |nullptr| (otherwise you would have had to use a base::LazyInstance). https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:53: #if defined(OS_WIN) Also DCHECK(!path.empty()) maybe? https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:162: base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteReply, I'd like us to break down this CL as much as possible, +1200 lines is a massive CL. The changes to the ImportantFileWriter API and incurred side-effects are a good candidate for the first CL. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:222: new PrefHashStoreImpl("ChromiumRegistryHashStoreValidationSeed", Use Chrome instead of Chromium I'd say (this is the general recommendation in comments too : Chromium is the project, Chrome is the product). https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:233: if (g_preference_validation_registry_path_for_testing.size() > 0) { !empty() instead of size() > 0 (but actually now that it will be a pointer, simply test for nullptr) https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:36: namespace chrome_prefs { Make it a static method of ProfilePrefStoreManager instead of adding it to a top-level namespace. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:40: void SetPreferenceValidationRegistryPathForTesting(base::string16 path); Always pass strings by const& https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:40: void SetPreferenceValidationRegistryPathForTesting(base::string16 path); Make this call OS_WIN only (registry isn't generic enough to have it on other platforms). https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... components/prefs/json_pref_store.cc:413: pref_filter_->FilterSerializeData(prefs_.get())); Save the return value in a var and only register it if |!callback.is_null()| https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... components/prefs/json_pref_store.h:115: // next successful write event of |writer|. Description and var names are no longer up-to-date. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/hash_store_contents.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:17: enum class HashStoreContentsType : int32_t { Can get rid of this now? https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:35: // you can't avoid it. In order to make this API cleaner, I'd like us to add // Returns true if this implementation of HashStoreContents can be copied via MakeCopy(). virtual bool IsCopyable() const = 0; and then I think we can make the documentation of MakeCopy(): // Returns a copy of this HashStoreContents. Must only be called on lightweight implementations (which return true from IsCopyable()) and only in scenarios where a copy cannot be avoided. (note lack of use of "you"/"we" pronouns in comment too which is preferred) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:38: // Returns the string to append to UMA histograms for this store type. "Returns the suffix to be appended" (the fact that it's a string is implicit IMO) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:41: virtual std::string GetUMASuffix() const = 0; Return a base::StringPiece instead of an std::string to avoid forming a string out of constants (i.e. avoid a copy). https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:12: #include "base/callback.h" callback.h is already in header, remove here https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:68: external_validation_hash_store_contents.release()), Why release? Also std::move this one. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:144: DictionaryHashStoreContents dictionary_contents(pref_store_contents); I like this, glad it's not refcounted after all :-) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:146: pref_hash_store_->BeginTransaction(&dictionary_contents)); w.r.t to splitting this CL, this API change (raw pointer in BeginTransaction() is a good candidate for another precursor CL. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:170: // Generate the post-write callback before modifying any hashes. s/hashes/MACs/ Also, does that order actually matter? It's the dictionary store's MACs which are updated, but that shouldn't affect the prefs the callback is based on. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:184: external_validation_hash_store_contents_ It's kind of weird to have those separate isn't it (been thinking this since constructor but now this double if is confirming it). How about we make the member variable a: using StoreContentsPair = std::pair<std::unique_ptr<PrefHashStore>, std::unique_ptr<HashStoreContents>>; const base::Optional<StoreContentsPair> external_validation_hash_store_pair_; and have constructor take a: StoreContentsPair* external_validation_hash_store_pair, and in impl: external_validation_hash_store_pair_( external_validation_hash_store_pair ? make_optional<StoreContentsPair>( std::move(external_validation_hash_store_pair)) : base::Optional::nullopt_t) then here you can: std::unique_ptr<PrefHashStoreTransaction> external_validation_hash_store_transaction; if(external_validation_hash_store_pair) { external_validation_hash_store_transaction = external_validation_hash_store_pair->first->BeginTransaction( external_validation_hash_store_pair->second.get()); } I guess it's not much less code, but having them go together as a pair makes more sense as that's really what they are (the main reason they're split in the DictionaryHashStoreContents use-case is that the |pref_store_contents| is obtained from the outside and is different on every call -- in this case we own the contents so...). Another option would be to have an ExternalPrefHashStore which just embeds a HashStoreContents for use-cases like any other store I can imagine where the data is owned by it not by its caller -- but I think just keeping it as a pair is good enough for now per this being a limited use case. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:197: // Clear prefs in the external store. They will be repopulated when // Clear MACs for modified prefs in the external store. (...) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:320: const base::Value* value = nullptr; s/value/new_value/ https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:323: if (changed_preference->GetType() == TrackedPreferenceType::ATOMIC) { Use a switch() statement with no default case so that compile breaks here should this enum ever be expanded (and then you don't need a NOTREACHED() :-)) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:339: return base::Bind( Just above this add: DCHECK(external_validation_hash_store_contents_->IsCopyable()) << "External HashStoreContents must be copyable at it needs to be used " << "off-thread."; https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:80: // validations without triggering resets. unwrap https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:84: std::unique_ptr<HashStoreContents> external_hash_store_contents, document as well https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:121: // Helper function to generate the callback returned by |FilterSerializeData|. + "The returned callback is thread-safe." https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:125: // Write to the external store the MACs contained in |changed_paths_and_macs|. // Flushes the MACs contained in |changed_paths_and_mac| to |external_hash_store_contents| if |write_success|, otherwise discards the changes. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:126: // This is called after the prefs are written to disk by the pref store to No need to mention how/when this is called (that comment belongs where it's registered -- this method's API shouldn't document who calls it) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:316: std::find(cleared_values_.begin(), cleared_values_.end(), path)); cleared_values.find(path) instead of std::find https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:413: // safe to dereference. This class appears to be storing real data, not pointer values, so this isn't true? And the only use case of this immediately deferences the pointer to compare strings... so why use pointers at all? I'm skeptical of the paradigm used here. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:414: const std::string* stored_mac(const std::string& path) const { I realize that you copied this from MockPrefHashStore, but these methods names should be using PascalCasing per not being trivial inline getters/setters (i.e. snake_case is fine for stored_hashes_count() only here). https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:426: PS: Will wait for this to be split in its own CL w/ above comments addressed to review further this test further. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store.h:45: virtual std::unique_ptr<base::DictionaryValue> ComputeSplitMacs( ComputeMac/ComputeSplitMacs is another good candidate to split up this CL. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl.cc:77: if (!split_values) Does it ever make sense to call this when |!split_values|? If not, DCHECK instead. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_impl_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:30: std::unique_ptr<base::DictionaryValue> pref_store_contents_; |pref_store_contents_| doesn't have to be in a unique_ptr AFAICT, you can get a pointer to it via & where needed. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:56: pref_hash_store.ComputeSplitMacs("extension.settings", &dict); "foo.bar" (no need to refer to extension.settings" here in this low-level component) https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_transaction.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_transaction.h:11: enum class HashStoreContentsType : int32_t; rm https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_transaction.h:46: // Returns the type of the store contained in this transaction. Update comment https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.cc:72: const base::string16& profile_name) Another CL to fix this : as aforementioned "profile" is a foreign concept here, it was an oversight of mine to allow this here. A more generic name like "store_key" should be used here. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:20: // long due to 8-bit to 16-bit char conversion. Hmmmm? UTF8ToUTF16 shouldn't change the size of the string? https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:80: if (external_validation_transaction && // Update MACs in the external store if there is one and there either was a // reset or the external validation failed. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:84: const base::Value* new_value = NULL; nullptr https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/tracked_preference.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_preference.h:15: enum class TrackedPreferenceType : int32_t { ATOMIC = 1, SPLIT = 2 }; Don't need to specify int32_t type and don't need to explicit label =1, =2 either -- they're never used as integers. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/tracked_preference_helper.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_preference_helper.cc:92: // Using FactoryGet to allow dynamic histogram names. Should be equivalent to s/Should be equivalent to/This is equivalent to/ https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_preference_helper.cc:97: histogram->Add(reporting_id_); The change to introduce GetUMASuffix() and changing this logic can also be split in its own precursor CL. https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101646: + <suffix name="FromRegistryStore" Suffix in code is only "FromRegistry" https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101647: + label="Validation using MACs in the WIndows Registry"/> Windows ^ https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101648: + <affected-histogram name="Settings.TrackedPreferenceUnchanged"/> Move with others
Addressed comments on #7. You can hold off on reviewing this CL further for now, I'll start working on doing the following in precursor CLs: * ImportantFileWriter changes * Use RawPtr for BeginTransaction * PrefHashFilterUnitTest changes * ComputeMac/ComputeSplicMac * Rename profile_name to store_key * GetUMASuffix change. https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... File base/files/important_file_writer.h (right): https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:99: // owning this ImportantFileWriter in reply to the next successful write. On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > + "Only one such callback can be set at once." > > here and below Done. https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:100: void RegisterOnNextSuccessfulWriteReply( On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > Need to update important_file_writer_unittest.cc Done. https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:106: void RegisterOnNextWriteSynchronousCallback( On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > Update CL description to explain why this is required (for the eventual base > owner reviewing this' sake) Acknowledged. Will be done in precursor CL. https://codereview.chromium.org/2204943002/diff/120001/base/files/important_f... base/files/important_file_writer.h:107: const Callback<void(bool success)>& on_next_write_callback); On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > Add test in important_file_writer_unittest.cc Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:45: base::string16 g_preference_validation_registry_path_for_testing; On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > This will result in an undesired static initializer, global variables have to be > POD types, given this is always used from the same thread you can make it a raw > pointer initialized to |nullptr| (otherwise you would have had to use a > base::LazyInstance). Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:53: #if defined(OS_WIN) On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > Also DCHECK(!path.empty()) maybe? Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:162: base::Bind(&JsonPrefStore::RegisterOnNextSuccessfulWriteReply, On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > I'd like us to break down this CL as much as possible, +1200 lines is a massive > CL. The changes to the ImportantFileWriter API and incurred side-effects are a > good candidate for the first CL. Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:222: new PrefHashStoreImpl("ChromiumRegistryHashStoreValidationSeed", On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > Use Chrome instead of Chromium I'd say (this is the general recommendation in > comments too : Chromium is the project, Chrome is the product). Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:233: if (g_preference_validation_registry_path_for_testing.size() > 0) { On 2016/08/08 04:37:44, gab (OOO Aug 15 - Sep 1) wrote: > !empty() instead of size() > 0 > > (but actually now that it will be a pointer, simply test for nullptr) Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:36: namespace chrome_prefs { On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Make it a static method of ProfilePrefStoreManager instead of adding it to a > top-level namespace. Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:40: void SetPreferenceValidationRegistryPathForTesting(base::string16 path); On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Always pass strings by const& Done. https://codereview.chromium.org/2204943002/diff/120001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:40: void SetPreferenceValidationRegistryPathForTesting(base::string16 path); On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Make this call OS_WIN only (registry isn't generic enough to have it on other > platforms). Done. https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... components/prefs/json_pref_store.cc:413: pref_filter_->FilterSerializeData(prefs_.get())); On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Save the return value in a var and only register it if |!callback.is_null()| Done. https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... File components/prefs/json_pref_store.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/prefs/json_... components/prefs/json_pref_store.h:115: // next successful write event of |writer|. On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Description and var names are no longer up-to-date. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/hash_store_contents.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:17: enum class HashStoreContentsType : int32_t { On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Can get rid of this now? Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:35: // you can't avoid it. On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > In order to make this API cleaner, I'd like us to add > > // Returns true if this implementation of HashStoreContents can be copied via > MakeCopy(). > virtual bool IsCopyable() const = 0; > > and then I think we can make the documentation of MakeCopy(): > > // Returns a copy of this HashStoreContents. Must only be called on lightweight > implementations (which return true from IsCopyable()) and only in scenarios > where a copy cannot be avoided. > > (note lack of use of "you"/"we" pronouns in comment too which is preferred) Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:38: // Returns the string to append to UMA histograms for this store type. On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > "Returns the suffix to be appended" > > (the fact that it's a string is implicit IMO) Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:41: virtual std::string GetUMASuffix() const = 0; On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Return a base::StringPiece instead of an std::string to avoid forming a string > out of constants (i.e. avoid a copy). Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:12: #include "base/callback.h" On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > callback.h is already in header, remove here Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:68: external_validation_hash_store_contents.release()), On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Why release? Also std::move this one. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:144: DictionaryHashStoreContents dictionary_contents(pref_store_contents); On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > I like this, glad it's not refcounted after all :-) Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:146: pref_hash_store_->BeginTransaction(&dictionary_contents)); On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > w.r.t to splitting this CL, this API change (raw pointer in BeginTransaction() > is a good candidate for another precursor CL. Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:170: // Generate the post-write callback before modifying any hashes. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > s/hashes/MACs/ > > Also, does that order actually matter? It's the dictionary store's MACs which > are updated, but that shouldn't affect the prefs the callback is based on. Edited the comment to better explain that changed_paths_ is what matters. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:184: external_validation_hash_store_contents_ On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > It's kind of weird to have those separate isn't it (been thinking this since > constructor but now this double if is confirming it). > > How about we make the member variable a: > > using StoreContentsPair = std::pair<std::unique_ptr<PrefHashStore>, > std::unique_ptr<HashStoreContents>>; > > const base::Optional<StoreContentsPair> external_validation_hash_store_pair_; > > and have constructor take a: > > StoreContentsPair* external_validation_hash_store_pair, > > and in impl: > > external_validation_hash_store_pair_( > external_validation_hash_store_pair > ? make_optional<StoreContentsPair>( > std::move(external_validation_hash_store_pair)) > : base::Optional::nullopt_t) > > then here you can: > > std::unique_ptr<PrefHashStoreTransaction> > external_validation_hash_store_transaction; > if(external_validation_hash_store_pair) { > external_validation_hash_store_transaction = > external_validation_hash_store_pair->first->BeginTransaction( > external_validation_hash_store_pair->second.get()); > } > > I guess it's not much less code, but having them go together as a pair makes > more sense as that's really what they are (the main reason they're split in the > DictionaryHashStoreContents use-case is that the |pref_store_contents| is > obtained from the outside and is different on every call -- in this case we own > the contents so...). > Another option would be to have an ExternalPrefHashStore which just embeds a > HashStoreContents for use-cases like any other store I can imagine where the > data is owned by it not by its caller -- but I think just keeping it as a pair > is good enough for now per this being a limited use case. Done, but the raw pointer in the PrefHashFilter constructor is making me uncomfortable. Can you double-check that I'm not leaking any memory? I can't tell if ownership is probably passed to pref_hash_filter. Would it make sense to pass in a std::unique_ptr<StoreContentsPair> instead of a raw pointer? https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:197: // Clear prefs in the external store. They will be repopulated when On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > // Clear MACs for modified prefs in the external store. (...) Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:320: const base::Value* value = nullptr; On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > s/value/new_value/ Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:323: if (changed_preference->GetType() == TrackedPreferenceType::ATOMIC) { On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Use a switch() statement with no default case so that compile breaks here should > this enum ever be expanded (and then you don't need a NOTREACHED() :-)) Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:339: return base::Bind( On 2016/08/08 04:37:45, gab (OOO Aug 15 - Sep 1) wrote: > Just above this add: > > DCHECK(external_validation_hash_store_contents_->IsCopyable()) > << "External HashStoreContents must be copyable at it needs to be used " > << "off-thread."; Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:80: // validations without triggering resets. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > unwrap Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:84: std::unique_ptr<HashStoreContents> external_hash_store_contents, On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > document as well Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:121: // Helper function to generate the callback returned by |FilterSerializeData|. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > + "The returned callback is thread-safe." Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:125: // Write to the external store the MACs contained in |changed_paths_and_macs|. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > // Flushes the MACs contained in |changed_paths_and_mac| to > |external_hash_store_contents| if |write_success|, otherwise discards the > changes. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:126: // This is called after the prefs are written to disk by the pref store to On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > No need to mention how/when this is called (that comment belongs where it's > registered -- this method's API shouldn't document who calls it) Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:316: std::find(cleared_values_.begin(), cleared_values_.end(), path)); On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > cleared_values.find(path) instead of std::find 'find': is not a member of 'std::vector<std::string,std::allocator<_Ty>>' Changed cleared_values_ to a set instead. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:413: // safe to dereference. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > This class appears to be storing real data, not pointer values, so this isn't > true? > > And the only use case of this immediately deferences the pointer to compare > strings... so why use pointers at all? I'm skeptical of the paradigm used here. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:414: const std::string* stored_mac(const std::string& path) const { On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > I realize that you copied this from MockPrefHashStore, but these methods names > should be using PascalCasing per not being trivial inline getters/setters (i.e. > snake_case is fine for stored_hashes_count() only here). Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:426: On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > PS: Will wait for this to be split in its own CL w/ above comments addressed to > review further this test further. Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store.h:45: virtual std::unique_ptr<base::DictionaryValue> ComputeSplitMacs( On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > ComputeMac/ComputeSplitMacs is another good candidate to split up this CL. Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl.cc:77: if (!split_values) On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > Does it ever make sense to call this when |!split_values|? If not, DCHECK > instead. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_impl_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:30: std::unique_ptr<base::DictionaryValue> pref_store_contents_; On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > |pref_store_contents_| doesn't have to be in a unique_ptr AFAICT, you can get a > pointer to it via & where needed. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl_unittest.cc:56: pref_hash_store.ComputeSplitMacs("extension.settings", &dict); On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > "foo.bar" (no need to refer to extension.settings" here in this low-level > component) Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_transaction.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_transaction.h:11: enum class HashStoreContentsType : int32_t; On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > rm Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_transaction.h:46: // Returns the type of the store contained in this transaction. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > Update comment Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.cc:72: const base::string16& profile_name) On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > Another CL to fix this : as aforementioned "profile" is a foreign concept here, > it was an oversight of mine to allow this here. A more generic name like > "store_key" should be used here. Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:20: // long due to 8-bit to 16-bit char conversion. On 2016/08/08 04:37:46, gab (OOO Aug 15 - Sep 1) wrote: > Hmmmm? UTF8ToUTF16 shouldn't change the size of the string? I asked @grt why he used REG_BINARY instead of REG_SZ when writing to the registry in platform_state_store_win.cc. He replied: "storing a std::string in the registry as REG_SZ will 2x the space used due to the 8-bit to 16-bit char conversion" Should I use REG_BINARY instead to save on registry space? https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:80: if (external_validation_transaction && On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > // Update MACs in the external store if there is one and there either was a > // reset or the external validation failed. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:84: const base::Value* new_value = NULL; On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > nullptr Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/tracked_preference.h (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_preference.h:15: enum class TrackedPreferenceType : int32_t { ATOMIC = 1, SPLIT = 2 }; On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > Don't need to specify int32_t type and don't need to explicit label =1, =2 > either -- they're never used as integers. Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... File components/user_prefs/tracked/tracked_preference_helper.cc (right): https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_preference_helper.cc:92: // Using FactoryGet to allow dynamic histogram names. Should be equivalent to On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > s/Should be equivalent to/This is equivalent to/ Done. https://codereview.chromium.org/2204943002/diff/120001/components/user_prefs/... components/user_prefs/tracked/tracked_preference_helper.cc:97: histogram->Add(reporting_id_); On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > The change to introduce GetUMASuffix() and changing this logic can also be split > in its own precursor CL. Acknowledged. https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101646: + <suffix name="FromRegistryStore" On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > Suffix in code is only "FromRegistry" Done. https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101647: + label="Validation using MACs in the WIndows Registry"/> On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > Windows > ^ Done. https://codereview.chromium.org/2204943002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101648: + <affected-histogram name="Settings.TrackedPreferenceUnchanged"/> On 2016/08/08 04:37:47, gab (OOO Aug 15 - Sep 1) wrote: > Move with others Done.
This should be ready for another review cycle
This is looking good :-) https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:215: std::pair<std::unique_ptr<PrefHashStore>, std::unique_ptr<HashStoreContents>>* Returning raw pointer is almost never correct (i.e. it only is when intentionally returning a pointer to member state), in this case it results in a memory leak. In this case returning an std::pair directly is the right thing to do as std::pair is moveable and as such its content will be moved instead of copied (it wouldn't compile if the copy constructor was implicitly used as unique_ptr isn't copyable). You can return a pair with nullptrs in the non-Windows case. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:220: std::unique_ptr<HashStoreContents>>(std::make_pair( Once you return by value I think you can just "return std::make_pair(...);" https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:222: new PrefHashStoreImpl("ChromeRegistryHashStoreValidationSeed", s/std::unique_ptr<Foo>(new FooImpl(...))/base::MakeUnique<FooImpl>(...)/ https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis... https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:112: // extra out-of-band verifications, or NULL if not available on this platform. s/NULL/null/ is the chromium-style as of C++11 where NULL is no longer a thing edit: comment will have to change more than that based on comments in impl https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:275: base::string16 registry_key = GetRegistryPathForTestProfile(); Override the entire registry hive for the test instead of doing manually (and also note that your current approach would be flaky when tests are run in parallel per using the same key for multiple tests). i.e. have a registry_util::RegistryOverrideManager registry_override_manager_; member and here do registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER); Then none of this custom registry overriding logic needs to be there :-) The one tricky thing here is that you need to preserve the override across PRE_ tests which isn't currently supported. As such we have two options: 1) Add something like RegistryOverrideManager::SerializeOverride(HKEY override, const FilePath& file) and RegistryOverrideManager::DeserializeOverride(HKEY override, const FilePath& file) which takes the existing override and saves its state to disk so that it can be regenerated in the next test (and unwinds the override in serialize phase so that the destructor leaves it alone instead of cleaning it up). Note: PathService::Get(chrome::DIR_USER_DATA,...) remains the same between PRE_ and main test so we can serialize files in there if needed. or 2) Since we are the only ones needing this, it's fine to do essentially that but ourselves (i.e. self-managing our registry override instead of using RegistryOverrideManager). https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:355: num_split_tracked_prefs); Instead of duplicating every check, would it be possible to have a method that can be called that does the old check + the new check when it applies. Then redirect all existing checks to it? https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1141: // PRE test TearDown instead. I think it should, this probably has to do with the custom override/cleanup used in this patch set, try again after setting up registry redirection as suggested above. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1169: } Note: for this test I'm fine not using the aforementioned helper as it actually does verify that they don't have the same result. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1170: } I think we're missing a test for the very use case this feature is intended to address : pref + matching MAC injected in prefs but registry unmodified (and thus catching the change). https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/hash_store_contents.h (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:34: // lightweight implementations(which return true from IsCopyable()) and only Space between word and '(' https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:64: external_validation_hash_store_pair To support this now that it will come in by value you can check one of its members for null and use nullopt if so (and DCHECK below that if the Optional member is set, both items are non-null). https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:197: // repopulated when |callback| is run. Add: "This avoids false positives in the event of a failure to complete the write." https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:199: external_validation_hash_store_transaction->ClearHash(changed_path); I just realized : it's possible with the current way ImportantFileWriter works for this to run in parallel with FlushToExternalStore() because ImportantFileWriter doesn't currently wait until its last write is done before scheduling a new one... This was fine before the introduction of a synchronous callback post-write... We could add locks here, but even then it wouldn't be correct because: 1) The old FlushToExternalStore() could still run after the latest write's ClearHash(), resulting in potential false positives as well. 2) We'd have to enforce in the HashStoreContents API that copyable ones do not do any expensive operations (e.g. I/O) in Set calls which seems out of place. I think the solution is to address this at the ImportantFileWriter level, i.e. we could cancel a write if it didn't occur yet by the time another one is fired (and/or postpone the new write until after the current one completes if it's underway). Happy to discuss how this could work in person (and I'm sorry to see you hit yet another corner case with this CL!). https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:312: new base::DictionaryValue); Use MakeUnique https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:319: pref_store_contents->Get(changed_path, &new_value); Move this into the switch so that we can GetDictionary directly and avoid the extra hoop. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:330: if (new_value && new_value->GetAsDictionary(&dict_value)) { When moving the Get() not that it's fine for |new_value| to be null so I think this if is incorrect (i.e. we do need to update the MACs to represent a null pref. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:446: inner_value->SetStringWithoutPathExpansion(split_path, mac); Won't unconditionally creating a new dict overwrite other MACs in the same split pref? https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:463: if (dictionary_.GetWithoutPathExpansion(path, &out_value)) { Assuming you're not using GetStringWithoutPathExpansion() so that you can verify type below? If so that makes sense to me :-) https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:607: new PrefHashFilter::StoreContentsPair( Note: you should question yourself anytime you see a naked "new" call, it's almost always a leak (or a poor design -- i.e. a comment that says "takes ownership" instead of explicitly using unique_ptr) In this case this is addressed by an aforementioned API change but just for the record as a teaching point. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1261: // Ownership of the following values is transfered to |root_dict|. DictionaryValue::Set() now supports unique_ptr so we no longer need to do this bad style temporary raw pointer (i.e. this is an example of an old bad API that required raw pointers and took ownership... in the process of being fixed in Chromium). Though I guess this is okay for now as it makes it consistent with surrounding style, so let's keep it as-is in this CL. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1283: // No pref write should occur before we run |callback|. s/before we run X/before X is run/ (avoid pronouns) https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1305: TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbackWithFalse) { s/WithFalse/WithFailure/ https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1311: // Skip updating kAtomicPref2. // Only update kAtomicPref. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1365: // and UNCHANGED preferences once the class supports external validation. Can be done now? https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl.cc:12: #include "base/memory/ptr_util.h" Not needed? https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.cc:86: new RegistryHashStoreContentsWin(preference_key_name_)); return base::MakeUnique<RegistryHashStoreContentsWin>(*this); https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.h (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.h:44: const base::string16& preference_key_name); Instead define the default copy constructor as private here, i.e. explicit RegistryHashStoreContentsWin(const RegistryHashStoreContentsWin& other); and in .cc RegistryHashStoreContentsWin::RegistryHashStoreContentsWin( const RegistryHashStoreContentsWin& other) = default; https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:88: external_validation_transaction->StoreHash(pref_path_, new_value); This is fine in practice because it runs as part of FilterOnLoad() and can't race with other registry writes, but architecturally it's so far from it that it's hard to justify this with a comment here... WDYT?
https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:215: std::pair<std::unique_ptr<PrefHashStore>, std::unique_ptr<HashStoreContents>>* On 2016/09/16 19:47:31, gab wrote: > Returning raw pointer is almost never correct (i.e. it only is when > intentionally returning a pointer to member state), in this case it results in a > memory leak. > > In this case returning an std::pair directly is the right thing to do as > std::pair is moveable and as such its content will be moved instead of copied > (it wouldn't compile if the copy constructor was implicitly used as unique_ptr > isn't copyable). > > You can return a pair with nullptrs in the non-Windows case. Done. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:220: std::unique_ptr<HashStoreContents>>(std::make_pair( On 2016/09/16 19:47:32, gab wrote: > Once you return by value I think you can just "return std::make_pair(...);" Done. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:222: new PrefHashStoreImpl("ChromeRegistryHashStoreValidationSeed", On 2016/09/16 19:47:31, gab wrote: > s/std::unique_ptr<Foo>(new FooImpl(...))/base::MakeUnique<FooImpl>(...)/ > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis... Done. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.h (right): https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.h:112: // extra out-of-band verifications, or NULL if not available on this platform. On 2016/09/16 19:47:32, gab wrote: > s/NULL/null/ is the chromium-style as of C++11 where NULL is no longer a thing > > edit: comment will have to change more than that based on comments in impl Done. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:275: base::string16 registry_key = GetRegistryPathForTestProfile(); On 2016/09/16 19:47:32, gab wrote: > Override the entire registry hive for the test instead of doing manually (and > also note that your current approach would be flaky when tests are run in > parallel per using the same key for multiple tests). > > i.e. have a registry_util::RegistryOverrideManager registry_override_manager_; > member and here do > > registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER); > > Then none of this custom registry overriding logic needs to be there :-) > > > The one tricky thing here is that you need to preserve the override across PRE_ > tests which isn't currently supported. As such we have two options: > > 1) Add something like RegistryOverrideManager::SerializeOverride(HKEY override, > const FilePath& file) and RegistryOverrideManager::DeserializeOverride(HKEY > override, const FilePath& file) which takes the existing override and saves its > state to disk so that it can be regenerated in the next test (and unwinds the > override in serialize phase so that the destructor leaves it alone instead of > cleaning it up). > > Note: PathService::Get(chrome::DIR_USER_DATA,...) remains the same between PRE_ > and main test so we can serialize files in there if needed. > > or > > 2) Since we are the only ones needing this, it's fine to do essentially that but > ourselves (i.e. self-managing our registry override instead of using > RegistryOverrideManager). This was tricky to get working. It was a few weeks ago so I might not remember all of it, but the current approach uses a different key for multiple tests. GetRegistryPathForTestProfile() uses PathService::Get(chrome::DIR_USER_DATA ...) to get a registry path which is different between tests, but the same for PRE_ and associated main test. In other words, I'm doing option #2 but without the file serialization. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:355: num_split_tracked_prefs); On 2016/09/16 19:47:32, gab wrote: > Instead of duplicating every check, would it be possible to have a method that > can be called that does the old check + the new check when it applies. Then > redirect all existing checks to it? It would be hard to read the test. The method would need to take in a the current GetTrackedPrefHistogramCount parameters and the two EXPECT_EQ arguments. Consider this case: EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM, num_tracked_prefs_ > 0); The second argument relies on the result of GetTrackedPrefHistogramCount, so we would want the last parameter to be a function "X > 0" which we then bind the result of GetTrackedPrefHistogramCount to. Afaik this would be pretty verbose in C++. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1141: // PRE test TearDown instead. On 2016/09/16 19:47:32, gab wrote: > I think it should, this probably has to do with the custom override/cleanup used > in this patch set, try again after setting up registry redirection as suggested > above. Hmm. I tried moving it back to AttackPreferencesOnDisk and it seems to work. Maybe I had a transient issue that has since then been fixed. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1169: } On 2016/09/16 19:47:32, gab wrote: > Note: for this test I'm fine not using the aforementioned helper as it actually > does verify that they don't have the same result. Acknowledged. https://codereview.chromium.org/2204943002/diff/180001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1170: } On 2016/09/16 19:47:32, gab wrote: > I think we're missing a test for the very use case this feature is intended to > address : pref + matching MAC injected in prefs but registry unmodified (and > thus catching the change). PrefHashBrowserTestRegistryValidationFailure is meant to test the case you're talking about. Implementing the pref + matching mac injection was daunting. In theory these two cases have identical expected behavior: 1. pref + matching mac injected, registry mac unmodified 2. pref + matching mac unmodified, registry mac tampered https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/hash_store_contents.h (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/hash_store_contents.h:34: // lightweight implementations(which return true from IsCopyable()) and only On 2016/09/16 19:47:32, gab wrote: > Space between word and '(' Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:64: external_validation_hash_store_pair On 2016/09/16 19:47:32, gab wrote: > To support this now that it will come in by value you can check one of its > members for null and use nullopt if so (and DCHECK below that if the Optional > member is set, both items are non-null). Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:197: // repopulated when |callback| is run. On 2016/09/16 19:47:32, gab wrote: > Add: "This avoids false positives in the event of a failure to complete the > write." Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:199: external_validation_hash_store_transaction->ClearHash(changed_path); On 2016/09/16 19:47:32, gab wrote: > I just realized : it's possible with the current way ImportantFileWriter works > for this to run in parallel with FlushToExternalStore() because > ImportantFileWriter doesn't currently wait until its last write is done before > scheduling a new one... This was fine before the introduction of a synchronous > callback post-write... > > We could add locks here, but even then it wouldn't be correct because: > 1) The old FlushToExternalStore() could still run after the latest write's > ClearHash(), resulting in potential false positives as well. > 2) We'd have to enforce in the HashStoreContents API that copyable ones do not > do any expensive operations (e.g. I/O) in Set calls which seems out of place. > > I think the solution is to address this at the ImportantFileWriter level, i.e. > we could cancel a write if it didn't occur yet by the time another one is fired > (and/or postpone the new write until after the current one completes if it's > underway). Happy to discuss how this could work in person (and I'm sorry to see > you hit yet another corner case with this CL!). IIUC you're worried about: 1. Pref write #1 is requested. Pref A's registry entry is cleared. 2. Pref write #1 does WriteNow(), but isn't done writing to disk yet. 3. Pref write #2 is requested. Pref A's registry entry is cleared. 4. Pref write #1's WriteNow finished. Pref A's registry entry is updated with pref write #1's data. 5. Pref write #2 does WriteNow(). Two cases are possible: a) Writing to disk succeeds and we update the registry. b) Writing to disk fails and we don't update the registry. In either case, there's consistency (#1 file with #1 registry or #2 file with #2 registry). Are you thinking of another case? https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:312: new base::DictionaryValue); On 2016/09/16 19:47:32, gab wrote: > Use MakeUnique Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:319: pref_store_contents->Get(changed_path, &new_value); On 2016/09/16 19:47:32, gab wrote: > Move this into the switch so that we can GetDictionary directly and avoid the > extra hoop. It's used by both ATOMIC and SPLIT branches. I don't understand what's requested here. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:330: if (new_value && new_value->GetAsDictionary(&dict_value)) { On 2016/09/16 19:47:32, gab wrote: > When moving the Get() not that it's fine for |new_value| to be null so I think > this if is incorrect (i.e. we do need to update the MACs to represent a null > pref. Acknowledged. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:446: inner_value->SetStringWithoutPathExpansion(split_path, mac); On 2016/09/16 19:47:32, gab wrote: > Won't unconditionally creating a new dict overwrite other MACs in the same split > pref? I *think* that setting a dictionary to an entry with an existing dictionary just merges the entries. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:463: if (dictionary_.GetWithoutPathExpansion(path, &out_value)) { On 2016/09/16 19:47:33, gab wrote: > Assuming you're not using GetStringWithoutPathExpansion() so that you can verify > type below? If so that makes sense to me :-) Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:607: new PrefHashFilter::StoreContentsPair( On 2016/09/16 19:47:32, gab wrote: > Note: you should question yourself anytime you see a naked "new" call, it's > almost always a leak (or a poor design -- i.e. a comment that says "takes > ownership" instead of explicitly using unique_ptr) > > In this case this is addressed by an aforementioned API change but just for the > record as a teaching point. Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1261: // Ownership of the following values is transfered to |root_dict|. On 2016/09/16 19:47:33, gab wrote: > DictionaryValue::Set() now supports unique_ptr so we no longer need to do this > bad style temporary raw pointer (i.e. this is an example of an old bad API that > required raw pointers and took ownership... in the process of being fixed in > Chromium). Though I guess this is okay for now as it makes it consistent with > surrounding style, so let's keep it as-is in this CL. Acknowledged. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1283: // No pref write should occur before we run |callback|. On 2016/09/16 19:47:33, gab wrote: > s/before we run X/before X is run/ (avoid pronouns) Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1305: TEST_P(PrefHashFilterTest, CallFilterSerializeDataCallbackWithFalse) { On 2016/09/16 19:47:33, gab wrote: > s/WithFalse/WithFailure/ Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1311: // Skip updating kAtomicPref2. On 2016/09/16 19:47:32, gab wrote: > // Only update kAtomicPref. Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:1365: // and UNCHANGED preferences once the class supports external validation. On 2016/09/16 19:47:32, gab wrote: > Can be done now? Not yet. See https://codereview.chromium.org/2345723002/ https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_store_impl.cc:12: #include "base/memory/ptr_util.h" On 2016/09/16 19:47:33, gab wrote: > Not needed? Reverted changes to this file; probably a bad merge. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.cc:86: new RegistryHashStoreContentsWin(preference_key_name_)); On 2016/09/16 19:47:33, gab wrote: > return base::MakeUnique<RegistryHashStoreContentsWin>(*this); Done, but getting compiler errors on this line related to cannot access private member declared in class 'RegistryHashStoreContentsWin' https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.h (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.h:44: const base::string16& preference_key_name); On 2016/09/16 19:47:33, gab wrote: > Instead define the default copy constructor as private here, i.e. > > explicit RegistryHashStoreContentsWin(const RegistryHashStoreContentsWin& > other); > > and in .cc > > RegistryHashStoreContentsWin::RegistryHashStoreContentsWin( > const RegistryHashStoreContentsWin& other) = default; Done, but had to remove the DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:88: external_validation_transaction->StoreHash(pref_path_, new_value); On 2016/09/16 19:47:33, gab wrote: > This is fine in practice because it runs as part of FilterOnLoad() and can't > race with other registry writes, but architecturally it's so far from it that > it's hard to justify this with a comment here... WDYT? This is similar logic to line 77. Also, in theory we shouldn't be writing any prefs at this time since they're not loaded yet.
Comments on diff and replies. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:199: external_validation_hash_store_transaction->ClearHash(changed_path); On 2016/09/20 21:35:44, proberge wrote: > On 2016/09/16 19:47:32, gab wrote: > > I just realized : it's possible with the current way ImportantFileWriter works > > for this to run in parallel with FlushToExternalStore() because > > ImportantFileWriter doesn't currently wait until its last write is done before > > scheduling a new one... This was fine before the introduction of a synchronous > > callback post-write... > > > > We could add locks here, but even then it wouldn't be correct because: > > 1) The old FlushToExternalStore() could still run after the latest write's > > ClearHash(), resulting in potential false positives as well. > > 2) We'd have to enforce in the HashStoreContents API that copyable ones do > not > > do any expensive operations (e.g. I/O) in Set calls which seems out of place. > > > > I think the solution is to address this at the ImportantFileWriter level, i.e. > > we could cancel a write if it didn't occur yet by the time another one is > fired > > (and/or postpone the new write until after the current one completes if it's > > underway). Happy to discuss how this could work in person (and I'm sorry to > see > > you hit yet another corner case with this CL!). > > IIUC you're worried about: > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > 2. Pref write #1 does WriteNow(), but isn't done writing to disk yet. > 3. Pref write #2 is requested. Pref A's registry entry is cleared. > 4. Pref write #1's WriteNow finished. Pref A's registry entry is updated with > pref write #1's data. > 5. Pref write #2 does WriteNow(). Two cases are possible: > a) Writing to disk succeeds and we update the registry. > b) Writing to disk fails and we don't update the registry. > > In either case, there's consistency (#1 file with #1 registry or #2 file with #2 > registry). Are you thinking of another case? > Not quite, what I mean is: 1. Pref write #1 is requested. Pref A's registry entry is cleared. 2. Pref write #2 is requested. Pref A's registry entry is cleared. 3. Pref write #1's write happens. Pref A's registry entry is updated with pref write #1's data. 4. Pref write #2 write happens. Process crashes before registry flush. We now have #2 file with #1 registry. Also, this assumes locks, without them 2 & 3 can occur in parallel and result in even less clear behavior (what's a Clear mixed with a parallel Write of multiple entries?!). The solution to this is probably to add the ability to cancel writes (or at least ignore the notification that the write occurred). I'm okay landing this CL without this if you're willing to live with the potential false positives caused by it temporarily (add a TODO if so). https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:319: pref_store_contents->Get(changed_path, &new_value); On 2016/09/20 21:35:44, proberge wrote: > On 2016/09/16 19:47:32, gab wrote: > > Move this into the switch so that we can GetDictionary directly and avoid the > > extra hoop. > > It's used by both ATOMIC and SPLIT branches. I don't understand what's requested > here. In the ATOMIC branch use Get directly and in the SPLIT branch use GetDictionary directly. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:446: inner_value->SetStringWithoutPathExpansion(split_path, mac); On 2016/09/20 21:35:45, proberge wrote: > On 2016/09/16 19:47:32, gab wrote: > > Won't unconditionally creating a new dict overwrite other MACs in the same > split > > pref? > > I *think* that setting a dictionary to an entry with an existing dictionary just > merges the entries. I don't think this is true, every pref is a node when you set a pref which happens to be a dictionary, you replace that node with the new dictionary, no merge is performed AFAIK. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.cc:86: new RegistryHashStoreContentsWin(preference_key_name_)); On 2016/09/20 21:35:45, proberge wrote: > On 2016/09/16 19:47:33, gab wrote: > > return base::MakeUnique<RegistryHashStoreContentsWin>(*this); > > Done, but getting compiler errors on this line related to > cannot access private member declared in class 'RegistryHashStoreContentsWin' Ah indeed because MakeUnique needs access to the constructor, oops, use WrapUnique instead in such rare cases : https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer... https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:88: external_validation_transaction->StoreHash(pref_path_, new_value); On 2016/09/20 21:35:45, proberge wrote: > On 2016/09/16 19:47:33, gab wrote: > > This is fine in practice because it runs as part of FilterOnLoad() and can't > > race with other registry writes, but architecturally it's so far from it that > > it's hard to justify this with a comment here... WDYT? > > This is similar logic to line 77. Also, in theory we shouldn't be writing any > prefs at this time since they're not loaded yet. It's different than 77 because 77 writes to |transaction| which is known to be owned by this sequence (and therefore we have exclusive access to its state right now). |external_validation_transaction| is different because it writes to unowned state which other threads also write to. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:224: g_preference_validation_registry_path_for_testing != nullptr Drop "!= nullptr", the ternary operator implies a boolean check. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:296: // as well. If you mean tests that fail by assertion, I think TearDown() runs regardless (and otherwise the destructor of the fixture sure does, so maybe put the code there). If you mean on crash, there's not much we can do there. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1135: #if defined(OS_WIN) Wrap the 1125-1166 in the if-def? Or is there a point verifying a non-attack on an inactive external hash store? https://codereview.chromium.org/2204943002/diff/200001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/200001/components/prefs/json_... components/prefs/json_pref_store.cc:329: void JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback( Note for future patch sets : always upload a separate patch set for merges, it makes the diff to this specific CL easier to review. https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:73: // key and value are present. Pair has a concept of first and second items (not key/value). So I'd say "both its items are non-null". https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:75: external_validation_hash_store_pair_->first); DCHECK(!has || (first && second)); all in one check? https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:203: // repopulated when |callback| is run. This avoisd false positives in avoids
https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:199: external_validation_hash_store_transaction->ClearHash(changed_path); On 2016/09/21 17:55:30, gab wrote: > On 2016/09/20 21:35:44, proberge wrote: > > On 2016/09/16 19:47:32, gab wrote: > > > I just realized : it's possible with the current way ImportantFileWriter > works > > > for this to run in parallel with FlushToExternalStore() because > > > ImportantFileWriter doesn't currently wait until its last write is done > before > > > scheduling a new one... This was fine before the introduction of a > synchronous > > > callback post-write... > > > > > > We could add locks here, but even then it wouldn't be correct because: > > > 1) The old FlushToExternalStore() could still run after the latest write's > > > ClearHash(), resulting in potential false positives as well. > > > 2) We'd have to enforce in the HashStoreContents API that copyable ones do > > not > > > do any expensive operations (e.g. I/O) in Set calls which seems out of > place. > > > > > > I think the solution is to address this at the ImportantFileWriter level, > i.e. > > > we could cancel a write if it didn't occur yet by the time another one is > > fired > > > (and/or postpone the new write until after the current one completes if it's > > > underway). Happy to discuss how this could work in person (and I'm sorry to > > see > > > you hit yet another corner case with this CL!). > > > > IIUC you're worried about: > > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > > 2. Pref write #1 does WriteNow(), but isn't done writing to disk yet. > > 3. Pref write #2 is requested. Pref A's registry entry is cleared. > > 4. Pref write #1's WriteNow finished. Pref A's registry entry is updated with > > pref write #1's data. > > 5. Pref write #2 does WriteNow(). Two cases are possible: > > a) Writing to disk succeeds and we update the registry. > > b) Writing to disk fails and we don't update the registry. > > > > In either case, there's consistency (#1 file with #1 registry or #2 file with > #2 > > registry). Are you thinking of another case? > > > > Not quite, what I mean is: > > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > 2. Pref write #2 is requested. Pref A's registry entry is cleared. > 3. Pref write #1's write happens. Pref A's registry entry is updated with > pref write #1's data. > 4. Pref write #2 write happens. Process crashes before registry flush. > > We now have #2 file with #1 registry. > > Also, this assumes locks, without them 2 & 3 can occur in parallel and result in > even less clear behavior (what's a Clear mixed with a parallel Write of multiple > entries?!). > > The solution to this is probably to add the ability to cancel writes (or at > least ignore the notification that the write occurred). I'm okay landing this CL > without this if you're willing to live with the potential false positives caused > by it temporarily (add a TODO if so). I don't think cancelling the first write would work. If the write task has already started on the IFW's sequenced thread runner, how would we safely cancel it? I can think of three options: 1. Prevent posting write tasks if there's an unfinished write. This negatively impacts prefs performance even when the platform doesn't support registry validation, so I don't think it's a good idea. 2. Make the synchronous callback no-op if a more recent write was scheduled. This would increase the number of blank registry entries on startup (registry entries wouldn't get written if the second write was for different changed preferences) but that's better than false positives. The main pain point is working around the requirement that it should work during Chrome shutdown. From your example, if Chrome starts to shut down before step #3, we wouldn't be able to rely on the JsonPrefStore as a synchronization mechanism to know that write #1's callback shouldn't write prefs to the registry. In that case we would need another mechanism such as writing a number in the registry when clearing the hashes, and reading that number to see if we're the last callback to have cleared the hashes. 3. Move the registry clears to a synchronous pre-write callback. This would require adding another callback (RegisterOnNextBeforeWriteCallback()) past a base/ reviewer's watchful eyes, but I think it would be the simplest to implement. Since the IFW uses a sequenced thread pool, the [Clear registry hashes, Write file to disk, Write registry hashes] operations are isolated from other write's. wdyt? https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:319: pref_store_contents->Get(changed_path, &new_value); On 2016/09/21 17:55:30, gab wrote: > On 2016/09/20 21:35:44, proberge wrote: > > On 2016/09/16 19:47:32, gab wrote: > > > Move this into the switch so that we can GetDictionary directly and avoid > the > > > extra hoop. > > > > It's used by both ATOMIC and SPLIT branches. I don't understand what's > requested > > here. > > In the ATOMIC branch use Get directly and in the SPLIT branch use GetDictionary > directly. Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:446: inner_value->SetStringWithoutPathExpansion(split_path, mac); On 2016/09/21 17:55:30, gab wrote: > On 2016/09/20 21:35:45, proberge wrote: > > On 2016/09/16 19:47:32, gab wrote: > > > Won't unconditionally creating a new dict overwrite other MACs in the same > > split > > > pref? > > > > I *think* that setting a dictionary to an entry with an existing dictionary > just > > merges the entries. > > I don't think this is true, every pref is a node when you set a pref which > happens to be a dictionary, you replace that node with the new dictionary, no > merge is performed AFAIK. You're right. Added some logic to not clobber the dictionary. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win.cc:86: new RegistryHashStoreContentsWin(preference_key_name_)); On 2016/09/21 17:55:30, gab wrote: > On 2016/09/20 21:35:45, proberge wrote: > > On 2016/09/16 19:47:33, gab wrote: > > > return base::MakeUnique<RegistryHashStoreContentsWin>(*this); > > > > Done, but getting compiler errors on this line related to > > cannot access private member declared in class 'RegistryHashStoreContentsWin' > > Ah indeed because MakeUnique needs access to the constructor, oops, use > WrapUnique instead in such rare cases : > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer... Done. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:88: external_validation_transaction->StoreHash(pref_path_, new_value); On 2016/09/21 17:55:30, gab wrote: > On 2016/09/20 21:35:45, proberge wrote: > > On 2016/09/16 19:47:33, gab wrote: > > > This is fine in practice because it runs as part of FilterOnLoad() and can't > > > race with other registry writes, but architecturally it's so far from it > that > > > it's hard to justify this with a comment here... WDYT? > > > > This is similar logic to line 77. Also, in theory we shouldn't be writing any > > prefs at this time since they're not loaded yet. > > It's different than 77 because 77 writes to |transaction| which is known to be > owned by this sequence (and therefore we have exclusive access to its state > right now). |external_validation_transaction| is different because it writes to > unowned state which other threads also write to. Hmm, you're right. Your original comment said "it's so far from it that it's hard to justify this with a comment here". Are you suggesting to simply add a comment, or to move this to a separate method? or to move this logic out into pref_hash_filter? https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:224: g_preference_validation_registry_path_for_testing != nullptr On 2016/09/21 17:55:30, gab wrote: > Drop "!= nullptr", the ternary operator implies a boolean check. Done. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:296: // as well. On 2016/09/21 17:55:30, gab wrote: > If you mean tests that fail by assertion, I think TearDown() runs regardless > (and otherwise the destructor of the fixture sure does, so maybe put the code > there). > > If you mean on crash, there's not much we can do there. Yeah I meant by crash (or CTRL-C). There's some logic in https://cs.chromium.org/chromium/src/base/test/test_reg_util_win.cc?sq=packag... we might be able to use. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1135: #if defined(OS_WIN) On 2016/09/21 17:55:30, gab wrote: > Wrap the 1125-1166 in the if-def? Or is there a point verifying a non-attack on > an inactive external hash store? Done. https://codereview.chromium.org/2204943002/diff/200001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/200001/components/prefs/json_... components/prefs/json_pref_store.cc:329: void JsonPrefStore::RunOrScheduleNextSuccessfulWriteCallback( On 2016/09/21 17:55:30, gab wrote: > Note for future patch sets : always upload a separate patch set for merges, it > makes the diff to this specific CL easier to review. Acknowledged. https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:73: // key and value are present. On 2016/09/21 17:55:30, gab wrote: > Pair has a concept of first and second items (not key/value). > > So I'd say "both its items are non-null". Done. https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:75: external_validation_hash_store_pair_->first); On 2016/09/21 17:55:30, gab wrote: > DCHECK(!has || (first && second)); > > all in one check? Done. https://codereview.chromium.org/2204943002/diff/200001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:203: // repopulated when |callback| is run. This avoisd false positives in On 2016/09/21 17:55:30, gab wrote: > avoids Done.
https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:199: external_validation_hash_store_transaction->ClearHash(changed_path); On 2016/09/21 21:09:24, proberge wrote: > On 2016/09/21 17:55:30, gab wrote: > > On 2016/09/20 21:35:44, proberge wrote: > > > On 2016/09/16 19:47:32, gab wrote: > > > > I just realized : it's possible with the current way ImportantFileWriter > > works > > > > for this to run in parallel with FlushToExternalStore() because > > > > ImportantFileWriter doesn't currently wait until its last write is done > > before > > > > scheduling a new one... This was fine before the introduction of a > > synchronous > > > > callback post-write... > > > > > > > > We could add locks here, but even then it wouldn't be correct because: > > > > 1) The old FlushToExternalStore() could still run after the latest > write's > > > > ClearHash(), resulting in potential false positives as well. > > > > 2) We'd have to enforce in the HashStoreContents API that copyable ones > do > > > not > > > > do any expensive operations (e.g. I/O) in Set calls which seems out of > > place. > > > > > > > > I think the solution is to address this at the ImportantFileWriter level, > > i.e. > > > > we could cancel a write if it didn't occur yet by the time another one is > > > fired > > > > (and/or postpone the new write until after the current one completes if > it's > > > > underway). Happy to discuss how this could work in person (and I'm sorry > to > > > see > > > > you hit yet another corner case with this CL!). > > > > > > IIUC you're worried about: > > > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > > > 2. Pref write #1 does WriteNow(), but isn't done writing to disk yet. > > > 3. Pref write #2 is requested. Pref A's registry entry is cleared. > > > 4. Pref write #1's WriteNow finished. Pref A's registry entry is updated > with > > > pref write #1's data. > > > 5. Pref write #2 does WriteNow(). Two cases are possible: > > > a) Writing to disk succeeds and we update the registry. > > > b) Writing to disk fails and we don't update the registry. > > > > > > In either case, there's consistency (#1 file with #1 registry or #2 file > with > > #2 > > > registry). Are you thinking of another case? > > > > > > > Not quite, what I mean is: > > > > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > > 2. Pref write #2 is requested. Pref A's registry entry is cleared. > > 3. Pref write #1's write happens. Pref A's registry entry is updated with > > pref write #1's data. > > 4. Pref write #2 write happens. Process crashes before registry flush. > > > > We now have #2 file with #1 registry. > > > > Also, this assumes locks, without them 2 & 3 can occur in parallel and result > in > > even less clear behavior (what's a Clear mixed with a parallel Write of > multiple > > entries?!). > > > > The solution to this is probably to add the ability to cancel writes (or at > > least ignore the notification that the write occurred). I'm okay landing this > CL > > without this if you're willing to live with the potential false positives > caused > > by it temporarily (add a TODO if so). > > I don't think cancelling the first write would work. If the write task has > already started on the IFW's sequenced thread runner, how would we safely cancel > it? > > I can think of three options: > > 1. Prevent posting write tasks if there's an unfinished write. > > This negatively impacts prefs performance even when the platform doesn't support > registry validation, so I don't think it's a good idea. > > 2. Make the synchronous callback no-op if a more recent write was scheduled. > > This would increase the number of blank registry entries on startup (registry > entries wouldn't get written if the second write was for different changed > preferences) but that's better than false positives. The main pain point is > working around the requirement that it should work during Chrome shutdown. From > your example, if Chrome starts to shut down before step #3, we wouldn't be able > to rely on the JsonPrefStore as a synchronization mechanism to know that write > #1's callback shouldn't write prefs to the registry. In that case we would need > another mechanism such as writing a number in the registry when clearing the > hashes, and reading that number to see if we're the last callback to have > cleared the hashes. > > 3. Move the registry clears to a synchronous pre-write callback. > > This would require adding another callback (RegisterOnNextBeforeWriteCallback()) > past a base/ reviewer's watchful eyes, but I think it would be the simplest to > implement. Since the IFW uses a sequenced thread pool, the [Clear registry > hashes, Write file to disk, Write registry hashes] operations are isolated from > other write's. > > wdyt? Good points, I like 3 most. I think instead of registering two independent callbacks though it'd be better to register an ImportantFileWriter::WriteObserver which can override and handle either notification at will. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:88: external_validation_transaction->StoreHash(pref_path_, new_value); On 2016/09/21 21:09:24, proberge wrote: > On 2016/09/21 17:55:30, gab wrote: > > On 2016/09/20 21:35:45, proberge wrote: > > > On 2016/09/16 19:47:33, gab wrote: > > > > This is fine in practice because it runs as part of FilterOnLoad() and > can't > > > > race with other registry writes, but architecturally it's so far from it > > that > > > > it's hard to justify this with a comment here... WDYT? > > > > > > This is similar logic to line 77. Also, in theory we shouldn't be writing > any > > > prefs at this time since they're not loaded yet. > > > > It's different than 77 because 77 writes to |transaction| which is known to be > > owned by this sequence (and therefore we have exclusive access to its state > > right now). |external_validation_transaction| is different because it writes > to > > unowned state which other threads also write to. > > Hmm, you're right. Your original comment said "it's so far from it that it's > hard to justify this with a comment here". Are you suggesting to simply add a > comment, or to move this to a separate method? or to move this logic out into > pref_hash_filter? I think just adding to TrackedPreference::EnforceAndReport's definition something like "This call assumes exclusive access to this preference and its associated state and as such should only be called before any other subsystem is made aware of it." should do it. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:296: // as well. On 2016/09/21 21:09:24, proberge wrote: > On 2016/09/21 17:55:30, gab wrote: > > If you mean tests that fail by assertion, I think TearDown() runs regardless > > (and otherwise the destructor of the fixture sure does, so maybe put the code > > there). > > > > If you mean on crash, there's not much we can do there. > > Yeah I meant by crash (or CTRL-C). There's some logic in > https://cs.chromium.org/chromium/src/base/test/test_reg_util_win.cc?sq=packag... > we might be able to use. I see, but if we're going to re-use too much of test_reg_util_win, I'd vote for augmenting it to support our use case instead of stealing from it. Up to you. https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:337: if (pref_store_contents->GetDictionary(changed_path, &dict_value)) { Don't think this should be conditional on whether there is a dictionary. If there isn't a dictionary, the hashes should still be updated to reflect that (i.e. deleting the dictionary is also a valid pref change); just like you did for the ATOMIC case. https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:453: } The typical way this is done in nearby tracked pref code is: base::DictionaryValue* mac_dict = nullptr; dictionary_.GetDictionaryWithoutPathExpansion( path, &mac_dict); if (!mac_dict) { mac_dict = new base::DictionaryValue; dictionary_.SetWithoutPathExpansion(path, mac_dict); } mac_dict->SetStringWithoutPathExpansion(split_path, mac); (i.e., GetOrCreate()->Set())
https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:199: external_validation_hash_store_transaction->ClearHash(changed_path); On 2016/09/22 18:38:27, gab wrote: > On 2016/09/21 21:09:24, proberge wrote: > > On 2016/09/21 17:55:30, gab wrote: > > > On 2016/09/20 21:35:44, proberge wrote: > > > > On 2016/09/16 19:47:32, gab wrote: > > > > > I just realized : it's possible with the current way ImportantFileWriter > > > works > > > > > for this to run in parallel with FlushToExternalStore() because > > > > > ImportantFileWriter doesn't currently wait until its last write is done > > > before > > > > > scheduling a new one... This was fine before the introduction of a > > > synchronous > > > > > callback post-write... > > > > > > > > > > We could add locks here, but even then it wouldn't be correct because: > > > > > 1) The old FlushToExternalStore() could still run after the latest > > write's > > > > > ClearHash(), resulting in potential false positives as well. > > > > > 2) We'd have to enforce in the HashStoreContents API that copyable ones > > do > > > > not > > > > > do any expensive operations (e.g. I/O) in Set calls which seems out of > > > place. > > > > > > > > > > I think the solution is to address this at the ImportantFileWriter > level, > > > i.e. > > > > > we could cancel a write if it didn't occur yet by the time another one > is > > > > fired > > > > > (and/or postpone the new write until after the current one completes if > > it's > > > > > underway). Happy to discuss how this could work in person (and I'm sorry > > to > > > > see > > > > > you hit yet another corner case with this CL!). > > > > > > > > IIUC you're worried about: > > > > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > > > > 2. Pref write #1 does WriteNow(), but isn't done writing to disk yet. > > > > 3. Pref write #2 is requested. Pref A's registry entry is cleared. > > > > 4. Pref write #1's WriteNow finished. Pref A's registry entry is updated > > with > > > > pref write #1's data. > > > > 5. Pref write #2 does WriteNow(). Two cases are possible: > > > > a) Writing to disk succeeds and we update the registry. > > > > b) Writing to disk fails and we don't update the registry. > > > > > > > > In either case, there's consistency (#1 file with #1 registry or #2 file > > with > > > #2 > > > > registry). Are you thinking of another case? > > > > > > > > > > Not quite, what I mean is: > > > > > > 1. Pref write #1 is requested. Pref A's registry entry is cleared. > > > 2. Pref write #2 is requested. Pref A's registry entry is cleared. > > > 3. Pref write #1's write happens. Pref A's registry entry is updated with > > > pref write #1's data. > > > 4. Pref write #2 write happens. Process crashes before registry flush. > > > > > > We now have #2 file with #1 registry. > > > > > > Also, this assumes locks, without them 2 & 3 can occur in parallel and > result > > in > > > even less clear behavior (what's a Clear mixed with a parallel Write of > > multiple > > > entries?!). > > > > > > The solution to this is probably to add the ability to cancel writes (or at > > > least ignore the notification that the write occurred). I'm okay landing > this > > CL > > > without this if you're willing to live with the potential false positives > > caused > > > by it temporarily (add a TODO if so). > > > > I don't think cancelling the first write would work. If the write task has > > already started on the IFW's sequenced thread runner, how would we safely > cancel > > it? > > > > I can think of three options: > > > > 1. Prevent posting write tasks if there's an unfinished write. > > > > This negatively impacts prefs performance even when the platform doesn't > support > > registry validation, so I don't think it's a good idea. > > > > 2. Make the synchronous callback no-op if a more recent write was scheduled. > > > > This would increase the number of blank registry entries on startup (registry > > entries wouldn't get written if the second write was for different changed > > preferences) but that's better than false positives. The main pain point is > > working around the requirement that it should work during Chrome shutdown. > From > > your example, if Chrome starts to shut down before step #3, we wouldn't be > able > > to rely on the JsonPrefStore as a synchronization mechanism to know that write > > #1's callback shouldn't write prefs to the registry. In that case we would > need > > another mechanism such as writing a number in the registry when clearing the > > hashes, and reading that number to see if we're the last callback to have > > cleared the hashes. > > > > 3. Move the registry clears to a synchronous pre-write callback. > > > > This would require adding another callback > (RegisterOnNextBeforeWriteCallback()) > > past a base/ reviewer's watchful eyes, but I think it would be the simplest to > > implement. Since the IFW uses a sequenced thread pool, the [Clear registry > > hashes, Write file to disk, Write registry hashes] operations are isolated > from > > other write's. > > > > wdyt? > > Good points, I like 3 most. I think instead of registering two independent > callbacks though it'd be better to register an > ImportantFileWriter::WriteObserver which can override and handle either > notification at will. sgtm. I'll send a quick e-mail to @dcheng to verify the approach and start another pre-work CL. https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... File components/user_prefs/tracked/tracked_atomic_preference.cc (right): https://codereview.chromium.org/2204943002/diff/180001/components/user_prefs/... components/user_prefs/tracked/tracked_atomic_preference.cc:88: external_validation_transaction->StoreHash(pref_path_, new_value); On 2016/09/22 18:38:28, gab wrote: > On 2016/09/21 21:09:24, proberge wrote: > > On 2016/09/21 17:55:30, gab wrote: > > > On 2016/09/20 21:35:45, proberge wrote: > > > > On 2016/09/16 19:47:33, gab wrote: > > > > > This is fine in practice because it runs as part of FilterOnLoad() and > > can't > > > > > race with other registry writes, but architecturally it's so far from it > > > that > > > > > it's hard to justify this with a comment here... WDYT? > > > > > > > > This is similar logic to line 77. Also, in theory we shouldn't be writing > > any > > > > prefs at this time since they're not loaded yet. > > > > > > It's different than 77 because 77 writes to |transaction| which is known to > be > > > owned by this sequence (and therefore we have exclusive access to its state > > > right now). |external_validation_transaction| is different because it writes > > to > > > unowned state which other threads also write to. > > > > Hmm, you're right. Your original comment said "it's so far from it that it's > > hard to justify this with a comment here". Are you suggesting to simply add a > > comment, or to move this to a separate method? or to move this logic out into > > pref_hash_filter? > > I think just adding to TrackedPreference::EnforceAndReport's definition > something like "This call assumes exclusive access to this preference and its > associated state and as such should only be called before any other subsystem is > made aware of it." should do it. Done. https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/200001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:296: // as well. On 2016/09/22 18:38:28, gab wrote: > On 2016/09/21 21:09:24, proberge wrote: > > On 2016/09/21 17:55:30, gab wrote: > > > If you mean tests that fail by assertion, I think TearDown() runs regardless > > > (and otherwise the destructor of the fixture sure does, so maybe put the > code > > > there). > > > > > > If you mean on crash, there's not much we can do there. > > > > Yeah I meant by crash (or CTRL-C). There's some logic in > > > https://cs.chromium.org/chromium/src/base/test/test_reg_util_win.cc?sq=packag... > > we might be able to use. > > I see, but if we're going to re-use too much of test_reg_util_win, I'd vote for > augmenting it to support our use case instead of stealing from it. Up to you. Thinking about it a bit more, the cleanup logic relies on a timestamp in the registry override path. We want the PRE_ and normal test to use the same registry override path, so the timestamp has to be the same. The simplest solution (rounding or flooring the timestamp by hour or day) has issues on the time boundaries. I think I'll skip addressing this for now. https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:337: if (pref_store_contents->GetDictionary(changed_path, &dict_value)) { On 2016/09/22 18:38:28, gab wrote: > Don't think this should be conditional on whether there is a dictionary. If > there isn't a dictionary, the hashes should still be updated to reflect that > (i.e. deleting the dictionary is also a valid pref change); just like you did > for the ATOMIC case. Good catch. Done. https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/220001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:453: } On 2016/09/22 18:38:28, gab wrote: > The typical way this is done in nearby tracked pref code is: > > base::DictionaryValue* mac_dict = nullptr; > dictionary_.GetDictionaryWithoutPathExpansion( > path, &mac_dict); > if (!mac_dict) { > mac_dict = new base::DictionaryValue; > dictionary_.SetWithoutPathExpansion(path, mac_dict); > } > mac_dict->SetStringWithoutPathExpansion(split_path, mac); > > (i.e., GetOrCreate()->Set()) Done.
Did a final full pass, looks great, I think we're good to go after these nits and the precursor CL we discussed :-)! https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:44: // UseTestingPreferenceValidationRegistryPath(). Forces a different registry key UseTestingPreferenceValidationRegistryPath() is the wrong name now. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:98: g_preference_validation_registry_path_for_testing = new base::string16(path); This will leak the state outside of a given unittest. One reliable way to avoid that would be to return a ScopedPreferenceValidationRegistryOverride object (implemented as a nested class of ProfilePrefStoreManager) which when destroyed resets this pointer. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:70: return kRegistryTestPathPrefix + profile_dir.BaseName().value(); Just to make sure, in browser_tests, BaseName() is a unique scoped_dir identifier, not "User Data", right? https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:81: if (*histogram_suffix != 0) Drop "!= 0" https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:279: // For PRE tests, delete the Registry key to avoid collisions. // Keys should be unique, but to avoid flakes in the long run make sure an identical test key wasn't left behind by a previous test. (no need to mention PRE, it's implicit that the beginning of the test is in the PRE phase) https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:293: // For non-PRE tests, delete the Registry key to avoid polluting the s/For non-PRE tests/When done/ https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:501: // Expect all prefs to be reported as Unchanged with no resets. Drop "with no resets" as it doesn't apply to registry checks https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:602: // Expect homepage to have been cleared. // Expect homepage clearance to have been noticed by the registry validation. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:942: // pref #5 (extensions). Does the registry validation produce an histogram equivalent to "Settings.TrackedSplitPreferenceChanged.extensions.settings"? If so, verify it, if not adapt this comment to only mention what it does verify. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1156: EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0, // Expect that the registry validation caught the invalid MAC for pref #2 (homepage). https://codereview.chromium.org/2204943002/diff/240001/components/prefs/pref_... File components/prefs/pref_filter.h (right): https://codereview.chromium.org/2204943002/diff/240001/components/prefs/pref_... components/prefs/pref_filter.h:54: virtual base::Callback<void(bool)> FilterSerializeData( As mentioned in post-commit of other CL, I think this can be done in a precursor CL, probably mixable with the other required change we discussed. https://codereview.chromium.org/2204943002/diff/240001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/240001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:315: } No {} on single-line body+condition https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:473: return NULL; s/NULL/nullptr (here and elsewhere) https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:20: // long due to 8-bit to 16-bit char conversion. This is still unexpected to me, i.e. this is passed to contents->SetMac() which takes an std::string and as such I would think expects a 32-char string, no?
Thanks for the review! https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:44: // UseTestingPreferenceValidationRegistryPath(). Forces a different registry key On 2016/09/23 14:01:17, gab wrote: > UseTestingPreferenceValidationRegistryPath() is the wrong name now. Done. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:98: g_preference_validation_registry_path_for_testing = new base::string16(path); On 2016/09/23 14:01:17, gab wrote: > This will leak the state outside of a given unittest. > > One reliable way to avoid that would be to return a > ScopedPreferenceValidationRegistryOverride object (implemented as a nested class > of ProfilePrefStoreManager) which when destroyed resets this pointer. This method is used by browser tests, not by unit tests. It leaks state in the same way as https://cs.chromium.org/chromium/src/chrome/browser/prefs/chrome_pref_service.... I think it's fine as-is, since overriding the registry path should be done for all tests anyways. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:70: return kRegistryTestPathPrefix + profile_dir.BaseName().value(); On 2016/09/23 14:01:18, gab wrote: > Just to make sure, in browser_tests, BaseName() is a unique scoped_dir > identifier, not "User Data", right? That's right! In browser tests, profile_dir.BaseName() seems to have the form "d[0-9]{4}_{0-9){2-5}", with a different identifier for each test. (but with the PRE_ test having the same identifier as the non-PRE test) https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:81: if (*histogram_suffix != 0) On 2016/09/23 14:01:18, gab wrote: > Drop "!= 0" Done. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:279: // For PRE tests, delete the Registry key to avoid collisions. On 2016/09/23 14:01:18, gab wrote: > // Keys should be unique, but to avoid flakes in the long run make sure an > identical test key wasn't left behind by a previous test. > > (no need to mention PRE, it's implicit that the beginning of the test is in the > PRE phase) Done. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:293: // For non-PRE tests, delete the Registry key to avoid polluting the On 2016/09/23 14:01:18, gab wrote: > s/For non-PRE tests/When done/ Done. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:501: // Expect all prefs to be reported as Unchanged with no resets. On 2016/09/23 14:01:18, gab wrote: > Drop "with no resets" as it doesn't apply to registry checks Done. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:602: // Expect homepage to have been cleared. On 2016/09/23 14:01:18, gab wrote: > // Expect homepage clearance to have been noticed by the registry validation. Done. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:942: // pref #5 (extensions). On 2016/09/23 14:01:17, gab wrote: > Does the registry validation produce an histogram equivalent to > "Settings.TrackedSplitPreferenceChanged.extensions.settings"? If so, verify it, > if not adapt this comment to only mention what it does verify. You're right, it doesn't. We just have one histogram like so: Settings.TrackedPreferenceChanged.FromRegistry 0 ... 5 ------------------O (1 = 100%) 6 ... Updated the comment. https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1156: EXPECT_EQ(protection_level_ > PROTECTION_DISABLED_ON_PLATFORM ? 1 : 0, On 2016/09/23 14:01:17, gab wrote: > // Expect that the registry validation caught the invalid MAC for pref #2 > (homepage). Done. https://codereview.chromium.org/2204943002/diff/240001/components/prefs/pref_... File components/prefs/pref_filter.h (right): https://codereview.chromium.org/2204943002/diff/240001/components/prefs/pref_... components/prefs/pref_filter.h:54: virtual base::Callback<void(bool)> FilterSerializeData( On 2016/09/23 14:01:18, gab wrote: > As mentioned in post-commit of other CL, I think this can be done in a precursor > CL, probably mixable with the other required change we discussed. I'll add that to https://codereview.chromium.org/2366743002/ https://codereview.chromium.org/2204943002/diff/240001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/240001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:315: } On 2016/09/23 14:01:18, gab wrote: > No {} on single-line body+condition Done. https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:473: return NULL; On 2016/09/23 14:01:18, gab wrote: > s/NULL/nullptr (here and elsewhere) Just on the string method returns, or in the asserts as well? https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:20: // long due to 8-bit to 16-bit char conversion. On 2016/09/23 14:01:18, gab wrote: > This is still unexpected to me, i.e. this is passed to contents->SetMac() which > takes an std::string and as such I would think expects a 32-char string, no? My bad, I completely misunderstood what was going on. SetMac() is called with a 64-char string. pref_hash_calculator generates a 32-char string, but then base::HexEncode()s it. base::HexEncode() turns it into a 64-char string: https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.c...
https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:98: g_preference_validation_registry_path_for_testing = new base::string16(path); On 2016/09/23 15:24:33, proberge wrote: > On 2016/09/23 14:01:17, gab wrote: > > This will leak the state outside of a given unittest. > > > > One reliable way to avoid that would be to return a > > ScopedPreferenceValidationRegistryOverride object (implemented as a nested > class > > of ProfilePrefStoreManager) which when destroyed resets this pointer. > > This method is used by browser tests, not by unit tests. It leaks state in the > same way as > https://cs.chromium.org/chromium/src/chrome/browser/prefs/chrome_pref_service.... > I think it's fine as-is, since overriding the registry path should be done for > all tests anyways. It's not quite the same because the method you're referring to leaks state, not memory. ASAN would trip on this CL (but it doesn't merely because we don't have win-only ASAN try bots..). Also, those tests always set it to the same boolean value whereas here it's set to a different value in every test. You're right that it's "okay" in practice because it's used in browser tests which are each isolated in their own processes but it still feels weird to have this API. To at least avoid the memory leak, one option is to pass the string via const base::string16* so that this class merely has a pointer to state it doesn't own (which would also in turn blow up with a user-after-free if this class tried to access that string after the test fixture that passed it in had been destroyed). https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:473: return NULL; On 2016/09/23 15:24:34, proberge wrote: > On 2016/09/23 14:01:18, gab wrote: > > s/NULL/nullptr (here and elsewhere) > > Just on the string method returns, or in the asserts as well? Oh oops, hadn't noticed this returned string (which makes NULL and nullptr both incorrect), return std::string(); is the right thing for these (and otherwise for pointers always use nullptr over NULL). https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/registry_hash_store_contents_win_unittest.cc:20: // long due to 8-bit to 16-bit char conversion. On 2016/09/23 15:24:34, proberge wrote: > On 2016/09/23 14:01:18, gab wrote: > > This is still unexpected to me, i.e. this is passed to contents->SetMac() > which > > takes an std::string and as such I would think expects a 32-char string, no? > > My bad, I completely misunderstood what was going on. SetMac() is called with a > 64-char string. pref_hash_calculator generates a 32-char string, but then > base::HexEncode()s it. > > base::HexEncode() turns it into a 64-char string: > https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.c... Ah ok, now that makes more sense, thanks!
https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... File chrome/browser/prefs/profile_pref_store_manager.cc (right): https://codereview.chromium.org/2204943002/diff/240001/chrome/browser/prefs/p... chrome/browser/prefs/profile_pref_store_manager.cc:98: g_preference_validation_registry_path_for_testing = new base::string16(path); On 2016/09/23 15:56:58, gab wrote: > On 2016/09/23 15:24:33, proberge wrote: > > On 2016/09/23 14:01:17, gab wrote: > > > This will leak the state outside of a given unittest. > > > > > > One reliable way to avoid that would be to return a > > > ScopedPreferenceValidationRegistryOverride object (implemented as a nested > > class > > > of ProfilePrefStoreManager) which when destroyed resets this pointer. > > > > This method is used by browser tests, not by unit tests. It leaks state in the > > same way as > > > https://cs.chromium.org/chromium/src/chrome/browser/prefs/chrome_pref_service.... > > I think it's fine as-is, since overriding the registry path should be done for > > all tests anyways. > > It's not quite the same because the method you're referring to leaks state, not > memory. > > ASAN would trip on this CL (but it doesn't merely because we don't have win-only > ASAN try bots..). > > Also, those tests always set it to the same boolean value whereas here it's set > to a different value in every test. > You're right that it's "okay" in practice because it's used in browser tests > which are each isolated in their own processes but it still feels weird to have > this API. > > To at least avoid the memory leak, one option is to pass the string via const > base::string16* so that this class merely has a pointer to state it doesn't own > (which would also in turn blow up with a user-after-free if this class tried to > access that string after the test fixture that passed it in had been destroyed). Done. https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter_unittest.cc (right): https://codereview.chromium.org/2204943002/diff/260001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter_unittest.cc:473: return NULL; On 2016/09/23 15:56:58, gab wrote: > On 2016/09/23 15:24:34, proberge wrote: > > On 2016/09/23 14:01:18, gab wrote: > > > s/NULL/nullptr (here and elsewhere) > > > > Just on the string method returns, or in the asserts as well? > > Oh oops, hadn't noticed this returned string (which makes NULL and nullptr both > incorrect), > > return std::string(); > > is the right thing for these (and otherwise for pointers always use nullptr over > NULL). Done.
Thanks, lvg, will do a final review after precursor CL lands is integrated with this, please ping then :-)
Precursor CL (https://codereview.chromium.org/2372663003/) has landed. PTAL. (I once again forgot to upload a patchset right after the merge. Sorry about that)
lg https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:319: return std::make_pair(base::Closure(), {} for multi-line body https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:332: changed_paths->insert(changed_path); For SPLIT macs I think you only want to delete the split keys that did change. Instead of using a separate set for what should be deleted one option is for ClearFromExternalStore() to take a raw DictionaryValue* to |changed_paths_mac| and operate on its keys. Ownership would remain in the hands of the post-write callback and as such it's okay for the pre-write callback to use a raw pointer. The same is true for the HashStoreContents (the pre-write callback can use a raw pointer since it's sequenced with the post-write callback and can therefore share state).
https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:319: return std::make_pair(base::Closure(), On 2016/09/29 15:07:18, gab wrote: > {} for multi-line body Done. https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:332: changed_paths->insert(changed_path); On 2016/09/29 15:07:18, gab wrote: > For SPLIT macs I think you only want to delete the split keys that did change. > > Instead of using a separate set for what should be deleted one option is for > ClearFromExternalStore() to take a raw DictionaryValue* to |changed_paths_mac| > and operate on its keys. Ownership would remain in the hands of the post-write > callback and as such it's okay for the pre-write callback to use a raw pointer. > > The same is true for the HashStoreContents (the pre-write callback can use a raw > pointer since it's sequenced with the post-write callback and can therefore > share state). Deleting only the split keys that did change would be quite involved, as we would need to look into that split preference and compare the values against old values (do we even have the old values?). The current logic in the folder is to write all split keys if the split preference changed, so I think it's ok for the clear to do the same. I changed the set<> to a raw DictionaryValue so we avoid constructing a new object and same for HashStoreContents. I'm a little bit confused by the pointer logic. If I inline the base::Unretained(foo.get()) instead of having intermediate raw pointers it crashes. Ex: base::Bind(&ClearFromExternalStore, base::Unretained(foo.get()), ... crashes while: Foo* raw_foo = foo.get(); base::Bind(&ClearFromExternalStore, base::Unretained(raw_foo), ... works
LGTM w/ one last const-nit =D!! https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:332: changed_paths->insert(changed_path); On 2016/09/30 14:24:44, proberge wrote: > On 2016/09/29 15:07:18, gab wrote: > > For SPLIT macs I think you only want to delete the split keys that did change. > > > > Instead of using a separate set for what should be deleted one option is for > > ClearFromExternalStore() to take a raw DictionaryValue* to |changed_paths_mac| > > and operate on its keys. Ownership would remain in the hands of the post-write > > callback and as such it's okay for the pre-write callback to use a raw > pointer. > > > > The same is true for the HashStoreContents (the pre-write callback can use a > raw > > pointer since it's sequenced with the post-write callback and can therefore > > share state). > > Deleting only the split keys that did change would be quite involved, as we > would need to look into that split preference and compare the values against old > values (do we even have the old values?). The current logic in the folder is to > write all split keys if the split preference changed, so I think it's ok for the > clear to do the same. Ah, somehow had thought we only rewrote the ones that had changes, given we rewrite all, then clearing all SGTM. > > I changed the set<> to a raw DictionaryValue so we avoid constructing a new > object and same for HashStoreContents. > > I'm a little bit confused by the pointer logic. If I inline the > base::Unretained(foo.get()) instead of having intermediate raw pointers it > crashes. Ex: > > base::Bind(&ClearFromExternalStore, base::Unretained(foo.get()), ... > crashes > > while: > > Foo* raw_foo = foo.get(); > base::Bind(&ClearFromExternalStore, base::Unretained(raw_foo), ... > works Ah, right :-), I was going to mention this in previous comment but thought I didn't have to... turns out it matters because of the std::make_pair call, here's what's happening: foo.get() gives you the underlying raw pointer stored in the unique_ptr. base::Passed() moves the unique_ptr into another one (making foo's underlying pointer null). Thus this would work if you did in sequence: base::Bind(base::Unretained(foo.get())...); base::Bind(base::Passed(foo)...); but because the Bind calls are inside a call to std::make_pair and because it's undefined in C++ (I think) which parameter gets evaluated first, if Passed(foo) gets evaluated first then |foo|'s underlying pointer was already moved when |foo.get()| occurs (which therefore binds a null raw ptr). Getting the raw pointer out first explicitly is one solution. Another one would be to do: // Do not inline |pre_write| and |post_write| to force base::Passed()'s evaluation to occur after // binding the matching raw pointers. const base::Closure pre_write = base::Bind(...); const base::Closure post_write = base::Bind(...); return std::make_pair(pre_write, post_write); In this situation I like your current approach better I think as getting the raw pointers explicitly first also makes for a nice place to add a comment stating why that's okay (as you did :-)). https://codereview.chromium.org/2204943002/diff/360001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.h (right): https://codereview.chromium.org/2204943002/diff/360001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:132: base::DictionaryValue* changed_paths_and_macs); const base::DictionaryValue*
Yay! https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.cc (right): https://codereview.chromium.org/2204943002/diff/340001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.cc:332: changed_paths->insert(changed_path); On 2016/09/30 16:04:21, gab wrote: > On 2016/09/30 14:24:44, proberge wrote: > > On 2016/09/29 15:07:18, gab wrote: > > > For SPLIT macs I think you only want to delete the split keys that did > change. > > > > > > Instead of using a separate set for what should be deleted one option is for > > > ClearFromExternalStore() to take a raw DictionaryValue* to > |changed_paths_mac| > > > and operate on its keys. Ownership would remain in the hands of the > post-write > > > callback and as such it's okay for the pre-write callback to use a raw > > pointer. > > > > > > The same is true for the HashStoreContents (the pre-write callback can use a > > raw > > > pointer since it's sequenced with the post-write callback and can therefore > > > share state). > > > > Deleting only the split keys that did change would be quite involved, as we > > would need to look into that split preference and compare the values against > old > > values (do we even have the old values?). The current logic in the folder is > to > > write all split keys if the split preference changed, so I think it's ok for > the > > clear to do the same. > > Ah, somehow had thought we only rewrote the ones that had changes, given we > rewrite all, then clearing all SGTM. > > > > > I changed the set<> to a raw DictionaryValue so we avoid constructing a new > > object and same for HashStoreContents. > > > > I'm a little bit confused by the pointer logic. If I inline the > > base::Unretained(foo.get()) instead of having intermediate raw pointers it > > crashes. Ex: > > > > base::Bind(&ClearFromExternalStore, base::Unretained(foo.get()), ... > > crashes > > > > while: > > > > Foo* raw_foo = foo.get(); > > base::Bind(&ClearFromExternalStore, base::Unretained(raw_foo), ... > > works > > Ah, right :-), I was going to mention this in previous comment but thought I > didn't have to... turns out it matters because of the std::make_pair call, > here's what's happening: > > foo.get() gives you the underlying raw pointer stored in the unique_ptr. > > base::Passed() moves the unique_ptr into another one (making foo's underlying > pointer null). > > Thus this would work if you did in sequence: > > base::Bind(base::Unretained(foo.get())...); > base::Bind(base::Passed(foo)...); > > but because the Bind calls are inside a call to std::make_pair and because it's > undefined in C++ (I think) which parameter gets evaluated first, if Passed(foo) > gets evaluated first then |foo|'s underlying pointer was already moved when > |foo.get()| occurs (which therefore binds a null raw ptr). > > Getting the raw pointer out first explicitly is one solution. > > Another one would be to do: > > // Do not inline |pre_write| and |post_write| to force base::Passed()'s > evaluation to occur after > // binding the matching raw pointers. > const base::Closure pre_write = base::Bind(...); > const base::Closure post_write = base::Bind(...); > return std::make_pair(pre_write, post_write); > > > In this situation I like your current approach better I think as getting the raw > pointers explicitly first also makes for a nice place to add a comment stating > why that's okay (as you did :-)). Great explanation! Thank you :) https://codereview.chromium.org/2204943002/diff/360001/components/user_prefs/... File components/user_prefs/tracked/pref_hash_filter.h (right): https://codereview.chromium.org/2204943002/diff/360001/components/user_prefs/... components/user_prefs/tracked/pref_hash_filter.h:132: base::DictionaryValue* changed_paths_and_macs); On 2016/09/30 16:04:21, gab wrote: > const base::DictionaryValue* Done.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2204943002/#ps380001 (title: "Add const qualifier to ClearFromExternalStore's DictionaryValue argument")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2204943002/#ps400001 (title: "Changes to make non-win browsertests compile, as they handle file paths and base::string16 differe…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2204943002/diff/380001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/380001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:51: constexpr base::char16 kRegistryTestPathPrefix[] = "registry" things don't make sense on non-Windows anyways, I'd say keep it as it was and ifdef it for OS_WIN instead.
The CQ bit was unchecked by gab@chromium.org
https://codereview.chromium.org/2204943002/diff/380001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/380001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:51: constexpr base::char16 kRegistryTestPathPrefix[] = On 2016/09/30 19:03:28, gab wrote: > "registry" things don't make sense on non-Windows anyways, I'd say keep it as it > was and ifdef it for OS_WIN instead. Great idea. Done.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2204943002/#ps440001 (title: "Put GetRegistryPathForTestProfile in the ifdef as well")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2204943002/diff/440001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/440001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:66: L"SOFTWARE\\Chromium\\PrefHashBrowserTest\\"; Move as local variable in GetRegistryPathForTestProfile()
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2204943002/diff/440001/chrome/browser/prefs/t... File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right): https://codereview.chromium.org/2204943002/diff/440001/chrome/browser/prefs/t... chrome/browser/prefs/tracked/pref_hash_browsertest.cc:66: L"SOFTWARE\\Chromium\\PrefHashBrowserTest\\"; On 2016/09/30 19:25:27, gab wrote: > Move as local variable in GetRegistryPathForTestProfile() Done.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2204943002/#ps460001 (title: "Inline registry path string into GetRegistryPathForTestProfile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240}
Message was sent while issue was closed.
A revert of this CL (patchset #24 id:460001) has been created in https://codereview.chromium.org/2391933003/ by guidou@chromium.org. The reason for reverting is: This CL is suspect of breaking WebRTC Windows bots. I will reland if it doesn't fix the problem. See, for example, https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/30... [4936:6236:0930/181348:INFO:webrtc_video_quality_browsertest.cc(245)] Running "C:\b\depot_tools\python276_bin\python.exe" -u "C:\b\c\b\Win7_Tester\src\third_party/webrtc/tools/compare_videos.py" --label=720p_VP9 --ref_video "C:\b\c\b\Win7_Tester\src\chrome\test\data\webrtc/resources\reference_video_1280x720_30fps.yuv" --test_video "C:\Users\CHROME~1.LAB\AppData\Local\Temp\scoped_dir4936_9527\captured_video.yuv" --frame_analyzer "C:\b\c\b\Win7_Tester\src\out\Release\frame_analyzer.exe" --yuv_frame_width 1280 --yuv_frame_height 720 --zxing_path "C:\b\c\b\Win7_Tester\src\chrome\test\data\webrtc/resources\tools\win\zxing.exe" --ffmpeg_path "C:\b\c\b\Win7_Tester\src\chrome\test\data\webrtc/resources\tools\win\ffmpeg.exe" --stats_file "C:\Users\CHROME~1.LAB\AppData\Local\Temp\scoped_dir4936_9527\stats.txt" [4936:6236:0930/181420:INFO:user_input_monitor_win.cc(171)] RegisterRawInputDevices() failed for RIDEV_REMOVE: The parameter is incorrect. (0x57) [4936:6236:0930/181421:FATAL:json_pref_store.cc(385)] Check failed: !has_pending_write_callbacks_. Backtrace: base::debug::StackTrace::StackTrace [0x02BA3CF7+23] logging::LogMessage::~LogMessage [0x02B5B061+49] JsonPrefStore::RegisterOnNextWriteSynchronousCallbacks [0x03668FDF+154] JsonPrefStore::SerializeData [0x03669601+179] base::ImportantFileWriter::DoScheduledWrite [0x02BE4B15+149] JsonPrefStore::CommitPendingWrite [0x0366803E+120] JsonPrefStore::~JsonPrefStore [0x03667DF8+21] scoped_refptr<TestingPrefStore>::Release [0x056591C1+23] SegregatedPrefStore::~SegregatedPrefStore [0x04359463+95] scoped_refptr<TestingPrefStore>::Release [0x056591C1+23] PrefService::~PrefService [0x03663ED9+141] syncable_prefs::PrefServiceSyncable::~PrefServiceSyncable [0x04088703+93] syncable_prefs::PrefServiceSyncable::`scalar deleting destructor' [0x04088714+11] ProfileImpl::~ProfileImpl [0x02C5706B+271] ProfileDestroyer::DestroyProfileWhenAppropriate [0x02D1A381+484] ProfileManager::ProfileInfo::~ProfileInfo [0x02C04D62+14] std::_Tree<std::_Tmap_traits<base::FilePath,linked_ptr<ProfileManager::ProfileInfo>,std::less<base::FilePath>,std::allocator<std::pair<base::FilePath const ,linked_ptr<ProfileManager::ProfileInfo> > >,0> >::_Erase [0x02B54889+41] std::_Tree<std::_Tmap_traits<base::FilePath,linked_ptr<ProfileManager::ProfileInfo>,std::less<base::FilePath>,std::allocator<std::pair<base::FilePath const ,linked_ptr<ProfileManager::ProfileInfo> > >,0> >::clear [0x02B548B2+13] std::_Tree<std::_Tmap_traits<base::FilePath,linked_ptr<ProfileManager::ProfileInfo>,std::less<base::FilePath>,std::allocator<std::pair<base::FilePath const ,linked_ptr<ProfileManager::ProfileInfo> > >,0> >::~_Tree<std::_Tmap_traits<base::FilePath,linked_p [0x02C04D0E+21] ProfileManager::~ProfileManager [0x02C04DAF+54] BrowserProcessImpl::StartTearDown [0x02DD70E9+458] ChromeBrowserMainParts::PostMainMessageLoopRun [0x02E333B0+304] content::BrowserMainLoop::ShutdownThreadsAndCleanUp [0x024E327A+350] content::BrowserMainRunnerImpl::Shutdown [0x024E4498+632] content::BrowserMain [0x024DFE41+139] content::RunNamedProcessTypeMain [0x02B46FAB+206] content::ContentMainRunnerImpl::Run [0x02B46EAC+274] content::ContentMain [0x02B46275+35] content::BrowserTestBase::SetUp [0x02F73565+964] InProcessBrowserTest::SetUp [0x02BF45D7+268] testing::internal::HandleExceptionsInMethodIfSupported<testing::TestCase,void> [0x033F7AB9+32] testing::Test::Run [0x033FEAA3+51] testing::TestCase::Run [0x033FEB79+133] testing::internal::UnitTestImpl::RunAllTests [0x033FEEF8+433] testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x033F7AFD+32] testing::UnitTest::Run [0x033FED22+133] base::TestSuite::Run [0x02BFD110+95] ChromeTestSuiteRunner::RunTestSuite [0x052456CE+40] content::LaunchTests [0x02F6D485+585] LaunchChromeTests [0x052456A1+49] main [0x052454BA+63] __scrt_common_main_seh [0x0520AD1F+249] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253) BaseThreadInitThunk [0x75A0338A+18] RtlInitializeExceptionChain [0x77899902+99] RtlInitializeExceptionChain [0x778998D5+54].
Message was sent while issue was closed.
A revert of this CL (patchset #24 id:460001) has been created in https://codereview.chromium.org/2396443002/ by guidou@chromium.org. The reason for reverting is: This CL is suspect of breaking WebRTC Windows bots. I will reland if it doesn't fix the problem. See, for example, https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/30... [4936:6236:0930/181348:INFO:webrtc_video_quality_browsertest.cc(245)] Running "C:\b\depot_tools\python276_bin\python.exe" -u "C:\b\c\b\Win7_Tester\src\third_party/webrtc/tools/compare_videos.py" --label=720p_VP9 --ref_video "C:\b\c\b\Win7_Tester\src\chrome\test\data\webrtc/resources\reference_video_1280x720_30fps.yuv" --test_video "C:\Users\CHROME~1.LAB\AppData\Local\Temp\scoped_dir4936_9527\captured_video.yuv" --frame_analyzer "C:\b\c\b\Win7_Tester\src\out\Release\frame_analyzer.exe" --yuv_frame_width 1280 --yuv_frame_height 720 --zxing_path "C:\b\c\b\Win7_Tester\src\chrome\test\data\webrtc/resources\tools\win\zxing.exe" --ffmpeg_path "C:\b\c\b\Win7_Tester\src\chrome\test\data\webrtc/resources\tools\win\ffmpeg.exe" --stats_file "C:\Users\CHROME~1.LAB\AppData\Local\Temp\scoped_dir4936_9527\stats.txt" [4936:6236:0930/181420:INFO:user_input_monitor_win.cc(171)] RegisterRawInputDevices() failed for RIDEV_REMOVE: The parameter is incorrect. (0x57) [4936:6236:0930/181421:FATAL:json_pref_store.cc(385)] Check failed: !has_pending_write_callbacks_. Backtrace: base::debug::StackTrace::StackTrace [0x02BA3CF7+23] logging::LogMessage::~LogMessage [0x02B5B061+49] JsonPrefStore::RegisterOnNextWriteSynchronousCallbacks [0x03668FDF+154] JsonPrefStore::SerializeData [0x03669601+179] base::ImportantFileWriter::DoScheduledWrite [0x02BE4B15+149] JsonPrefStore::CommitPendingWrite [0x0366803E+120] JsonPrefStore::~JsonPrefStore [0x03667DF8+21] scoped_refptr<TestingPrefStore>::Release [0x056591C1+23] SegregatedPrefStore::~SegregatedPrefStore [0x04359463+95] scoped_refptr<TestingPrefStore>::Release [0x056591C1+23] PrefService::~PrefService [0x03663ED9+141] syncable_prefs::PrefServiceSyncable::~PrefServiceSyncable [0x04088703+93] syncable_prefs::PrefServiceSyncable::`scalar deleting destructor' [0x04088714+11] ProfileImpl::~ProfileImpl [0x02C5706B+271] ProfileDestroyer::DestroyProfileWhenAppropriate [0x02D1A381+484] ProfileManager::ProfileInfo::~ProfileInfo [0x02C04D62+14] std::_Tree<std::_Tmap_traits<base::FilePath,linked_ptr<ProfileManager::ProfileInfo>,std::less<base::FilePath>,std::allocator<std::pair<base::FilePath const ,linked_ptr<ProfileManager::ProfileInfo> > >,0> >::_Erase [0x02B54889+41] std::_Tree<std::_Tmap_traits<base::FilePath,linked_ptr<ProfileManager::ProfileInfo>,std::less<base::FilePath>,std::allocator<std::pair<base::FilePath const ,linked_ptr<ProfileManager::ProfileInfo> > >,0> >::clear [0x02B548B2+13] std::_Tree<std::_Tmap_traits<base::FilePath,linked_ptr<ProfileManager::ProfileInfo>,std::less<base::FilePath>,std::allocator<std::pair<base::FilePath const ,linked_ptr<ProfileManager::ProfileInfo> > >,0> >::~_Tree<std::_Tmap_traits<base::FilePath,linked_p [0x02C04D0E+21] ProfileManager::~ProfileManager [0x02C04DAF+54] BrowserProcessImpl::StartTearDown [0x02DD70E9+458] ChromeBrowserMainParts::PostMainMessageLoopRun [0x02E333B0+304] content::BrowserMainLoop::ShutdownThreadsAndCleanUp [0x024E327A+350] content::BrowserMainRunnerImpl::Shutdown [0x024E4498+632] content::BrowserMain [0x024DFE41+139] content::RunNamedProcessTypeMain [0x02B46FAB+206] content::ContentMainRunnerImpl::Run [0x02B46EAC+274] content::ContentMain [0x02B46275+35] content::BrowserTestBase::SetUp [0x02F73565+964] InProcessBrowserTest::SetUp [0x02BF45D7+268] testing::internal::HandleExceptionsInMethodIfSupported<testing::TestCase,void> [0x033F7AB9+32] testing::Test::Run [0x033FEAA3+51] testing::TestCase::Run [0x033FEB79+133] testing::internal::UnitTestImpl::RunAllTests [0x033FEEF8+433] testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x033F7AFD+32] testing::UnitTest::Run [0x033FED22+133] base::TestSuite::Run [0x02BFD110+95] ChromeTestSuiteRunner::RunTestSuite [0x052456CE+40] content::LaunchTests [0x02F6D485+585] LaunchChromeTests [0x052456A1+49] main [0x052454BA+63] __scrt_common_main_seh [0x0520AD1F+249] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253) BaseThreadInitThunk [0x75A0338A+18] RtlInitializeExceptionChain [0x77899902+99] RtlInitializeExceptionChain [0x778998D5+54] .
Message was sent while issue was closed.
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240} ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240} ==========
PTAL
https://codereview.chromium.org/2204943002/diff/500001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/500001/components/prefs/json_... components/prefs/json_pref_store.cc:389: // as |writer_|'s WriteNow() will be called in sequence. I don't think this comment is required, it's never a problem to re-register |writer_|'s write callbacks as they are moved with the write task (so if they're overridden it's because they would never have been necessary anyways). Looking at this some more though I think the confusion here comes from the two bools |has_pending_successful_write_reply_| and |has_pending_write_callbacks_|. Tracking |has_pending_write_callbacks_| is weird because it only becomes false too late (i.e. on sequence, on the reply). I think we can do with a single bool |has_pending_write_reply| which merely indicates whether a PostWriteCallback task is scheduled currently. The DCHECK at the top of RegisterOnNextSuccessfulWriteReply() can use DCHECK(on_next_successful_write_reply_.is_null()) instead of its bool and the rest of the logic can be tweaked slightly to make this work just as well with a single bool.
https://codereview.chromium.org/2204943002/diff/500001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/500001/components/prefs/json_... components/prefs/json_pref_store.cc:389: // as |writer_|'s WriteNow() will be called in sequence. On 2016/10/04 19:05:32, gab wrote: > I don't think this comment is required, it's never a problem to re-register > |writer_|'s write callbacks as they are moved with the write task (so if they're > overridden it's because they would never have been necessary anyways). > > Looking at this some more though I think the confusion here comes from the two > bools |has_pending_successful_write_reply_| and |has_pending_write_callbacks_|. > > Tracking |has_pending_write_callbacks_| is weird because it only becomes false > too late (i.e. on sequence, on the reply). > > I think we can do with a single bool |has_pending_write_reply| which merely > indicates whether a PostWriteCallback task is scheduled currently. > > The DCHECK at the top of RegisterOnNextSuccessfulWriteReply() can use > DCHECK(on_next_successful_write_reply_.is_null()) instead of its bool and the > rest of the logic can be tweaked slightly to make this work just as well with a > single bool. Done. https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:334: base::Closure on_successful_write = on_next_successful_write_reply_; is this fine? I need to clear on_next_successful_reply_ before re-registering the callback with RegisterOnNextSuccessfulWriteReply
lgtm % nits https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:334: base::Closure on_successful_write = on_next_successful_write_reply_; On 2016/10/04 20:17:58, proberge wrote: > is this fine? I need to clear on_next_successful_reply_ before re-registering > the callback with RegisterOnNextSuccessfulWriteReply Yes except do: base::Closure on_successful_write = std::move(on_next_successful_write_reply_); instead. https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:394: callbacks.second, base::SequencedTaskRunnerHandle::Get())); nit: Just noticed while reading this closely that it's a bit weird that it reads in order: callbacks.first, RunOrScheduleNextSuccessfulWriteCallback, callbacks.second. WDYT of flipping the args of PostWriteCallback() so that |on_next_write_callback| is first?
https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:335: on_next_successful_write_reply_ = base::Closure(); And of course the move makes this line not be required, just to be clear.
https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... File components/prefs/json_pref_store.cc (right): https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:334: base::Closure on_successful_write = on_next_successful_write_reply_; On 2016/10/04 20:25:13, gab wrote: > On 2016/10/04 20:17:58, proberge wrote: > > is this fine? I need to clear on_next_successful_reply_ before re-registering > > the callback with RegisterOnNextSuccessfulWriteReply > > Yes except do: > > base::Closure on_successful_write = std::move(on_next_successful_write_reply_); > > instead. Done. https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:335: on_next_successful_write_reply_ = base::Closure(); On 2016/10/04 20:25:40, gab wrote: > And of course the move makes this line not be required, just to be clear. Done. https://codereview.chromium.org/2204943002/diff/520001/components/prefs/json_... components/prefs/json_pref_store.cc:394: callbacks.second, base::SequencedTaskRunnerHandle::Get())); On 2016/10/04 20:25:13, gab wrote: > nit: Just noticed while reading this closely that it's a bit weird that it reads > in order: callbacks.first, RunOrScheduleNextSuccessfulWriteCallback, > callbacks.second. > > WDYT of flipping the args of PostWriteCallback() so that > |on_next_write_callback| is first? Done.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2204943002/#ps540001 (title: "Apply comments from patch set 27")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240} ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240} ==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Cr-Commit-Position: refs/heads/master@{#422240} ========== to ========== Integrate registry_hash_store_contents with the rest of tracked prefs. This change adds Windows-only logic to the PrefHashFilter such that it verifies preferences against MACs stored in the registry. Unlike the current tracked preference logic, this extra check does NOT reset settings. To avoid inconsistent state with the MACs in secure_preferences, we clear the registry MACs before writing secure_preferences, and write the registry MACs after the file is successfully written. BUG=624858 Committed: https://crrev.com/4683dfcecfa72a73e00498d7c06fcf6716f8366c Committed: https://crrev.com/269fd091f46bbe9615024efe1ab6cfd44ca2547a Cr-Original-Commit-Position: refs/heads/master@{#422240} Cr-Commit-Position: refs/heads/master@{#422957} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/269fd091f46bbe9615024efe1ab6cfd44ca2547a Cr-Commit-Position: refs/heads/master@{#422957} |
