Skip to content

Commit

Permalink
Fix base_unittests crash on Windows with --single-process-tests.
Browse files Browse the repository at this point in the history
The problem was caused by ScopedFeatureList not resetting the
FeatureList to null if it was that way prior to the scope. The
FeatureList that stuck around had references to FieldTrial objects
that were destroyed by a previous test and this caused a crash
when FieldTrialListTest.TestCopyFieldTrialStateToFlags went to use
them.

As part of the fix, also adds a ScopedFeatureList to that test
since it was previously relying on the left-over instance.

Finally, a lot of tests that used feature params were implicitly
relying on a FeatureList having been set (and leaked) by some
earlier test in the same batch. To keep these tests working
without modifying them all to have a ScopedFeatureList (when
they're not actually trying to test param override values), this
change updates FeatureList::GetFieldTrial() to have similar logic
to IsEnabled() when the FeatureList instance was not initialized.

With the new behavior, GetFieldTrial() - which is generally not
invoked directly but queried by the implementation of
base::GetFieldTrialParamValueByFeature() - will return a sane
default when FeatureList is not initialized. This way, it's
consistent with what we do for FeatureList::IsEnabled() - and in
both cases, they will set a bool that is checked to ensure that
once used that way, setting a FeatureList instance will be an
error.

BUG=757651

Change-Id: I79ce1ddcd000d1628c7396faee25ed848b1d8e1a
Reviewed-on: https://chromium-review.googlesource.com/671290
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502707}
  • Loading branch information
asvitkine-chromium authored and Commit Bot committed Sep 18, 2017
1 parent 0c5df4c commit c49d1f4
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 4 deletions.
6 changes: 5 additions & 1 deletion base/feature_list.cc
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,11 @@ bool FeatureList::IsEnabled(const Feature& feature) {

// static
FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
return GetInstance()->GetAssociatedFieldTrial(feature);
if (!g_instance) {
g_initialized_from_accessor = true;
return nullptr;
}
return g_instance->GetAssociatedFieldTrial(feature);
}

// static
Expand Down
3 changes: 3 additions & 0 deletions base/metrics/field_trial_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,9 @@ TEST(FieldTrialDeathTest, OneTimeRandomizedTrialWithoutFieldTrialList) {
TEST(FieldTrialListTest, TestCopyFieldTrialStateToFlags) {
base::FieldTrialList field_trial_list(
std::make_unique<base::MockEntropyProvider>());
test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.Init();

base::FieldTrialList::CreateFieldTrial("Trial1", "Group1");
base::FilePath test_file_path = base::FilePath(FILE_PATH_LITERAL("Program"));
base::CommandLine cmd_line = base::CommandLine(test_file_path);
Expand Down
5 changes: 2 additions & 3 deletions base/test/scoped_feature_list.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,9 @@ ScopedFeatureList::~ScopedFeatureList() {
field_trial_override_->trial_name(),
field_trial_override_->group_name());

if (original_feature_list_) {
FeatureList::ClearInstanceForTesting();
FeatureList::ClearInstanceForTesting();
if (original_feature_list_)
FeatureList::RestoreInstanceForTesting(std::move(original_feature_list_));
}
}

void ScopedFeatureList::Init() {
Expand Down

0 comments on commit c49d1f4

Please sign in to comment.