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

This reverts commit 92ffe77a.

Reason for revert: I suspect that the original revert was inappropriate – more detailed reasoning left as a review comment on the revert CL.

Original change's description:
> 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/891119
> Reviewed-by: vitaliii <vitaliii@chromium.org>
> Commit-Queue: vitaliii <vitaliii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#532401}

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

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

Bug: 727984, 806712
Change-Id: I1627111dacc8d59bc43aa7b1b7ed76db8f165ee2
Reviewed-on: https://chromium-review.googlesource.com/894622Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533098}

components/variations/variations_seed_store.cc

@@ -26,7 +26,6 @@
 #endif  // OS_ANDROID
 
 namespace variations {
-
 namespace {
 
 // The ECDSA public key of the variations server for verifying variations seed
@@ -41,6 +40,11 @@ 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
@@ -243,11 +247,43 @@ bool VariationsSeedStore::StoreSafeSeed(
   if (result != StoreSeedResult::SUCCESS)
     return false;
 
-  local_state_->SetString(prefs::kVariationsSafeCompressedSeed,
-                          base64_seed_data);
+  // 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,
@@ -256,6 +292,15 @@ 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;
 }
 
@@ -423,6 +468,13 @@ 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)) {
@@ -469,6 +521,13 @@ 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

@@ -97,7 +97,6 @@ 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

@@ -40,6 +40,14 @@ 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)
@@ -261,6 +269,35 @@ 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);
@@ -391,6 +428,33 @@ 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());
@@ -700,6 +764,102 @@ 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;