Skip to content

GitLab

T
tangled
Commit 6c14f269 authored by thestig's avatar thestig Committed by Commit bot
Browse files
Options

Revert of Change ScopedFeatureList to overrides FeatureList not reset... 

Revert of Change ScopedFeatureList to overrides FeatureList not reset (patchset #6 id:140001 of https://codereview.chromium.org/2834583002/ )

Reason for revert:
Mac ASAN bots reporting use-after-free errors.

Original issue's description:
> Change ScopedFeatureList to overrides FeatureList not reset
>
> The current situation is that using ScopedFeatureList resets to an
> empty feature list and then enables/disables an explicit list of
> features.
>
> That's never what you want for browser tests (or other higher-level
> tests) since it effectively overrides higher-level test configurations
> (e.g. those in fieldtrial_testing_config.json, or a bot set up to
> specifically test a feature).
>
> In this patch:
>
> 1. Keep SFL::Init, SFL::InitWithFeatureList,
>    SFL::InitFromCommandLine reset to empty list but add warning to
>    remind developer should use them with care.
> 2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and
>    SFL::InitWithFeatures to not reset but override current FeatureList
>    with given enables/disables.
>
> We also add unit tests for ScopedFeatureList.
>
> BUG=713390
>
> Review-Url: https://codereview.chromium.org/2834583002
> Cr-Commit-Position: refs/heads/master@{#468210}
> Committed: https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e8981e6738881a9

TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,chaopeng@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=713390

Review-Url: https://codereview.chromium.org/2850073002
Cr-Commit-Position: refs/heads/master@{#468263}
parent 1d2108cc
Hide whitespace changes
Inline Side-by-side
Showing with 25 additions and 306 deletions
base/BUILD.gn
View file @ 6c14f269
...@@ -2130,7 +2130,6 @@ test("base_unittests") { ...@@ -2130,7 +2130,6 @@ test("base_unittests") {
"template_util_unittest.cc", "template_util_unittest.cc",
"test/histogram_tester_unittest.cc", "test/histogram_tester_unittest.cc",
"test/mock_callback_unittest.cc", "test/mock_callback_unittest.cc",
"test/scoped_feature_list_unittest.cc",
"test/scoped_mock_time_message_loop_task_runner_unittest.cc", "test/scoped_mock_time_message_loop_task_runner_unittest.cc",
"test/scoped_task_scheduler_unittest.cc", "test/scoped_task_scheduler_unittest.cc",
"test/test_pending_task_unittest.cc", "test/test_pending_task_unittest.cc",
......
base/test/scoped_feature_list.cc
View file @ 6c14f269
...@@ -4,79 +4,24 @@ ...@@ -4,79 +4,24 @@
#include "base/test/scoped_feature_list.h" #include "base/test/scoped_feature_list.h"
#include <algorithm>
#include <string> #include <string>
#include <vector>
#include "base/stl_util.h"
#include "base/strings/string_split.h"
#include "base/strings/string_util.h"
namespace base { namespace base {
namespace test { namespace test {
namespace { namespace {
std::vector<StringPiece> GetFeatureVector( static std::string GetFeatureString(
const std::initializer_list<base::Feature>& features) { const std::initializer_list<base::Feature>& features) {
std::vector<StringPiece> output; std::string output;
for (const base::Feature& feature : features) { for (const base::Feature& feature : features) {
output.push_back(feature.name); if (!output.empty())
output += ",";
output += feature.name;
} }
return output; return output;
} }
// Extracts a feature name from a feature state string. For example, given
// the input "*MyLovelyFeature<SomeFieldTrial", returns "MyLovelyFeature".
StringPiece GetFeatureName(StringPiece feature) {
StringPiece feature_name = feature;
// Remove default info.
if (feature_name.starts_with("*"))
feature_name = feature_name.substr(1);
// Remove field_trial info.
std::size_t index = feature_name.find("<");
if (index != std::string::npos)
feature_name = feature_name.substr(0, index);
return feature_name;
}
struct Features {
std::vector<StringPiece> enabled_feature_list;
std::vector<StringPiece> disabled_feature_list;
};
// Merges previously-specified feature overrides with those passed into one of
// the Init() methods. |features| should be a list of features previously
// overridden to be in the |override_state|. |merged_features| should contain
// the enabled and disabled features passed into the Init() method, plus any
// overrides merged as a result of previous calls to this function.
void OverrideFeatures(const std::string& features,
base::FeatureList::OverrideState override_state,
Features* merged_features) {
std::vector<StringPiece> features_list =
SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
for (StringPiece feature : features_list) {
StringPiece feature_name = GetFeatureName(feature);
if (ContainsValue(merged_features->enabled_feature_list, feature_name) ||
ContainsValue(merged_features->disabled_feature_list, feature_name))
continue;
if (override_state == FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE) {
merged_features->enabled_feature_list.push_back(feature);
} else {
DCHECK_EQ(override_state,
FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE);
merged_features->disabled_feature_list.push_back(feature);
}
}
}
} // namespace } // namespace
ScopedFeatureList::ScopedFeatureList() {} ScopedFeatureList::ScopedFeatureList() {}
...@@ -95,6 +40,13 @@ void ScopedFeatureList::Init() { ...@@ -95,6 +40,13 @@ void ScopedFeatureList::Init() {
InitWithFeatureList(std::move(feature_list)); InitWithFeatureList(std::move(feature_list));
} }
void ScopedFeatureList::InitWithFeatures(
const std::initializer_list<base::Feature>& enabled_features,
const std::initializer_list<base::Feature>& disabled_features) {
InitFromCommandLine(GetFeatureString(enabled_features),
GetFeatureString(disabled_features));
}
void ScopedFeatureList::InitWithFeatureList( void ScopedFeatureList::InitWithFeatureList(
std::unique_ptr<FeatureList> feature_list) { std::unique_ptr<FeatureList> feature_list) {
DCHECK(!original_feature_list_); DCHECK(!original_feature_list_);
...@@ -110,43 +62,12 @@ void ScopedFeatureList::InitFromCommandLine( ...@@ -110,43 +62,12 @@ void ScopedFeatureList::InitFromCommandLine(
InitWithFeatureList(std::move(feature_list)); InitWithFeatureList(std::move(feature_list));
} }
void ScopedFeatureList::InitWithFeatures(
const std::initializer_list<base::Feature>& enabled_features,
const std::initializer_list<base::Feature>& disabled_features) {
Features merged_features;
merged_features.enabled_feature_list = GetFeatureVector(enabled_features);
merged_features.disabled_feature_list = GetFeatureVector(disabled_features);
base::FeatureList* feature_list = base::FeatureList::GetInstance();
// |current_enabled_features| and |current_disabled_features| must declare out
// of if scope to avoid them out of scope before JoinString calls because
// |merged_features| may contains StringPiece which holding pointer points to
// |current_enabled_features| and |current_disabled_features|.
std::string current_enabled_features;
std::string current_disabled_features;
if (feature_list) {
base::FeatureList::GetInstance()->GetFeatureOverrides(
&current_enabled_features, &current_disabled_features);
OverrideFeatures(current_enabled_features,
FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
&merged_features);
OverrideFeatures(current_disabled_features,
FeatureList::OverrideState::OVERRIDE_DISABLE_FEATURE,
&merged_features);
}
std::string enabled = JoinString(merged_features.enabled_feature_list, ",");
std::string disabled = JoinString(merged_features.disabled_feature_list, ",");
InitFromCommandLine(enabled, disabled);
}
void ScopedFeatureList::InitAndEnableFeature(const base::Feature& feature) { void ScopedFeatureList::InitAndEnableFeature(const base::Feature& feature) {
InitWithFeatures({feature}, {}); InitFromCommandLine(feature.name, std::string());
} }
void ScopedFeatureList::InitAndDisableFeature(const base::Feature& feature) { void ScopedFeatureList::InitAndDisableFeature(const base::Feature& feature) {
InitWithFeatures({}, {feature}); InitFromCommandLine(std::string(), feature.name);
} }
} // namespace test } // namespace test
......
base/test/scoped_feature_list.h
View file @ 6c14f269
...@@ -22,42 +22,29 @@ class ScopedFeatureList final { ...@@ -22,42 +22,29 @@ class ScopedFeatureList final {
ScopedFeatureList(); ScopedFeatureList();
~ScopedFeatureList(); ~ScopedFeatureList();
// WARNING: This method will reset any globally configured features to their
// default values, which can hide feature interaction bugs. Please use
// sparingly. https://crbug.com/713390
// Initializes and registers a FeatureList instance with no overrides. // Initializes and registers a FeatureList instance with no overrides.
void Init(); void Init();
// WARNING: This method will reset any globally configured features to their
// default values, which can hide feature interaction bugs. Please use
// sparingly. https://crbug.com/713390
// Initializes and registers the given FeatureList instance. // Initializes and registers the given FeatureList instance.
void InitWithFeatureList(std::unique_ptr<FeatureList> feature_list); void InitWithFeatureList(std::unique_ptr<FeatureList> feature_list);
// WARNING: This method will reset any globally configured features to their // Initializes and registers a FeatureList instance with the given enabled
// default values, which can hide feature interaction bugs. Please use // and disabled features.
// sparingly. https://crbug.com/713390
// Initializes and registers a FeatureList instance with only the given
// enabled and disabled features (comma-separated names).
void InitFromCommandLine(const std::string& enable_features,
const std::string& disable_features);
// Initializes and registers a FeatureList instance based on present
// FeatureList and overridden with the given enabled and disabled features.
// Any feature overrides already present in the global FeatureList will
// continue to apply, unless they conflict with the overrides passed into this
// method. This is important for testing potentially unexpected feature
// interactions.
void InitWithFeatures( void InitWithFeatures(
const std::initializer_list<base::Feature>& enabled_features, const std::initializer_list<base::Feature>& enabled_features,
const std::initializer_list<base::Feature>& disabled_features); const std::initializer_list<base::Feature>& disabled_features);
// Initializes and registers a FeatureList instance based on present // Initializes and registers a FeatureList instance with the given
// FeatureList and overridden with single enabled feature. // enabled and disabled features (comma-separated names).
void InitFromCommandLine(const std::string& enable_features,
const std::string& disable_features);
// Initializes and registers a FeatureList instance enabling a single
// feature.
void InitAndEnableFeature(const base::Feature& feature); void InitAndEnableFeature(const base::Feature& feature);
// Initializes and registers a FeatureList instance based on present // Initializes and registers a FeatureList instance disabling a single
// FeatureList and overridden with single disabled feature. // feature.
void InitAndDisableFeature(const base::Feature& feature); void InitAndDisableFeature(const base::Feature& feature);
private: private:
......
base/test/scoped_feature_list_unittest.cc deleted 100644 → 0
View file @ 1d2108cc
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "base/test/scoped_feature_list.h"
#include <string>
#include "base/metrics/field_trial.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace base {
namespace test {
namespace {
const base::Feature kTestFeature1{"TestFeature1",
base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kTestFeature2{"TestFeature2",
base::FEATURE_DISABLED_BY_DEFAULT};
void ExpectFeatures(const std::string& enabled_features,
const std::string& disabled_features) {
base::FeatureList* list = base::FeatureList::GetInstance();
std::string actual_enabled_features;
std::string actual_disabled_features;
list->GetFeatureOverrides(&actual_enabled_features,
&actual_disabled_features);
EXPECT_EQ(enabled_features, actual_enabled_features);
EXPECT_EQ(disabled_features, actual_disabled_features);
}
} // namespace
class ScopedFeatureListTest : public testing::Test {
public:
ScopedFeatureListTest() {
// Clear default feature list.
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
feature_list->InitializeFromCommandLine(std::string(), std::string());
original_feature_list_ = base::FeatureList::ClearInstanceForTesting();
base::FeatureList::SetInstance(std::move(feature_list));
}
~ScopedFeatureListTest() override {
// Restore feature list.
if (original_feature_list_) {
base::FeatureList::ClearInstanceForTesting();
base::FeatureList::RestoreInstanceForTesting(
std::move(original_feature_list_));
}
}
private:
// Save the present FeatureList and restore it after test finish.
std::unique_ptr<FeatureList> original_feature_list_;
DISALLOW_COPY_AND_ASSIGN(ScopedFeatureListTest);
};
TEST_F(ScopedFeatureListTest, BasicScoped) {
ExpectFeatures(std::string(), std::string());
EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature1));
{
test::ScopedFeatureList feature_list1;
feature_list1.InitFromCommandLine("TestFeature1", std::string());
ExpectFeatures("TestFeature1", std::string());
EXPECT_TRUE(FeatureList::IsEnabled(kTestFeature1));
}
ExpectFeatures(std::string(), std::string());
EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature1));
}
TEST_F(ScopedFeatureListTest, EnableFeatureOverrideDisable) {
test::ScopedFeatureList feature_list1;
feature_list1.InitWithFeatures({}, {kTestFeature1});
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({kTestFeature1}, {});
ExpectFeatures("TestFeature1", std::string());
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideNotMakeDuplicate) {
test::ScopedFeatureList feature_list1;
feature_list1.InitWithFeatures({}, {kTestFeature1});
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({}, {kTestFeature1});
ExpectFeatures(std::string(), "TestFeature1");
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithDefault) {
test::ScopedFeatureList feature_list1;
feature_list1.InitFromCommandLine("*TestFeature1", std::string());
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({kTestFeature1}, {});
ExpectFeatures("TestFeature1", std::string());
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithDefault2) {
test::ScopedFeatureList feature_list1;
feature_list1.InitFromCommandLine("*TestFeature1", std::string());
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({}, {kTestFeature1});
ExpectFeatures(std::string(), "TestFeature1");
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithEnabledFieldTried) {
test::ScopedFeatureList feature_list1;
std::unique_ptr<FeatureList> feature_list(new FeatureList);
FieldTrialList field_trial_list(nullptr);
FieldTrial* trial = FieldTrialList::CreateFieldTrial("TrialExample", "A");
feature_list->RegisterFieldTrialOverride(
kTestFeature1.name, FeatureList::OVERRIDE_ENABLE_FEATURE, trial);
feature_list1.InitWithFeatureList(std::move(feature_list));
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({kTestFeature1}, {});
ExpectFeatures("TestFeature1", std::string());
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideFeatureWithDisabledFieldTried) {
test::ScopedFeatureList feature_list1;
std::unique_ptr<FeatureList> feature_list(new FeatureList);
FieldTrialList field_trial_list(nullptr);
FieldTrial* trial = FieldTrialList::CreateFieldTrial("TrialExample", "A");
feature_list->RegisterFieldTrialOverride(
kTestFeature1.name, FeatureList::OVERRIDE_DISABLE_FEATURE, trial);
feature_list1.InitWithFeatureList(std::move(feature_list));
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({kTestFeature1}, {});
ExpectFeatures("TestFeature1", std::string());
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideKeepsOtherExistingFeature) {
test::ScopedFeatureList feature_list1;
feature_list1.InitWithFeatures({}, {kTestFeature1});
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({}, {kTestFeature2});
EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature1));
EXPECT_FALSE(FeatureList::IsEnabled(kTestFeature2));
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideKeepsOtherExistingFeature2) {
test::ScopedFeatureList feature_list1;
feature_list1.InitWithFeatures({}, {kTestFeature1});
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({kTestFeature2}, {});
ExpectFeatures("TestFeature2", "TestFeature1");
}
}
TEST_F(ScopedFeatureListTest, FeatureOverrideKeepsOtherExistingDefaultFeature) {
test::ScopedFeatureList feature_list1;
feature_list1.InitFromCommandLine("*TestFeature1", std::string());
{
test::ScopedFeatureList feature_list2;
feature_list2.InitWithFeatures({}, {kTestFeature2});
ExpectFeatures("*TestFeature1", "TestFeature2");
}
}
} // namespace test
} // namespace base
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment