diff --git a/BUILD.gn b/BUILD.gn index f962484154..ca60bdc24b 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -273,6 +273,12 @@ config("common_config") { defines += [ "WEBRTC_ENABLE_PROTOBUF=0" ] } + if (rtc_strict_field_trials) { + defines += [ "WEBRTC_STRICT_FIELD_TRIALS=1" ] + } else { + defines += [ "WEBRTC_STRICT_FIELD_TRIALS=0" ] + } + if (rtc_include_internal_audio_device) { defines += [ "WEBRTC_INCLUDE_INTERNAL_AUDIO_DEVICE" ] } diff --git a/api/BUILD.gn b/api/BUILD.gn index 147a8c1b5d..f0bc7ba322 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -1308,6 +1308,7 @@ if (rtc_include_tests) { "../rtc_base:rtc_event", "../rtc_base:rtc_task_queue", "../rtc_base:task_queue_for_test", + "../rtc_base/containers:flat_set", "../rtc_base/task_utils:repeating_task", "../system_wrappers:field_trial", "../test:fileutils", @@ -1322,7 +1323,10 @@ if (rtc_include_tests) { "video:rtp_video_frame_assembler_unittests", "video:video_unittests", ] - absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/strings", + "//third_party/abseil-cpp/absl/types:optional", + ] } rtc_library("compile_all_headers") { diff --git a/api/field_trials_unittest.cc b/api/field_trials_unittest.cc index dc8289881b..5f0188e557 100644 --- a/api/field_trials_unittest.cc +++ b/api/field_trials_unittest.cc @@ -11,8 +11,11 @@ #include "api/field_trials.h" #include +#include +#include "absl/strings/string_view.h" #include "api/transport/field_trial_based_config.h" +#include "rtc_base/containers/flat_set.h" #include "system_wrappers/include/field_trial.h" #include "test/gmock.h" #include "test/gtest.h" @@ -26,22 +29,16 @@ namespace { using ::testing::NotNull; using ::webrtc::field_trial::InitFieldTrialsFromString; +using ::webrtc::field_trial::ScopedGlobalFieldTrialsForTesting; -class FieldTrialsTest : public testing::Test { - protected: - FieldTrialsTest() { - // Make sure global state is consistent between test runs. - InitFieldTrialsFromString(nullptr); - } -}; - -TEST_F(FieldTrialsTest, EmptyStringHasNoEffect) { +TEST(FieldTrialsTest, EmptyStringHasNoEffect) { + ScopedGlobalFieldTrialsForTesting g({"MyCoolTrial"}); FieldTrials f(""); EXPECT_FALSE(f.IsEnabled("MyCoolTrial")); EXPECT_FALSE(f.IsDisabled("MyCoolTrial")); } -TEST_F(FieldTrialsTest, EnabledDisabledMustBeFirstInValue) { +TEST(FieldTrialsTest, EnabledDisabledMustBeFirstInValue) { FieldTrials f( "MyCoolTrial/EnabledFoo/" "MyUncoolTrial/DisabledBar/" @@ -51,7 +48,8 @@ TEST_F(FieldTrialsTest, EnabledDisabledMustBeFirstInValue) { EXPECT_FALSE(f.IsEnabled("AnotherTrial")); } -TEST_F(FieldTrialsTest, FieldTrialsDoesNotReadGlobalString) { +TEST(FieldTrialsTest, FieldTrialsDoesNotReadGlobalString) { + ScopedGlobalFieldTrialsForTesting g({"MyCoolTrial", "MyUncoolTrial"}); static constexpr char s[] = "MyCoolTrial/Enabled/MyUncoolTrial/Disabled/"; InitFieldTrialsFromString(s); FieldTrials f(""); @@ -59,13 +57,14 @@ TEST_F(FieldTrialsTest, FieldTrialsDoesNotReadGlobalString) { EXPECT_FALSE(f.IsDisabled("MyUncoolTrial")); } -TEST_F(FieldTrialsTest, FieldTrialsWritesGlobalString) { +TEST(FieldTrialsTest, FieldTrialsWritesGlobalString) { + ScopedGlobalFieldTrialsForTesting g({"MyCoolTrial", "MyUncoolTrial"}); FieldTrials f("MyCoolTrial/Enabled/MyUncoolTrial/Disabled/"); EXPECT_TRUE(webrtc::field_trial::IsEnabled("MyCoolTrial")); EXPECT_TRUE(webrtc::field_trial::IsDisabled("MyUncoolTrial")); } -TEST_F(FieldTrialsTest, FieldTrialsRestoresGlobalStringAfterDestruction) { +TEST(FieldTrialsTest, FieldTrialsRestoresGlobalStringAfterDestruction) { static constexpr char s[] = "SomeString/Enabled/"; InitFieldTrialsFromString(s); { @@ -77,19 +76,20 @@ TEST_F(FieldTrialsTest, FieldTrialsRestoresGlobalStringAfterDestruction) { } #if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -TEST_F(FieldTrialsTest, FieldTrialsDoesNotSupportSimultaneousInstances) { +TEST(FieldTrialsTest, FieldTrialsDoesNotSupportSimultaneousInstances) { FieldTrials f("SomeString/Enabled/"); RTC_EXPECT_DEATH(FieldTrials("SomeOtherString/Enabled/").Lookup("Whatever"), "Only one instance"); } #endif // GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -TEST_F(FieldTrialsTest, FieldTrialsSupportsSeparateInstances) { +TEST(FieldTrialsTest, FieldTrialsSupportsSeparateInstances) { { FieldTrials f("SomeString/Enabled/"); } { FieldTrials f("SomeOtherString/Enabled/"); } } -TEST_F(FieldTrialsTest, NonGlobalFieldTrialsInstanceDoesNotModifyGlobalString) { +TEST(FieldTrialsTest, NonGlobalFieldTrialsInstanceDoesNotModifyGlobalString) { + ScopedGlobalFieldTrialsForTesting g({"SomeString"}); std::unique_ptr f = FieldTrials::CreateNoGlobal("SomeString/Enabled/"); ASSERT_THAT(f, NotNull()); @@ -97,7 +97,7 @@ TEST_F(FieldTrialsTest, NonGlobalFieldTrialsInstanceDoesNotModifyGlobalString) { EXPECT_FALSE(webrtc::field_trial::IsEnabled("SomeString")); } -TEST_F(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) { +TEST(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) { std::unique_ptr f1 = FieldTrials::CreateNoGlobal("SomeString/Enabled/"); std::unique_ptr f2 = @@ -112,7 +112,8 @@ TEST_F(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) { EXPECT_TRUE(f2->IsEnabled("SomeOtherString")); } -TEST_F(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) { +TEST(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) { + ScopedGlobalFieldTrialsForTesting g({"SomeString", "SomeOtherString"}); FieldTrials f1("SomeString/Enabled/"); std::unique_ptr f2 = FieldTrials::CreateNoGlobal("SomeOtherString/Enabled/"); @@ -125,7 +126,8 @@ TEST_F(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) { EXPECT_TRUE(f2->IsEnabled("SomeOtherString")); } -TEST_F(FieldTrialsTest, FieldTrialBasedConfigReadsGlobalString) { +TEST(FieldTrialsTest, FieldTrialBasedConfigReadsGlobalString) { + ScopedGlobalFieldTrialsForTesting g({"MyCoolTrial", "MyUncoolTrial"}); static constexpr char s[] = "MyCoolTrial/Enabled/MyUncoolTrial/Disabled/"; InitFieldTrialsFromString(s); FieldTrialBasedConfig f; diff --git a/rtc_base/experiments/field_trial_parser_unittest.cc b/rtc_base/experiments/field_trial_parser_unittest.cc index 9916edee97..ea423526ef 100644 --- a/rtc_base/experiments/field_trial_parser_unittest.cc +++ b/rtc_base/experiments/field_trial_parser_unittest.cc @@ -18,7 +18,8 @@ namespace webrtc { namespace { -const char kDummyExperiment[] = "WebRTC-DummyExperiment"; + +constexpr char kDummyExperiment[] = "WebRTC-DummyExperiment"; struct DummyExperiment { FieldTrialFlag enabled = FieldTrialFlag("Enabled"); @@ -29,6 +30,8 @@ struct DummyExperiment { FieldTrialParameter hash = FieldTrialParameter("h", "a80"); + field_trial::ScopedGlobalFieldTrialsForTesting g{{kDummyExperiment}}; + explicit DummyExperiment(absl::string_view field_trial) { ParseFieldTrial({&enabled, &factor, &retries, &size, &ping, &hash}, field_trial); diff --git a/system_wrappers/BUILD.gn b/system_wrappers/BUILD.gn index c979a6aae6..a6bcc9c28e 100644 --- a/system_wrappers/BUILD.gn +++ b/system_wrappers/BUILD.gn @@ -87,11 +87,16 @@ rtc_library("field_trial") { defines = [ "WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT" ] } deps = [ + "../experiments:registered_field_trials", "../rtc_base:checks", "../rtc_base:logging", "../rtc_base:stringutils", + "../rtc_base/containers:flat_set", + ] + absl_deps = [ + "//third_party/abseil-cpp/absl/algorithm:container", + "//third_party/abseil-cpp/absl/strings", ] - absl_deps = [ "//third_party/abseil-cpp/absl/strings" ] } rtc_library("metrics") { diff --git a/system_wrappers/include/field_trial.h b/system_wrappers/include/field_trial.h index eb57f2908e..ffbd864a6a 100644 --- a/system_wrappers/include/field_trial.h +++ b/system_wrappers/include/field_trial.h @@ -14,6 +14,7 @@ #include #include "absl/strings/string_view.h" +#include "rtc_base/containers/flat_set.h" // Field trials allow webrtc clients (such as Chrome) to turn on feature code // in binaries out in the field and gather information with that. @@ -97,6 +98,13 @@ bool FieldTrialsStringIsValid(absl::string_view trials_string); std::string MergeFieldTrialsStrings(absl::string_view first, absl::string_view second); +// RAII type that ensures global state is consistent between tests. +class ScopedGlobalFieldTrialsForTesting { + public: + explicit ScopedGlobalFieldTrialsForTesting(flat_set keys); + ~ScopedGlobalFieldTrialsForTesting(); +}; + } // namespace field_trial } // namespace webrtc diff --git a/system_wrappers/source/DEPS b/system_wrappers/source/DEPS new file mode 100644 index 0000000000..ac7f5a234f --- /dev/null +++ b/system_wrappers/source/DEPS @@ -0,0 +1,6 @@ +specific_include_rules = { + # TODO(bugs.webrtc.org/10335): Remove rule when global string is removed. + "field_trial\.cc": [ + "+experiments/registered_field_trials.h", + ], +} diff --git a/system_wrappers/source/field_trial.cc b/system_wrappers/source/field_trial.cc index f83876be03..bdf84bd626 100644 --- a/system_wrappers/source/field_trial.cc +++ b/system_wrappers/source/field_trial.cc @@ -13,9 +13,13 @@ #include #include +#include +#include "absl/algorithm/container.h" #include "absl/strings/string_view.h" +#include "experiments/registered_field_trials.h" #include "rtc_base/checks.h" +#include "rtc_base/containers/flat_set.h" #include "rtc_base/logging.h" #include "rtc_base/string_encode.h" @@ -27,7 +31,14 @@ namespace field_trial { static const char* trials_init_string = NULL; namespace { + constexpr char kPersistentStringSeparator = '/'; + +flat_set& TestKeys() { + static auto* test_keys = new flat_set(); + return *test_keys; +} + // Validates the given field trial string. // E.g.: // "WebRTC-experimentFoo/Enabled/WebRTC-experimentBar/Enabled100kbps/" @@ -67,6 +78,7 @@ bool FieldTrialsStringIsValidInternal(const absl::string_view trials) { return true; } + } // namespace bool FieldTrialsStringIsValid(absl::string_view trials_string) { @@ -104,6 +116,12 @@ std::string MergeFieldTrialsStrings(absl::string_view first, #ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT std::string FindFullName(absl::string_view name) { +#if WEBRTC_STRICT_FIELD_TRIALS + RTC_DCHECK(absl::c_linear_search(kRegisteredFieldTrials, name) || + TestKeys().contains(name)) + << name << " is not registered."; +#endif + if (trials_init_string == NULL) return std::string(); @@ -150,5 +168,14 @@ const char* GetFieldTrialString() { return trials_init_string; } +ScopedGlobalFieldTrialsForTesting::ScopedGlobalFieldTrialsForTesting( + flat_set keys) { + TestKeys() = std::move(keys); +} + +ScopedGlobalFieldTrialsForTesting::~ScopedGlobalFieldTrialsForTesting() { + TestKeys().clear(); +} + } // namespace field_trial } // namespace webrtc diff --git a/webrtc.gni b/webrtc.gni index 8ab7b27f0c..8dfcc9d244 100644 --- a/webrtc.gni +++ b/webrtc.gni @@ -231,6 +231,11 @@ declare_args() { # Includes the dav1d decoder in the internal decoder factory when set to true. rtc_include_dav1d_in_internal_decoder_factory = true + + # When set to true, a run-time check will make sure that all field trial keys + # have been registered in accordance with the field trial policy. The check + # will only run with builds that have RTC_DCHECKs enabled. + rtc_strict_field_trials = false } if (!build_with_mozilla) {