Remove webrtc::test::InitFieldTrialsFromString(const std::string&).

This is done to solve a problem where a string literal is implicitly cast to a temporary std::string when calling webrtc::test::InitFieldTrialsFromString which passes a pointer to the internal representation to webrtc::field_trial::InitFieldTrialFromString(char*). This pointer is stored for later use, but the temporary std::string is destroyed as soon as the function returns. Using webrtc::field_trial::InitFieldTrialFromString(char*) instead, avoids the implicit casts (but the caller still needs to ensure that the char* outlives the program). The validation previously done by webrtc::test::InitFieldTrialsFromString can now be done by manually calling webrtc::test::ValidateFieldTrialsStringOrDie(const std::string&). Add system_wrappers:field_trial_default as a direct dependency to various targets to allow including the field_trials_default.h header. Bug: webrtc:8812 Change-Id: Ib5a641ea255b1c16a8f7f35e1fe67f6c38a61da6 Reviewed-on: https://webrtc-review.googlesource.com/46141 Reviewed-by: Tommi <[email protected]> Commit-Queue: Oleh Prypin <[email protected]> Cr-Commit-Position: refs/heads/master@{#21856}
wslyx · Feb 1, 2018 · edab301 · edab301
1 parent 9a5c6f8
commit edab301
Show file tree

Hide file tree

Showing 12 changed files with 48 additions and 22 deletions.
diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn
@@ -896,6 +896,7 @@ if (rtc_include_tests) {
       ":rtc_base",
       ":rtc_base_approved",
       ":rtc_base_tests_utils",
+      "../system_wrappers:field_trial_default",
       "../test:field_trial",
       "../test:test_support",
     ]

diff --git a/rtc_base/unittest_main.cc b/rtc_base/unittest_main.cc
@@ -19,6 +19,7 @@
 #include "rtc_base/logging.h"
 #include "rtc_base/ssladapter.h"
 #include "rtc_base/sslstreamadapter.h"
+#include "system_wrappers/include/field_trial_default.h"
 #include "test/field_trial.h"
 #include "test/testsupport/fileutils.h"
 
@@ -76,7 +77,10 @@ int main(int argc, char* argv[]) {
   }
 
   webrtc::test::SetExecutablePath(argv[0]);
-  webrtc::test::InitFieldTrialsFromString(FLAG_force_fieldtrials);
+  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);
 
 #if defined(WEBRTC_WIN)
   if (!FLAG_default_error_handlers) {

diff --git a/rtc_tools/BUILD.gn b/rtc_tools/BUILD.gn
@@ -267,6 +267,7 @@ if (rtc_include_tests) {
         "../logging:rtc_event_log_parser",
         "../rtc_base:protobuf_utils",
         "../rtc_base:rtc_base_approved",
+        "../system_wrappers:field_trial_default",
         "../test:field_trial",
         "../test:test_support",
       ]

diff --git a/rtc_tools/event_log_visualizer/main.cc b/rtc_tools/event_log_visualizer/main.cc
@@ -15,6 +15,7 @@
 #include "rtc_tools/event_log_visualizer/analyzer.h"
 #include "rtc_tools/event_log_visualizer/plot_base.h"
 #include "rtc_tools/event_log_visualizer/plot_python.h"
+#include "system_wrappers/include/field_trial_default.h"
 #include "test/field_trial.h"
 #include "test/testsupport/fileutils.h"
 
@@ -195,7 +196,10 @@ int main(int argc, char* argv[]) {
   }
 
   webrtc::test::SetExecutablePath(argv[0]);
-  webrtc::test::InitFieldTrialsFromString(FLAG_force_fieldtrials);
+  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);
 
   std::string filename = argv[1];
 

diff --git a/test/BUILD.gn b/test/BUILD.gn
@@ -227,6 +227,7 @@ if (rtc_include_tests) {
       ":field_trial",
       ":perf_test",
       "../rtc_base:rtc_base_approved",
+      "../system_wrappers:field_trial_default",
       "../system_wrappers:metrics_default",
       "../system_wrappers:runtime_enabled_features_default",
       "//testing/gtest",

diff --git a/test/field_trial.cc b/test/field_trial.cc
@@ -26,9 +26,8 @@ bool field_trials_initiated_ = false;
 }  // namespace
 
 namespace test {
-// Note: this code is copied from src/base/metrics/field_trial.cc since the aim
-// is to mimic chromium --force-fieldtrials.
-void InitFieldTrialsFromString(const std::string& trials_string) {
+
+void ValidateFieldTrialsStringOrDie(const std::string& trials_string) {
   static const char kPersistentStringSeparator = '/';
 
   // Catch an error if this is called more than once.
@@ -63,7 +62,7 @@ void InitFieldTrialsFromString(const std::string& trials_string) {
 
     // Successfully parsed all field trials from the string.
     if (next_item == trials_string.length()) {
-      webrtc::field_trial::InitFieldTrialsFromString(trials_string.c_str());
+      // webrtc::field_trial::InitFieldTrialsFromString(trials_string.c_str());
       return;
     }
   }
@@ -79,7 +78,8 @@ ScopedFieldTrials::ScopedFieldTrials(const std::string& config)
   assert(field_trials_initiated_);
   field_trials_initiated_ = false;
   current_field_trials_ = config;
-  InitFieldTrialsFromString(current_field_trials_);
+  ValidateFieldTrialsStringOrDie(current_field_trials_);
+  webrtc::field_trial::InitFieldTrialsFromString(current_field_trials_.c_str());
 }
 
 ScopedFieldTrials::~ScopedFieldTrials() {

diff --git a/test/field_trial.h b/test/field_trial.h
@@ -30,7 +30,7 @@ namespace test {
 //
 // 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 InitFieldTrialsFromString(const std::string& config);
+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.

diff --git a/test/test_main.cc b/test/test_main.cc
@@ -10,6 +10,7 @@
 
 #include "rtc_base/flags.h"
 #include "rtc_base/logging.h"
+#include "system_wrappers/include/field_trial_default.h"
 #include "system_wrappers/include/metrics_default.h"
 #include "test/field_trial.h"
 #include "test/gmock.h"
@@ -70,8 +71,10 @@ int main(int argc, char* argv[]) {
   }
 
   webrtc::test::SetExecutablePath(argv[0]);
-  std::string fieldtrials = FLAG_force_fieldtrials;
-  webrtc::test::InitFieldTrialsFromString(fieldtrials);
+  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);
   webrtc::metrics::Enable();
 
 

diff --git a/video/BUILD.gn b/video/BUILD.gn
@@ -168,6 +168,7 @@ if (rtc_include_tests) {
     deps = [
       ":video_quality_test",
       "../rtc_base:rtc_base_approved",
+      "../system_wrappers:field_trial_default",
       "../system_wrappers:metrics_default",
       "../system_wrappers:runtime_enabled_features_default",
       "../test:field_trial",
@@ -193,6 +194,7 @@ if (rtc_include_tests) {
     deps = [
       ":video_quality_test",
       "../rtc_base:rtc_base_approved",
+      "../system_wrappers:field_trial_default",
       "../system_wrappers:metrics_default",
       "../system_wrappers:runtime_enabled_features_default",
       "../test:field_trial",
@@ -217,6 +219,7 @@ if (rtc_include_tests) {
     deps = [
       ":video_quality_test",
       "../rtc_base:rtc_base_approved",
+      "../system_wrappers:field_trial_default",
       "../system_wrappers:metrics_default",
       "../system_wrappers:runtime_enabled_features_default",
       "../test:field_trial",

diff --git a/video/screenshare_loopback.cc b/video/screenshare_loopback.cc
@@ -12,6 +12,7 @@
 
 #include "rtc_base/flags.h"
 #include "rtc_base/stringencode.h"
+#include "system_wrappers/include/field_trial_default.h"
 #include "test/field_trial.h"
 #include "test/gtest.h"
 #include "test/run_test.h"
@@ -330,10 +331,12 @@ int main(int argc, char* argv[]) {
     return 0;
   }
 
-  // InitFieldTrialsFromString needs a reference to an std::string instance,
-  // with a scope that outlives the test.
-  std::string field_trials = webrtc::flags::FLAG_force_fieldtrials;
-  webrtc::test::InitFieldTrialsFromString(field_trials);
+  webrtc::test::ValidateFieldTrialsStringOrDie(
+      webrtc::flags::FLAG_force_fieldtrials);
+  // InitFieldTrialsFromString stores the char*, so the char array must outlive
+  // the application.
+  webrtc::field_trial::InitFieldTrialsFromString(
+      webrtc::flags::FLAG_force_fieldtrials);
 
   webrtc::test::RunTest(webrtc::Loopback);
   return 0;

diff --git a/video/sv_loopback.cc b/video/sv_loopback.cc
@@ -12,6 +12,7 @@
 
 #include "rtc_base/flags.h"
 #include "rtc_base/stringencode.h"
+#include "system_wrappers/include/field_trial_default.h"
 #include "test/field_trial.h"
 #include "test/gtest.h"
 #include "test/run_test.h"
@@ -568,10 +569,12 @@ int main(int argc, char* argv[]) {
     return 0;
   }
 
-  // InitFieldTrialsFromString needs a reference to an std::string instance,
-  // with a scope that outlives the test.
-  std::string field_trials = webrtc::flags::FLAG_force_fieldtrials;
-  webrtc::test::InitFieldTrialsFromString(field_trials);
+  webrtc::test::ValidateFieldTrialsStringOrDie(
+      webrtc::flags::FLAG_force_fieldtrials);
+  // InitFieldTrialsFromString stores the char*, so the char array must outlive
+  // the application.
+  webrtc::field_trial::InitFieldTrialsFromString(
+      webrtc::flags::FLAG_force_fieldtrials);
 
   webrtc::test::RunTest(webrtc::Loopback);
   return 0;

diff --git a/video/video_loopback.cc b/video/video_loopback.cc
@@ -11,6 +11,7 @@
 #include <stdio.h>
 
 #include "rtc_base/flags.h"
+#include "system_wrappers/include/field_trial_default.h"
 #include "test/field_trial.h"
 #include "test/gtest.h"
 #include "test/run_test.h"
@@ -328,10 +329,12 @@ int main(int argc, char* argv[]) {
     return 0;
   }
 
-  // InitFieldTrialsFromString needs a reference to an std::string instance,
-  // with a scope that outlives the test.
-  std::string field_trials = webrtc::flags::FLAG_force_fieldtrials;
-  webrtc::test::InitFieldTrialsFromString(field_trials);
+  webrtc::test::ValidateFieldTrialsStringOrDie(
+      webrtc::flags::FLAG_force_fieldtrials);
+  // InitFieldTrialsFromString stores the char*, so the char array must outlive
+  // the application.
+  webrtc::field_trial::InitFieldTrialsFromString(
+      webrtc::flags::FLAG_force_fieldtrials);
 
   webrtc::test::RunTest(webrtc::Loopback);
   return 0;