diff options
Diffstat (limited to 'chromium/components/variations')
37 files changed, 1137 insertions, 353 deletions
diff --git a/chromium/components/variations/BUILD.gn b/chromium/components/variations/BUILD.gn index fc9e3be7f76..00b73491cda 100644 --- a/chromium/components/variations/BUILD.gn +++ b/chromium/components/variations/BUILD.gn @@ -156,10 +156,12 @@ source_set("unit_tests") { "//components/crash/core/common:crash_key", "//components/prefs:test_support", "//components/variations/field_trial_config:field_trial_config", + "//services/network/public/cpp:cpp_base", "//testing/gmock", "//testing/gtest", "//third_party/smhasher:murmurhash3", "//third_party/zlib/google:compression_utils", + "//url", ] } diff --git a/chromium/components/variations/android/BUILD.gn b/chromium/components/variations/android/BUILD.gn index 287ad61216b..11f35e73564 100644 --- a/chromium/components/variations/android/BUILD.gn +++ b/chromium/components/variations/android/BUILD.gn @@ -28,6 +28,9 @@ java_library("components_variations_junit_tests") { "//base:base_java", "//base:base_java_test_support", "//base:base_junit_test_support", + "//third_party/android_deps:robolectric_all_java", "//third_party/hamcrest:hamcrest_java", + "//third_party/junit", + "//third_party/mockito:mockito_java", ] } diff --git a/chromium/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java b/chromium/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java index 0cc411f5c6a..bd74bffa4a4 100644 --- a/chromium/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java +++ b/chromium/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedBridge.java @@ -8,6 +8,7 @@ import android.util.Base64; import org.chromium.base.ContextUtils; import org.chromium.base.annotations.CalledByNative; +import org.chromium.base.metrics.RecordHistogram; import org.chromium.components.variations.firstrun.VariationsSeedFetcher.SeedInfo; import java.text.ParseException; @@ -32,6 +33,20 @@ public class VariationsSeedBridge { protected static final String VARIATIONS_FIRST_RUN_SEED_NATIVE_STORED = "variations_seed_native_stored"; + // These must be kept in sync with VariationsFirstRunPrefEvents in enums.xml. + private static final int DEBUG_PREFS_STORED = 0; + private static final int DEBUG_PREFS_CLEARED = 1; + private static final int DEBUG_PREFS_RETRIEVED_DATA_EMPTY = 2; + private static final int DEBUG_PREFS_RETRIEVED_DATA_NON_EMPTY = 3; + private static final int DEBUG_PREFS_CLEARED_NON_EMPTY = 4; + private static final int DEBUG_PREFS_MAX = 5; + + // TODO(crbug.com/1090968): Debug histogram to investigate a regression. Remove when resolved. + private static void logDebugHistogram(int value) { + RecordHistogram.recordEnumeratedHistogram( + "Variations.FirstRunPrefsDebug", value, DEBUG_PREFS_MAX); + } + protected static String getVariationsFirstRunSeedPref(String prefName) { return ContextUtils.getAppSharedPreferences().getString(prefName, ""); } @@ -52,10 +67,14 @@ public class VariationsSeedBridge { .putLong(VARIATIONS_FIRST_RUN_SEED_DATE, date) .putBoolean(VARIATIONS_FIRST_RUN_SEED_IS_GZIP_COMPRESSED, isGzipCompressed) .apply(); + logDebugHistogram(DEBUG_PREFS_STORED); } @CalledByNative private static void clearFirstRunPrefs() { + if (hasJavaPref()) { + logDebugHistogram(DEBUG_PREFS_CLEARED_NON_EMPTY); + } ContextUtils.getAppSharedPreferences() .edit() .remove(VARIATIONS_FIRST_RUN_SEED_BASE64) @@ -65,6 +84,7 @@ public class VariationsSeedBridge { .remove(VARIATIONS_FIRST_RUN_SEED_DATE_HEADER) .remove(VARIATIONS_FIRST_RUN_SEED_IS_GZIP_COMPRESSED) .apply(); + logDebugHistogram(DEBUG_PREFS_CLEARED); } /** @@ -94,8 +114,11 @@ public class VariationsSeedBridge { @CalledByNative private static byte[] getVariationsFirstRunSeedData() { - return Base64.decode( + byte[] data = Base64.decode( getVariationsFirstRunSeedPref(VARIATIONS_FIRST_RUN_SEED_BASE64), Base64.NO_WRAP); + logDebugHistogram(data.length == 0 ? DEBUG_PREFS_RETRIEVED_DATA_EMPTY + : DEBUG_PREFS_RETRIEVED_DATA_NON_EMPTY); + return data; } @CalledByNative diff --git a/chromium/components/variations/client_filterable_state.cc b/chromium/components/variations/client_filterable_state.cc index f39123e11f7..6fb4597dfaa 100644 --- a/chromium/components/variations/client_filterable_state.cc +++ b/chromium/components/variations/client_filterable_state.cc @@ -4,6 +4,9 @@ #include "components/variations/client_filterable_state.h" +#include "base/strings/string_util.h" +#include "base/strings/stringprintf.h" +#include "base/system/sys_info.h" #include "build/build_config.h" namespace variations { @@ -31,6 +34,30 @@ Study::Platform ClientFilterableState::GetCurrentPlatform() { #endif } +// TODO(b/957197): Improve how we handle OS versions. +// Add os_version.h and os_version_<platform>.cc that handle retrieving and +// parsing OS versions. Then get rid of all the platform-dependent code here. +// +// static +base::Version ClientFilterableState::GetOSVersion() { + base::Version ret; + +#if defined(OS_WIN) + std::string win_version = base::SysInfo::OperatingSystemVersion(); + base::ReplaceSubstringsAfterOffset(&win_version, 0, " SP", "."); + ret = base::Version(win_version); + DCHECK(ret.IsValid()) << win_version; +#else + // Every other OS is supported by OperatingSystemVersionNumbers + int major, minor, build; + base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &build); + ret = base::Version(base::StringPrintf("%d.%d.%d", major, minor, build)); + DCHECK(ret.IsValid()); +#endif + + return ret; +} + ClientFilterableState::ClientFilterableState( IsEnterpriseFunction is_enterprise_function) : is_enterprise_function_(std::move(is_enterprise_function)) {} diff --git a/chromium/components/variations/client_filterable_state.h b/chromium/components/variations/client_filterable_state.h index de7aa0cb649..ec74b48f94d 100644 --- a/chromium/components/variations/client_filterable_state.h +++ b/chromium/components/variations/client_filterable_state.h @@ -34,6 +34,9 @@ using IsEnterpriseFunction = base::OnceCallback<bool()>; struct ClientFilterableState { static Study::Platform GetCurrentPlatform(); + // base::Version used in {min,max}_os_version filtering. + static base::Version GetOSVersion(); + explicit ClientFilterableState(IsEnterpriseFunction is_enterprise_function); ~ClientFilterableState(); diff --git a/chromium/components/variations/field_trial_config/field_trial_testing_config_schema.json b/chromium/components/variations/field_trial_config/field_trial_testing_config_schema.json index 13e731a6baf..68499c29c4d 100644 --- a/chromium/components/variations/field_trial_config/field_trial_testing_config_schema.json +++ b/chromium/components/variations/field_trial_config/field_trial_testing_config_schema.json @@ -39,6 +39,10 @@ "optional": "True" }, { + "field": "min_os_version", + "type": "string" + }, + { "field": "params", "type": "array", "contents": { diff --git a/chromium/components/variations/field_trial_config/field_trial_util.cc b/chromium/components/variations/field_trial_config/field_trial_util.cc index d5fd4443128..71897b4f6c2 100644 --- a/chromium/components/variations/field_trial_config/field_trial_util.cc +++ b/chromium/components/variations/field_trial_config/field_trial_util.cc @@ -19,6 +19,7 @@ #include "base/optional.h" #include "base/strings/utf_string_conversions.h" #include "base/system/sys_info.h" +#include "components/variations/client_filterable_state.h" #include "components/variations/field_trial_config/fieldtrial_testing_config.h" #include "components/variations/variations_seed_processor.h" #include "net/base/escape.h" @@ -70,6 +71,15 @@ bool HasFormFactor(const FieldTrialTestingExperiment& experiment) { return experiment.form_factors_size == 0; } +// Returns true if the experiment config has a missing |min_os_version| or +// GetOSVersion() >= |min_os_version|. +bool HasMinOSVersion(const FieldTrialTestingExperiment& experiment) { + if (!experiment.min_os_version) + return true; + return base::Version(experiment.min_os_version) <= + ClientFilterableState::GetOSVersion(); +} + // Records the override ui string config. Mainly used for testing. void ApplyUIStringOverrides( const FieldTrialTestingExperiment& experiment, @@ -137,9 +147,8 @@ void ChooseExperiment( for (size_t i = 0; i < study.experiments_size; ++i) { const FieldTrialTestingExperiment* experiment = study.experiments + i; if (HasPlatform(*experiment, platform)) { - if (!chosen_experiment && - !HasDeviceLevelMismatch(*experiment) && - HasFormFactor(*experiment)) { + if (!chosen_experiment && !HasDeviceLevelMismatch(*experiment) && + HasFormFactor(*experiment) && HasMinOSVersion(*experiment)) { chosen_experiment = experiment; } diff --git a/chromium/components/variations/field_trial_config/field_trial_util_unittest.cc b/chromium/components/variations/field_trial_config/field_trial_util_unittest.cc index 0adbffb224d..946e1dfc4bc 100644 --- a/chromium/components/variations/field_trial_config/field_trial_util_unittest.cc +++ b/chromium/components/variations/field_trial_config/field_trial_util_unittest.cc @@ -16,6 +16,7 @@ #include "base/strings/utf_string_conversions.h" #include "base/system/sys_info.h" #include "base/test/scoped_feature_list.h" +#include "components/variations/client_filterable_state.h" #include "components/variations/field_trial_config/fieldtrial_testing_config.h" #include "components/variations/variations_associated_data.h" #include "components/variations/variations_seed_processor.h" @@ -113,19 +114,58 @@ TEST_F(FieldTrialUtilTest, AssociateParamsFromFieldTrialConfig) { const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params_0[] = {{"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = { - {"TestGroup1", &platform, 1, {}, 0, base::nullopt, - array_kFieldTrialConfig_params_0, 2, nullptr, 0, nullptr, 0, nullptr, - nullptr, 0}, + {"TestGroup1", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + array_kFieldTrialConfig_params_0, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params_1[] = {{"x", "3"}, {"y", "4"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = { - {"TestGroup2", &platform, 1, {}, 0, base::nullopt, - array_kFieldTrialConfig_params_0, 2, nullptr, 0, nullptr, 0, nullptr, - nullptr, 0}, - {"TestGroup2-2", &platform, 1, {}, 0, base::nullopt, - array_kFieldTrialConfig_params_1, 2, nullptr, 0, nullptr, 0, nullptr, - nullptr, 0}, + {"TestGroup2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + array_kFieldTrialConfig_params_0, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, + {"TestGroup2-2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + array_kFieldTrialConfig_params_1, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { {"TestTrial1", array_kFieldTrialConfig_experiments_0, 1}, @@ -178,9 +218,22 @@ TEST_F(FieldTrialUtilTest, for (size_t i = 0; i < base::size(all_platforms); ++i) { const Study::Platform platform = all_platforms[i]; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, {}, 0, base::nullopt, - array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, - nullptr, 0}, + {"TestGroup", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { {"TestTrial", array_kFieldTrialConfig_experiments, 1} @@ -214,9 +267,22 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = {{"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, {}, 0, base::nullopt, - array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, - nullptr, 0}, + {"TestGroup", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {{"TestTrial", array_kFieldTrialConfig_experiments, 1}}; @@ -245,9 +311,22 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = {{"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", platforms, 2, {}, 0, base::nullopt, - array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, - nullptr, 0}, + {"TestGroup", + platforms, + 2, + {}, + 0, + base::nullopt, + nullptr, + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {{"TestTrial", array_kFieldTrialConfig_experiments, 1}}; @@ -280,7 +359,7 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = {{"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, form_factors, 4, base::nullopt, + {"TestGroup", &platform, 1, form_factors, 4, base::nullopt, nullptr, array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}, }; @@ -315,7 +394,7 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = {{"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, &form_factor, 1, base::nullopt, + {"TestGroup", &platform, 1, &form_factor, 1, base::nullopt, nullptr, array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}, }; @@ -358,7 +437,7 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = {{"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, &form_factor, 1, base::nullopt, + {"TestGroup", &platform, 1, &form_factor, 1, base::nullopt, nullptr, array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}, }; @@ -394,14 +473,56 @@ TEST_F(FieldTrialUtilTest, AssociateFeaturesFromFieldTrialConfig) { const Study::Platform platform = Study::PLATFORM_LINUX; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = { - {"TestGroup1", &platform, 1, {}, 0, base::nullopt, nullptr, - 0, enable_features, 2, nullptr, 0, nullptr, nullptr, 0}, + {"TestGroup1", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + enable_features, + 2, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = { - {"TestGroup2", &platform, 1, {}, 0, base::nullopt, nullptr, - 0, nullptr, 0, disable_features, 2, nullptr, nullptr, 0}, - {"TestGroup2-2", &platform, 1, {}, 0, base::nullopt, - nullptr, 0, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}, + {"TestGroup2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + disable_features, + 2, + nullptr, + nullptr, + 0}, + {"TestGroup2-2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { @@ -437,21 +558,105 @@ TEST_F(FieldTrialUtilTest, AssociateFeaturesFromFieldTrialConfig) { TEST_F(FieldTrialUtilTest, AssociateForcingFlagsFromFieldTrialConfig) { const Study::Platform platform = Study::PLATFORM_LINUX; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = { - {"TestGroup1", &platform, 1, {}, 0, base::nullopt, nullptr, - 0, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}}; + {"TestGroup1", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = { - {"TestGroup2", &platform, 1, {}, 0, base::nullopt, nullptr, - 0, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}, - {"ForcedGroup2", &platform, 1, {}, 0, base::nullopt, - nullptr, 0, nullptr, 0, nullptr, 0, "flag-2", nullptr, 0}, + {"TestGroup2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, + {"ForcedGroup2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + "flag-2", + nullptr, + 0}, }; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_2[] = { - {"TestGroup3", &platform, 1, {}, 0, base::nullopt, nullptr, - 0, nullptr, 0, nullptr, 0, nullptr, nullptr, 0}, - {"ForcedGroup3", &platform, 1, {}, 0, base::nullopt, - nullptr, 0, nullptr, 0, nullptr, 0, "flag-3", nullptr, 0}, - {"ForcedGroup3-2", &platform, 1, {}, 0, base::nullopt, - nullptr, 0, nullptr, 0, nullptr, 0, "flag-3-2", nullptr, 0}, + {"TestGroup3", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + nullptr, + nullptr, + 0}, + {"ForcedGroup3", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + "flag-3", + nullptr, + 0}, + {"ForcedGroup3-2", + &platform, + 1, + {}, + 0, + base::nullopt, + nullptr, + nullptr, + 0, + nullptr, + 0, + nullptr, + 0, + "flag-3-2", + nullptr, + 0}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { {"TestTrial1", array_kFieldTrialConfig_experiments_0, 1}, @@ -484,7 +689,7 @@ TEST_F(FieldTrialUtilTest, const OverrideUIString array_kFieldTrialConfig_override_ui_string[] = {{1234, "test1"}, {5678, "test2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, nullptr, 0, base::nullopt, + {"TestGroup", &platform, 1, nullptr, 0, base::nullopt, nullptr, array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr, array_kFieldTrialConfig_override_ui_string, 2}, }; @@ -527,8 +732,20 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = { {"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, {}, 0, base::SysInfo::IsLowEndDevice(), - array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr}, + {"TestGroup", + &platform, + 1, + {}, + 0, + base::SysInfo::IsLowEndDevice(), + nullptr, + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { {"TestTrial", array_kFieldTrialConfig_experiments, 1}}; @@ -557,8 +774,20 @@ TEST_F(FieldTrialUtilTest, const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = { {"x", "1"}, {"y", "2"}}; const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { - {"TestGroup", &platform, 1, {}, 0, !base::SysInfo::IsLowEndDevice(), - array_kFieldTrialConfig_params, 2, nullptr, 0, nullptr, 0, nullptr}, + {"TestGroup", + &platform, + 1, + {}, + 0, + !base::SysInfo::IsLowEndDevice(), + nullptr, + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr}, }; const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { {"TestTrial", array_kFieldTrialConfig_experiments, 1}}; @@ -578,6 +807,93 @@ TEST_F(FieldTrialUtilTest, EXPECT_EQ("", base::FieldTrialList::FindFullName("TestTrial")); } +TEST_F(FieldTrialUtilTest, + AssociateParamsFromFieldTrialConfigWithMinOsVersionMatch) { + base::Version version = ClientFilterableState::GetOSVersion(); + std::string min_os_version = version.GetString(); + const Study::Platform platform = Study::PLATFORM_WINDOWS; + const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = { + {"x", "1"}, {"y", "2"}}; + const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { + {"TestGroup", + &platform, + 1, + {}, + 0, + base::nullopt, + min_os_version.c_str(), + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr}, + }; + const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { + {"TestTrial", array_kFieldTrialConfig_experiments, 1}}; + const FieldTrialTestingConfig kConfig = {array_kFieldTrialConfig_studies, 1}; + + // The min_os_version filter matches, so trial should be added. + base::FeatureList feature_list; + AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(), + platform, &feature_list); + + EXPECT_EQ("1", GetVariationParamValue("TestTrial", "x")); + EXPECT_EQ("2", GetVariationParamValue("TestTrial", "y")); + + std::map<std::string, std::string> params; + EXPECT_TRUE(GetVariationParams("TestTrial", ¶ms)); + EXPECT_EQ(2U, params.size()); + EXPECT_EQ("1", params["x"]); + EXPECT_EQ("2", params["y"]); + + EXPECT_EQ("TestGroup", base::FieldTrialList::FindFullName("TestTrial")); +} + +TEST_F(FieldTrialUtilTest, + AssociateParamsFromFieldTrialConfigWithMinOsVersionMismatch) { + base::Version version = ClientFilterableState::GetOSVersion(); + base::Version higher_version = + base::Version({version.components()[0] + 1, 0, 0}); + std::string min_os_version = higher_version.GetString(); + const Study::Platform platform = Study::PLATFORM_WINDOWS; + const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params[] = { + {"x", "1"}, {"y", "2"}}; + const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = { + {"TestGroup", + &platform, + 1, + {}, + 0, + base::nullopt, + min_os_version.c_str(), + array_kFieldTrialConfig_params, + 2, + nullptr, + 0, + nullptr, + 0, + nullptr}, + }; + const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = { + {"TestTrial", array_kFieldTrialConfig_experiments, 1}}; + const FieldTrialTestingConfig kConfig = {array_kFieldTrialConfig_studies, 1}; + + // The min_os_version doesn't match, so trial shouldn't be added. + base::FeatureList feature_list; + AssociateParamsFromFieldTrialConfig(kConfig, override_callback_.callback(), + platform, &feature_list); + + EXPECT_EQ("", GetVariationParamValue("TestTrial", "x")); + EXPECT_EQ("", GetVariationParamValue("TestTrial", "y")); + + std::map<std::string, std::string> params; + EXPECT_FALSE(GetVariationParams("TestTrial", ¶ms)); + + EXPECT_EQ("", base::FieldTrialList::FindFullName("TestTrial")); +} + TEST_F(FieldTrialUtilTest, TestEscapeValue) { std::string str = "trail.:/,*"; std::string escaped_str = EscapeValue(str); diff --git a/chromium/components/variations/net/variations_command_line_unittest.cc b/chromium/components/variations/net/variations_command_line_unittest.cc index 1a7ef70e90e..2ca4a019a86 100644 --- a/chromium/components/variations/net/variations_command_line_unittest.cc +++ b/chromium/components/variations/net/variations_command_line_unittest.cc @@ -22,8 +22,7 @@ TEST(VariationsCommandLineTest, TestGetVariationsCommandLine) { std::string disable_feature_list = "feature2<trial2"; AssociateParamsFromString(param_list); - base::FieldTrialList::CreateTrialsFromString(trial_list, - std::set<std::string>()); + base::FieldTrialList::CreateTrialsFromString(trial_list); base::test::ScopedFeatureList scoped_feature_list; scoped_feature_list.InitFromCommandLine(enable_feature_list, disable_feature_list); diff --git a/chromium/components/variations/net/variations_http_headers.cc b/chromium/components/variations/net/variations_http_headers.cc index c4f4f56d837..6f733ee3fc1 100644 --- a/chromium/components/variations/net/variations_http_headers.cc +++ b/chromium/components/variations/net/variations_http_headers.cc @@ -4,20 +4,20 @@ #include "components/variations/net/variations_http_headers.h" -#include <stddef.h> - #include <utility> #include <vector> #include "base/bind.h" #include "base/feature_list.h" #include "base/macros.h" +#include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" #include "base/stl_util.h" #include "base/strings/string_util.h" #include "components/google/core/common/google_util.h" #include "components/variations/net/omnibox_http_headers.h" #include "components/variations/variations_http_header_provider.h" +#include "net/base/isolation_info.h" #include "net/traffic_annotation/network_traffic_annotation.h" #include "net/url_request/redirect_info.h" #include "services/network/public/cpp/resource_request.h" @@ -33,44 +33,147 @@ const char kClientDataHeader[] = "X-Client-Data"; namespace { -// The result of checking if a URL should have variations headers appended. +// The result of checking whether a request to a URL should have variations +// headers appended to it. +// // This enum is used to record UMA histogram values, and should not be // reordered. -enum URLValidationResult { - INVALID_URL, - NOT_HTTPS, - NOT_GOOGLE_DOMAIN, - SHOULD_APPEND, - NEITHER_HTTP_HTTPS, - IS_GOOGLE_NOT_HTTPS, - URL_VALIDATION_RESULT_SIZE, +enum class URLValidationResult { + kNotValidInvalidUrl = 0, + // kNotValidNotHttps = 1, // Deprecated. + kNotValidNotGoogleDomain = 2, + kShouldAppend = 3, + kNotValidNeitherHttpHttps = 4, + kNotValidIsGoogleNotHttps = 5, + kMaxValue = kNotValidIsGoogleNotHttps, +}; + +// These values are persisted to logs. Entries should not be renumbered and +// numeric values should never be reused. +enum RequestContextCategory { + // First-party contexts. + kBrowserInitiated = 0, + kInternalChromePageInitiated = 1, + kGooglePageInitiated = 2, + kGoogleSubFrameOnGooglePageInitiated = 3, + // Third-party contexts. + kNonGooglePageInitiatedFromRequestInitiator = 4, + kNoTrustedParams = 5, + kNoIsolationInfo = 6, + kGoogleSubFrameOnNonGooglePageInitiated = 7, + kNonGooglePageInitiatedFromFrameOrigin = 8, + kMaxValue = kNonGooglePageInitiatedFromFrameOrigin, }; -void LogUrlValidationHistogram(URLValidationResult result) { - UMA_HISTOGRAM_ENUMERATION("Variations.Headers.URLValidationResult", result, - URL_VALIDATION_RESULT_SIZE); +void LogRequestContextHistogram(RequestContextCategory result) { + base::UmaHistogramEnumeration("Variations.Headers.RequestContextCategory", + result); +} + +// Returns a URLValidationResult for |url|. A valid URL for headers has the +// following qualities: (i) it is well-formed, (ii) its scheme is HTTPS, and +// (iii) it has a Google-associated domain. +URLValidationResult GetUrlValidationResult(const GURL& url) { + if (!url.is_valid()) + return URLValidationResult::kNotValidInvalidUrl; + + if (!url.SchemeIsHTTPOrHTTPS()) + return URLValidationResult::kNotValidNeitherHttpHttps; + + if (!google_util::IsGoogleAssociatedDomainUrl(url)) + return URLValidationResult::kNotValidNotGoogleDomain; + + // HTTPS is checked here, rather than before the IsGoogleAssociatedDomainUrl() + // check, to know how many Google domains are rejected by the change to append + // headers to only HTTPS requests. + if (!url.SchemeIs(url::kHttpsScheme)) + return URLValidationResult::kNotValidIsGoogleNotHttps; + + return URLValidationResult::kShouldAppend; +} + +// Returns true if the request to |url| should include a variations header. +// Also, logs the result of validating |url| in histograms, one of which ends in +// |suffix|. +bool ShouldAppendVariationsHeader(const GURL& url, const std::string& suffix) { + URLValidationResult result = GetUrlValidationResult(url); + base::UmaHistogramEnumeration( + "Variations.Headers.URLValidationResult." + suffix, result); + return result == URLValidationResult::kShouldAppend; } -bool ShouldAppendVariationsHeader(const GURL& url) { - if (!url.is_valid()) { - LogUrlValidationHistogram(INVALID_URL); +// Returns true if the request is sent from a Google-associated property, i.e. +// from a first-party context. This determination is made using the request +// context derived from |resource_request|. +bool IsFirstPartyContext(const network::ResourceRequest& resource_request) { + if (!resource_request.request_initiator) { + // The absence of |request_initiator| means that the request was initiated + // by the browser, e.g. a request from the browser to Autofill upon form + // detection. + LogRequestContextHistogram(RequestContextCategory::kBrowserInitiated); + return true; + } + + const GURL request_initiator_url = + resource_request.request_initiator->GetURL(); + if (request_initiator_url.SchemeIs("chrome-search") || + request_initiator_url.SchemeIs("chrome")) { + // A scheme matching the above patterns means that the request was + // initiated by an internal page, e.g. a request from + // chrome-search://local-ntp/ for App Launcher resources. + LogRequestContextHistogram(kInternalChromePageInitiated); + return true; + } + if (GetUrlValidationResult(request_initiator_url) != + URLValidationResult::kShouldAppend) { + // The request was initiated by a non-Google-associated page, e.g. a request + // from https://www.bbc.com/. + LogRequestContextHistogram(kNonGooglePageInitiatedFromRequestInitiator); return false; } - if (!url.SchemeIsHTTPOrHTTPS()) { - LogUrlValidationHistogram(NEITHER_HTTP_HTTPS); + if (resource_request.is_main_frame) { + // The request is from a Google-associated page--not a sub-frame--e.g. a + // request from https://calendar.google.com/. + LogRequestContextHistogram(kGooglePageInitiated); + return true; + } + if (!resource_request.trusted_params) { + LogRequestContextHistogram(kNoTrustedParams); + // Without TrustedParams, we cannot be certain that the request is from a + // first-party context. return false; } - if (!google_util::IsGoogleAssociatedDomainUrl(url)) { - LogUrlValidationHistogram(NOT_GOOGLE_DOMAIN); + + const net::IsolationInfo* isolation_info = + &resource_request.trusted_params->isolation_info; + if (isolation_info->IsEmpty()) { + LogRequestContextHistogram(kNoIsolationInfo); + // Without IsolationInfo, we cannot be certain that the request is from a + // first-party context. return false; } - // We check https here, rather than before the IsGoogleDomain() check, to know - // how many Google domains are being rejected by the change to https only. - if (!url.SchemeIs(url::kHttpsScheme)) { - LogUrlValidationHistogram(IS_GOOGLE_NOT_HTTPS); + if (GetUrlValidationResult(isolation_info->top_frame_origin()->GetURL()) != + URLValidationResult::kShouldAppend) { + // The request is from a Google-associated sub-frame on a + // non-Google-associated page, e.g. a request to DoubleClick from an ad's + // sub-frame on https://www.lexico.com/. + LogRequestContextHistogram(kGoogleSubFrameOnNonGooglePageInitiated); return false; } - LogUrlValidationHistogram(SHOULD_APPEND); + if (GetUrlValidationResult(isolation_info->frame_origin()->GetURL()) != + URLValidationResult::kShouldAppend) { + // The request was initiated by a non-Google-associated page, e.g. a request + // from https://www.bbc.com/. + // + // TODO(crbug/1094303): This case should be covered by checking the request + // initiator's URL. Maybe deprecate kNonGooglePageInitiatedFromFrameOrigin + // if this bucket is never used. + LogRequestContextHistogram(kNonGooglePageInitiatedFromFrameOrigin); + return false; + } + // The request is from a Google-associated sub-frame on a Google-associated + // page, e.g. a request from a Docs sub-frame on https://drive.google.com/. + LogRequestContextHistogram(kGoogleSubFrameOnGooglePageInitiated); return true; } @@ -100,9 +203,13 @@ class VariationsHeaderHelper { // international TLD domains *.google.<TLD> or *.youtube.<TLD>. // 2. Only transmit for non-Incognito profiles. // 3. For the X-Client-Data header, only include non-empty variation IDs. - if ((incognito == InIncognito::kYes) || !ShouldAppendVariationsHeader(url)) + if ((incognito == InIncognito::kYes) || + !ShouldAppendVariationsHeader(url, "Append")) return false; + // TODO(crbug/1094303): Use the result to determine which IDs to include. + IsFirstPartyContext(*resource_request_); + if (variations_header_.empty()) return false; @@ -153,7 +260,7 @@ void RemoveVariationsHeaderIfNeeded( const net::RedirectInfo& redirect_info, const network::mojom::URLResponseHead& response_head, std::vector<std::string>* to_be_removed_headers) { - if (!ShouldAppendVariationsHeader(redirect_info.new_url)) + if (!ShouldAppendVariationsHeader(redirect_info.new_url, "Remove")) to_be_removed_headers->push_back(kClientDataHeader); } @@ -194,8 +301,10 @@ bool HasVariationsHeader(const network::ResourceRequest& request) { return request.cors_exempt_headers.HasHeader(kClientDataHeader); } -bool ShouldAppendVariationsHeaderForTesting(const GURL& url) { - return ShouldAppendVariationsHeader(url); +bool ShouldAppendVariationsHeaderForTesting( + const GURL& url, + const std::string& histogram_suffix) { + return ShouldAppendVariationsHeader(url, histogram_suffix); } void UpdateCorsExemptHeaderForVariations( diff --git a/chromium/components/variations/net/variations_http_headers.h b/chromium/components/variations/net/variations_http_headers.h index 29506e0f81f..6e30fdd9d14 100644 --- a/chromium/components/variations/net/variations_http_headers.h +++ b/chromium/components/variations/net/variations_http_headers.h @@ -100,7 +100,9 @@ bool IsVariationsHeader(const std::string& header_name); bool HasVariationsHeader(const network::ResourceRequest& request); // Calls the internal ShouldAppendVariationsHeader() for testing. -bool ShouldAppendVariationsHeaderForTesting(const GURL& url); +bool ShouldAppendVariationsHeaderForTesting( + const GURL& url, + const std::string& histogram_suffix); // Updates |cors_exempt_header_list| field of the given |param| to register the // variation headers. diff --git a/chromium/components/variations/net/variations_http_headers_unittest.cc b/chromium/components/variations/net/variations_http_headers_unittest.cc index a07883107ad..79ab8b3dd3f 100644 --- a/chromium/components/variations/net/variations_http_headers_unittest.cc +++ b/chromium/components/variations/net/variations_http_headers_unittest.cc @@ -4,12 +4,18 @@ #include "components/variations/net/variations_http_headers.h" -#include <stddef.h> +#include <string> #include "base/macros.h" #include "base/stl_util.h" +#include "base/test/metrics/histogram_tester.h" +#include "net/base/isolation_info.h" +#include "net/cookies/site_for_cookies.h" +#include "services/network/public/cpp/resource_request.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/gurl.h" +#include "url/origin.h" namespace variations { @@ -155,9 +161,136 @@ TEST(VariationsHttpHeadersTest, ShouldAppendVariationsHeader) { for (size_t i = 0; i < base::size(cases); ++i) { const GURL url(cases[i].url); EXPECT_EQ(cases[i].should_append_headers, - ShouldAppendVariationsHeaderForTesting(url)) + ShouldAppendVariationsHeaderForTesting(url, "Append")) << url; } } +struct PopulateRequestContextHistogramData { + const char* request_initiator_url; + bool is_main_frame; + bool has_trusted_params; + const char* isolation_info_top_frame_origin_url; + const char* isolation_info_frame_origin_url; + int bucket; + const char* name; +}; + +class PopulateRequestContextHistogramTest + : public testing::TestWithParam<PopulateRequestContextHistogramData> { + public: + static const PopulateRequestContextHistogramData kCases[]; +}; + +const PopulateRequestContextHistogramData + PopulateRequestContextHistogramTest::kCases[] = { + {"", false, false, "", "", 0, "kBrowserInitiated"}, + {"chrome-search://local-ntp/", false, false, "", "", 1, + "kInternalChromePageInitiated"}, + {"https://www.youtube.com/", true, false, "", "", 2, + "kGooglePageInitiated"}, + {"https://docs.google.com/", false, true, "https://drive.google.com/", + "https://docs.google.com/", 3, "kGoogleSubFrameOnGooglePageInitiated"}, + {"https://www.un.org/", false, false, "", "", 4, + "kNonGooglePageInitiatedFromRequestInitiator"}, + {"https://foo.client-channel.google.com/", false, false, "", "", 5, + "kNoTrustedParams"}, + {"https://foo.google.com/", false, true, "", "", 6, "kNoIsolationInfo"}, + {"https://123acb.safeframe.googlesyndication.com/", false, true, + "https://www.lexico.com/", "", 7, + "kGoogleSubFrameOnNonGooglePageInitiated"}, + {"https://foo.google.com/", false, true, "https://foo.google.com/", + "https://www.reddit.com/", 8, + "kNonGooglePageInitiatedFromFrameOrigin"}, +}; + +TEST(VariationsHttpHeadersTest, PopulateUrlValidationResultHistograms) { + const GURL invalid_url("invalid"); + const GURL not_google("https://heavnlydonuts.com/"); + const GURL should_append("https://youtube.com"); + const GURL wrong_scheme("ftp://foo.com/"); + const GURL google_not_https("http://google.com/"); + + const std::string append = "Append"; + const std::string remove = "Remove"; + base::HistogramTester tester; + + ASSERT_FALSE(ShouldAppendVariationsHeaderForTesting(invalid_url, append)); + ASSERT_FALSE(ShouldAppendVariationsHeaderForTesting(not_google, append)); + ASSERT_TRUE(ShouldAppendVariationsHeaderForTesting(should_append, append)); + + ASSERT_FALSE(ShouldAppendVariationsHeaderForTesting(wrong_scheme, remove)); + ASSERT_FALSE( + ShouldAppendVariationsHeaderForTesting(google_not_https, remove)); + + // Verify that the Append suffixed histogram has a sample corresponding to + // the validation result for the three URLs validated for appending. + const std::string append_histogram = + "Variations.Headers.URLValidationResult.Append"; + tester.ExpectTotalCount(append_histogram, 3); + EXPECT_THAT(tester.GetAllSamples(append_histogram), + testing::ElementsAre(base::Bucket(0, 1), base::Bucket(2, 1), + base::Bucket(3, 1))); + + // Verify that the Remove suffixed histogram has a sample corresponding to + // the validation result for the two URLs validated for removal. + const std::string remove_histogram = + "Variations.Headers.URLValidationResult.Remove"; + tester.ExpectTotalCount(remove_histogram, 2); + EXPECT_THAT(tester.GetAllSamples(remove_histogram), + testing::ElementsAre(base::Bucket(4, 1), base::Bucket(5, 1))); +} + +INSTANTIATE_TEST_SUITE_P( + VariationsHttpHeadersTest, + PopulateRequestContextHistogramTest, + testing::ValuesIn(PopulateRequestContextHistogramTest::kCases)); + +// Returns a ResourceRequest created from the given values. +network::ResourceRequest CreateResourceRequest( + const std::string& request_initiator_url, + bool is_main_frame, + bool has_trusted_params, + const std::string& isolation_info_top_frame_origin_url, + const std::string& isolation_info_frame_origin_url) { + network::ResourceRequest request; + if (request_initiator_url.empty()) + return request; + + request.request_initiator = url::Origin::Create(GURL(request_initiator_url)); + request.is_main_frame = is_main_frame; + if (!has_trusted_params) + return request; + + request.trusted_params = network::ResourceRequest::TrustedParams(); + if (isolation_info_top_frame_origin_url.empty()) + return request; + + request.trusted_params->isolation_info = net::IsolationInfo::Create( + net::IsolationInfo::RedirectMode::kUpdateNothing, + url::Origin::Create(GURL(isolation_info_top_frame_origin_url)), + url::Origin::Create(GURL(isolation_info_frame_origin_url)), + net::SiteForCookies()); + return request; +} + +TEST_P(PopulateRequestContextHistogramTest, PopulateRequestContextHistogram) { + PopulateRequestContextHistogramData data = GetParam(); + SCOPED_TRACE(data.name); + + network::ResourceRequest request = CreateResourceRequest( + data.request_initiator_url, data.is_main_frame, data.has_trusted_params, + data.isolation_info_top_frame_origin_url, + data.isolation_info_frame_origin_url); + + base::HistogramTester tester; + AppendVariationsHeaderUnknownSignedIn(GURL("https://foo.google.com"), + variations::InIncognito::kNo, &request); + + // Verify that the histogram has a single sample corresponding to the request + // context category. + const std::string histogram = "Variations.Headers.RequestContextCategory"; + tester.ExpectUniqueSample(histogram, data.bucket, 1); +} + } // namespace variations diff --git a/chromium/components/variations/net/variations_url_loader_throttle.cc b/chromium/components/variations/net/variations_url_loader_throttle.cc index 2f1aed783c6..d0fd43ff595 100644 --- a/chromium/components/variations/net/variations_url_loader_throttle.cc +++ b/chromium/components/variations/net/variations_url_loader_throttle.cc @@ -21,7 +21,7 @@ VariationsURLLoaderThrottle::~VariationsURLLoaderThrottle() = default; void VariationsURLLoaderThrottle::AppendThrottleIfNeeded( const variations::VariationsClient* variations_client, std::vector<std::unique_ptr<blink::URLLoaderThrottle>>* throttles) { - if (!variations_client || variations_client->IsIncognito()) + if (!variations_client || variations_client->IsOffTheRecord()) return; throttles->push_back(std::make_unique<VariationsURLLoaderThrottle>( diff --git a/chromium/components/variations/processed_study.cc b/chromium/components/variations/processed_study.cc index b8abc438682..18ffa8d33ea 100644 --- a/chromium/components/variations/processed_study.cc +++ b/chromium/components/variations/processed_study.cc @@ -7,6 +7,7 @@ #include <set> #include <string> +#include "base/logging.h" #include "base/version.h" #include "components/variations/proto/study.pb.h" diff --git a/chromium/components/variations/proto/BUILD.gn b/chromium/components/variations/proto/BUILD.gn index 2de83c52b2f..e68dba914cd 100644 --- a/chromium/components/variations/proto/BUILD.gn +++ b/chromium/components/variations/proto/BUILD.gn @@ -4,6 +4,19 @@ import("//third_party/protobuf/proto_library.gni") +if (is_android) { + import("//build/config/android/rules.gni") + + proto_java_library("proto_java") { + proto_path = "." + sources = [ + "client_variations.proto", + "study.proto", + "variations_seed.proto", + ] + } +} + proto_library("proto") { sources = [ "client_variations.proto", diff --git a/chromium/components/variations/proto/client_variations.proto b/chromium/components/variations/proto/client_variations.proto index fbb3367e0bf..38b02837c44 100644 --- a/chromium/components/variations/proto/client_variations.proto +++ b/chromium/components/variations/proto/client_variations.proto @@ -7,6 +7,7 @@ syntax = "proto2"; option optimize_for = LITE_RUNTIME; +option java_package = "org.chromium.components.variations"; package variations; diff --git a/chromium/components/variations/proto/study.proto b/chromium/components/variations/proto/study.proto index 0e891106904..3c4bb0f85fc 100644 --- a/chromium/components/variations/proto/study.proto +++ b/chromium/components/variations/proto/study.proto @@ -5,6 +5,7 @@ syntax = "proto2"; option optimize_for = LITE_RUNTIME; +option java_package = "org.chromium.components.variations"; package variations; diff --git a/chromium/components/variations/proto/variations_seed.proto b/chromium/components/variations/proto/variations_seed.proto index 0659414e39c..6b21dc2a45c 100644 --- a/chromium/components/variations/proto/variations_seed.proto +++ b/chromium/components/variations/proto/variations_seed.proto @@ -5,6 +5,7 @@ syntax = "proto2"; option optimize_for = LITE_RUNTIME; +option java_package = "org.chromium.components.variations"; package variations; diff --git a/chromium/components/variations/service/variations_field_trial_creator.cc b/chromium/components/variations/service/variations_field_trial_creator.cc index 131250bf138..386b8585eba 100644 --- a/chromium/components/variations/service/variations_field_trial_creator.cc +++ b/chromium/components/variations/service/variations_field_trial_creator.cc @@ -84,28 +84,6 @@ base::Time GetReferenceDateForExpiryChecks(PrefService* local_state) { return reference_date; } -// TODO(b/957197): Improve how we handle OS versions. -// Add os_version.h and os_version_<platform>.cc that handle retrieving and -// parsing OS versions. Then get rid of all the platform-dependent code here. -base::Version GetOSVersion() { - base::Version ret; - -#if defined(OS_WIN) - std::string win_version = base::SysInfo::OperatingSystemVersion(); - base::ReplaceSubstringsAfterOffset(&win_version, 0, " SP", "."); - ret = base::Version(win_version); - DCHECK(ret.IsValid()) << win_version; -#else - // Every other OS is supported by OperatingSystemVersionNumbers - int major, minor, build; - base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &build); - ret = base::Version(base::StringPrintf("%d.%d.%d", major, minor, build)); - DCHECK(ret.IsValid()); -#endif - - return ret; -} - // Just maps one set of enum values to another. Nothing to see here. Study::Channel ConvertProductChannelToStudyChannel( version_info::Channel product_channel) { @@ -257,7 +235,7 @@ VariationsFieldTrialCreator::GetClientFilterableStateForVersion( state->locale = application_locale_; state->reference_date = GetReferenceDateForExpiryChecks(local_state()); state->version = version; - state->os_version = GetOSVersion(); + state->os_version = ClientFilterableState::GetOSVersion(); state->channel = ConvertProductChannelToStudyChannel(client_->GetChannelForVariations()); state->form_factor = GetCurrentFormFactor(); @@ -463,7 +441,6 @@ bool VariationsFieldTrialCreator::SetupFieldTrials( const char* kEnableGpuBenchmarking, const char* kEnableFeatures, const char* kDisableFeatures, - const std::set<std::string>& unforceable_field_trials, const std::vector<std::string>& variation_ids, const std::vector<base::FeatureList::FeatureOverrideInfo>& extra_overrides, std::unique_ptr<const base::FieldTrial::EntropyProvider> @@ -497,8 +474,7 @@ bool VariationsFieldTrialCreator::SetupFieldTrials( // Create field trials without activating them, so that this behaves in a // consistent manner with field trials created from the server. bool result = base::FieldTrialList::CreateTrialsFromString( - command_line->GetSwitchValueASCII(::switches::kForceFieldTrials), - unforceable_field_trials); + command_line->GetSwitchValueASCII(::switches::kForceFieldTrials)); if (!result) { ExitWithMessage(base::StringPrintf("Invalid --%s list specified.", ::switches::kForceFieldTrials)); diff --git a/chromium/components/variations/service/variations_field_trial_creator.h b/chromium/components/variations/service/variations_field_trial_creator.h index f2ea1364945..a67ec4f4283 100644 --- a/chromium/components/variations/service/variations_field_trial_creator.h +++ b/chromium/components/variations/service/variations_field_trial_creator.h @@ -6,7 +6,6 @@ #define COMPONENTS_VARIATIONS_SERVICE_VARIATIONS_FIELD_TRIAL_CREATOR_H_ #include <memory> -#include <set> #include <string> #include <unordered_map> #include <vector> @@ -62,8 +61,6 @@ class VariationsFieldTrialCreator { // setup completed successfully. // |kEnableGpuBenchmarking|, |kEnableFeatures|, |kDisableFeatures| are // feature controlling flags not directly accesible from variations. - // |unforcable_field_trials| contains the list of trials that can not be - // overridden. // |variation_ids| allows for forcing ids selected in chrome://flags and/or // specified using the command-line flag. // |low_entropy_provider| allows for field trial randomization. @@ -84,7 +81,6 @@ class VariationsFieldTrialCreator { const char* kEnableGpuBenchmarking, const char* kEnableFeatures, const char* kDisableFeatures, - const std::set<std::string>& unforceable_field_trials, const std::vector<std::string>& variation_ids, const std::vector<base::FeatureList::FeatureOverrideInfo>& extra_overrides, diff --git a/chromium/components/variations/service/variations_field_trial_creator_unittest.cc b/chromium/components/variations/service/variations_field_trial_creator_unittest.cc index 27fead4996a..6729a080f31 100644 --- a/chromium/components/variations/service/variations_field_trial_creator_unittest.cc +++ b/chromium/components/variations/service/variations_field_trial_creator_unittest.cc @@ -246,7 +246,7 @@ class TestVariationsFieldTrialCreator : public VariationsFieldTrialCreator { bool SetupFieldTrials() { TestPlatformFieldTrials platform_field_trials; return VariationsFieldTrialCreator::SetupFieldTrials( - "", "", "", std::set<std::string>(), std::vector<std::string>(), + "", "", "", std::vector<std::string>(), std::vector<base::FeatureList::FeatureOverrideInfo>(), nullptr, std::make_unique<base::FeatureList>(), &platform_field_trials, safe_seed_manager_); @@ -487,7 +487,7 @@ TEST_F(FieldTrialCreatorTest, SetupFieldTrials_LoadsCountryOnFirstRun) { // the interaction between these two classes is what's being tested. auto seed_store = std::make_unique<VariationsSeedStore>( &prefs_, std::move(initial_seed), - /*on_initial_seed_stored=*/base::DoNothing()); + /*signature_verification_enabled=*/false); VariationsFieldTrialCreator field_trial_creator( &prefs_, &variations_service_client, std::move(seed_store), UIStringOverrider()); @@ -497,7 +497,7 @@ TEST_F(FieldTrialCreatorTest, SetupFieldTrials_LoadsCountryOnFirstRun) { // |initial_seed| included the country code for India, this study should be // active. EXPECT_TRUE(field_trial_creator.SetupFieldTrials( - "", "", "", std::set<std::string>(), std::vector<std::string>(), + "", "", "", std::vector<std::string>(), std::vector<base::FeatureList::FeatureOverrideInfo>(), nullptr, std::make_unique<base::FeatureList>(), &platform_field_trials, &safe_seed_manager)); diff --git a/chromium/components/variations/service/variations_service.cc b/chromium/components/variations/service/variations_service.cc index b86adfc671e..3ca55f6bcba 100644 --- a/chromium/components/variations/service/variations_service.cc +++ b/chromium/components/variations/service/variations_service.cc @@ -267,14 +267,6 @@ std::unique_ptr<SeedResponse> MaybeImportFirstRunSeed( return nullptr; } -// Called when the VariationsSeedStore first stores a seed. -void OnInitialSeedStored() { -#if defined(OS_ANDROID) - android::MarkVariationsSeedAsStored(); - android::ClearJavaFirstRunPrefs(); -#endif -} - } // namespace #if defined(OS_CHROMEOS) @@ -377,7 +369,7 @@ VariationsService::VariationsService( std::make_unique<VariationsSeedStore>( local_state, MaybeImportFirstRunSeed(local_state), - base::BindOnce(&OnInitialSeedStored)), + /*signature_verification_enabled=*/true), ui_string_overrider), last_request_was_http_retry_(false) { DCHECK(client_); @@ -699,15 +691,13 @@ bool VariationsService::StoreSeed(const std::string& seed_data, const std::string& country_code, base::Time date_fetched, bool is_delta_compressed, - bool is_gzip_compressed, - bool fetched_insecurely) { + bool is_gzip_compressed) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); std::unique_ptr<VariationsSeed> seed(new VariationsSeed); if (!field_trial_creator_.seed_store()->StoreSeedData( seed_data, seed_signature, country_code, date_fetched, - is_delta_compressed, is_gzip_compressed, fetched_insecurely, - seed.get())) { + is_delta_compressed, is_gzip_compressed, seed.get())) { return false; } @@ -810,19 +800,12 @@ void VariationsService::OnSimpleLoaderCompleteOrRedirect( scoped_refptr<net::HttpResponseHeaders> headers; int response_code = -1; - // We use the final URL HTTPS/HTTP value to pass to StoreSeed, since - // signature validation should be forced on for any HTTP fetch, including - // redirects from HTTPS to HTTP. We default to false since we can't get this - // value (nor will it be used) in the redirect case. - bool final_url_was_https = false; // Variations seed fetches should not follow redirects, so if this request was // redirected, keep the default values for |net_error| and |is_success| (treat // it as a net::ERR_INVALID_REDIRECT), and the fetch will be cancelled when // pending_seed_request is reset. if (!was_redirect) { - final_url_was_https = - pending_seed_request_->GetFinalURL().SchemeIs(url::kHttpsScheme); const network::mojom::URLResponseHead* response_info = pending_seed_request_->ResponseInfo(); if (response_info && response_info->headers) { @@ -919,7 +902,7 @@ void VariationsService::OnSimpleLoaderCompleteOrRedirect( const std::string country_code = GetHeaderValue(headers.get(), "X-Country"); const bool store_success = StoreSeed(*response_body, signature, country_code, response_date, - is_delta_compressed, is_gzip_compressed, !final_url_was_https); + is_delta_compressed, is_gzip_compressed); if (!store_success && is_delta_compressed) { disable_deltas_for_next_request_ = true; // |request_scheduler_| will be null during unit tests. @@ -1016,15 +999,13 @@ bool VariationsService::SetupFieldTrials( const char* kEnableGpuBenchmarking, const char* kEnableFeatures, const char* kDisableFeatures, - const std::set<std::string>& unforceable_field_trials, const std::vector<std::string>& variation_ids, const std::vector<base::FeatureList::FeatureOverrideInfo>& extra_overrides, std::unique_ptr<base::FeatureList> feature_list, variations::PlatformFieldTrials* platform_field_trials) { return field_trial_creator_.SetupFieldTrials( - kEnableGpuBenchmarking, kEnableFeatures, kDisableFeatures, - unforceable_field_trials, variation_ids, extra_overrides, - CreateLowEntropyProvider(), std::move(feature_list), + kEnableGpuBenchmarking, kEnableFeatures, kDisableFeatures, variation_ids, + extra_overrides, CreateLowEntropyProvider(), std::move(feature_list), platform_field_trials, &safe_seed_manager_); } diff --git a/chromium/components/variations/service/variations_service.h b/chromium/components/variations/service/variations_service.h index f57077723ed..af9449648b1 100644 --- a/chromium/components/variations/service/variations_service.h +++ b/chromium/components/variations/service/variations_service.h @@ -6,7 +6,6 @@ #define COMPONENTS_VARIATIONS_SERVICE_VARIATIONS_SERVICE_H_ #include <memory> -#include <set> #include <string> #include <vector> @@ -194,7 +193,6 @@ class VariationsService const char* kEnableGpuBenchmarking, const char* kEnableFeatures, const char* kDisableFeatures, - const std::set<std::string>& unforceable_field_trials, const std::vector<std::string>& variation_ids, const std::vector<base::FeatureList::FeatureOverrideInfo>& extra_overrides, @@ -235,8 +233,7 @@ class VariationsService const std::string& country_code, base::Time date_fetched, bool is_delta_compressed, - bool is_gzip_compressed, - bool fetched_insecurely); + bool is_gzip_compressed); // Create an entropy provider based on low entropy. This is used to create // trials for studies that should only depend on low entropy, such as studies diff --git a/chromium/components/variations/service/variations_service_client.cc b/chromium/components/variations/service/variations_service_client.cc index bda4511f546..80a3df48668 100644 --- a/chromium/components/variations/service/variations_service_client.cc +++ b/chromium/components/variations/service/variations_service_client.cc @@ -5,6 +5,7 @@ #include "components/variations/service/variations_service_client.h" #include "base/command_line.h" +#include "base/logging.h" #include "components/variations/variations_switches.h" namespace variations { diff --git a/chromium/components/variations/service/variations_service_unittest.cc b/chromium/components/variations/service/variations_service_unittest.cc index 75cc1a21aa2..ecad3d15b14 100644 --- a/chromium/components/variations/service/variations_service_unittest.cc +++ b/chromium/components/variations/service/variations_service_unittest.cc @@ -136,8 +136,7 @@ class TestVariationsService : public VariationsService { fetch_attempted_(false), seed_stored_(false), delta_compressed_seed_(false), - gzip_compressed_seed_(false), - insecurely_fetched_seed_(false) { + gzip_compressed_seed_(false) { interception_url_ = GetVariationsServerURL(use_secure_url ? USE_HTTPS : USE_HTTP); set_variations_server_url(interception_url_); @@ -160,7 +159,6 @@ class TestVariationsService : public VariationsService { const std::string& stored_country() const { return stored_country_; } bool delta_compressed_seed() const { return delta_compressed_seed_; } bool gzip_compressed_seed() const { return gzip_compressed_seed_; } - bool insecurely_fetched_seed() const { return insecurely_fetched_seed_; } bool CallMaybeRetryOverHTTP() { return CallMaybeRetryOverHTTPForTesting(); } @@ -188,14 +186,12 @@ class TestVariationsService : public VariationsService { const std::string& country_code, base::Time date_fetched, bool is_delta_compressed, - bool is_gzip_compressed, - bool fetched_insecurely) override { + bool is_gzip_compressed) override { seed_stored_ = true; stored_seed_data_ = seed_data; stored_country_ = country_code; delta_compressed_seed_ = is_delta_compressed; gzip_compressed_seed_ = is_gzip_compressed; - insecurely_fetched_seed_ = fetched_insecurely; RecordSuccessfulFetch(); return true; } @@ -223,7 +219,6 @@ class TestVariationsService : public VariationsService { std::string stored_country_; bool delta_compressed_seed_; bool gzip_compressed_seed_; - bool insecurely_fetched_seed_; DISALLOW_COPY_AND_ASSIGN(TestVariationsService); }; @@ -944,43 +939,6 @@ TEST_F(VariationsServiceTest, FieldTrialCreatorInitializedCorrectly) { service.GetClientFilterableStateForVersionCalledForTesting(); } -TEST_F(VariationsServiceTest, InsecurelyFetchedSetWhenHTTP) { - std::string serialized_seed = SerializeSeed(CreateTestSeed()); - VariationsService::EnableFetchForTesting(); - TestVariationsService service( - std::make_unique<web_resource::TestRequestAllowedNotifier>( - &prefs_, network_tracker_), - &prefs_, GetMetricsStateManager(), false); - service.set_intercepts_fetch(false); - service.test_url_loader_factory()->AddResponse( - service.interception_url().spec(), serialized_seed); - base::HistogramTester histogram_tester; - // Note: We call DoFetchFromURL() here instead of DoActualFetch() since the - // latter doesn't pass true to |http_retry|. - service.DoFetchFromURL(service.interception_url(), true); - base::RunLoop().RunUntilIdle(); - EXPECT_TRUE(service.insecurely_fetched_seed()); - histogram_tester.ExpectUniqueSample( - "Variations.SeedFetchResponseOrErrorCode.HTTP", net::HTTP_OK, 1); -} - -TEST_F(VariationsServiceTest, InsecurelyFetchedNotSetWhenHTTPS) { - std::string serialized_seed = SerializeSeed(CreateTestSeed()); - TestVariationsService service( - std::make_unique<web_resource::TestRequestAllowedNotifier>( - &prefs_, network_tracker_), - &prefs_, GetMetricsStateManager(), true); - VariationsService::EnableFetchForTesting(); - service.set_intercepts_fetch(false); - service.test_url_loader_factory()->AddResponse( - service.interception_url().spec(), serialized_seed); - base::HistogramTester histogram_tester; - service.DoActualFetch(); - EXPECT_FALSE(service.insecurely_fetched_seed()); - histogram_tester.ExpectUniqueSample("Variations.SeedFetchResponseOrErrorCode", - net::HTTP_OK, 1); -} - TEST_F(VariationsServiceTest, RetryOverHTTPIfURLisSet) { TestVariationsService service( std::make_unique<web_resource::TestRequestAllowedNotifier>( diff --git a/chromium/components/variations/study_filtering.cc b/chromium/components/variations/study_filtering.cc index 8705151e0d6..66ac6c77ab7 100644 --- a/chromium/components/variations/study_filtering.cc +++ b/chromium/components/variations/study_filtering.cc @@ -9,6 +9,7 @@ #include <set> +#include "base/logging.h" #include "base/stl_util.h" #include "base/strings/string_util.h" diff --git a/chromium/components/variations/synthetic_trial_registry.cc b/chromium/components/variations/synthetic_trial_registry.cc index ea07f680fbd..c0afd33c474 100644 --- a/chromium/components/variations/synthetic_trial_registry.cc +++ b/chromium/components/variations/synthetic_trial_registry.cc @@ -4,11 +4,32 @@ #include "components/variations/synthetic_trial_registry.h" +#include <algorithm> + +#include "base/metrics/histogram_functions.h" #include "base/stl_util.h" +#include "base/strings/string_number_conversions.h" +#include "components/variations/hashing.h" +#include "components/variations/variations_associated_data.h" namespace variations { +namespace internal { + +const base::Feature kExternalExperimentAllowlist{ + "ExternalExperimentAllowlist", base::FEATURE_ENABLED_BY_DEFAULT}; + +} // namespace internal -SyntheticTrialRegistry::SyntheticTrialRegistry() = default; +SyntheticTrialRegistry::SyntheticTrialRegistry( + bool enable_external_experiment_allowlist) + : enable_external_experiment_allowlist_( + enable_external_experiment_allowlist && + base::FeatureList::IsEnabled( + internal::kExternalExperimentAllowlist)) {} + +SyntheticTrialRegistry::SyntheticTrialRegistry() + : enable_external_experiment_allowlist_(base::FeatureList::IsEnabled( + internal::kExternalExperimentAllowlist)) {} SyntheticTrialRegistry::~SyntheticTrialRegistry() = default; void SyntheticTrialRegistry::AddSyntheticTrialObserver( @@ -23,13 +44,78 @@ void SyntheticTrialRegistry::RemoveSyntheticTrialObserver( synthetic_trial_observer_list_.RemoveObserver(observer); } +void SyntheticTrialRegistry::RegisterExternalExperiments( + const std::string& fallback_study_name, + const std::vector<int>& experiment_ids, + SyntheticTrialRegistry::OverrideMode mode) { + DCHECK(!fallback_study_name.empty()); + + base::FieldTrialParams params; + if (enable_external_experiment_allowlist_ && + !GetFieldTrialParamsByFeature(internal::kExternalExperimentAllowlist, + ¶ms)) { + return; + } + + // When overriding previous external experiments, remove them now. + if (mode == kOverrideExistingIds) { + auto is_external = [](const SyntheticTrialGroup& group) { + return group.is_external; + }; + base::EraseIf(synthetic_trial_groups_, is_external); + } + + const base::TimeTicks start_time = base::TimeTicks::Now(); + int trials_added = 0; + for (int experiment_id : experiment_ids) { + const std::string experiment_id_str = base::NumberToString(experiment_id); + const base::StringPiece study_name = + GetStudyNameForExpId(fallback_study_name, params, experiment_id_str); + if (study_name.empty()) + continue; + + const uint32_t trial_hash = HashName(study_name); + // If existing ids shouldn't be overridden, skip entries whose study names + // are already registered. + if (mode == kDoNotOverrideExistingIds) { + auto matches_trial = [trial_hash](const SyntheticTrialGroup& group) { + return group.id.name == trial_hash; + }; + const auto& groups = synthetic_trial_groups_; + if (std::any_of(groups.begin(), groups.end(), matches_trial)) { + continue; + } + } + + const uint32_t group_hash = HashName(experiment_id_str); + + // Since external experiments are not based on Chrome's low entropy source, + // they are only sent to Google web properties for signed-in users to make + // sure they couldn't be used to identify a user that's not signed-in. + AssociateGoogleVariationIDForceHashes( + GOOGLE_WEB_PROPERTIES_SIGNED_IN, {trial_hash, group_hash}, + static_cast<VariationID>(experiment_id)); + SyntheticTrialGroup entry(trial_hash, group_hash); + entry.start_time = start_time; + entry.is_external = true; + synthetic_trial_groups_.push_back(entry); + trials_added++; + } + + base::UmaHistogramCounts100("UMA.ExternalExperiment.GroupCount", + trials_added); + + if (trials_added > 0) + NotifySyntheticTrialObservers(); +} + void SyntheticTrialRegistry::RegisterSyntheticFieldTrial( const SyntheticTrialGroup& trial) { - for (size_t i = 0; i < synthetic_trial_groups_.size(); ++i) { - if (synthetic_trial_groups_[i].id.name == trial.id.name) { - if (synthetic_trial_groups_[i].id.group != trial.id.group) { - synthetic_trial_groups_[i].id.group = trial.id.group; - synthetic_trial_groups_[i].start_time = base::TimeTicks::Now(); + for (auto& entry : synthetic_trial_groups_) { + if (entry.id.name == trial.id.name) { + if (entry.id.group != trial.id.group) { + entry.id.group = trial.id.group; + entry.start_time = base::TimeTicks::Now(); NotifySyntheticTrialObservers(); } return; @@ -42,26 +128,26 @@ void SyntheticTrialRegistry::RegisterSyntheticFieldTrial( NotifySyntheticTrialObservers(); } -void SyntheticTrialRegistry::RegisterSyntheticMultiGroupFieldTrial( - uint32_t trial_name_hash, - const std::vector<uint32_t>& group_name_hashes) { - auto has_same_trial_name = [trial_name_hash](const SyntheticTrialGroup& x) { - return x.id.name == trial_name_hash; - }; - base::EraseIf(synthetic_trial_groups_, has_same_trial_name); +base::StringPiece SyntheticTrialRegistry::GetStudyNameForExpId( + const std::string& fallback_study_name, + const base::FieldTrialParams& params, + const std::string& experiment_id) { + if (!enable_external_experiment_allowlist_) + return fallback_study_name; - if (group_name_hashes.empty()) - return; + const auto it = params.find(experiment_id); + if (it == params.end()) + return base::StringPiece(); - SyntheticTrialGroup trial_group(trial_name_hash, group_name_hashes[0]); - trial_group.start_time = base::TimeTicks::Now(); - for (uint32_t group_name_hash : group_name_hashes) { - // Note: Adding the trial group will copy it, so this re-uses the same - // |trial_group| struct for convenience (e.g. so start_time's all match). - trial_group.id.group = group_name_hash; - synthetic_trial_groups_.push_back(trial_group); - } - NotifySyntheticTrialObservers(); + // To support additional parameters being passed, besides the study name, + // truncate the study name at the first ',' character. + // For example, for an entry like {"1234": "StudyName,FOO"}, we only want the + // "StudyName" part. This allows adding support for additional things like FOO + // in the future without backwards compatibility problems. + const size_t comma_pos = it->second.find(','); + const size_t truncate_pos = + (comma_pos == std::string::npos ? it->second.length() : comma_pos); + return base::StringPiece(it->second.data(), truncate_pos); } void SyntheticTrialRegistry::NotifySyntheticTrialObservers() { @@ -72,12 +158,12 @@ void SyntheticTrialRegistry::NotifySyntheticTrialObservers() { void SyntheticTrialRegistry::GetSyntheticFieldTrialsOlderThan( base::TimeTicks time, - std::vector<ActiveGroupId>* synthetic_trials) { + std::vector<ActiveGroupId>* synthetic_trials) const { DCHECK(synthetic_trials); synthetic_trials->clear(); - for (size_t i = 0; i < synthetic_trial_groups_.size(); ++i) { - if (synthetic_trial_groups_[i].start_time <= time) - synthetic_trials->push_back(synthetic_trial_groups_[i].id); + for (const auto& entry : synthetic_trial_groups_) { + if (entry.start_time <= time) + synthetic_trials->push_back(entry.id); } } diff --git a/chromium/components/variations/synthetic_trial_registry.h b/chromium/components/variations/synthetic_trial_registry.h index 4be74affa05..1509c351785 100644 --- a/chromium/components/variations/synthetic_trial_registry.h +++ b/chromium/components/variations/synthetic_trial_registry.h @@ -7,6 +7,8 @@ #include <vector> +#include "base/feature_list.h" +#include "base/metrics/field_trial_params.h" #include "base/observer_list.h" #include "components/variations/synthetic_trials.h" @@ -19,9 +21,22 @@ namespace variations { struct ActiveGroupId; class FieldTrialsProvider; class FieldTrialsProviderTest; +class SyntheticTrialRegistryTest; + +namespace internal { +extern const base::Feature kExternalExperimentAllowlist; +} // namespace internal class SyntheticTrialRegistry { public: + // Constructor that specifies whether the SyntheticTrialRegistry should use + // an allowlist for external experiments. Some embedders such as WebLayer + // do not run as Chrome and do not use the allowlist. + // Note: The allowlist is enabled only if |kExternalExperimentAllowlist| is + // also enabled, even if the parameter value is true. The default constructor + // defaults to the feature state. + explicit SyntheticTrialRegistry(bool enable_external_experiment_allowlist); + SyntheticTrialRegistry(); ~SyntheticTrialRegistry(); @@ -31,14 +46,42 @@ class SyntheticTrialRegistry { // Removes an existing observer of synthetic trials list changes. void RemoveSyntheticTrialObserver(SyntheticTrialObserver* observer); + // Specifies the mode of RegisterExternalExperiments() operation. + enum OverrideMode { + // Previously-registered external experiment ids are overridden (replaced) + // with the new list. + kOverrideExistingIds, + // Previously-registered external experiment ids are not overridden, but + // new experiment ids may be added. + kDoNotOverrideExistingIds, + }; + + // Registers a list of experiment ids coming from an external application. + // The input ids are in the VariationID format. + // + // When |enable_external_experiment_allowlist| is true, the supplied ids must + // have corresponding entries in the "ExternalExperimentAllowlist" (coming via + // a feature param) to be applied. The allowlist also supplies the + // corresponding trial name that should be used for reporting to UMA. + // + // When |enable_external_experiment_allowlist| is false, |fallback_study_name| + // will be used as the trial name for all provided experiment ids. + // + // If |mode| is kOverrideExistingIds, this API clears previously-registered + // external experiment ids, replacing them with the new list (which may be + // empty). If |mode| is kDoNotOverrideExistingIds, any new ids that are not + // already registered will be added, but existing ones will not be replaced. + void RegisterExternalExperiments(const std::string& fallback_study_name, + const std::vector<int>& experiment_ids, + OverrideMode mode); + private: friend metrics::MetricsServiceAccessor; friend FieldTrialsProvider; friend FieldTrialsProviderTest; + friend SyntheticTrialRegistryTest; FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest, RegisterSyntheticTrial); FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest, - RegisterSyntheticMultiGroupFieldTrial); - FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest, GetSyntheticFieldTrialActiveGroups); FRIEND_TEST_ALL_PREFIXES(VariationsCrashKeysTest, BasicFunctionality); @@ -50,24 +93,29 @@ class SyntheticTrialRegistry { // is registered for a given trial name will be recorded. The values passed // in must not correspond to any real field trial in the code. // Note: Should not be used to replace trials that were registered with - // RegisterSyntheticMultiGroupFieldTrial(). + // RegisterExternalExperiments(). void RegisterSyntheticFieldTrial(const SyntheticTrialGroup& trial_group); - // Similar to RegisterSyntheticFieldTrial(), but registers a synthetic trial - // that has multiple active groups for a given trial name hash. Any previous - // groups registered for |trial_name_hash| will be replaced. - void RegisterSyntheticMultiGroupFieldTrial( - uint32_t trial_name_hash, - const std::vector<uint32_t>& group_name_hashes); + // Returns the study name corresponding to |experiment_id| from the allowlist + // contained in |params| if the allowlist is enabled, otherwise returns + // |fallback_study_name|. An empty string piece is returned when the + // experiment is not in the allowlist. + base::StringPiece GetStudyNameForExpId(const std::string& fallback_study_name, + const base::FieldTrialParams& params, + const std::string& experiment_id); // Returns a list of synthetic field trials that are older than |time|. void GetSyntheticFieldTrialsOlderThan( base::TimeTicks time, - std::vector<ActiveGroupId>* synthetic_trials); + std::vector<ActiveGroupId>* synthetic_trials) const; // Notifies observers on a synthetic trial list change. void NotifySyntheticTrialObservers(); + // Whether the allowlist is enabled. Some configurations, like WebLayer + // do not use the allowlist. + bool enable_external_experiment_allowlist_ = true; + // Field trial groups that map to Chrome configuration states. std::vector<SyntheticTrialGroup> synthetic_trial_groups_; diff --git a/chromium/components/variations/synthetic_trial_registry_unittest.cc b/chromium/components/variations/synthetic_trial_registry_unittest.cc index b95f55ebd9b..7bb77445781 100644 --- a/chromium/components/variations/synthetic_trial_registry_unittest.cc +++ b/chromium/components/variations/synthetic_trial_registry_unittest.cc @@ -8,6 +8,7 @@ #include "base/metrics/field_trial.h" #include "base/strings/stringprintf.h" +#include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" #include "components/variations/active_field_trials.h" #include "components/variations/hashing.h" @@ -17,8 +18,6 @@ namespace variations { -namespace { - class SyntheticTrialRegistryTest : public ::testing::Test { public: SyntheticTrialRegistryTest() { InitCrashKeys(); } @@ -46,14 +45,21 @@ class SyntheticTrialRegistryTest : public ::testing::Test { } } + // Gets the current synthetic trials. + void GetSyntheticTrials(const SyntheticTrialRegistry& registry, + std::vector<ActiveGroupId>* synthetic_trials) { + // Ensure that time has advanced by at least a tick before proceeding. + WaitUntilTimeChanges(base::TimeTicks::Now()); + registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(), + synthetic_trials); + } + private: base::test::SingleThreadTaskEnvironment task_environment_; DISALLOW_COPY_AND_ASSIGN(SyntheticTrialRegistryTest); }; -} // namespace - TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticTrial) { SyntheticTrialRegistry registry; @@ -95,57 +101,89 @@ TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticTrial) { EXPECT_EQ(1U, synthetic_trials.size()); EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial2", "Group2")); - // Ensure that time has advanced by at least a tick before proceeding. - WaitUntilTimeChanges(base::TimeTicks::Now()); - // Start a new log and ensure all three trials appear in it. - registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(), - &synthetic_trials); + GetSyntheticTrials(registry, &synthetic_trials); EXPECT_EQ(3U, synthetic_trials.size()); EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "Group2")); EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial2", "Group2")); EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial3", "Group3")); } -TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticMultiGroupFieldTrial) { - SyntheticTrialRegistry registry; - - // Register a synthetic trial TestTrial1 with groups A and B. - uint32_t trial_name_hash = HashName("TestTrial1"); - std::vector<uint32_t> group_name_hashes = {HashName("A"), HashName("B")}; - registry.RegisterSyntheticMultiGroupFieldTrial(trial_name_hash, - group_name_hashes); - // Ensure that time has advanced by at least a tick before proceeding. - WaitUntilTimeChanges(base::TimeTicks::Now()); +TEST_F(SyntheticTrialRegistryTest, RegisterExternalExperiments_NoAllowlist) { + SyntheticTrialRegistry registry(false); + const std::string context = "TestTrial1"; + const auto mode = SyntheticTrialRegistry::kOverrideExistingIds; + registry.RegisterExternalExperiments(context, {100, 200}, mode); std::vector<ActiveGroupId> synthetic_trials; - registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(), - &synthetic_trials); + GetSyntheticTrials(registry, &synthetic_trials); EXPECT_EQ(2U, synthetic_trials.size()); - EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "A")); - EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "B")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "100")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "200")); // Change the group for the trial to a single group. - group_name_hashes = {HashName("X")}; - registry.RegisterSyntheticMultiGroupFieldTrial(trial_name_hash, - group_name_hashes); - // Ensure that time has advanced by at least a tick before proceeding. - WaitUntilTimeChanges(base::TimeTicks::Now()); - - registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(), - &synthetic_trials); + registry.RegisterExternalExperiments(context, {500}, mode); + GetSyntheticTrials(registry, &synthetic_trials); EXPECT_EQ(1U, synthetic_trials.size()); - EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "X")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "500")); // Register a trial with no groups, which should effectively remove the trial. - group_name_hashes.clear(); - registry.RegisterSyntheticMultiGroupFieldTrial(trial_name_hash, - group_name_hashes); - // Ensure that time has advanced by at least a tick before proceeding. - WaitUntilTimeChanges(base::TimeTicks::Now()); + registry.RegisterExternalExperiments(context, {}, mode); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(0U, synthetic_trials.size()); +} - registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(), - &synthetic_trials); +TEST_F(SyntheticTrialRegistryTest, RegisterExternalExperiments_WithAllowlist) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeatureWithParameters( + internal::kExternalExperimentAllowlist, + {{"100", "A"}, {"101", "A"}, {"300", "C,xyz"}}); + + const std::string context = "Test"; + const auto override_mode = SyntheticTrialRegistry::kOverrideExistingIds; + SyntheticTrialRegistry registry; + std::vector<ActiveGroupId> synthetic_trials; + + // Register a synthetic trial TestTrial1 with groups A and B. + registry.RegisterExternalExperiments(context, {100, 200, 300}, override_mode); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(2U, synthetic_trials.size()); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "100")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300")); + + // A new call that only contains 100 will clear the other ones. + registry.RegisterExternalExperiments(context, {101}, override_mode); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(1U, synthetic_trials.size()); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101")); + + const auto dont_override = SyntheticTrialRegistry::kDoNotOverrideExistingIds; + // Now, register another id that doesn't exist with kDoNotOverrideExistingIds. + registry.RegisterExternalExperiments(context, {300}, dont_override); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(2U, synthetic_trials.size()); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300")); + + // Registering 100, which already has a trial A registered, shouldn't work. + registry.RegisterExternalExperiments(context, {100}, dont_override); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(2U, synthetic_trials.size()); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300")); + + // Registering an empty set should also do nothing. + registry.RegisterExternalExperiments(context, {}, dont_override); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(2U, synthetic_trials.size()); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101")); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300")); + + // Registering with an override should reset existing ones. + registry.RegisterExternalExperiments(context, {100}, override_mode); + GetSyntheticTrials(registry, &synthetic_trials); + EXPECT_EQ(1U, synthetic_trials.size()); + EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "100")); } TEST_F(SyntheticTrialRegistryTest, GetSyntheticFieldTrialActiveGroups) { diff --git a/chromium/components/variations/synthetic_trials.h b/chromium/components/variations/synthetic_trials.h index fa47e64ba76..89cbd1bff67 100644 --- a/chromium/components/variations/synthetic_trials.h +++ b/chromium/components/variations/synthetic_trials.h @@ -25,6 +25,9 @@ struct SyntheticTrialGroup { ActiveGroupId id; base::TimeTicks start_time; + + // If this is an external experiment. + bool is_external = false; }; // Interface class to observe changes to synthetic trials in MetricsService. diff --git a/chromium/components/variations/variations_client.h b/chromium/components/variations/variations_client.h index 987695bbf61..40385777954 100644 --- a/chromium/components/variations/variations_client.h +++ b/chromium/components/variations/variations_client.h @@ -14,10 +14,10 @@ class VariationsClient { public: virtual ~VariationsClient() = default; - // Returns whether the user is operating in an incognito context. + // Returns whether the user is operating in an OffTheRecord context. // Note components/variations code can't call the BrowserContext method // directly or we'd end up with a circular dependency. - virtual bool IsIncognito() const = 0; + virtual bool IsOffTheRecord() const = 0; // Returns the variations header that should be appended for google requests. virtual std::string GetVariationsHeader() const = 0; diff --git a/chromium/components/variations/variations_http_header_provider.cc b/chromium/components/variations/variations_http_header_provider.cc index d620cc5af5d..7433a3a638d 100644 --- a/chromium/components/variations/variations_http_header_provider.cc +++ b/chromium/components/variations/variations_http_header_provider.cc @@ -6,6 +6,7 @@ #include <stddef.h> +#include <algorithm> #include <set> #include <string> #include <vector> @@ -85,24 +86,16 @@ std::string VariationsHttpHeaderProvider::GetVariationsString() { std::vector<VariationID> VariationsHttpHeaderProvider::GetVariationsVector( IDCollectionKey key) { - InitVariationIDsCacheIfNeeded(); - - // Get all the active variation ids while holding the lock. - std::set<VariationIDEntry> all_variation_ids; - { - base::AutoLock scoped_lock(lock_); - all_variation_ids = GetAllVariationIds(); - } + return GetVariationsVectorImpl(std::set<IDCollectionKey>{key}); +} - // Copy the requested variations to the output vector. Note that the ids will - // be in sorted order because they're coming from a std::set. - std::vector<VariationID> result; - result.reserve(all_variation_ids.size()); - for (const VariationIDEntry& entry : all_variation_ids) { - if (entry.second == key) - result.push_back(entry.first); - } - return result; +std::vector<VariationID> +VariationsHttpHeaderProvider::GetVariationsVectorForWebPropertiesKeys() { + const std::set<IDCollectionKey> web_properties_keys{ + variations::GOOGLE_WEB_PROPERTIES, + variations::GOOGLE_WEB_PROPERTIES_SIGNED_IN, + variations::GOOGLE_WEB_PROPERTIES_TRIGGER}; + return GetVariationsVectorImpl(web_properties_keys); } VariationsHttpHeaderProvider::ForceIdsResult @@ -357,4 +350,29 @@ VariationsHttpHeaderProvider::GetAllVariationIds() { return all_variation_ids_set; } +std::vector<VariationID> VariationsHttpHeaderProvider::GetVariationsVectorImpl( + const std::set<IDCollectionKey>& keys) { + InitVariationIDsCacheIfNeeded(); + + // Get all the active variation ids while holding the lock. + std::set<VariationIDEntry> all_variation_ids; + { + base::AutoLock scoped_lock(lock_); + all_variation_ids = GetAllVariationIds(); + } + + // Copy the requested variations to the output vector. + std::vector<VariationID> result; + result.reserve(all_variation_ids.size()); + for (const VariationIDEntry& entry : all_variation_ids) { + if (keys.find(entry.second) != keys.end()) + result.push_back(entry.first); + } + + // Make sure each enry is unique. As a side-effect, the output will be sorted. + std::sort(result.begin(), result.end()); + result.erase(std::unique(result.begin(), result.end()), result.end()); + return result; +} + } // namespace variations diff --git a/chromium/components/variations/variations_http_header_provider.h b/chromium/components/variations/variations_http_header_provider.h index bf324c7ef80..206fdbaaa65 100644 --- a/chromium/components/variations/variations_http_header_provider.h +++ b/chromium/components/variations/variations_http_header_provider.h @@ -64,10 +64,14 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer, // apps. std::string GetGoogleAppVariationsString(); - // Returns the collection of of variation ids matching the given |key|. Each + // Returns the collection of variation ids matching the given |key|. Each // entry in the returned vector will be unique. std::vector<VariationID> GetVariationsVector(IDCollectionKey key); + // Returns the collection of variations ids for all Google Web Properties + // related keys. + std::vector<VariationID> GetVariationsVectorForWebPropertiesKeys(); + // Result of ForceVariationIds() call. enum class ForceIdsResult { SUCCESS, @@ -121,6 +125,10 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer, GetVariationsString); FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest, GetVariationsVector); + FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest, + GetVariationsVectorForWebPropertiesKeys); + FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest, + GetVariationsVectorImpl); VariationsHttpHeaderProvider(); ~VariationsHttpHeaderProvider() override; @@ -175,6 +183,11 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer, // default values, synthetic variations and actual field trial variations. std::set<VariationIDEntry> GetAllVariationIds(); + // Returns the collection of variation ids matching any of the given + // |keys|. Each entry in the returned vector will be unique. + std::vector<VariationID> GetVariationsVectorImpl( + const std::set<IDCollectionKey>& key); + // Guards access to variables below. base::Lock lock_; diff --git a/chromium/components/variations/variations_http_header_provider_unittest.cc b/chromium/components/variations/variations_http_header_provider_unittest.cc index e4bdf31630d..1521f93e4ad 100644 --- a/chromium/components/variations/variations_http_header_provider_unittest.cc +++ b/chromium/components/variations/variations_http_header_provider_unittest.cc @@ -263,4 +263,41 @@ TEST_F(VariationsHttpHeaderProviderTest, GetVariationsVector) { provider.GetVariationsVector(GOOGLE_APP)); } +TEST_F(VariationsHttpHeaderProviderTest, + GetVariationsVectorForWebPropertiesKeys) { + CreateTrialAndAssociateId("t1", "g1", GOOGLE_WEB_PROPERTIES, 121); + CreateTrialAndAssociateId("t2", "g2", GOOGLE_WEB_PROPERTIES_TRIGGER, 122); + CreateTrialAndAssociateId("t3", "g3", GOOGLE_WEB_PROPERTIES_SIGNED_IN, 123); + CreateTrialAndAssociateId("t4", "g4", GOOGLE_APP, 124); // Will be excluded. + VariationsHttpHeaderProvider provider; + provider.ForceVariationIds({"100", "t101"}, ""); + EXPECT_EQ((std::vector<VariationID>{100, 101, 121, 122, 123}), + provider.GetVariationsVectorForWebPropertiesKeys()); +} + +TEST_F(VariationsHttpHeaderProviderTest, GetVariationsVectorImpl) { + base::test::SingleThreadTaskEnvironment task_environment; + CreateTrialAndAssociateId("t1", "g1", GOOGLE_WEB_PROPERTIES, 121); + CreateTrialAndAssociateId("t2", "g2", GOOGLE_WEB_PROPERTIES, 122); + CreateTrialAndAssociateId("t3", "g3", GOOGLE_WEB_PROPERTIES_TRIGGER, 123); + CreateTrialAndAssociateId("t4", "g4", GOOGLE_WEB_PROPERTIES_TRIGGER, 124); + CreateTrialAndAssociateId("t5", "g5", GOOGLE_WEB_PROPERTIES_SIGNED_IN, 125); + CreateTrialAndAssociateId("t6", "g6", GOOGLE_WEB_PROPERTIES_SIGNED_IN, + 124); // Note: Duplicate. + CreateTrialAndAssociateId("t7", "g7", GOOGLE_APP, 126); + + VariationsHttpHeaderProvider provider; + provider.ForceVariationIds({"100", "200", "t101"}, ""); + + EXPECT_EQ((std::vector<VariationID>{100, 101, 121, 122, 123, 124, 200}), + provider.GetVariationsVectorImpl( + {GOOGLE_WEB_PROPERTIES, GOOGLE_WEB_PROPERTIES_TRIGGER})); + EXPECT_EQ((std::vector<VariationID>{101, 123, 124, 125}), + provider.GetVariationsVectorImpl({GOOGLE_WEB_PROPERTIES_SIGNED_IN, + GOOGLE_WEB_PROPERTIES_TRIGGER})); + EXPECT_EQ((std::vector<VariationID>{124, 125, 126}), + provider.GetVariationsVectorImpl( + {GOOGLE_APP, GOOGLE_WEB_PROPERTIES_SIGNED_IN})); +} + } // namespace variations diff --git a/chromium/components/variations/variations_seed_store.cc b/chromium/components/variations/variations_seed_store.cc index b54cca36172..32fb7eee076 100644 --- a/chromium/components/variations/variations_seed_store.cc +++ b/chromium/components/variations/variations_seed_store.cc @@ -9,6 +9,7 @@ #include "base/base64.h" #include "base/bind_helpers.h" +#include "base/logging.h" #include "base/macros.h" #include "base/metrics/histogram_macros.h" #include "base/numerics/safe_math.h" @@ -112,22 +113,21 @@ UpdateSeedDateResult GetSeedDateChangeState( } // namespace VariationsSeedStore::VariationsSeedStore(PrefService* local_state) - : VariationsSeedStore(local_state, nullptr, base::DoNothing()) {} + : VariationsSeedStore(local_state, nullptr, true) {} VariationsSeedStore::VariationsSeedStore( PrefService* local_state, std::unique_ptr<SeedResponse> initial_seed, - base::OnceCallback<void()> on_initial_seed_stored) + bool signature_verification_enabled) : local_state_(local_state), - on_initial_seed_stored_(std::move(on_initial_seed_stored)) { + signature_verification_enabled_(signature_verification_enabled) { #if defined(OS_ANDROID) if (initial_seed) ImportInitialSeed(std::move(initial_seed)); #endif // OS_ANDROID } -VariationsSeedStore::~VariationsSeedStore() { -} +VariationsSeedStore::~VariationsSeedStore() = default; bool VariationsSeedStore::LoadSeed(VariationsSeed* seed, std::string* seed_data, @@ -149,7 +149,6 @@ bool VariationsSeedStore::StoreSeedData( const base::Time& date_fetched, bool is_delta_compressed, bool is_gzip_compressed, - bool fetched_insecurely, VariationsSeed* parsed_seed) { UMA_HISTOGRAM_COUNTS_1000("Variations.StoreSeed.DataSize", data.length() / 1024); @@ -181,8 +180,7 @@ bool VariationsSeedStore::StoreSeedData( if (!is_delta_compressed) { return StoreSeedDataNoDelta(ungzipped_data, base64_seed_signature, - country_code, date_fetched, fetched_insecurely, - parsed_seed); + country_code, date_fetched, parsed_seed); } // If the data is delta compressed, first decode it. @@ -200,9 +198,9 @@ bool VariationsSeedStore::StoreSeedData( return false; } - const bool result = StoreSeedDataNoDelta( - updated_seed_data, base64_seed_signature, country_code, date_fetched, - fetched_insecurely, parsed_seed); + const bool result = + StoreSeedDataNoDelta(updated_seed_data, base64_seed_signature, + country_code, date_fetched, parsed_seed); if (!result) RecordStoreSeedResult(StoreSeedResult::FAILED_DELTA_STORE); return result; @@ -237,9 +235,9 @@ bool VariationsSeedStore::StoreSafeSeed( const ClientFilterableState& client_state, base::Time seed_fetch_time) { std::string base64_seed_data; - StoreSeedResult result = VerifyAndCompressSeedData( - seed_data, base64_seed_signature, false /* fetched_insecurely */, - SeedType::SAFE, &base64_seed_data, nullptr); + StoreSeedResult result = + VerifyAndCompressSeedData(seed_data, base64_seed_signature, + SeedType::SAFE, &base64_seed_data, nullptr); UMA_HISTOGRAM_ENUMERATION("Variations.SafeMode.StoreSafeSeed.Result", result, StoreSeedResult::ENUM_SIZE); if (result != StoreSeedResult::SUCCESS) @@ -378,17 +376,6 @@ void VariationsSeedStore::RegisterPrefs(PrefRegistrySimple* registry) { std::string()); } -bool VariationsSeedStore::SignatureVerificationEnabled() { -#if defined(OS_IOS) || defined(OS_ANDROID) - // Signature verification is disabled on mobile platforms for now, since it - // adds about ~15ms to the startup time on mobile (vs. a couple ms on - // desktop). - return false; -#else - return true; -#endif -} - void VariationsSeedStore::ClearPrefs(SeedType seed_type) { if (seed_type == SeedType::LATEST) { local_state_->ClearPref(prefs::kVariationsCompressedSeed); @@ -413,11 +400,19 @@ void VariationsSeedStore::ClearPrefs(SeedType seed_type) { void VariationsSeedStore::ImportInitialSeed( std::unique_ptr<SeedResponse> initial_seed) { if (initial_seed->data.empty()) { + // Note: This is an expected case on non-first run starts. RecordFirstRunSeedImportResult( FirstRunSeedImportResult::FAIL_NO_FIRST_RUN_SEED); return; } + // Clear the Java-side seed prefs. At this point, the seed has + // already been fetched from the Java side, so it's no longer + // needed there. This is done regardless if we fail or succeed + // below - since if we succeed, we're good to go and if we fail, + // we probably don't want to keep around the bad content anyway. + android::ClearJavaFirstRunPrefs(); + if (initial_seed->date == 0) { RecordFirstRunSeedImportResult( FirstRunSeedImportResult::FAIL_INVALID_RESPONSE_DATE); @@ -428,7 +423,7 @@ void VariationsSeedStore::ImportInitialSeed( if (!StoreSeedData(initial_seed->data, initial_seed->signature, initial_seed->country, date, false, - initial_seed->is_gzip_compressed, false, nullptr)) { + initial_seed->is_gzip_compressed, nullptr)) { RecordFirstRunSeedImportResult(FirstRunSeedImportResult::FAIL_STORE_FAILED); LOG(WARNING) << "First run variations seed is invalid."; return; @@ -449,7 +444,7 @@ LoadSeedResult VariationsSeedStore::LoadSeedImpl( *base64_seed_signature = local_state_->GetString( seed_type == SeedType::LATEST ? prefs::kVariationsSeedSignature : prefs::kVariationsSafeSeedSignature); - if (SignatureVerificationEnabled()) { + if (signature_verification_enabled_) { const VerifySignatureResult result = VerifySeedSignature(*seed_data, *base64_seed_signature); if (seed_type == SeedType::LATEST) { @@ -509,26 +504,23 @@ bool VariationsSeedStore::StoreSeedDataNoDelta( const std::string& base64_seed_signature, const std::string& country_code, const base::Time& date_fetched, - bool fetched_insecurely, VariationsSeed* parsed_seed) { std::string base64_seed_data; VariationsSeed seed; - StoreSeedResult result = VerifyAndCompressSeedData( - seed_data, base64_seed_signature, fetched_insecurely, SeedType::LATEST, - &base64_seed_data, &seed); + StoreSeedResult result = + VerifyAndCompressSeedData(seed_data, base64_seed_signature, + SeedType::LATEST, &base64_seed_data, &seed); if (result != StoreSeedResult::SUCCESS) { RecordStoreSeedResult(result); return false; } - // This callback is only useful on Chrome for Android. - if (!on_initial_seed_stored_.is_null()) { - // If currently we do not have any stored pref then we mark seed storing as - // successful on the Java side to avoid repeated seed fetches and clear - // preferences on the Java side. - if (local_state_->GetString(prefs::kVariationsCompressedSeed).empty()) - std::move(on_initial_seed_stored_).Run(); - } +#if defined(OS_ANDROID) + // If currently we do not have any stored pref then we mark seed storing as + // successful on the Java side to avoid repeated seed fetches. + if (local_state_->GetString(prefs::kVariationsCompressedSeed).empty()) + android::MarkVariationsSeedAsStored(); +#endif // Update the saved country code only if one was returned from the server. if (!country_code.empty()) @@ -556,7 +548,6 @@ bool VariationsSeedStore::StoreSeedDataNoDelta( StoreSeedResult VariationsSeedStore::VerifyAndCompressSeedData( const std::string& seed_data, const std::string& base64_seed_signature, - bool fetched_insecurely, SeedType seed_type, std::string* base64_seed_data, VariationsSeed* parsed_seed) { @@ -568,7 +559,7 @@ StoreSeedResult VariationsSeedStore::VerifyAndCompressSeedData( if (!seed.ParseFromString(seed_data)) return StoreSeedResult::FAILED_PARSE; - if (SignatureVerificationEnabled() || fetched_insecurely) { + if (signature_verification_enabled_) { const VerifySignatureResult result = VerifySeedSignature(seed_data, base64_seed_signature); switch (seed_type) { diff --git a/chromium/components/variations/variations_seed_store.h b/chromium/components/variations/variations_seed_store.h index 839c5b6b692..3ee8dd8a78f 100644 --- a/chromium/components/variations/variations_seed_store.h +++ b/chromium/components/variations/variations_seed_store.h @@ -29,14 +29,16 @@ class VariationsSeed; // seed from Local State. class VariationsSeedStore { public: + // Standard constructor. Enables signature verification. explicit VariationsSeedStore(PrefService* local_state); // |initial_seed| may be null. If not null, then it will be stored in this // seed store. This is used by Android Chrome to supply the first run seed, // and by Android WebView to supply the seed on every run. - // |on_initial_seed_stored| will be called the first time a seed is stored. + // |signature_verification_enabled| can be used in unit tests to disable + // signature checks on the seed. VariationsSeedStore(PrefService* local_state, std::unique_ptr<SeedResponse> initial_seed, - base::OnceCallback<void()> on_initial_seed_stored); + bool signature_verification_enabled); virtual ~VariationsSeedStore(); // Loads the variations seed data from local state into |seed|, as well as the @@ -65,7 +67,6 @@ class VariationsSeedStore { const base::Time& date_fetched, bool is_delta_compressed, bool is_gzip_compressed, - bool fetched_insecurely, VariationsSeed* parsed_seed) WARN_UNUSED_RESULT; // Loads the safe variations seed data from local state into |seed| and @@ -120,10 +121,6 @@ class VariationsSeedStore { PrefService* local_state() { return local_state_; } const PrefService* local_state() const { return local_state_; } - protected: - // Whether signature verification is enabled. Overridable for tests. - virtual bool SignatureVerificationEnabled(); - private: FRIEND_TEST_ALL_PREFIXES(VariationsSeedStoreTest, VerifySeedSignature); FRIEND_TEST_ALL_PREFIXES(VariationsSeedStoreTest, ApplyDeltaPatch); @@ -179,21 +176,17 @@ class VariationsSeedStore { const std::string& base64_seed_signature, const std::string& country_code, const base::Time& date_fetched, - bool fetched_insecurely, VariationsSeed* parsed_seed) WARN_UNUSED_RESULT; // Validates the |seed_data|, comparing it (if enabled) against the provided // cryptographic signature. Returns the result of the operation. On success, // fills |base64_seed_data| with the compressed and base64-encoded seed data; // and if |parsed_seed| is non-null, fills it with the parsed seed data. - // |fetched_insecurely| indicates whether |seed_data| was just fetched from - // the server over an insecure channel (i.e. over HTTP rathern than HTTPS). // |seed_type| specifies whether |seed_data| is for the safe seed (vs. the // regular/normal seed). StoreSeedResult VerifyAndCompressSeedData( const std::string& seed_data, const std::string& base64_seed_signature, - bool fetched_insecurely, SeedType seed_type, std::string* base64_seed_data, VariationsSeed* parsed_seed) WARN_UNUSED_RESULT; @@ -210,7 +203,8 @@ class VariationsSeedStore { // Cached serial number from the most recently fetched variations seed. std::string latest_serial_number_; - base::OnceCallback<void()> on_initial_seed_stored_; + // Whether to validate signatures on the seed. Always on except in tests. + bool signature_verification_enabled_ = true; DISALLOW_COPY_AND_ASSIGN(VariationsSeedStore); }; diff --git a/chromium/components/variations/variations_seed_store_unittest.cc b/chromium/components/variations/variations_seed_store_unittest.cc index 91b03c19f7f..c37c6ed31ef 100644 --- a/chromium/components/variations/variations_seed_store_unittest.cc +++ b/chromium/components/variations/variations_seed_store_unittest.cc @@ -8,6 +8,7 @@ #include <utility> #include "base/base64.h" +#include "base/bind_helpers.h" #include "base/macros.h" #include "base/test/metrics/histogram_tester.h" #include "base/time/time.h" @@ -51,16 +52,16 @@ constexpr char kIdenticalToSafeSeedSentinel[] = "safe_seed_content"; class TestVariationsSeedStore : public VariationsSeedStore { public: explicit TestVariationsSeedStore(PrefService* local_state) - : VariationsSeedStore(local_state) {} - ~TestVariationsSeedStore() override {} + : VariationsSeedStore(local_state, + nullptr, + /*signature_verification_enabled=*/false) {} + ~TestVariationsSeedStore() override = default; bool StoreSeedForTesting(const std::string& seed_data) { return StoreSeedData(seed_data, std::string(), std::string(), - base::Time::Now(), false, false, false, nullptr); + base::Time::Now(), false, false, nullptr); } - bool SignatureVerificationEnabled() override { return false; } - private: DISALLOW_COPY_AND_ASSIGN(TestVariationsSeedStore); }; @@ -72,9 +73,7 @@ class SignatureVerifyingVariationsSeedStore : public VariationsSeedStore { public: explicit SignatureVerifyingVariationsSeedStore(PrefService* local_state) : VariationsSeedStore(local_state) {} - ~SignatureVerifyingVariationsSeedStore() override {} - - bool SignatureVerificationEnabled() override { return true; } + ~SignatureVerifyingVariationsSeedStore() override = default; private: DISALLOW_COPY_AND_ASSIGN(SignatureVerifyingVariationsSeedStore); @@ -348,7 +347,7 @@ TEST(VariationsSeedStoreTest, StoreSeedData_ParsedSeed) { VariationsSeed parsed_seed; EXPECT_TRUE(seed_store.StoreSeedData(serialized_seed, std::string(), std::string(), base::Time::Now(), false, - false, false, &parsed_seed)); + false, &parsed_seed)); EXPECT_EQ(serialized_seed, SerializeSeed(parsed_seed)); } @@ -360,13 +359,13 @@ TEST(VariationsSeedStoreTest, StoreSeedData_CountryCode) { // Test with a valid header value. std::string seed = SerializeSeed(CreateTestSeed()); EXPECT_TRUE(seed_store.StoreSeedData(seed, std::string(), "test_country", - base::Time::Now(), false, false, false, + base::Time::Now(), false, false, nullptr)); EXPECT_EQ("test_country", prefs.GetString(prefs::kVariationsCountry)); // Test with no country code specified - which should preserve the old value. EXPECT_TRUE(seed_store.StoreSeedData(seed, std::string(), std::string(), - base::Time::Now(), false, false, false, + base::Time::Now(), false, false, nullptr)); EXPECT_EQ("test_country", prefs.GetString(prefs::kVariationsCountry)); } @@ -384,7 +383,7 @@ TEST(VariationsSeedStoreTest, StoreSeedData_GzippedSeed) { VariationsSeed parsed_seed; EXPECT_TRUE(seed_store.StoreSeedData(compressed_seed, std::string(), std::string(), base::Time::Now(), false, - true, false, &parsed_seed)); + true, &parsed_seed)); EXPECT_EQ(serialized_seed, SerializeSeed(parsed_seed)); } @@ -920,7 +919,7 @@ TEST(VariationsSeedStoreTest, StoreSeedData_GzippedEmptySeed) { VariationsSeed parsed_seed; EXPECT_FALSE(seed_store.StoreSeedData(compressed_seed, std::string(), std::string(), base::Time::Now(), false, - true, false, &parsed_seed)); + true, &parsed_seed)); } TEST(VariationsSeedStoreTest, VerifySeedSignature) { |