From edab3011fa79803ed4e93836d924d266cd4df013 Mon Sep 17 00:00:00 2001 From: Bjorn Terelius Date: Wed, 31 Jan 2018 17:23:40 +0100 Subject: [PATCH] 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 Commit-Queue: Oleh Prypin Cr-Commit-Position: refs/heads/master@{#21856} --- rtc_base/BUILD.gn | 1 + rtc_base/unittest_main.cc | 6 +++++- rtc_tools/BUILD.gn | 1 + rtc_tools/event_log_visualizer/main.cc | 6 +++++- test/BUILD.gn | 1 + test/field_trial.cc | 10 +++++----- test/field_trial.h | 2 +- test/test_main.cc | 7 +++++-- video/BUILD.gn | 3 +++ video/screenshare_loopback.cc | 11 +++++++---- video/sv_loopback.cc | 11 +++++++---- video/video_loopback.cc | 11 +++++++---- 12 files changed, 48 insertions(+), 22 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index f18c0f5239..f0e2fd79cf 100644 --- 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 index 330bd3f813..aa8a11bb5a 100644 --- 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 index 976fa06838..2cdf9b57ff 100644 --- 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 index b38f1a4fcf..4a6ec64125 100644 --- 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 index bfc521de1f..72bc203827 100644 --- 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 index 00e5b69041..9be36c566e 100644 --- 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 index 99ace4e864..40f7b1ba24 100644 --- 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 index f86dd1f4e2..962c677d9b 100644 --- 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 index 593e2b9793..009b889bc3 100644 --- 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 index 8a0b247752..315753799d 100644 --- 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 index 24d3bedf32..c8e0d740d7 100644 --- 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 index 37793cce83..d195ceb38f 100644 --- a/video/video_loopback.cc +++ b/video/video_loopback.cc @@ -11,6 +11,7 @@ #include #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;