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 <lndmrk@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38447}
diff --git a/BUILD.gn b/BUILD.gn
index f962484..ca60bdc 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -273,6 +273,12 @@
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 147a8c1..f0bc7ba 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -1308,6 +1308,7 @@
"../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 @@
"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 dc82898..5f0188e 100644
--- a/api/field_trials_unittest.cc
+++ b/api/field_trials_unittest.cc
@@ -11,8 +11,11 @@
#include "api/field_trials.h"
#include <memory>
+#include <utility>
+#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 @@
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 @@
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 @@
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 @@
}
#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<FieldTrials> f =
FieldTrials::CreateNoGlobal("SomeString/Enabled/");
ASSERT_THAT(f, NotNull());
@@ -97,7 +97,7 @@
EXPECT_FALSE(webrtc::field_trial::IsEnabled("SomeString"));
}
-TEST_F(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) {
+TEST(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) {
std::unique_ptr<FieldTrials> f1 =
FieldTrials::CreateNoGlobal("SomeString/Enabled/");
std::unique_ptr<FieldTrials> f2 =
@@ -112,7 +112,8 @@
EXPECT_TRUE(f2->IsEnabled("SomeOtherString"));
}
-TEST_F(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) {
+TEST(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) {
+ ScopedGlobalFieldTrialsForTesting g({"SomeString", "SomeOtherString"});
FieldTrials f1("SomeString/Enabled/");
std::unique_ptr<FieldTrials> f2 =
FieldTrials::CreateNoGlobal("SomeOtherString/Enabled/");
@@ -125,7 +126,8 @@
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 9916ede..ea42352 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 @@
FieldTrialParameter<std::string> hash =
FieldTrialParameter<std::string>("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 c979a6a..a6bcc9c 100644
--- a/system_wrappers/BUILD.gn
+++ b/system_wrappers/BUILD.gn
@@ -87,11 +87,16 @@
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/strings" ]
+ absl_deps = [
+ "//third_party/abseil-cpp/absl/algorithm:container",
+ "//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 eb57f29..ffbd864 100644
--- a/system_wrappers/include/field_trial.h
+++ b/system_wrappers/include/field_trial.h
@@ -14,6 +14,7 @@
#include <string>
#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 @@
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<std::string> keys);
+ ~ScopedGlobalFieldTrialsForTesting();
+};
+
} // namespace field_trial
} // namespace webrtc
diff --git a/system_wrappers/source/DEPS b/system_wrappers/source/DEPS
new file mode 100644
index 0000000..ac7f5a2
--- /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 f83876b..bdf84bd 100644
--- a/system_wrappers/source/field_trial.cc
+++ b/system_wrappers/source/field_trial.cc
@@ -13,9 +13,13 @@
#include <map>
#include <string>
+#include <utility>
+#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 @@
static const char* trials_init_string = NULL;
namespace {
+
constexpr char kPersistentStringSeparator = '/';
+
+flat_set<std::string>& TestKeys() {
+ static auto* test_keys = new flat_set<std::string>();
+ return *test_keys;
+}
+
// Validates the given field trial string.
// E.g.:
// "WebRTC-experimentFoo/Enabled/WebRTC-experimentBar/Enabled100kbps/"
@@ -67,6 +78,7 @@
return true;
}
+
} // namespace
bool FieldTrialsStringIsValid(absl::string_view trials_string) {
@@ -104,6 +116,12 @@
#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 @@
return trials_init_string;
}
+ScopedGlobalFieldTrialsForTesting::ScopedGlobalFieldTrialsForTesting(
+ flat_set<std::string> keys) {
+ TestKeys() = std::move(keys);
+}
+
+ScopedGlobalFieldTrialsForTesting::~ScopedGlobalFieldTrialsForTesting() {
+ TestKeys().clear();
+}
+
} // namespace field_trial
} // namespace webrtc
diff --git a/webrtc.gni b/webrtc.gni
index 8ab7b27..8dfcc9d 100644
--- a/webrtc.gni
+++ b/webrtc.gni
@@ -231,6 +231,11 @@
# 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) {
