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}

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);

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);

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 @@ 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);

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;

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 @@ 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);

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 = [
+      deps += [ "../../webrtc_overrides:field_trial" ]
-        "../../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) {

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();

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 @@ 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<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 @@ 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;
 }
 

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

test/field_trial.cc

@@ -21,62 +21,11 @@
 
 namespace webrtc {
 namespace test {
-namespace {
+void ValidateFieldTrialsStringOrDie(const std::string&) {}
-
-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);
-}
 
 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());
 }
 

test/field_trial.h

@@ -16,21 +16,8 @@
 
 namespace webrtc {
 namespace test {
-
+// TODO(jonasolsson): remove once all internal usages are gone.
-// Parses enabled field trials from a string config, such as the one passed
+void ValidateFieldTrialsStringOrDie(const std::string&);
-// 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);
 
 // 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.

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);

video/screenshare_loopback.cc

@@ -9,6 +9,7 @@
  */
 
 #include <stdio.h>
+
 #include <memory>
 #include <string>
 #include <vector>
@@ -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(

video/sv_loopback.cc

@@ -9,6 +9,7 @@
  */
 
 #include <stdio.h>
+
 #include <memory>
 #include <string>
 #include <vector>
@@ -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(

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 @@ 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(