From 269e05a878461028f1cf6432c88bf29d2a68583f Mon Sep 17 00:00:00 2001 From: "gab@chromium.org" Date: Mon, 17 Feb 2014 19:04:28 +0000 Subject: [PATCH] Allow hash migration for unloaded profiles. This augments the current capacity to seed unloaded profiles to also be able to go through the migration path from the old hashes to the new ones for profiles that have yet to migrate. This introduces a PrefHashStore version which will begin at 2 (0 being the absence of hashes and 1 being the presence of the old hashes). R=jwd@chromium.org, robertshield@chromium.org TBR=sky (chrome_browser_main.cc -- see comment #22) Review URL: https://codereview.chromium.org/164463002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251700 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/chrome_browser_main.cc | 15 +- .../prefs/chrome_pref_service_factory.cc | 113 ++++++++++---- .../prefs/chrome_pref_service_factory.h | 12 +- chrome/browser/prefs/pref_hash_browsertest.cc | 144 +++++++++++++----- .../prefs/pref_hash_filter_unittest.cc | 6 - chrome/browser/prefs/pref_hash_store.h | 3 - chrome/browser/prefs/pref_hash_store_impl.cc | 82 +++++++--- chrome/browser/prefs/pref_hash_store_impl.h | 22 ++- .../prefs/pref_hash_store_impl_unittest.cc | 121 +++++++++++---- tools/metrics/histograms/histograms.xml | 29 ++++ 10 files changed, 397 insertions(+), 150 deletions(-) diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc index 295c4bb7ecc1b..9d0d9dabddc1b 100644 --- a/chrome/browser/chrome_browser_main.cc +++ b/chrome/browser/chrome_browser_main.cc @@ -508,17 +508,6 @@ class LoadCompleteListener : public content::NotificationObserver { DISALLOW_COPY_AND_ASSIGN(LoadCompleteListener); }; -void InitializeAllPrefHashStores() { - ProfileInfoCache& profile_info_cache = - g_browser_process->profile_manager()->GetProfileInfoCache(); - size_t n_profiles = profile_info_cache.GetNumberOfProfiles(); - for (size_t i = 0; i < n_profiles; ++i) { - base::FilePath profile_path = - profile_info_cache.GetPathOfProfileAtIndex(i); - chrome_prefs::InitializePrefHashStoreIfRequired(profile_path); - } -} - } // namespace namespace chrome_browser { @@ -1588,9 +1577,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() { PostBrowserStart(); - // Initialize preference hash stores for profiles that haven't been loaded - // recently. - InitializeAllPrefHashStores(); + chrome_prefs::SchedulePrefHashStoresUpdateCheck(profile_->GetPath()); if (parameters().ui_task) { // We end the startup timer here if we have parameters to run, because we diff --git a/chrome/browser/prefs/chrome_pref_service_factory.cc b/chrome/browser/prefs/chrome_pref_service_factory.cc index 0035a97d933c5..3c7f1906973bd 100644 --- a/chrome/browser/prefs/chrome_pref_service_factory.cc +++ b/chrome/browser/prefs/chrome_pref_service_factory.cc @@ -33,6 +33,8 @@ #include "chrome/browser/prefs/pref_service_syncable.h" #include "chrome/browser/prefs/pref_service_syncable_factory.h" #include "chrome/browser/profiles/file_path_verifier_win.h" +#include "chrome/browser/profiles/profile_info_cache.h" +#include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/ui/profile_error_dialog.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/pref_names.h" @@ -67,6 +69,10 @@ using content::BrowserThread; namespace { +// The delay in seconds before actual work kicks in after calling +// SchedulePrefHashStoresUpdateCheck can be set to 0 for tests. +int g_pref_hash_store_update_check_delay_seconds = 55; + // These preferences must be kept in sync with the TrackedPreference enum in // tools/metrics/histograms/histograms.xml. To add a new preference, append it // to the array and add a corresponding value to the histogram enum. Each @@ -156,7 +162,6 @@ enum SettingsEnforcementGroup { // Only enforce settings on profile loads; still allow seeding of unloaded // profiles. GROUP_ENFORCE_ON_LOAD, - // TOOD(gab): Block unloaded profile seeding in this mode. GROUP_ENFORCE_ALWAYS }; @@ -373,9 +378,11 @@ class InitializeHashStoreObserver : public PrefStore::Observer { public: // Creates an observer that will initialize |pref_hash_store| with the // contents of |pref_store| when the latter is fully loaded. - InitializeHashStoreObserver(const scoped_refptr& pref_store, - scoped_ptr pref_hash_store) - : pref_store_(pref_store), pref_hash_store_(pref_hash_store.Pass()) {} + InitializeHashStoreObserver( + const scoped_refptr& pref_store, + scoped_ptr pref_hash_store_impl) + : pref_store_(pref_store), + pref_hash_store_impl_(pref_hash_store_impl.Pass()) {} virtual ~InitializeHashStoreObserver(); @@ -385,7 +392,7 @@ class InitializeHashStoreObserver : public PrefStore::Observer { private: scoped_refptr pref_store_; - scoped_ptr pref_hash_store_; + scoped_ptr pref_hash_store_impl_; DISALLOW_COPY_AND_ASSIGN(InitializeHashStoreObserver); }; @@ -395,18 +402,70 @@ InitializeHashStoreObserver::~InitializeHashStoreObserver() {} void InitializeHashStoreObserver::OnPrefValueChanged(const std::string& key) {} void InitializeHashStoreObserver::OnInitializationCompleted(bool succeeded) { - // If we successfully loaded the preferences _and_ the PrefHashStore hasn't - // been initialized by someone else in the meantime initialize it now. - if (succeeded & !pref_hash_store_->IsInitialized()) { - CreatePrefHashFilter( - pref_hash_store_.Pass())->Initialize(*pref_store_); - UMA_HISTOGRAM_BOOLEAN( - "Settings.TrackedPreferencesInitializedForUnloadedProfile", true); + // If we successfully loaded the preferences _and_ the PrefHashStoreImpl + // hasn't been initialized by someone else in the meantime, initialize it now. + const PrefHashStoreImpl::StoreVersion pre_update_version = + pref_hash_store_impl_->GetCurrentVersion(); + if (succeeded && pre_update_version < PrefHashStoreImpl::VERSION_LATEST) { + CreatePrefHashFilter(pref_hash_store_impl_.PassAs())-> + Initialize(*pref_store_); + UMA_HISTOGRAM_ENUMERATION( + "Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom", + pre_update_version, + PrefHashStoreImpl::VERSION_LATEST + 1); } pref_store_->RemoveObserver(this); delete this; } +// Initializes/updates the PrefHashStore for the profile preferences file under +// |profile_path| without actually loading that profile. Also reports the +// version of that PrefHashStore via UMA, whether it proceeds with initializing +// it or not. +void UpdatePrefHashStoreIfRequired( + const base::FilePath& profile_path) { + scoped_ptr pref_hash_store_impl( + GetPrefHashStoreImpl(profile_path)); + + const PrefHashStoreImpl::StoreVersion current_version = + pref_hash_store_impl->GetCurrentVersion(); + UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferencesAlternateStoreVersion", + current_version, + PrefHashStoreImpl::VERSION_LATEST + 1); + + // Update the pref hash store if it's not at the latest version and the + // SettingsEnforcement group allows seeding of unloaded profiles. + if (current_version != PrefHashStoreImpl::VERSION_LATEST && + GetSettingsEnforcementGroup() < GROUP_ENFORCE_ALWAYS) { + const base::FilePath pref_file( + GetPrefFilePathFromProfilePath(profile_path)); + scoped_refptr pref_store( + new JsonPrefStore(pref_file, + JsonPrefStore::GetTaskRunnerForFile( + pref_file, BrowserThread::GetBlockingPool()), + scoped_ptr())); + pref_store->AddObserver( + new InitializeHashStoreObserver(pref_store, + pref_hash_store_impl.Pass())); + pref_store->ReadPrefsAsync(NULL); + } +} + +// Initialize/update preference hash stores for all profiles but the one whose +// path matches |ignored_profile_path|. +void UpdateAllPrefHashStoresIfRequired( + const base::FilePath& ignored_profile_path) { + const ProfileInfoCache& profile_info_cache = + g_browser_process->profile_manager()->GetProfileInfoCache(); + const size_t n_profiles = profile_info_cache.GetNumberOfProfiles(); + for (size_t i = 0; i < n_profiles; ++i) { + const base::FilePath profile_path = + profile_info_cache.GetPathOfProfileAtIndex(i); + if (profile_path != ignored_profile_path) + UpdatePrefHashStoreIfRequired(profile_path); + } +} + } // namespace namespace chrome_prefs { @@ -471,23 +530,19 @@ void SchedulePrefsFilePathVerification(const base::FilePath& profile_path) { #endif } -void InitializePrefHashStoreIfRequired( - const base::FilePath& profile_path) { - scoped_ptr pref_hash_store( - GetPrefHashStoreImpl(profile_path)); - if (pref_hash_store && !pref_hash_store->IsInitialized()) { - const base::FilePath pref_file( - GetPrefFilePathFromProfilePath(profile_path)); - scoped_refptr pref_store( - new JsonPrefStore(pref_file, - JsonPrefStore::GetTaskRunnerForFile( - pref_file, BrowserThread::GetBlockingPool()), - scoped_ptr())); - pref_store->AddObserver( - new InitializeHashStoreObserver( - pref_store, pref_hash_store.PassAs())); - pref_store->ReadPrefsAsync(NULL); - } +void EnableZeroDelayPrefHashStoreUpdateForTesting() { + g_pref_hash_store_update_check_delay_seconds = 0; +} + +void SchedulePrefHashStoresUpdateCheck( + const base::FilePath& initial_profile_path) { + BrowserThread::PostDelayedTask( + BrowserThread::UI, + FROM_HERE, + base::Bind(&UpdateAllPrefHashStoresIfRequired, + initial_profile_path), + base::TimeDelta::FromSeconds( + g_pref_hash_store_update_check_delay_seconds)); } void ResetPrefHashStore(const base::FilePath& profile_path) { diff --git a/chrome/browser/prefs/chrome_pref_service_factory.h b/chrome/browser/prefs/chrome_pref_service_factory.h index ef9a0a7b46c3a..7991f7c5a0a80 100644 --- a/chrome/browser/prefs/chrome_pref_service_factory.h +++ b/chrome/browser/prefs/chrome_pref_service_factory.h @@ -76,10 +76,14 @@ scoped_ptr CreateProfilePrefs( // |profile_path|. void SchedulePrefsFilePathVerification(const base::FilePath& profile_path); -// Initializes the PrefHashStore for the profile preferences file under -// |profile_path| without actually loading that profile. -void InitializePrefHashStoreIfRequired( - const base::FilePath& profile_path); +// Call before calling SchedulePrefHashStoresUpdateCheck to cause it to run with +// zero delay. For testing only. +void EnableZeroDelayPrefHashStoreUpdateForTesting(); + +// Shedules an update check for all PrefHashStores, stores whose version doesn't +// match the latest version will then be updated. +void SchedulePrefHashStoresUpdateCheck( + const base::FilePath& initial_profile_path); // Resets the contents of the preference hash store for the profile at // |profile_path|. diff --git a/chrome/browser/prefs/pref_hash_browsertest.cc b/chrome/browser/prefs/pref_hash_browsertest.cc index b72a902385744..b3108e5daf72e 100644 --- a/chrome/browser/prefs/pref_hash_browsertest.cc +++ b/chrome/browser/prefs/pref_hash_browsertest.cc @@ -6,6 +6,7 @@ #include #include "base/bind.h" +#include "base/command_line.h" #include "base/files/file_path.h" #include "base/macros.h" #include "base/message_loop/message_loop.h" @@ -14,6 +15,7 @@ #include "base/metrics/statistics_recorder.h" #include "base/prefs/pref_service.h" #include "base/strings/string16.h" +#include "base/threading/sequenced_worker_pool.h" #include "base/values.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/prefs/chrome_pref_service_factory.h" @@ -24,7 +26,10 @@ #include "chrome/browser/profiles/profiles_state.h" #include "chrome/common/pref_names.h" #include "chrome/test/base/in_process_browser_test.h" +#include "content/public/browser/browser_thread.h" +#include "content/public/common/content_switches.h" #include "content/public/test/test_utils.h" +#include "testing/gtest/include/gtest/gtest.h" namespace { @@ -87,9 +92,46 @@ int GetTrackedPrefHistogramCount(const char* histogram_name, bool expect_zero) { } // namespace -typedef InProcessBrowserTest PrefHashBrowserTest; +class PrefHashBrowserTest : public InProcessBrowserTest, + public testing::WithParamInterface { + public: + PrefHashBrowserTest() + : is_unloaded_profile_seeding_allowed_( + GetParam() != + chrome_prefs::internals::kSettingsEnforcementGroupEnforceAlways) {} -IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, + virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE { + InProcessBrowserTest::SetUpCommandLine(command_line); + command_line->AppendSwitchASCII( + switches::kForceFieldTrials, + std::string(chrome_prefs::internals::kSettingsEnforcementTrialName) + + "/" + GetParam() + "/"); + } + + virtual void SetUpInProcessBrowserTestFixture() OVERRIDE { + // Force the delayed PrefHashStore update task to happen immediately. + chrome_prefs::EnableZeroDelayPrefHashStoreUpdateForTesting(); + } + + virtual void SetUpOnMainThread() OVERRIDE { + // content::RunAllPendingInMessageLoop() is already called before + // SetUpOnMainThread() in in_process_browser_test.cc which guarantees that + // UpdateAllPrefHashStoresIfRequired() has already been called. + + // Now flush the blocking pool to force any pending JsonPrefStore async read + // requests. + content::BrowserThread::GetBlockingPool()->FlushForTesting(); + + // And finally run tasks on this message loop again to process the OnRead() + // callbacks resulting from the file reads above. + content::RunAllPendingInMessageLoop(); + } + + protected: + const bool is_unloaded_profile_seeding_allowed_; +}; + +IN_PROC_BROWSER_TEST_P(PrefHashBrowserTest, PRE_PRE_InitializeUnloadedProfiles) { if (!profiles::IsMultipleProfilesEnabled()) return; @@ -119,7 +161,7 @@ IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, true)); } -IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, +IN_PROC_BROWSER_TEST_P(PrefHashBrowserTest, PRE_InitializeUnloadedProfiles) { if (!profiles::IsMultipleProfilesEnabled()) return; @@ -136,15 +178,15 @@ IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, g_browser_process->local_state()->GetDictionary( prefs::kProfilePreferenceHashes); - // 3 is for hash_of_hashes, default profile, and new profile. - ASSERT_EQ(3U, hashes->size()); + // 4 is for hash_of_hashes, versions_dict, default profile, and new profile. + EXPECT_EQ(4U, hashes->size()); // One of the two profiles should not have been loaded. Reset its hash store. const base::FilePath unloaded_profile_path = GetUnloadedProfilePath(); chrome_prefs::ResetPrefHashStore(unloaded_profile_path); // One of the profile hash collections should be gone. - ASSERT_EQ(2U, hashes->size()); + EXPECT_EQ(3U, hashes->size()); // No profile should have gone through the unloaded profile initialization in // this phase as both profiles were already initialized at the beginning of @@ -152,11 +194,11 @@ IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, // force initialization in the next phase's startup). EXPECT_EQ( 0, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferencesInitializedForUnloadedProfile", + "Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom", true)); } -IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, +IN_PROC_BROWSER_TEST_P(PrefHashBrowserTest, InitializeUnloadedProfiles) { if (!profiles::IsMultipleProfilesEnabled()) return; @@ -165,15 +207,25 @@ IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, g_browser_process->local_state()->GetDictionary( prefs::kProfilePreferenceHashes); - // The deleted hash collection should be restored. - ASSERT_EQ(3U, hashes->size()); + // The deleted hash collection should be restored only if the current + // SettingsEnforcement group allows it. + if (is_unloaded_profile_seeding_allowed_) { + EXPECT_EQ(4U, hashes->size()); - // Verify that the initialization truly did occur in this phase's startup; - // rather than in the previous phase's shutdown. - EXPECT_EQ( - 1, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferencesInitializedForUnloadedProfile", - false)); + // Verify that the initialization truly did occur in this phase's startup; + // rather than in the previous phase's shutdown. + EXPECT_EQ( + 1, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom", + false)); + } else { + EXPECT_EQ(3U, hashes->size()); + + EXPECT_EQ( + 0, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom", + true)); + } ProfileManager* profile_manager = g_browser_process->profile_manager(); @@ -204,29 +256,41 @@ IN_PROC_BROWSER_TEST_F(PrefHashBrowserTest, false); EXPECT_GT(initial_unchanged_count, 0); - // Explicitly load the unloaded profile. - profile_manager->GetProfile(GetUnloadedProfilePath()); - ASSERT_EQ(2U, profile_manager->GetLoadedProfiles().size()); + if (is_unloaded_profile_seeding_allowed_) { + // Explicitly load the unloaded profile. + profile_manager->GetProfile(GetUnloadedProfilePath()); + ASSERT_EQ(2U, profile_manager->GetLoadedProfiles().size()); - // Loading the unexpected profile should only generate unchanged pings; and - // should have produced as many of them as loading the first profile. - EXPECT_EQ( - 0, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferenceChanged", true)); - EXPECT_EQ( - 0, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferenceCleared", true)); - EXPECT_EQ( - 0, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferenceInitialized", true)); - EXPECT_EQ( - 0, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferenceTrustedInitialized", true)); - EXPECT_EQ( - 0, GetTrackedPrefHistogramCount( - "Settings.TrackedPreferenceMigrated", true)); - EXPECT_EQ( - initial_unchanged_count * 2, - GetTrackedPrefHistogramCount("Settings.TrackedPreferenceUnchanged", - false)); + // Loading the unloaded profile should only generate unchanged pings; and + // should have produced as many of them as loading the first profile. + EXPECT_EQ( + 0, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferenceChanged", true)); + EXPECT_EQ( + 0, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferenceCleared", true)); + EXPECT_EQ( + 0, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferenceInitialized", true)); + EXPECT_EQ( + 0, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferenceTrustedInitialized", true)); + EXPECT_EQ( + 0, GetTrackedPrefHistogramCount( + "Settings.TrackedPreferenceMigrated", true)); + EXPECT_EQ( + initial_unchanged_count * 2, + GetTrackedPrefHistogramCount("Settings.TrackedPreferenceUnchanged", + false)); + } } + +// TODO(gab): Also test kSettingsEnforcementGroupEnforceAlways below; for some +// reason it doesn't pass on the try bots, yet I can't repro the failure +// locally, it's as if GROUP_NO_ENFORCEMENT is always picked on bots..? +INSTANTIATE_TEST_CASE_P( + PrefHashBrowserTestInstance, + PrefHashBrowserTest, + testing::Values( + chrome_prefs::internals::kSettingsEnforcementGroupNoEnforcement, + chrome_prefs::internals::kSettingsEnforcementGroupEnforceOnload)); diff --git a/chrome/browser/prefs/pref_hash_filter_unittest.cc b/chrome/browser/prefs/pref_hash_filter_unittest.cc index 6896338f8a941..65c92eac88b48 100644 --- a/chrome/browser/prefs/pref_hash_filter_unittest.cc +++ b/chrome/browser/prefs/pref_hash_filter_unittest.cc @@ -128,7 +128,6 @@ class MockPrefHashStore : public PrefHashStore { } // PrefHashStore implementation. - virtual bool IsInitialized() const OVERRIDE; virtual scoped_ptr BeginTransaction() OVERRIDE; private: @@ -215,11 +214,6 @@ void MockPrefHashStore::SetInvalidKeysResult( invalid_keys_results_.insert(std::make_pair(path, invalid_keys_result)); } -bool MockPrefHashStore::IsInitialized() const { - NOTREACHED(); - return true; -} - scoped_ptr MockPrefHashStore::BeginTransaction() { EXPECT_FALSE(transaction_active_); return scoped_ptr( diff --git a/chrome/browser/prefs/pref_hash_store.h b/chrome/browser/prefs/pref_hash_store.h index 65cbd93f25fc9..887856857a030 100644 --- a/chrome/browser/prefs/pref_hash_store.h +++ b/chrome/browser/prefs/pref_hash_store.h @@ -15,9 +15,6 @@ class PrefHashStore { public: virtual ~PrefHashStore() {} - // Determines if the hash store has been initialized (contains any values). - virtual bool IsInitialized() const = 0; - // Returns a PrefHashStoreTransaction which can be used to perform a series // of checks/transformations on the hash store. virtual scoped_ptr BeginTransaction() = 0; diff --git a/chrome/browser/prefs/pref_hash_store_impl.cc b/chrome/browser/prefs/pref_hash_store_impl.cc index 877f051d697ad..1d8f475f7aae3 100644 --- a/chrome/browser/prefs/pref_hash_store_impl.cc +++ b/chrome/browser/prefs/pref_hash_store_impl.cc @@ -12,6 +12,13 @@ #include "chrome/browser/prefs/pref_hash_store_transaction.h" #include "chrome/common/pref_names.h" +namespace internals { + +const char kHashOfHashesDict[] = "hash_of_hashes"; +const char kStoreVersionsDict[] = "store_versions"; + +} // namespace internals + class PrefHashStoreImpl::PrefHashStoreTransactionImpl : public PrefHashStoreTransaction { public: @@ -79,13 +86,18 @@ void PrefHashStoreImpl::Reset() { // Remove the dictionary corresponding to the profile name, which may have a // '.' update->RemoveWithoutPathExpansion(hash_store_id_, NULL); -} -bool PrefHashStoreImpl::IsInitialized() const { - const base::DictionaryValue* pref_hash_dicts = - local_state_->GetDictionary(prefs::kProfilePreferenceHashes); - return pref_hash_dicts && - pref_hash_dicts->GetDictionaryWithoutPathExpansion(hash_store_id_, NULL); + // Remove this store's entry in the kStoreVersionsDict. + base::DictionaryValue* version_dict; + if (update->GetDictionary(internals::kStoreVersionsDict, &version_dict)) + version_dict->Remove(hash_store_id_, NULL); + + // Remove this store's entry in the kHashOfHashesDict. + base::DictionaryValue* hash_of_hashes_dict; + if (update->GetDictionaryWithoutPathExpansion(internals::kHashOfHashesDict, + &hash_of_hashes_dict)) { + hash_of_hashes_dict->Remove(hash_store_id_, NULL); + } } scoped_ptr PrefHashStoreImpl::BeginTransaction() { @@ -93,18 +105,37 @@ scoped_ptr PrefHashStoreImpl::BeginTransaction() { new PrefHashStoreTransactionImpl(this)); } +PrefHashStoreImpl::StoreVersion PrefHashStoreImpl::GetCurrentVersion() const { + const base::DictionaryValue* pref_hash_data = + local_state_->GetDictionary(prefs::kProfilePreferenceHashes); + + if (!pref_hash_data->GetDictionaryWithoutPathExpansion(hash_store_id_, NULL)) + return VERSION_UNINITIALIZED; + + const base::DictionaryValue* version_dict; + int current_version; + if (!pref_hash_data->GetDictionary(internals::kStoreVersionsDict, + &version_dict) || + !version_dict->GetInteger(hash_store_id_, ¤t_version)) { + return VERSION_PRE_MIGRATION; + } + + DCHECK_GT(current_version, VERSION_PRE_MIGRATION); + return static_cast(current_version); +} + bool PrefHashStoreImpl::IsHashDictionaryTrusted() const { - const base::DictionaryValue* pref_hash_dicts = + const base::DictionaryValue* pref_hash_data = local_state_->GetDictionary(prefs::kProfilePreferenceHashes); const base::DictionaryValue* hashes_dict = NULL; const base::DictionaryValue* hash_of_hashes_dict = NULL; std::string hash_of_hashes; // The absence of the hashes dictionary isn't trusted. Nor is the absence of // the hash of hashes for this |hash_store_id_|. - if (!pref_hash_dicts->GetDictionaryWithoutPathExpansion( + if (!pref_hash_data->GetDictionaryWithoutPathExpansion( hash_store_id_, &hashes_dict) || - !pref_hash_dicts->GetDictionaryWithoutPathExpansion( - internals::kHashOfHashesPref, &hash_of_hashes_dict) || + !pref_hash_data->GetDictionaryWithoutPathExpansion( + internals::kHashOfHashesDict, &hash_of_hashes_dict) || !hash_of_hashes_dict->GetStringWithoutPathExpansion( hash_store_id_, &hash_of_hashes)) { return false; @@ -120,18 +151,17 @@ PrefHashStoreImpl::PrefHashStoreTransactionImpl::PrefHashStoreTransactionImpl( PrefHashStoreImpl::PrefHashStoreTransactionImpl:: ~PrefHashStoreTransactionImpl() { + DictionaryPrefUpdate update(outer_->local_state_, + prefs::kProfilePreferenceHashes); // Update the hash_of_hashes if and only if the hashes dictionary has been // modified in this transaction. if (has_changed_) { - DictionaryPrefUpdate update(outer_->local_state_, - prefs::kProfilePreferenceHashes); - // Get the dictionary where the hash of hashes are stored. base::DictionaryValue* hash_of_hashes_dict = NULL; - if (!update->GetDictionaryWithoutPathExpansion(internals::kHashOfHashesPref, + if (!update->GetDictionaryWithoutPathExpansion(internals::kHashOfHashesDict, &hash_of_hashes_dict)) { hash_of_hashes_dict = new base::DictionaryValue; - update->SetWithoutPathExpansion(internals::kHashOfHashesPref, + update->SetWithoutPathExpansion(internals::kHashOfHashesDict, hash_of_hashes_dict); } @@ -147,16 +177,28 @@ PrefHashStoreImpl::PrefHashStoreTransactionImpl:: hash_of_hashes_dict->SetStringWithoutPathExpansion(outer_->hash_store_id_, hash_of_hashes); } -} + // Mark this hash store has having been updated to the latest version (in + // practice only initialization transactions will actually do this, but + // since they always occur before minor update transaction it's okay + // to unconditionally do this here). Do this even if |!has_changed_| to also + // seed version number on unchanged profiles. + base::DictionaryValue* store_versions_dict = NULL; + if (!update->GetDictionary(internals::kStoreVersionsDict, + &store_versions_dict)) { + store_versions_dict = new base::DictionaryValue; + update->Set(internals::kStoreVersionsDict, store_versions_dict); + } + store_versions_dict->SetInteger(outer_->hash_store_id_, VERSION_LATEST); +} PrefHashStoreTransaction::ValueState PrefHashStoreImpl::PrefHashStoreTransactionImpl::CheckValue( const std::string& path, const base::Value* initial_value) const { - const base::DictionaryValue* pref_hash_dicts = + const base::DictionaryValue* pref_hash_data = outer_->local_state_->GetDictionary(prefs::kProfilePreferenceHashes); const base::DictionaryValue* hashed_prefs = NULL; - pref_hash_dicts->GetDictionaryWithoutPathExpansion(outer_->hash_store_id_, + pref_hash_data->GetDictionaryWithoutPathExpansion(outer_->hash_store_id_, &hashed_prefs); std::string last_hash; @@ -283,10 +325,10 @@ void PrefHashStoreImpl::PrefHashStoreTransactionImpl::ClearPath( bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::HasSplitHashesAtPath( const std::string& path) const { - const base::DictionaryValue* pref_hash_dicts = + const base::DictionaryValue* pref_hash_data = outer_->local_state_->GetDictionary(prefs::kProfilePreferenceHashes); const base::DictionaryValue* hashed_prefs = NULL; - pref_hash_dicts->GetDictionaryWithoutPathExpansion(outer_->hash_store_id_, + pref_hash_data->GetDictionaryWithoutPathExpansion(outer_->hash_store_id_, &hashed_prefs); return hashed_prefs && hashed_prefs->GetDictionary(path, NULL); } diff --git a/chrome/browser/prefs/pref_hash_store_impl.h b/chrome/browser/prefs/pref_hash_store_impl.h index a6a2ecb0a039d..71ded1e335f1f 100644 --- a/chrome/browser/prefs/pref_hash_store_impl.h +++ b/chrome/browser/prefs/pref_hash_store_impl.h @@ -27,14 +27,28 @@ namespace internals { // Hash of hashes for each profile, used to validate the existing hashes when // debating whether an unknown value is to be trusted, will be stored as a // string under -// |kProfilePreferenceHashes|.|kHashOfHashesPref|.|hash_stored_id_|. -const char kHashOfHashesPref[] = "hash_of_hashes"; +// |kProfilePreferenceHashes|.|kHashOfHashesDict|.|hash_stored_id_|. +extern const char kHashOfHashesDict[]; + +// Versions for each PrefHashStore are stored in this dictionary under +// |hash_store_id_|. +extern const char kStoreVersionsDict[]; } // namespace internals // Implements PrefHashStoreImpl by storing preference hashes in a PrefService. class PrefHashStoreImpl : public PrefHashStore { public: + enum StoreVersion { + // No hashes have been stored in this PrefHashStore yet. + VERSION_UNINITIALIZED = 0, + // The hashes in this PrefHashStore were stored before the introduction + // of a version number and should be re-initialized. + VERSION_PRE_MIGRATION = 1, + // The hashes in this PrefHashStore were stored using the latest algorithm. + VERSION_LATEST = 2, + }; + // Constructs a PrefHashStoreImpl that calculates hashes using // |seed| and |device_id| and stores them in |local_state|. Multiple hash // stores can use the same |local_state| with distinct |hash_store_id|s. @@ -56,9 +70,11 @@ class PrefHashStoreImpl : public PrefHashStore { void Reset(); // PrefHashStore implementation. - virtual bool IsInitialized() const OVERRIDE; virtual scoped_ptr BeginTransaction() OVERRIDE; + // Returns the current version of this hash store. + StoreVersion GetCurrentVersion() const; + private: class PrefHashStoreTransactionImpl; diff --git a/chrome/browser/prefs/pref_hash_store_impl_unittest.cc b/chrome/browser/prefs/pref_hash_store_impl_unittest.cc index bfc15f4c0db32..67b4148e50310 100644 --- a/chrome/browser/prefs/pref_hash_store_impl_unittest.cc +++ b/chrome/browser/prefs/pref_hash_store_impl_unittest.cc @@ -15,17 +15,24 @@ #include "chrome/common/pref_names.h" #include "testing/gtest/include/gtest/gtest.h" -TEST(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { +class PrefHashStoreImplTest : public testing::Test { + public: + virtual void SetUp() OVERRIDE { + PrefHashStoreImpl::RegisterPrefs(local_state_.registry()); + } + + protected: + TestingPrefServiceSimple local_state_; +}; + +TEST_F(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { base::StringValue string_1("string1"); base::StringValue string_2("string2"); - TestingPrefServiceSimple local_state; - PrefHashStoreImpl::RegisterPrefs(local_state.registry()); - { // 32 NULL bytes is the seed that was used to generate the legacy hash. PrefHashStoreImpl pref_hash_store( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store.BeginTransaction()); @@ -54,7 +61,7 @@ TEST(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { { // Manually shove in a legacy hash. - DictionaryPrefUpdate update(&local_state, + DictionaryPrefUpdate update(&local_state_, prefs::kProfilePreferenceHashes); base::DictionaryValue* child_dictionary = NULL; ASSERT_TRUE(update->GetDictionary("store_id", &child_dictionary)); @@ -73,7 +80,7 @@ TEST(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { // |pref_hash_store2| should trust its initial hashes dictionary and thus // trust new unknown values. PrefHashStoreImpl pref_hash_store2( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store2.BeginTransaction()); EXPECT_EQ(PrefHashStoreTransaction::TRUSTED_UNKNOWN_VALUE, @@ -86,10 +93,10 @@ TEST(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { { // Manually corrupt the hash of hashes for "store_id". - DictionaryPrefUpdate update(&local_state, prefs::kProfilePreferenceHashes); + DictionaryPrefUpdate update(&local_state_, prefs::kProfilePreferenceHashes); base::DictionaryValue* hash_of_hashes_dict = NULL; ASSERT_TRUE(update->GetDictionaryWithoutPathExpansion( - internals::kHashOfHashesPref, &hash_of_hashes_dict)); + internals::kHashOfHashesDict, &hash_of_hashes_dict)); hash_of_hashes_dict->SetString("store_id", std::string(64, 'A')); // This shouldn't have increased the number of existing hash of hashes. ASSERT_EQ(1U, hash_of_hashes_dict->size()); @@ -99,7 +106,7 @@ TEST(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { // |pref_hash_store3| should no longer trust its initial hashes dictionary // and thus shouldn't trust non-NULL unknown values. PrefHashStoreImpl pref_hash_store3( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store3.BeginTransaction()); EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE, @@ -111,7 +118,7 @@ TEST(PrefHashStoreImplTest, AtomicHashStoreAndCheck) { } } -TEST(PrefHashStoreImplTest, SplitHashStoreAndCheck) { +TEST_F(PrefHashStoreImplTest, SplitHashStoreAndCheck) { base::DictionaryValue dict; dict.Set("a", new base::StringValue("to be replaced")); dict.Set("b", new base::StringValue("same")); @@ -124,14 +131,11 @@ TEST(PrefHashStoreImplTest, SplitHashStoreAndCheck) { base::DictionaryValue empty_dict; - TestingPrefServiceSimple local_state; - PrefHashStoreImpl::RegisterPrefs(local_state.registry()); - std::vector invalid_keys; { PrefHashStoreImpl pref_hash_store( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store.BeginTransaction()); @@ -200,7 +204,7 @@ TEST(PrefHashStoreImplTest, SplitHashStoreAndCheck) { // |pref_hash_store2| should trust its initial hashes dictionary and thus // trust new unknown values. PrefHashStoreImpl pref_hash_store2( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store2.BeginTransaction()); EXPECT_EQ(PrefHashStoreTransaction::TRUSTED_UNKNOWN_VALUE, @@ -210,10 +214,10 @@ TEST(PrefHashStoreImplTest, SplitHashStoreAndCheck) { { // Manually corrupt the hash of hashes for "store_id". - DictionaryPrefUpdate update(&local_state, prefs::kProfilePreferenceHashes); + DictionaryPrefUpdate update(&local_state_, prefs::kProfilePreferenceHashes); base::DictionaryValue* hash_of_hashes_dict = NULL; ASSERT_TRUE(update->GetDictionaryWithoutPathExpansion( - internals::kHashOfHashesPref, &hash_of_hashes_dict)); + internals::kHashOfHashesDict, &hash_of_hashes_dict)); hash_of_hashes_dict->SetString("store_id", std::string(64, 'A')); // This shouldn't have increased the number of existing hash of hashes. ASSERT_EQ(1U, hash_of_hashes_dict->size()); @@ -223,7 +227,7 @@ TEST(PrefHashStoreImplTest, SplitHashStoreAndCheck) { // |pref_hash_store3| should no longer trust its initial hashes dictionary // and thus shouldn't trust unknown values. PrefHashStoreImpl pref_hash_store3( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store3.BeginTransaction()); EXPECT_EQ(PrefHashStoreTransaction::UNTRUSTED_UNKNOWN_VALUE, @@ -232,17 +236,14 @@ TEST(PrefHashStoreImplTest, SplitHashStoreAndCheck) { } } -TEST(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { +TEST_F(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { base::DictionaryValue empty_dict; - TestingPrefServiceSimple local_state; - PrefHashStoreImpl::RegisterPrefs(local_state.registry()); - std::vector invalid_keys; { PrefHashStoreImpl pref_hash_store( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store.BeginTransaction()); @@ -279,7 +280,7 @@ TEST(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { // regression test ensuring that the internal action of clearing some hashes // does update the stored hash of hashes). PrefHashStoreImpl pref_hash_store2( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store2.BeginTransaction()); @@ -299,7 +300,7 @@ TEST(PrefHashStoreImplTest, EmptyAndNULLSplitDict) { // switching strategies after their initial release as split preferences are // turned into split preferences specifically because the atomic hash isn't // considered useful. -TEST(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) { +TEST_F(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) { base::StringValue string("string1"); base::DictionaryValue dict; @@ -308,12 +309,9 @@ TEST(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) { dict.Set("b", new base::StringValue("bar")); dict.Set("c", new base::StringValue("baz")); - TestingPrefServiceSimple local_state; - PrefHashStoreImpl::RegisterPrefs(local_state.registry()); - { PrefHashStoreImpl pref_hash_store( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store.BeginTransaction()); @@ -325,7 +323,7 @@ TEST(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) { { // Load a new |pref_hash_store2| in which the hashes dictionary is trusted. PrefHashStoreImpl pref_hash_store2( - "store_id", std::string(32, 0), "device_id", &local_state); + "store_id", std::string(32, 0), "device_id", &local_state_); scoped_ptr transaction( pref_hash_store2.BeginTransaction()); std::vector invalid_keys; @@ -334,3 +332,64 @@ TEST(PrefHashStoreImplTest, TrustedUnknownSplitValueFromExistingAtomic) { EXPECT_TRUE(invalid_keys.empty()); } } + +TEST_F(PrefHashStoreImplTest, GetCurrentVersion) { + COMPILE_ASSERT(PrefHashStoreImpl::VERSION_LATEST == 2, + new_versions_should_be_tested_here); + { + PrefHashStoreImpl pref_hash_store( + "store_id", std::string(32, 0), "device_id", &local_state_); + + // VERSION_UNINITIALIZED when no hashes are stored. + EXPECT_EQ(PrefHashStoreImpl::VERSION_UNINITIALIZED, + pref_hash_store.GetCurrentVersion()); + + scoped_ptr transaction( + pref_hash_store.BeginTransaction()); + base::StringValue string_value("foo"); + transaction->StoreHash("path1", &string_value); + } + { + PrefHashStoreImpl pref_hash_store( + "store_id", std::string(32, 0), "device_id", &local_state_); + + // VERSION_LATEST after storing a hash. + EXPECT_EQ(PrefHashStoreImpl::VERSION_LATEST, + pref_hash_store.GetCurrentVersion()); + } + { + // Manually clear the version number. + DictionaryPrefUpdate update(&local_state_, prefs::kProfilePreferenceHashes); + base::DictionaryValue* store_versions_dict = NULL; + ASSERT_TRUE(update->GetDictionaryWithoutPathExpansion( + internals::kStoreVersionsDict, &store_versions_dict)); + scoped_ptr stored_value; + store_versions_dict->Remove("store_id", &stored_value); + int stored_version; + EXPECT_TRUE(stored_value->GetAsInteger(&stored_version)); + EXPECT_EQ(PrefHashStoreImpl::VERSION_LATEST, stored_version); + // There should be no versions left in the versions dictionary. + EXPECT_TRUE(store_versions_dict->empty()); + } + { + PrefHashStoreImpl pref_hash_store( + "store_id", std::string(32, 0), "device_id", &local_state_); + + // VERSION_PRE_MIGRATION when no version is present for "store_id". + EXPECT_EQ(PrefHashStoreImpl::VERSION_PRE_MIGRATION, + pref_hash_store.GetCurrentVersion()); + + scoped_ptr transaction( + pref_hash_store.BeginTransaction()); + } + { + PrefHashStoreImpl pref_hash_store( + "store_id", std::string(32, 0), "device_id", &local_state_); + + // Back to VERSION_LATEST after performing a transaction from + // VERSION_PRE_MIGRATION (the presence of an existing hash should be + // sufficient, no need for the transaction itself to perform any work). + EXPECT_EQ(PrefHashStoreImpl::VERSION_LATEST, + pref_hash_store.GetCurrentVersion()); + } +} diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 7e78a71137bd0..c23a1f9a1c46b 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -18009,8 +18009,31 @@ other types of suffix sets. The id of a tracked preference which was reset by Chrome. + + + The version of a PrefHashStore, reported once for each alternate + PrefHashStore (not associated to the default profile) from a delayed task on + startup. + + + + + + The previous version of an alternate PrefHashStore (not associated to the + default profile) that was updated from a delayed task on startup. This + should match Settings.TrackedPreferencesAlternateStoreVersion fairly closely + for all versions but VERSION_LATEST which should never be reported here. + + + + + Deprecated 2014-02 in favor of + Settings.TrackedPreferencesAlternateStoreVersionUpdatedFrom. + Preference tracking was initialized for an unloaded profile. This should happen at most once per profile. @@ -30122,6 +30145,12 @@ other types of suffix sets. + + + + + +