From 6bf20cc76a59e1f548ca53c6ef21f4143e3fe974 Mon Sep 17 00:00:00 2001 From: Emil Lundmark Date: Wed, 21 Sep 2022 15:20:22 +0200 Subject: [PATCH] Verify field trials looked up through field_trial::FindFullName For now, the run-time check will only be enabled if the rtc_strict_field_trials GN arg is set. In order to allow testing with imaginary field trial keys, two test helpers have been added. It's a bit awkward to test these since the field trial string is already global, hence the helpers are also modifying global state. Tests must make sure this global state is reset between runs. Things won't be an issue anymore when [1] has removed the global string. [1] https://crbug.com/webrtc/10335 Bug: webrtc:14154 Change-Id: Ida44cc817079d7177325e2228cf1f1d242b799e2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/276269 Commit-Queue: Emil Lundmark Reviewed-by: Danil Chapovalov Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#38447} --- BUILD.gn | 6 +++ api/BUILD.gn | 6 ++- api/field_trials_unittest.cc | 40 ++++++++++--------- .../field_trial_parser_unittest.cc | 5 ++- system_wrappers/BUILD.gn | 7 +++- system_wrappers/include/field_trial.h | 8 ++++ system_wrappers/source/DEPS | 6 +++ system_wrappers/source/field_trial.cc | 27 +++++++++++++ webrtc.gni | 5 +++ 9 files changed, 88 insertions(+), 22 deletions(-) create mode 100644 system_wrappers/source/DEPS 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) {