diff --git a/examples/peerconnection/client/linux/main.cc b/examples/peerconnection/client/linux/main.cc index 7c073149e3..da2fd24de3 100644 --- a/examples/peerconnection/client/linux/main.cc +++ b/examples/peerconnection/client/linux/main.cc @@ -83,7 +83,6 @@ int main(int argc, char* argv[]) { return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/examples/peerconnection/client/main.cc b/examples/peerconnection/client/main.cc index 6c7178af55..4299440d52 100644 --- a/examples/peerconnection/client/main.cc +++ b/examples/peerconnection/client/main.cc @@ -89,7 +89,6 @@ int PASCAL wWinMain(HINSTANCE instance, return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/examples/peerconnection/server/main.cc b/examples/peerconnection/server/main.cc index fdc84ddbc2..79b2c2ce3b 100644 --- a/examples/peerconnection/server/main.cc +++ b/examples/peerconnection/server/main.cc @@ -15,6 +15,7 @@ #include #endif #include + #include #include @@ -76,7 +77,6 @@ int main(int argc, char* argv[]) { return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc index 2b89597a90..4868a95a71 100644 --- a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc +++ b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc @@ -326,7 +326,6 @@ int main(int argc, char* argv[]) { RTC_CHECK(ValidateExtensionId(FLAG_video_content_type)); RTC_CHECK(ValidateExtensionId(FLAG_video_timing)); - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); webrtc::test::NetEqTestFactory::Config config; config.pcmu = FLAG_pcmu; diff --git a/rtc_tools/event_log_visualizer/main.cc b/rtc_tools/event_log_visualizer/main.cc index 25e5bfb19b..e7de720315 100644 --- a/rtc_tools/event_log_visualizer/main.cc +++ b/rtc_tools/event_log_visualizer/main.cc @@ -10,6 +10,7 @@ #include #include + #include #include #include @@ -275,7 +276,6 @@ int main(int argc, char* argv[]) { return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/system_wrappers/BUILD.gn b/system_wrappers/BUILD.gn index 1f17a94209..59f61aecff 100644 --- a/system_wrappers/BUILD.gn +++ b/system_wrappers/BUILD.gn @@ -93,14 +93,17 @@ rtc_source_set("field_trial") { if (rtc_exclude_field_trial_default) { defines = [ "WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT" ] } + deps = [ + "../rtc_base:checks", + "../rtc_base:logging", + "//third_party/abseil-cpp/absl/strings", + ] if (build_with_chromium) { # When WebRTC is built as part of Chromium it should exclude the default # implementation of field_trial unless it is building for NACL or # Chromecast. if (!is_nacl && !is_chromecast) { - deps = [ - "../../webrtc_overrides:field_trial", - ] + deps += [ "../../webrtc_overrides:field_trial" ] } } } @@ -168,6 +171,7 @@ if (rtc_include_tests) { testonly = true sources = [ "source/clock_unittest.cc", + "source/field_trial_unittest.cc", "source/metrics_default_unittest.cc", "source/metrics_unittest.cc", "source/ntp_time_unittest.cc", @@ -175,6 +179,7 @@ if (rtc_include_tests) { ] deps = [ + ":field_trial", ":metrics", ":system_wrappers", "../rtc_base:checks", @@ -182,6 +187,7 @@ if (rtc_include_tests) { "../test:test_main", "../test:test_support", "//testing/gtest", + "//third_party/abseil-cpp/absl/strings", ] if (is_android) { diff --git a/system_wrappers/include/field_trial.h b/system_wrappers/include/field_trial.h index 41bea9bf58..1d0cef447d 100644 --- a/system_wrappers/include/field_trial.h +++ b/system_wrappers/include/field_trial.h @@ -33,7 +33,7 @@ // // 1 - Develop the feature with default behaviour off: // -// if (FieldTrial::FindFullName("WebRTCExperimenMethod2") == "Enabled") +// if (FieldTrial::FindFullName("WebRTCExperimentMethod2") == "Enabled") // method2(); // else // method1(); diff --git a/system_wrappers/source/field_trial.cc b/system_wrappers/source/field_trial.cc index ac7131144a..5b8a7562c9 100644 --- a/system_wrappers/source/field_trial.cc +++ b/system_wrappers/source/field_trial.cc @@ -10,8 +10,14 @@ #include "system_wrappers/include/field_trial.h" #include + +#include #include +#include "absl/strings/string_view.h" +#include "rtc_base/checks.h" +#include "rtc_base/logging.h" + // Simple field trial implementation, which allows client to // specify desired flags in InitFieldTrialsFromString. namespace webrtc { @@ -20,6 +26,49 @@ namespace field_trial { static const char* trials_init_string = NULL; #ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT +namespace { +constexpr char kPersistentStringSeparator = '/'; +// Validates the given field trial string. +// E.g.: +// "WebRTC-experimentFoo/Enabled/WebRTC-experimentBar/Enabled100kbps/" +// Assigns the process to group "Enabled" on WebRTCExperimentFoo trial +// and to group "Enabled100kbps" on WebRTCExperimentBar. +// +// E.g. invalid config: +// "WebRTC-experiment1/Enabled" (note missing / separator at the end). +bool FieldTrialsStringIsValid(const absl::string_view trials) { + if (trials.empty()) + return true; + + size_t next_item = 0; + std::map field_trials; + while (next_item < trials.length()) { + size_t name_end = trials.find(kPersistentStringSeparator, next_item); + if (name_end == trials.npos || next_item == name_end) + return false; + size_t group_name_end = + trials.find(kPersistentStringSeparator, name_end + 1); + if (group_name_end == trials.npos || name_end + 1 == group_name_end) + return false; + absl::string_view name = trials.substr(next_item, name_end - next_item); + absl::string_view group_name = + trials.substr(name_end + 1, group_name_end - name_end - 1); + + next_item = group_name_end + 1; + + // Fail if duplicate with different group name. + if (field_trials.find(name) != field_trials.end() && + field_trials.find(name)->second != group_name) { + return false; + } + + field_trials[name] = group_name; + } + + return true; +} +} // namespace + std::string FindFullName(const std::string& name) { if (trials_init_string == NULL) return std::string(); @@ -28,7 +77,6 @@ std::string FindFullName(const std::string& name) { if (trials_string.empty()) return std::string(); - static const char kPersistentStringSeparator = '/'; size_t next_item = 0; while (next_item < trials_string.length()) { // Find next name/value pair in field trial configuration string. @@ -56,6 +104,13 @@ std::string FindFullName(const std::string& name) { // Optionally initialize field trial from a string. void InitFieldTrialsFromString(const char* trials_string) { + RTC_LOG(LS_INFO) << "Setting field trial string:" << trials_string; +#ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT + if (trials_string) { + RTC_DCHECK(FieldTrialsStringIsValid(trials_string)) + << "Invalid field trials string:" << trials_string; + }; +#endif // WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT trials_init_string = trials_string; } diff --git a/system_wrappers/source/field_trial_unittest.cc b/system_wrappers/source/field_trial_unittest.cc new file mode 100644 index 0000000000..f6819486eb --- /dev/null +++ b/system_wrappers/source/field_trial_unittest.cc @@ -0,0 +1,53 @@ +/* + * Copyright 2019 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#include "system_wrappers/include/field_trial.h" + +#include "rtc_base/checks.h" +#include "test/gtest.h" + +namespace webrtc { +namespace field_trial { +#if GTEST_HAS_DEATH_TEST && RTC_DCHECK_IS_ON && !defined(WEBRTC_ANDROID) && \ + !defined(WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT) +TEST(FieldTrialValidationTest, AcceptsValidInputs) { + InitFieldTrialsFromString(""); + InitFieldTrialsFromString("Audio/Enabled/"); + InitFieldTrialsFromString("Audio/Enabled/Video/Disabled/"); + + // Duplicate trials with the same value is fine + InitFieldTrialsFromString("Audio/Enabled/Audio/Enabled/"); + InitFieldTrialsFromString("Audio/Enabled/B/C/Audio/Enabled/"); +} + +TEST(FieldTrialValidationTest, RejectsBadInputs) { + // Bad delimiters + EXPECT_DEATH(InitFieldTrialsFromString("Audio/EnabledVideo/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled//Video/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("/Audio/Enabled/Video/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/Video/Disabled"), + "Invalid field trials string:"); + EXPECT_DEATH( + InitFieldTrialsFromString("Audio/Enabled/Video/Disabled/garbage"), + "Invalid field trials string:"); + + // Duplicate trials with different values is not fine + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/Audio/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/B/C/Audio/Disabled/"), + "Invalid field trials string:"); +} +#endif // GTEST_HAS_DEATH_TEST && RTC_DCHECK_IS_ON && !defined(WEBRTC_ANDROID) + // && !defined(WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT) + +} // namespace field_trial +} // namespace webrtc diff --git a/test/field_trial.cc b/test/field_trial.cc index 48d0bd3858..80fee0c427 100644 --- a/test/field_trial.cc +++ b/test/field_trial.cc @@ -21,62 +21,11 @@ namespace webrtc { namespace test { -namespace { - -void InnerValidateFieldTrialsStringOrDie(const std::string& trials_string) { - static const char kPersistentStringSeparator = '/'; - - if (trials_string.empty()) - return; - - size_t next_item = 0; - std::map field_trials; - while (next_item < trials_string.length()) { - size_t name_end = trials_string.find(kPersistentStringSeparator, next_item); - if (name_end == trials_string.npos || next_item == name_end) - break; - size_t group_name_end = - trials_string.find(kPersistentStringSeparator, name_end + 1); - if (group_name_end == trials_string.npos || name_end + 1 == group_name_end) - break; - std::string name(trials_string, next_item, name_end - next_item); - std::string group_name(trials_string, name_end + 1, - group_name_end - name_end - 1); - next_item = group_name_end + 1; - - // Fail if duplicate with different group name. - if (field_trials.find(name) != field_trials.end() && - field_trials.find(name)->second != group_name) { - break; - } - - field_trials[name] = group_name; - - // Successfully parsed all field trials from the string. - if (next_item == trials_string.length()) { - return; - } - } - // Using fprintf as RTC_LOG does not print when this is called early in main. - fprintf(stderr, "Invalid field trials string.\n"); - - // Using abort so it crashes in both debug and release mode. - abort(); -} -} // namespace - -void ValidateFieldTrialsStringOrDie(const std::string& trials_string) { - static bool field_trials_initiated_ = false; - // Catch an error if this is called more than once. - assert(!field_trials_initiated_); - field_trials_initiated_ = true; - InnerValidateFieldTrialsStringOrDie(trials_string); -} +void ValidateFieldTrialsStringOrDie(const std::string&) {} ScopedFieldTrials::ScopedFieldTrials(const std::string& config) : previous_field_trials_(webrtc::field_trial::GetFieldTrialString()) { current_field_trials_ = config; - InnerValidateFieldTrialsStringOrDie(current_field_trials_); webrtc::field_trial::InitFieldTrialsFromString(current_field_trials_.c_str()); } diff --git a/test/field_trial.h b/test/field_trial.h index a500142e8e..c482ca5aa8 100644 --- a/test/field_trial.h +++ b/test/field_trial.h @@ -16,21 +16,8 @@ namespace webrtc { namespace test { - -// Parses enabled field trials from a string config, such as the one passed -// to chrome's argument --force-fieldtrials and initializes webrtc::field_trial -// with such a config. -// E.g.: -// "WebRTC-experimentFoo/Enabled/WebRTC-experimentBar/Enabled100kbps/" -// Assigns the process to group "Enabled" on WebRTCExperimentFoo trial -// and to group "Enabled100kbps" on WebRTCExperimentBar. -// -// E.g. invalid config: -// "WebRTC-experiment1/Enabled" (note missing / separator at the end). -// -// Note: This method crashes with an error message if an invalid config is -// passed to it. That can be used to find out if a binary is parsing the flags. -void ValidateFieldTrialsStringOrDie(const std::string& config); +// TODO(jonasolsson): remove once all internal usages are gone. +void ValidateFieldTrialsStringOrDie(const std::string&); // This class is used to override field-trial configs within specific tests. // After this class goes out of scope previous field trials will be restored. diff --git a/test/test_main_lib.cc b/test/test_main_lib.cc index 0ab0ebdc87..52939afd69 100644 --- a/test/test_main_lib.cc +++ b/test/test_main_lib.cc @@ -112,7 +112,6 @@ class TestMainImpl : public TestMain { // downstream implementation has been eliminated. (void)webrtc::test::JoinFilename("horrible", "hack"); - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must // outlive the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/video/screenshare_loopback.cc b/video/screenshare_loopback.cc index 5e781e4acf..fc10fc3926 100644 --- a/video/screenshare_loopback.cc +++ b/video/screenshare_loopback.cc @@ -9,6 +9,7 @@ */ #include + #include #include #include @@ -384,8 +385,6 @@ int main(int argc, char* argv[]) { rtc::LogMessage::SetLogToStderr(webrtc::flags::FLAG_logs); - webrtc::test::ValidateFieldTrialsStringOrDie( - webrtc::flags::FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString( diff --git a/video/sv_loopback.cc b/video/sv_loopback.cc index 8785f154ef..248708e6b2 100644 --- a/video/sv_loopback.cc +++ b/video/sv_loopback.cc @@ -9,6 +9,7 @@ */ #include + #include #include #include @@ -644,8 +645,6 @@ int main(int argc, char* argv[]) { rtc::LogMessage::SetLogToStderr(webrtc::flags::FLAG_logs); - webrtc::test::ValidateFieldTrialsStringOrDie( - webrtc::flags::FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString( diff --git a/video/video_loopback.cc b/video/video_loopback.cc index 9ceb124ba1..f4b2bfae35 100644 --- a/video/video_loopback.cc +++ b/video/video_loopback.cc @@ -10,6 +10,7 @@ #include "video/video_loopback.h" #include + #include #include #include @@ -401,8 +402,6 @@ int RunLoopbackTest(int argc, char* argv[]) { rtc::LogMessage::SetLogToStderr(webrtc::flags::FLAG_logs); - webrtc::test::ValidateFieldTrialsStringOrDie( - webrtc::flags::FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(