Skip to content

GitLab

T
tangled
Commit 92ffe77a authored by vitaliii's avatar vitaliii Committed by Commit Bot
Browse files
Options

Revert "[Variations] Avoid duplication of the safe seed value stored in prefs." 

This reverts commit 4db68ed9.

Reason for revert: Seems to cause failures on Mac ASan 64 Tests (see crbug.com/806712).

Original change's description:
> [Variations] Avoid duplication of the safe seed value stored in prefs.
> 
> When the safe seed and the latest seed have identical contents, use a sentinel
> value to avoid storing a duplicate value for the two seeds. This is purely a
> storage space optimization; no difference in observed functionality is expected.
> 
> R=​asvitkine@chromium.org
> 
> Bug: 727984
> Change-Id: Ica641a479228b0cdfcc4af2ec04ddfc54c36c5eb
> Reviewed-on: https://chromium-review.googlesource.com/880482
> Commit-Queue: Ilya Sherman <isherman@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#532027}

TBR=isherman@chromium.org,asvitkine@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 727984, 806712
Change-Id: Ic5de33c61bb2acf88d7445ded4f0a1743ab7f456
Reviewed-on: https://chromium-review.googlesource.com/891119Reviewed-by: default avatarvitaliii <vitaliii@chromium.org>
Commit-Queue: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532401}
parent 948b8dee
Show whitespace changes
Inline Side-by-side
Showing with 5 additions and 223 deletions
components/variations/variations_seed_store.cc
View file @ 92ffe77a
......@@ -26,6 +26,7 @@
#endif // OS_ANDROID
namespace variations {
namespace {
// The ECDSA public key of the variations server for verifying variations seed
......@@ -40,11 +41,6 @@ const uint8_t kPublicKey[] = {
0x2b, 0xff, 0x82, 0x4d, 0xd3, 0xfe, 0xc5, 0xef, 0x20, 0xc6, 0xa3, 0x10, 0xbf,
};
// A sentinel value that may be stored as the latest variations seed value in
// prefs to indicate that the latest seed is identical to the safe seed. Used to
// avoid duplicating storage space.
constexpr char kIdenticalToSafeSeedSentinel[] = "safe_seed_content";
// Verifies a variations seed (the serialized proto bytes) with the specified
// base-64 encoded signature that was received from the server and returns the
// result. The signature is assumed to be an "ECDSA with SHA-256" signature
......@@ -247,43 +243,11 @@ bool VariationsSeedStore::StoreSafeSeed(
if (result != StoreSeedResult::SUCCESS)
return false;
// The sentinel value should only ever be saved in place of the latest seed --
// it should never be saved in place of the safe seed.
DCHECK_NE(base64_seed_data, kIdenticalToSafeSeedSentinel);
// As a performance optimization, avoid an expensive no-op of overwriting the
// previous safe seed with an identical copy.
std::string previous_safe_seed =
local_state_->GetString(prefs::kVariationsSafeCompressedSeed);
if (base64_seed_data != previous_safe_seed) {
// It's theoretically possible to overwrite an existing safe seed value,
// which was identical to the latest seed, with a new value. This could
// happen, for example, if:
// (1) Seed A is received from the server and saved as both the safe and
// latest seed value.
// (2) Seed B is received from the server and saved as the latest seed
// value.
// (3) The user restarts Chrome, which is now running with the
// configuration from seed B.
// (4) Seed A is received again from the server, perhaps due to a
// rollback.
// In this situation, seed A should be saved as the latest seed, while seed
// B should be saved as the safe seed, i.e. the previously saved values
// should be swapped. Indeed, it is guaranteed that the latest seed value
// should be overwritten in this case, as a seed should not be considered
// safe unless a new seed can be both received *and saved* from the server.
std::string latest_seed =
local_state_->GetString(prefs::kVariationsCompressedSeed);
if (latest_seed == kIdenticalToSafeSeedSentinel) {
local_state_->SetString(prefs::kVariationsCompressedSeed,
previous_safe_seed);
}
local_state_->SetString(prefs::kVariationsSafeCompressedSeed,
base64_seed_data);
}
local_state_->SetString(prefs::kVariationsSafeSeedSignature,
base64_seed_signature);
local_state_->SetTime(prefs::kVariationsSafeSeedDate,
client_state.reference_date);
local_state_->SetString(prefs::kVariationsSafeSeedLocale,
......@@ -292,15 +256,6 @@ bool VariationsSeedStore::StoreSafeSeed(
client_state.permanent_consistency_country);
local_state_->SetString(prefs::kVariationsSafeSeedSessionConsistencyCountry,
client_state.session_consistency_country);
// As a space optimization, overwrite the stored latest seed data with an
// alias to the safe seed, if they are identical.
if (base64_seed_data ==
local_state_->GetString(prefs::kVariationsCompressedSeed)) {
local_state_->SetString(prefs::kVariationsCompressedSeed,
kIdenticalToSafeSeedSentinel);
}
return true;
}
......@@ -468,13 +423,6 @@ LoadSeedResult VariationsSeedStore::ReadSeedData(SeedType seed_type,
if (base64_seed_data.empty())
return LoadSeedResult::EMPTY;
// As a space optimization, the latest seed might not be stored directly, but
// rather aliased to the safe seed.
if (seed_type == SeedType::LATEST &&
base64_seed_data == kIdenticalToSafeSeedSentinel) {
return ReadSeedData(SeedType::SAFE, seed_data);
}
// If the decode process fails, assume the pref value is corrupt and clear it.
std::string decoded_data;
if (!base::Base64Decode(base64_seed_data, &decoded_data)) {
......@@ -521,13 +469,6 @@ bool VariationsSeedStore::StoreSeedDataNoDelta(
if (!country_code.empty())
local_state_->SetString(prefs::kVariationsCountry, country_code);
// As a space optimization, store an alias to the safe seed if the contents
// are identical.
if (base64_seed_data ==
local_state_->GetString(prefs::kVariationsSafeCompressedSeed)) {
base64_seed_data = kIdenticalToSafeSeedSentinel;
}
local_state_->SetString(prefs::kVariationsCompressedSeed, base64_seed_data);
UpdateSeedDateAndLogDayChange(date_fetched);
local_state_->SetString(prefs::kVariationsSeedSignature,
......
components/variations/variations_seed_store.h
View file @ 92ffe77a
......@@ -97,6 +97,7 @@ class VariationsSeedStore {
static void RegisterPrefs(PrefRegistrySimple* registry);
PrefService* local_state() { return local_state_; }
const PrefService* local_state() const { return local_state_; }
protected:
......
components/variations/variations_seed_store_unittest.cc
View file @ 92ffe77a
......@@ -40,14 +40,6 @@ const char kBase64SeedSignature[] =
"MEQCIDD1IVxjzWYncun+9IGzqYjZvqxxujQEayJULTlbTGA/AiAr0oVmEgVUQZBYq5VLOSvy"
"96JkMYgzTkHPwbv7K/CmgA==";
// The sentinel value that may be stored as the latest variations seed value in
// prefs to indicate that the latest seed is identical to the safe seed.
// Note: This constant is intentionally duplicated in the test because it is
// persisted to disk. In order to maintain backward-compatibility, it's
// important that code continue to correctly handle this specific constant, even
// if the constant used internally in the implementation changes.
constexpr char kIdenticalToSafeSeedSentinel[] = "safe_seed_content";
class TestVariationsSeedStore : public VariationsSeedStore {
public:
explicit TestVariationsSeedStore(PrefService* local_state)
......@@ -269,35 +261,6 @@ TEST(VariationsSeedStoreTest, LoadSeed_EmptySeed) {
&loaded_base64_seed_signature));
}
TEST(VariationsSeedStoreTest, LoadSeed_IdenticalToSafeSeed) {
// Store good seed data to the safe seed prefs, and store a sentinel value to
// the latest seed pref, to verify that loading via the alias works.
const VariationsSeed seed = CreateTestSeed();
const std::string base64_seed = SerializeSeedBase64(seed);
const std::string base64_seed_signature = "a test signature, ignored.";
TestingPrefServiceSimple prefs;
VariationsSeedStore::RegisterPrefs(prefs.registry());
prefs.SetString(prefs::kVariationsCompressedSeed,
kIdenticalToSafeSeedSentinel);
prefs.SetString(prefs::kVariationsSafeCompressedSeed, base64_seed);
prefs.SetString(prefs::kVariationsSeedSignature, base64_seed_signature);
TestVariationsSeedStore seed_store(&prefs);
VariationsSeed loaded_seed;
std::string loaded_seed_data;
std::string loaded_base64_seed_signature;
// Check that loading the seed works correctly.
EXPECT_TRUE(seed_store.LoadSeed(&loaded_seed, &loaded_seed_data,
&loaded_base64_seed_signature));
// Check that the loaded data is the same as the original.
EXPECT_EQ(SerializeSeed(seed), SerializeSeed(loaded_seed));
EXPECT_EQ(SerializeSeed(seed), loaded_seed_data);
EXPECT_EQ(base64_seed_signature, loaded_base64_seed_signature);
}
TEST(VariationsSeedStoreTest, StoreSeedData) {
const VariationsSeed seed = CreateTestSeed();
const std::string serialized_seed = SerializeSeed(seed);
......@@ -428,33 +391,6 @@ TEST(VariationsSeedStoreTest, LoadSafeSeed_ValidSeed) {
EXPECT_EQ(base64_seed, prefs.GetString(prefs::kVariationsSafeCompressedSeed));
}
TEST(VariationsSeedStoreTest, StoreSeedData_IdenticalToSafeSeed) {
const VariationsSeed seed = CreateTestSeed();
const std::string serialized_seed = SerializeSeed(seed);
const std::string base64_seed = SerializeSeedBase64(seed);
TestingPrefServiceSimple prefs;
VariationsSeedStore::RegisterPrefs(prefs.registry());
prefs.SetString(prefs::kVariationsSafeCompressedSeed, base64_seed);
TestVariationsSeedStore seed_store(&prefs);
EXPECT_TRUE(seed_store.StoreSeedForTesting(serialized_seed));
// Verify that the pref has a sentinel value, rather than the full string.
EXPECT_EQ(kIdenticalToSafeSeedSentinel,
prefs.GetString(prefs::kVariationsCompressedSeed));
// Verify that loading the stored seed returns the original seed value.
VariationsSeed loaded_seed;
std::string loaded_seed_data;
std::string unused_loaded_base64_seed_signature;
EXPECT_TRUE(seed_store.LoadSeed(&loaded_seed, &loaded_seed_data,
&unused_loaded_base64_seed_signature));
EXPECT_EQ(SerializeSeed(seed), SerializeSeed(loaded_seed));
EXPECT_EQ(SerializeSeed(seed), loaded_seed_data);
}
TEST(VariationsSeedStoreTest, LoadSafeSeed_InvalidSeed) {
TestingPrefServiceSimple prefs;
VariationsSeedStore::RegisterPrefs(prefs.registry());
......@@ -764,102 +700,6 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_ValidSignature) {
VerifySignatureResult::VALID_SIGNATURE, 1);
}
TEST(VariationsSeedStoreTest, StoreSafeSeed_IdenticalToLatestSeed) {
const VariationsSeed seed = CreateTestSeed();
const std::string serialized_seed = SerializeSeed(seed);
const std::string base64_seed = SerializeSeedBase64(seed);
const std::string signature = "a completely ignored signature";
ClientFilterableState unused_client_state;
TestingPrefServiceSimple prefs;
VariationsSeedStore::RegisterPrefs(prefs.registry());
prefs.SetString(prefs::kVariationsCompressedSeed, base64_seed);
TestVariationsSeedStore seed_store(&prefs);
base::HistogramTester histogram_tester;
EXPECT_TRUE(seed_store.StoreSafeSeed(serialized_seed, signature,
unused_client_state));
// Verify the latest seed value was migrated to a sentinel value, rather than
// the full string.
EXPECT_EQ(kIdenticalToSafeSeedSentinel,
prefs.GetString(prefs::kVariationsCompressedSeed));
// Verify that loading the stored seed returns the original seed value.
VariationsSeed loaded_seed;
std::string loaded_seed_data;
std::string unused_loaded_base64_seed_signature;
EXPECT_TRUE(seed_store.LoadSeed(&loaded_seed, &loaded_seed_data,
&unused_loaded_base64_seed_signature));
EXPECT_EQ(SerializeSeed(seed), SerializeSeed(loaded_seed));
EXPECT_EQ(SerializeSeed(seed), loaded_seed_data);
// Verify that the safe seed prefs indeed contain the serialized seed value.
EXPECT_EQ(base64_seed, prefs.GetString(prefs::kVariationsSafeCompressedSeed));
VariationsSeed loaded_safe_seed;
EXPECT_TRUE(seed_store.LoadSafeSeed(&loaded_safe_seed, &unused_client_state));
EXPECT_EQ(SerializeSeed(seed), SerializeSeed(loaded_safe_seed));
// Verify metrics.
histogram_tester.ExpectUniqueSample(
"Variations.SafeMode.StoreSafeSeed.Result", StoreSeedResult::SUCCESS, 1);
}
TEST(VariationsSeedStoreTest, StoreSafeSeed_PreviouslyIdenticalToLatestSeed) {
// Create two distinct seeds: an old one saved as both the safe and the latest
// seed value, and a new one that should overwrite only the stored safe seed
// value.
const VariationsSeed old_seed = CreateTestSeed();
VariationsSeed new_seed = CreateTestSeed();
new_seed.set_serial_number("12345678");
ASSERT_NE(SerializeSeed(old_seed), SerializeSeed(new_seed));
const std::string serialized_old_seed = SerializeSeed(old_seed);
const std::string base64_old_seed = SerializeSeedBase64(old_seed);
const std::string serialized_new_seed = SerializeSeed(new_seed);
const std::string base64_new_seed = SerializeSeedBase64(new_seed);
const std::string signature = "a completely ignored signature";
ClientFilterableState unused_client_state;
TestingPrefServiceSimple prefs;
VariationsSeedStore::RegisterPrefs(prefs.registry());
prefs.SetString(prefs::kVariationsSafeCompressedSeed, base64_old_seed);
prefs.SetString(prefs::kVariationsCompressedSeed,
kIdenticalToSafeSeedSentinel);
TestVariationsSeedStore seed_store(&prefs);
base::HistogramTester histogram_tester;
EXPECT_TRUE(seed_store.StoreSafeSeed(serialized_new_seed, signature,
unused_client_state));
// Verify the latest seed value was copied before the safe seed was
// overwritten.
EXPECT_EQ(base64_old_seed, prefs.GetString(prefs::kVariationsCompressedSeed));
// Verify that loading the stored seed returns the old seed value.
VariationsSeed loaded_seed;
std::string loaded_seed_data;
std::string unused_loaded_base64_seed_signature;
EXPECT_TRUE(seed_store.LoadSeed(&loaded_seed, &loaded_seed_data,
&unused_loaded_base64_seed_signature));
EXPECT_EQ(SerializeSeed(old_seed), SerializeSeed(loaded_seed));
EXPECT_EQ(SerializeSeed(old_seed), loaded_seed_data);
// Verify that the safe seed prefs indeed contain the new seed's serialized
// value.
EXPECT_EQ(base64_new_seed,
prefs.GetString(prefs::kVariationsSafeCompressedSeed));
VariationsSeed loaded_safe_seed;
EXPECT_TRUE(seed_store.LoadSafeSeed(&loaded_safe_seed, &unused_client_state));
EXPECT_EQ(SerializeSeed(new_seed), SerializeSeed(loaded_safe_seed));
// Verify metrics.
histogram_tester.ExpectUniqueSample(
"Variations.SafeMode.StoreSafeSeed.Result", StoreSeedResult::SUCCESS, 1);
}
TEST(VariationsSeedStoreTest, StoreSeedData_GzippedEmptySeed) {
std::string empty_seed;
std::string compressed_seed;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment