Add FieldTrialsRegistry that verifies looked up field trials

This new class implements the existing FieldTrialsView interface,
extending it with the verification functionality. For now, the
verification will only be performed if the rtc_strict_field_trials GN
arg is set.

Most classes extending FieldTrialsView today have been converted to
extend from FieldTrialsRegistry instead to automatically perform
verification.

Bug: webrtc:14154
Change-Id: I4819724cd66a04507e62fcc2bb1019187b6ba8c7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/276270
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Emil Lundmark <lndmrk@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38453}

api/BUILD.gn

@@ -1362,6 +1362,25 @@ if (rtc_include_tests) {
   }
 }
 
+rtc_source_set("field_trials_registry") {
+  visibility = [ "*" ]
+  sources = [
+    "field_trials_registry.cc",
+    "field_trials_registry.h",
+  ]
+  deps = [
+    ":field_trials_view",
+    "../experiments:registered_field_trials",
+    "../rtc_base:checks",
+    "../rtc_base/containers:flat_set",
+    "../rtc_base/system:rtc_export",
+  ]
+  absl_deps = [
+    "//third_party/abseil-cpp/absl/algorithm:container",
+    "//third_party/abseil-cpp/absl/strings",
+  ]
+}
+
 rtc_source_set("field_trials_view") {
   visibility = [ "*" ]
   sources = [ "field_trials_view.h" ]
@@ -1382,7 +1401,7 @@ rtc_library("field_trials") {
     "field_trials.h",
   ]
   deps = [
-    ":field_trials_view",
+    ":field_trials_registry",
     "../rtc_base:checks",
     "../rtc_base/containers:flat_map",
     "../system_wrappers:field_trial",

api/DEPS

@@ -312,6 +312,10 @@ specific_include_rules = {
     "+rtc_base/thread.h",
   ],
 
+  "field_trials_registry\.h": [
+    "+rtc_base/containers/flat_set.h",
+  ],
+
   # .cc files in api/ should not be restricted in what they can #include,
   # so we re-add all the top-level directories here. (That's because .h
   # files leak their #includes to whoever's #including them, but .cc files
@@ -322,6 +326,7 @@ specific_include_rules = {
     "+common_audio",
     "+common_video",
     "+examples",
+    "+experiments",
     "+logging",
     "+media",
     "+modules",

api/field_trials.cc

@@ -90,7 +90,7 @@ FieldTrials::~FieldTrials() {
   }
 }
 
-std::string FieldTrials::Lookup(absl::string_view key) const {
+std::string FieldTrials::GetValue(absl::string_view key) const {
   auto it = key_value_map_.find(std::string(key));
   if (it != key_value_map_.end())
     return it->second;

api/field_trials.h

@@ -15,7 +15,7 @@
 #include <string>
 
 #include "absl/strings/string_view.h"
-#include "api/field_trials_view.h"
+#include "api/field_trials_registry.h"
 #include "rtc_base/containers/flat_map.h"
 
 namespace webrtc {
@@ -34,7 +34,7 @@ namespace webrtc {
 // NOTE: Creating multiple FieldTrials-object is currently prohibited
 // until we remove the global string (TODO(bugs.webrtc.org/10335))
 // (unless using CreateNoGlobal):
-class FieldTrials : public FieldTrialsView {
+class FieldTrials : public FieldTrialsRegistry {
  public:
   explicit FieldTrials(const std::string& s);
   ~FieldTrials();
@@ -43,10 +43,11 @@ class FieldTrials : public FieldTrialsView {
   // global variable (i.e can not be used for all parts of webrtc).
   static std::unique_ptr<FieldTrials> CreateNoGlobal(const std::string& s);
 
-  std::string Lookup(absl::string_view key) const override;
-
  private:
   explicit FieldTrials(const std::string& s, bool);
+
+  std::string GetValue(absl::string_view key) const override;
+
   const bool uses_global_;
   const std::string field_trial_string_;
   const char* const previous_field_trial_string_;

api/field_trials_registry.cc

@@ -0,0 +1,31 @@
+/*
+ *  Copyright 2022 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 "api/field_trials_registry.h"
+
+#include <string>
+
+#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"
+
+namespace webrtc {
+
+std::string FieldTrialsRegistry::Lookup(absl::string_view key) const {
+#if WEBRTC_STRICT_FIELD_TRIALS
+  RTC_DCHECK(absl::c_linear_search(kRegisteredFieldTrials, key) ||
+             test_keys_.contains(key))
+      << key << " is not registered.";
+#endif
+  return GetValue(key);
+}
+
+}  // namespace webrtc

api/field_trials_registry.h

@@ -0,0 +1,54 @@
+/*
+ *  Copyright 2022 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.
+ */
+#ifndef API_FIELD_TRIALS_REGISTRY_H_
+#define API_FIELD_TRIALS_REGISTRY_H_
+
+#include <string>
+#include <utility>
+
+#include "absl/strings/string_view.h"
+#include "api/field_trials_view.h"
+#include "rtc_base/containers/flat_set.h"
+#include "rtc_base/system/rtc_export.h"
+
+namespace webrtc {
+
+// Abstract base class for a field trial registry that verifies that any looked
+// up key has been pre-registered in accordance with `g3doc/field-trials.md`.
+class RTC_EXPORT FieldTrialsRegistry : public FieldTrialsView {
+ public:
+  FieldTrialsRegistry() = default;
+
+  FieldTrialsRegistry(const FieldTrialsRegistry&) = default;
+  FieldTrialsRegistry& operator=(const FieldTrialsRegistry&) = default;
+
+  ~FieldTrialsRegistry() override = default;
+
+  // Verifies that `key` is a registered field trial and then returns the
+  // configured value for `key` or an empty string if the field trial isn't
+  // configured.
+  std::string Lookup(absl::string_view key) const override;
+
+  // Register additional `keys` for testing. This should only be used for
+  // imaginary keys that are never used outside test code.
+  void RegisterKeysForTesting(flat_set<std::string> keys) {
+    test_keys_ = std::move(keys);
+  }
+
+ private:
+  virtual std::string GetValue(absl::string_view key) const = 0;
+
+  // Imaginary keys only used for testing.
+  flat_set<std::string> test_keys_;
+};
+
+}  // namespace webrtc
+
+#endif  // API_FIELD_TRIALS_REGISTRY_H_

api/field_trials_unittest.cc

@@ -34,6 +34,8 @@ using ::webrtc::field_trial::ScopedGlobalFieldTrialsForTesting;
 TEST(FieldTrialsTest, EmptyStringHasNoEffect) {
   ScopedGlobalFieldTrialsForTesting g({"MyCoolTrial"});
   FieldTrials f("");
+  f.RegisterKeysForTesting({"MyCoolTrial"});
+
   EXPECT_FALSE(f.IsEnabled("MyCoolTrial"));
   EXPECT_FALSE(f.IsDisabled("MyCoolTrial"));
 }
@@ -43,6 +45,8 @@ TEST(FieldTrialsTest, EnabledDisabledMustBeFirstInValue) {
       "MyCoolTrial/EnabledFoo/"
       "MyUncoolTrial/DisabledBar/"
       "AnotherTrial/BazEnabled/");
+  f.RegisterKeysForTesting({"MyCoolTrial", "MyUncoolTrial", "AnotherTrial"});
+
   EXPECT_TRUE(f.IsEnabled("MyCoolTrial"));
   EXPECT_TRUE(f.IsDisabled("MyUncoolTrial"));
   EXPECT_FALSE(f.IsEnabled("AnotherTrial"));
@@ -53,6 +57,8 @@ TEST(FieldTrialsTest, FieldTrialsDoesNotReadGlobalString) {
   static constexpr char s[] = "MyCoolTrial/Enabled/MyUncoolTrial/Disabled/";
   InitFieldTrialsFromString(s);
   FieldTrials f("");
+  f.RegisterKeysForTesting({"MyCoolTrial", "MyUncoolTrial"});
+
   EXPECT_FALSE(f.IsEnabled("MyCoolTrial"));
   EXPECT_FALSE(f.IsDisabled("MyUncoolTrial"));
 }
@@ -93,6 +99,8 @@ TEST(FieldTrialsTest, NonGlobalFieldTrialsInstanceDoesNotModifyGlobalString) {
   std::unique_ptr<FieldTrials> f =
       FieldTrials::CreateNoGlobal("SomeString/Enabled/");
   ASSERT_THAT(f, NotNull());
+  f->RegisterKeysForTesting({"SomeString"});
+
   EXPECT_TRUE(f->IsEnabled("SomeString"));
   EXPECT_FALSE(webrtc::field_trial::IsEnabled("SomeString"));
 }
@@ -104,6 +112,8 @@ TEST(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) {
       FieldTrials::CreateNoGlobal("SomeOtherString/Enabled/");
   ASSERT_THAT(f1, NotNull());
   ASSERT_THAT(f2, NotNull());
+  f1->RegisterKeysForTesting({"SomeString", "SomeOtherString"});
+  f2->RegisterKeysForTesting({"SomeString", "SomeOtherString"});
 
   EXPECT_TRUE(f1->IsEnabled("SomeString"));
   EXPECT_FALSE(f1->IsEnabled("SomeOtherString"));
@@ -118,6 +128,8 @@ TEST(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) {
   std::unique_ptr<FieldTrials> f2 =
       FieldTrials::CreateNoGlobal("SomeOtherString/Enabled/");
   ASSERT_THAT(f2, NotNull());
+  f1.RegisterKeysForTesting({"SomeString", "SomeOtherString"});
+  f2->RegisterKeysForTesting({"SomeString", "SomeOtherString"});
 
   EXPECT_TRUE(f1.IsEnabled("SomeString"));
   EXPECT_FALSE(f1.IsEnabled("SomeOtherString"));
@@ -131,6 +143,8 @@ TEST(FieldTrialsTest, FieldTrialBasedConfigReadsGlobalString) {
   static constexpr char s[] = "MyCoolTrial/Enabled/MyUncoolTrial/Disabled/";
   InitFieldTrialsFromString(s);
   FieldTrialBasedConfig f;
+  f.RegisterKeysForTesting({"MyCoolTrial", "MyUncoolTrial"});
+
   EXPECT_TRUE(f.IsEnabled("MyCoolTrial"));
   EXPECT_TRUE(f.IsDisabled("MyUncoolTrial"));
 }

api/field_trials_view.h

@@ -17,15 +17,18 @@
 
 namespace webrtc {
 
-// An interface that provides a key-value mapping for configuring internal
-// details of WebRTC. Note that there's no guarantess that the meaning of a
-// particular key value mapping will be preserved over time and no announcements
-// will be made if they are changed. It's up to the library user to ensure that
-// the behavior does not break.
+// An interface that provides the means to access field trials.
+//
+// Note that there are no guarantess that the meaning of a particular key-value
+// mapping will be preserved over time and no announcements will be made if they
+// are changed. It's up to the library user to ensure that the behavior does not
+// break.
 class RTC_EXPORT FieldTrialsView {
  public:
   virtual ~FieldTrialsView() = default;
-  // The configured value for the given key. Defaults to an empty string.
+
+  // Returns the configured value for `key` or an empty string if the field
+  // trial isn't configured.
   virtual std::string Lookup(absl::string_view key) const = 0;
 
   bool IsEnabled(absl::string_view key) const {

api/transport/BUILD.gn

@@ -52,7 +52,7 @@ rtc_library("field_trial_based_config") {
     "field_trial_based_config.h",
   ]
   deps = [
-    "../../api:field_trials_view",
+    "../../api:field_trials_registry",
     "../../system_wrappers:field_trial",
   ]
   absl_deps = [ "//third_party/abseil-cpp/absl/strings" ]

api/transport/field_trial_based_config.cc

@@ -12,7 +12,7 @@
 #include "system_wrappers/include/field_trial.h"
 
 namespace webrtc {
-std::string FieldTrialBasedConfig::Lookup(absl::string_view key) const {
+std::string FieldTrialBasedConfig::GetValue(absl::string_view key) const {
   return webrtc::field_trial::FindFullName(std::string(key));
 }
 }  // namespace webrtc

api/transport/field_trial_based_config.h

@@ -13,13 +13,13 @@
 #include <string>
 
 #include "absl/strings/string_view.h"
-#include "api/field_trials_view.h"
+#include "api/field_trials_registry.h"
 
 namespace webrtc {
 // Implementation using the field trial API fo the key value lookup.
-class FieldTrialBasedConfig : public FieldTrialsView {
- public:
-  std::string Lookup(absl::string_view key) const override;
+class FieldTrialBasedConfig : public FieldTrialsRegistry {
+ private:
+  std::string GetValue(absl::string_view key) const override;
 };
 }  // namespace webrtc
 

modules/rtp_rtcp/BUILD.gn

@@ -604,6 +604,7 @@ if (rtc_include_tests) {
       ":rtp_rtcp_legacy",
       "../../api:array_view",
       "../../api:create_time_controller",
+      "../../api:field_trials_registry",
       "../../api:libjingle_peerconnection_api",
       "../../api:mock_frame_encryptor",
       "../../api:rtp_headers",

modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc

@@ -17,7 +17,7 @@
 #include <utility>
 
 #include "absl/types/optional.h"
-#include "api/transport/field_trial_based_config.h"
+#include "api/field_trials_registry.h"
 #include "api/units/time_delta.h"
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "modules/rtp_rtcp/source/rtcp_packet.h"
@@ -157,7 +157,7 @@ struct TestConfig {
   bool with_overhead = false;
 };
 
-class FieldTrialConfig : public FieldTrialsView {
+class FieldTrialConfig : public FieldTrialsRegistry {
  public:
   static FieldTrialConfig GetFromTestConfig(const TestConfig& config) {
     FieldTrialConfig trials;
@@ -170,14 +170,14 @@ class FieldTrialConfig : public FieldTrialsView {
 
   void SetOverHeadEnabled(bool enabled) { overhead_enabled_ = enabled; }
 
-  std::string Lookup(absl::string_view key) const override {
+ private:
+  std::string GetValue(absl::string_view key) const override {
     if (key == "WebRTC-SendSideBwe-WithOverhead") {
       return overhead_enabled_ ? "Enabled" : "Disabled";
     }
     return "";
   }
 
- private:
   bool overhead_enabled_;
 };
 

modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc

@@ -15,6 +15,7 @@
 #include "absl/types/optional.h"
 #include "api/array_view.h"
 #include "api/call/transport.h"
+#include "api/field_trials_registry.h"
 #include "api/units/data_size.h"
 #include "api/units/timestamp.h"
 #include "logging/rtc_event_log/mock/mock_rtc_event_log.h"
@@ -87,21 +88,21 @@ class MockSendSideDelayObserver : public SendSideDelayObserver {
               (override));
 };
 
-class FieldTrialConfig : public FieldTrialsView {
+class FieldTrialConfig : public FieldTrialsRegistry {
  public:
   FieldTrialConfig() : overhead_enabled_(false) {}
   ~FieldTrialConfig() override {}
 
   void SetOverHeadEnabled(bool enabled) { overhead_enabled_ = enabled; }
 
-  std::string Lookup(absl::string_view key) const override {
+ private:
+  std::string GetValue(absl::string_view key) const override {
     if (key == "WebRTC-SendSideBwe-WithOverhead") {
       return overhead_enabled_ ? "Enabled" : "Disabled";
     }
     return "";
   }
 
- private:
   bool overhead_enabled_;
 };
 

modules/rtp_rtcp/source/rtp_sender_video_unittest.cc

@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "absl/memory/memory.h"
+#include "api/field_trials_registry.h"
 #include "api/rtp_headers.h"
 #include "api/task_queue/task_queue_base.h"
 #include "api/task_queue/task_queue_factory.h"
@@ -148,7 +149,7 @@ class TestRtpSenderVideo : public RTPSenderVideo {
   }
 };
 
-class FieldTrials : public FieldTrialsView {
+class FieldTrials : public FieldTrialsRegistry {
  public:
   explicit FieldTrials(bool use_send_side_bwe_with_overhead)
       : use_send_side_bwe_with_overhead_(use_send_side_bwe_with_overhead),
@@ -158,7 +159,8 @@ class FieldTrials : public FieldTrialsView {
     include_capture_clock_offset_ = include_capture_clock_offset;
   }
 
-  std::string Lookup(absl::string_view key) const override {
+ private:
+  std::string GetValue(absl::string_view key) const override {
     if (key == "WebRTC-SendSideBwe-WithOverhead") {
       return use_send_side_bwe_with_overhead_ ? "Enabled" : "";
     } else if (key == "WebRTC-IncludeCaptureClockOffset") {
@@ -167,7 +169,6 @@ class FieldTrials : public FieldTrialsView {
     return "";
   }
 
- private:
   bool use_send_side_bwe_with_overhead_;
   bool include_capture_clock_offset_;
 };

test/BUILD.gn

@@ -255,7 +255,7 @@ rtc_library("explicit_key_value_config") {
   ]
 
   deps = [
-    "../api:field_trials_view",
+    "../api:field_trials_registry",
     "../rtc_base:checks",
   ]
   absl_deps = [ "//third_party/abseil-cpp/absl/strings:strings" ]
@@ -271,7 +271,7 @@ rtc_library("scoped_key_value_config") {
 
   deps = [
     ":field_trial",
-    "../api:field_trials_view",
+    "../api:field_trials_registry",
     "../rtc_base:checks",
     "../system_wrappers:field_trial",
   ]

test/explicit_key_value_config.cc

@@ -11,7 +11,6 @@
 #include "test/explicit_key_value_config.h"
 
 #include "absl/strings/string_view.h"
-#include "api/field_trials_view.h"
 #include "rtc_base/checks.h"
 
 namespace webrtc {
@@ -46,7 +45,7 @@ ExplicitKeyValueConfig::ExplicitKeyValueConfig(absl::string_view s) {
   RTC_CHECK_EQ(field_start, s.size());
 }
 
-std::string ExplicitKeyValueConfig::Lookup(absl::string_view key) const {
+std::string ExplicitKeyValueConfig::GetValue(absl::string_view key) const {
   auto it = key_value_map_.find(key);
   if (it != key_value_map_.end())
     return it->second;

test/explicit_key_value_config.h

@@ -16,17 +16,18 @@
 #include <string>
 
 #include "absl/strings/string_view.h"
-#include "api/field_trials_view.h"
+#include "api/field_trials_registry.h"
 
 namespace webrtc {
 namespace test {
 
-class ExplicitKeyValueConfig : public FieldTrialsView {
+class ExplicitKeyValueConfig : public FieldTrialsRegistry {
  public:
   explicit ExplicitKeyValueConfig(absl::string_view s);
-  std::string Lookup(absl::string_view key) const override;
 
  private:
+  std::string GetValue(absl::string_view key) const override;
+
   // Unlike std::less<std::string>, std::less<> is transparent and allows
   // heterogeneous lookup directly with absl::string_view.
   std::map<std::string, std::string, std::less<>> key_value_map_;

test/scoped_key_value_config.cc

@@ -10,7 +10,6 @@
 
 #include "test/scoped_key_value_config.h"
 
-#include "api/field_trials_view.h"
 #include "rtc_base/checks.h"
 #include "system_wrappers/include/field_trial.h"
 #include "test/field_trial.h"
@@ -97,7 +96,7 @@ ScopedKeyValueConfig* ScopedKeyValueConfig::GetRoot(ScopedKeyValueConfig* n) {
   return n;
 }
 
-std::string ScopedKeyValueConfig::Lookup(absl::string_view key) const {
+std::string ScopedKeyValueConfig::GetValue(absl::string_view key) const {
   if (parent_ == nullptr) {
     return leaf_->LookupRecurse(key);
   } else {

test/scoped_key_value_config.h

@@ -17,24 +17,23 @@
 #include <string>
 
 #include "absl/strings/string_view.h"
-#include "api/field_trials_view.h"
+#include "api/field_trials_registry.h"
 #include "test/field_trial.h"
 
 namespace webrtc {
 namespace test {
 
-class ScopedKeyValueConfig : public FieldTrialsView {
+class ScopedKeyValueConfig : public FieldTrialsRegistry {
  public:
   virtual ~ScopedKeyValueConfig();
   ScopedKeyValueConfig();
   explicit ScopedKeyValueConfig(absl::string_view s);
   ScopedKeyValueConfig(ScopedKeyValueConfig& parent, absl::string_view s);
 
-  std::string Lookup(absl::string_view key) const override;
-
  private:
   ScopedKeyValueConfig(ScopedKeyValueConfig* parent, absl::string_view s);
   ScopedKeyValueConfig* GetRoot(ScopedKeyValueConfig* n);
+  std::string GetValue(absl::string_view key) const override;
   std::string LookupRecurse(absl::string_view key) const;
 
   ScopedKeyValueConfig* const parent_;