Sanity-check field trial string at initialization.
It's easy to make small errors when building field trial strings, and
those errors can cause all sorts of weird problems. This CL checks if
the FT string has an odd number of delimiters, duplicate
names or any trailing chars.
If so we'll log a error message. On debug builds we'll also crash.
Bug: webrtc:10729
Change-Id: Iebf7155d9b117a02d1e9cfe7f64408e11df2aec5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/140866
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Jonas Olsson <jonasolsson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28234}
diff --git a/examples/peerconnection/client/linux/main.cc b/examples/peerconnection/client/linux/main.cc
index 7c07314..da2fd24 100644
--- a/examples/peerconnection/client/linux/main.cc
+++ b/examples/peerconnection/client/linux/main.cc
@@ -83,7 +83,6 @@
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 6c7178a..4299440 100644
--- a/examples/peerconnection/client/main.cc
+++ b/examples/peerconnection/client/main.cc
@@ -89,7 +89,6 @@
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 fdc84dd..79b2c2c 100644
--- a/examples/peerconnection/server/main.cc
+++ b/examples/peerconnection/server/main.cc
@@ -15,6 +15,7 @@
#include <sys/select.h>
#endif
#include <time.h>
+
#include <string>
#include <vector>
@@ -76,7 +77,6 @@
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 2b89597..4868a95 100644
--- a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc
+++ b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc
@@ -326,7 +326,6 @@
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 25e5bfb..e7de720 100644
--- a/rtc_tools/event_log_visualizer/main.cc
+++ b/rtc_tools/event_log_visualizer/main.cc
@@ -10,6 +10,7 @@
#include <stdio.h>
#include <string.h>
+
#include <iostream>
#include <map>
#include <memory>
@@ -275,7 +276,6 @@
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 1f17a94..59f61aec 100644
--- a/system_wrappers/BUILD.gn
+++ b/system_wrappers/BUILD.gn
@@ -93,14 +93,17 @@
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 @@
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 @@
]
deps = [
+ ":field_trial",
":metrics",
":system_wrappers",
"../rtc_base:checks",
@@ -182,6 +187,7 @@
"../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 41bea9b..1d0cef4 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 ac71311..5b8a756 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 <stddef.h>
+
+#include <map>
#include <string>
+#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 @@
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<absl::string_view, absl::string_view> 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 @@
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 @@
// 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 0000000..f681948
--- /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 48d0bd3..80fee0c 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<std::string, std::string> 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 a500142..c482ca5 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 0ab0ebd..52939af 100644
--- a/test/test_main_lib.cc
+++ b/test/test_main_lib.cc
@@ -112,7 +112,6 @@
// 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 5e781e4..fc10fc3 100644
--- a/video/screenshare_loopback.cc
+++ b/video/screenshare_loopback.cc
@@ -9,6 +9,7 @@
*/
#include <stdio.h>
+
#include <memory>
#include <string>
#include <vector>
@@ -384,8 +385,6 @@
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 8785f15..248708e 100644
--- a/video/sv_loopback.cc
+++ b/video/sv_loopback.cc
@@ -9,6 +9,7 @@
*/
#include <stdio.h>
+
#include <memory>
#include <string>
#include <vector>
@@ -644,8 +645,6 @@
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 9ceb124..f4b2bfa 100644
--- a/video/video_loopback.cc
+++ b/video/video_loopback.cc
@@ -10,6 +10,7 @@
#include "video/video_loopback.h"
#include <stdio.h>
+
#include <memory>
#include <string>
#include <vector>
@@ -401,8 +402,6 @@
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(
