Skip to content

Commit

Permalink
Reland "[base] Stop using ObserverListThreadSafe::NotifySynchronously…
Browse files Browse the repository at this point in the history 
… in FieldTrialList."

== Changes since original CL ==

This CL registers ChildProcessFieldTrialSyncer as a FieldTrialList
observer instead of ChildThreadImpl and it makes
ChildProcessFieldTrialSyncer a leaky singleton. This eliminates
lifetime issues.

An alternative would have been to remove ChildThreadImpl from the list
of FieldTrialList observers upon destruction. We decided not to do that
because it would have required extra synchronization.

Diff: https://crrev.com/c/2867567/1..16

== Original description ==

This CL removes usage of ObserverListThreadSafe::NotifySynchronously
in FieldTrialList, to support the removal of this method in a
separate CL.

Currently, FieldTrialList uses
ObserverListThreadSafe::NotifySynchronously to notify observers when a
group is selected for a field trial. This method dispatches synchronous
notifications to observers which were registered on the current
sequence, and uses PostTask to notify other observers asynchronously.
Through code inspection, we found that all implementations of
FieldTrialList::Observer::OnFieldTrialGroupFinalized are
thread-safe. Therefore, they can always be notified synchronously,
no matter which sequence they were registered from.

This CL makes these changes:

- Observers are stored in a lock-protected std::vector instead of
  in an ObserverListThreadSafe.
- To notify observers, the list of observers is copied to a local
  variable while holding the lock. Then, after releasing the
  lock, all observers in the local list are notified.
- In the scope where observers are notified, an atomic variable is
  incremented. The RemoveObserver method checks that the variable is
  equal to zero. This ensures that no observer is removed while
  dispatching notifications (which could cause a use-after-free).

For reference, the implementations of
FieldTrialList::Observer::OnFieldTrialGroupFinalized are:

base/android/field_trial_list.cc:
  LOG(INFO) is thread-safe

base/metrics/field_trial_unittest.cc:
  The test is single-threaded.

components/variations/child_process_field_trial_syncer_unittest.cc:
  The test is single-threaded.

components/variations/variations_crash_keys.cc:
  The observer checks whether it runs on the UI thread and
  forwards the notification to the UI thread if it's not the case.

components/variations/variations_ids_provider.cc:
  The observer uses a lock.

content/browser/field_trial_synchronizer.cc:
  The observer checks whether it runs on the UI thread and
  forwards the notification to the UI thread if it's not the case.

content/browser/net/network_field_trial_browsertest.cc
  RunLoop::Quit() is thread-safe.

content/child/child_thread_impl.cc
  A mojo::SharedRemote is thread-safe.

Bug: 1193750
Change-Id: I8351a2cbbce4d5deaafbe2aa75f902e6da822d53
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2867567
Commit-Queue: François Doray <fdoray@chromium.org>
Auto-Submit: François Doray <fdoray@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880105}
  • Loading branch information
@fdoray
fdoray authored and Chromium LUCI CQ committed May 6, 2021
1 parent a39638b commit da23db5
Show file tree
Hide file tree
Showing 15 changed files with 286 additions and 281 deletions.
47 changes: 24 additions & 23 deletions base/metrics/field_trial.cc
View file
Expand Up @@ -7,6 +7,7 @@
#include <algorithm>
#include <utility>

#include "base/auto_reset.h"
#include "base/base_switches.h"
#include "base/command_line.h"
#include "base/debug/activity_tracker.h"
Expand All @@ -17,6 +18,7 @@
#include "base/process/process_handle.h"
#include "base/process/process_info.h"
#include "base/rand_util.h"
#include "base/stl_util.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_piece.h"
#include "base/strings/string_split.h"
Expand Down Expand Up @@ -440,9 +442,7 @@ FieldTrialList::Observer::~Observer() = default;

FieldTrialList::FieldTrialList(
std::unique_ptr<const FieldTrial::EntropyProvider> entropy_provider)
: entropy_provider_(std::move(entropy_provider)),
observer_list_(new ObserverListThreadSafe<FieldTrialList::Observer>(
ObserverListPolicy::EXISTING_ONLY)) {
: entropy_provider_(std::move(entropy_provider)) {
DCHECK(!global_);
DCHECK(!used_without_global_);
global_ = this;
Expand Down Expand Up @@ -917,27 +917,19 @@ FieldTrial* FieldTrialList::CreateFieldTrial(StringPiece name,
bool FieldTrialList::AddObserver(Observer* observer) {
if (!global_)
return false;
global_->observer_list_->AddObserver(observer);
AutoLock auto_lock(global_->lock_);
global_->observers_.push_back(observer);
return true;
}

// static
void FieldTrialList::RemoveObserver(Observer* observer) {
if (!global_)
return;
global_->observer_list_->RemoveObserver(observer);
}

// static
void FieldTrialList::SetSynchronousObserver(Observer* observer) {
DCHECK(!global_->synchronous_observer_);
global_->synchronous_observer_ = observer;
}

// static
void FieldTrialList::RemoveSynchronousObserver(Observer* observer) {
DCHECK_EQ(global_->synchronous_observer_, observer);
global_->synchronous_observer_ = nullptr;
AutoLock auto_lock(global_->lock_);
Erase(global_->observers_, observer);
DCHECK_EQ(global_->num_ongoing_notify_field_trial_group_selection_calls_, 0)
<< "Cannot call RemoveObserver while accessing FieldTrial::group().";
}

// static
Expand All @@ -959,6 +951,8 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
if (!global_)
return;

std::vector<Observer*> local_observers;

{
AutoLock auto_lock(global_->lock_);
if (field_trial->group_reported_)
Expand All @@ -968,17 +962,24 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
if (!field_trial->enable_field_trial_)
return;

++global_->num_ongoing_notify_field_trial_group_selection_calls_;

ActivateFieldTrialEntryWhileLocked(field_trial);

// Copy observers to a local variable to access outside the scope of the
// lock. Since removing observers concurrently with this method is
// disallowed, pointers should remain valid while observers are notified.
local_observers = global_->observers_;
}

if (global_->synchronous_observer_) {
global_->synchronous_observer_->OnFieldTrialGroupFinalized(
field_trial->trial_name(), field_trial->group_name_internal());
for (Observer* observer : local_observers) {
observer->OnFieldTrialGroupFinalized(field_trial->trial_name(),
field_trial->group_name_internal());
}

global_->observer_list_->NotifySynchronously(
FROM_HERE, &FieldTrialList::Observer::OnFieldTrialGroupFinalized,
field_trial->trial_name(), field_trial->group_name_internal());
int previous_num_ongoing_notify_field_trial_group_selection_calls =
global_->num_ongoing_notify_field_trial_group_selection_calls_--;
DCHECK_GT(previous_num_ongoing_notify_field_trial_group_selection_calls, 0);
}

// static
Expand Down
38 changes: 16 additions & 22 deletions base/metrics/field_trial.h
View file
Expand Up @@ -57,6 +57,7 @@
#include <stddef.h>
#include <stdint.h>

#include <atomic>
#include <map>
#include <memory>
#include <string>
Expand Down Expand Up @@ -268,6 +269,7 @@ class BASE_EXPORT FieldTrial : public RefCounted<FieldTrial> {
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedTurnFeatureOn);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedChangeDefault_Default);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, SetForcedChangeDefault_NonDefault);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, ObserveReentrancy);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, FloatBoundariesGiveEqualGroupSizes);
FRIEND_TEST_ALL_PREFIXES(FieldTrialTest, DoesNotSurpassTotalProbability);
FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest,
Expand Down Expand Up @@ -608,26 +610,15 @@ class BASE_EXPORT FieldTrialList {
// Add an observer to be notified when a field trial is irrevocably committed
// to being part of some specific field_group (and hence the group_name is
// also finalized for that field_trial). Returns false and does nothing if
// there is no FieldTrialList singleton.
// there is no FieldTrialList singleton. The observer can be notified on any
// sequence; it must be thread-safe.
static bool AddObserver(Observer* observer);

// Remove an observer.
// Remove an observer. This cannot be invoked concurrently with
// FieldTrial::group() (typically, this means that no other thread should be
// running when this is invoked).
static void RemoveObserver(Observer* observer);

// Similar to AddObserver(), but the passed observer will be notified
// synchronously when a field trial is activated and its group selected. It
// will be notified synchronously on the same thread where the activation and
// group selection happened. It is the responsibility of the observer to make
// sure that this is a safe operation and the operation must be fast, as this
// work is done synchronously as part of group() or related APIs. Only a
// single such observer is supported, exposed specifically for crash
// reporting. Must be called on the main thread before any other threads
// have been started.
static void SetSynchronousObserver(Observer* observer);

// Removes the single synchronous observer.
static void RemoveSynchronousObserver(Observer* observer);

// Grabs the lock if necessary and adds the field trial to the allocator. This
// should only be called from FinalizeGroupChoice().
static void OnGroupFinalized(bool is_locked, FieldTrial* field_trial);
Expand Down Expand Up @@ -750,7 +741,7 @@ class BASE_EXPORT FieldTrialList {
typedef std::map<std::string, FieldTrial*, std::less<>> RegistrationMap;

// Helper function should be called only while holding lock_.
FieldTrial* PreLockedFind(StringPiece name);
FieldTrial* PreLockedFind(StringPiece name) EXCLUSIVE_LOCKS_REQUIRED(lock_);

// Register() stores a pointer to the given trial in a global map.
// This method also AddRef's the indicated trial.
Expand All @@ -768,19 +759,22 @@ class BASE_EXPORT FieldTrialList {
// FieldTrialList is created after that.
static bool used_without_global_;

// Lock for access to registered_ and field_trial_allocator_.
// Lock for access to |registered_|, |observers_| and
// |field_trial_allocator_|.
Lock lock_;
RegistrationMap registered_;
RegistrationMap registered_ GUARDED_BY(lock_);

// Entropy provider to be used for one-time randomized field trials. If NULL,
// one-time randomization is not supported.
std::unique_ptr<const FieldTrial::EntropyProvider> entropy_provider_;

// List of observers to be notified when a group is selected for a FieldTrial.
scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;
std::vector<Observer*> observers_ GUARDED_BY(lock_);

// Single synchronous observer to be notified when a trial group is chosen.
Observer* synchronous_observer_ = nullptr;
// Counts the ongoing calls to
// FieldTrialList::NotifyFieldTrialGroupSelection(). Used to ensure that
// RemoveObserver() isn't called while notifying observers.
std::atomic_int num_ongoing_notify_field_trial_group_selection_calls_{0};

// Allocator in shared memory containing field trial data. Used in both
// browser and child processes, but readonly in the child.
Expand Down
114 changes: 68 additions & 46 deletions base/metrics/field_trial_unittest.cc
View file
Expand Up @@ -55,29 +55,15 @@ scoped_refptr<FieldTrial> CreateFieldTrial(
FieldTrial::SESSION_RANDOMIZED, default_group_number);
}

// FieldTrialList::Observer implementation for testing.
// A FieldTrialList::Observer implementation which stores the trial name and
// group name received via OnFieldTrialGroupFinalized() for later inspection.
class TestFieldTrialObserver : public FieldTrialList::Observer {
public:
enum Type {
ASYNCHRONOUS,
SYNCHRONOUS,
};

explicit TestFieldTrialObserver(Type type) : type_(type) {
if (type == SYNCHRONOUS)
FieldTrialList::SetSynchronousObserver(this);
else
FieldTrialList::AddObserver(this);
}
TestFieldTrialObserver() { FieldTrialList::AddObserver(this); }
TestFieldTrialObserver(const TestFieldTrialObserver&) = delete;
TestFieldTrialObserver& operator=(const TestFieldTrialObserver&) = delete;

~TestFieldTrialObserver() override {
if (type_ == SYNCHRONOUS)
FieldTrialList::RemoveSynchronousObserver(this);
else
FieldTrialList::RemoveObserver(this);
}
~TestFieldTrialObserver() override { FieldTrialList::RemoveObserver(this); }

void OnFieldTrialGroupFinalized(const std::string& trial,
const std::string& group) override {
Expand All @@ -89,11 +75,39 @@ class TestFieldTrialObserver : public FieldTrialList::Observer {
const std::string& group_name() const { return group_name_; }

private:
const Type type_;
std::string trial_name_;
std::string group_name_;
};

// A FieldTrialList::Observer implementation which accesses the group of a
// FieldTrial from OnFieldTrialGroupFinalized(). Used to test reentrancy.
class FieldTrialObserverAccessingGroup : public FieldTrialList::Observer {
public:
// |trial_to_access| is the FieldTrial on which to invoke group() when
// receiving an OnFieldTrialGroupFinalized() notification.
explicit FieldTrialObserverAccessingGroup(
scoped_refptr<FieldTrial> trial_to_access)
: trial_to_access_(trial_to_access) {
FieldTrialList::AddObserver(this);
}
FieldTrialObserverAccessingGroup(const FieldTrialObserverAccessingGroup&) =
delete;
FieldTrialObserverAccessingGroup& operator=(
const FieldTrialObserverAccessingGroup&) = delete;

~FieldTrialObserverAccessingGroup() override {
FieldTrialList::RemoveObserver(this);
}

void OnFieldTrialGroupFinalized(const std::string& trial,
const std::string& group) override {
trial_to_access_->group();
}

private:
scoped_refptr<FieldTrial> trial_to_access_;
};

std::string MockEscapeQueryParamValue(const std::string& input) {
return input;
}
Expand Down Expand Up @@ -614,7 +628,7 @@ TEST_F(FieldTrialTest, CreateTrialsFromStringForceActivation) {
TEST_F(FieldTrialTest, CreateTrialsFromStringNotActiveObserver) {
ASSERT_FALSE(FieldTrialList::TrialExists("Abc"));

TestFieldTrialObserver observer(TestFieldTrialObserver::ASYNCHRONOUS);
TestFieldTrialObserver observer;
ASSERT_TRUE(FieldTrialList::CreateTrialsFromString("Abc/def/"));
RunLoop().RunUntilIdle();
// Observer shouldn't be notified.
Expand All @@ -623,7 +637,6 @@ TEST_F(FieldTrialTest, CreateTrialsFromStringNotActiveObserver) {
// Check that the values still get returned and querying them activates them.
EXPECT_EQ("def", FieldTrialList::FindFullName("Abc"));

RunLoop().RunUntilIdle();
EXPECT_EQ("Abc", observer.trial_name());
EXPECT_EQ("def", observer.group_name());
}
Expand Down Expand Up @@ -888,45 +901,55 @@ TEST_F(FieldTrialTest, Observe) {
const char kTrialName[] = "TrialToObserve1";
const char kSecondaryGroupName[] = "SecondaryGroup";

TestFieldTrialObserver observer(TestFieldTrialObserver::ASYNCHRONOUS);
TestFieldTrialObserver observer;
int default_group = -1;
scoped_refptr<FieldTrial> trial =
CreateFieldTrial(kTrialName, 100, kDefaultGroupName, &default_group);
const int secondary_group = trial->AppendGroup(kSecondaryGroupName, 50);
const int chosen_group = trial->group();
EXPECT_TRUE(chosen_group == default_group || chosen_group == secondary_group);

// The observer should be notified synchronously by the group() call.
EXPECT_EQ(kTrialName, observer.trial_name());
if (chosen_group == default_group)
EXPECT_EQ(kDefaultGroupName, observer.group_name());
else
EXPECT_EQ(kSecondaryGroupName, observer.group_name());
}

TEST_F(FieldTrialTest, SynchronousObserver) {
const char kTrialName[] = "TrialToObserve1";
const char kSecondaryGroupName[] = "SecondaryGroup";
// Verify that no hang occurs when a FieldTrial group is selected from a
// FieldTrialList::Observer::OnFieldTrialGroupFinalized() notification. If the
// FieldTrialList's lock is held when observers are notified, this test will
// hang due to reentrant lock acquisition when selecting the FieldTrial group.
TEST_F(FieldTrialTest, ObserveReentrancy) {
const char kTrialName1[] = "TrialToObserve1";
const char kTrialName2[] = "TrialToObserve2";

TestFieldTrialObserver observer(TestFieldTrialObserver::SYNCHRONOUS);
int default_group = -1;
scoped_refptr<FieldTrial> trial =
CreateFieldTrial(kTrialName, 100, kDefaultGroupName, &default_group);
const int secondary_group = trial->AppendGroup(kSecondaryGroupName, 50);
const int chosen_group = trial->group();
EXPECT_TRUE(chosen_group == default_group || chosen_group == secondary_group);
int default_group_1 = -1;
scoped_refptr<FieldTrial> trial_1 =
CreateFieldTrial(kTrialName1, 100, kDefaultGroupName, &default_group_1);

// The observer should be notified synchronously by the group() call.
EXPECT_EQ(kTrialName, observer.trial_name());
if (chosen_group == default_group)
EXPECT_EQ(kDefaultGroupName, observer.group_name());
else
EXPECT_EQ(kSecondaryGroupName, observer.group_name());
FieldTrialObserverAccessingGroup observer(trial_1);

int default_group_2 = -1;
scoped_refptr<FieldTrial> trial_2 =
CreateFieldTrial(kTrialName2, 100, kDefaultGroupName, &default_group_2);

// No group should be selected for |trial_1| yet.
EXPECT_EQ(FieldTrial::kNotFinalized, trial_1->group_);

// Force selection of a group for |trial_2|. This will notify |observer| which
// will force the selection of a group for |trial_1|. This should not hang.
trial_2->group();

// The above call should have selected a group for |trial_1|.
EXPECT_NE(FieldTrial::kNotFinalized, trial_1->group_);
}

TEST_F(FieldTrialTest, ObserveDisabled) {
const char kTrialName[] = "TrialToObserve2";

TestFieldTrialObserver observer(TestFieldTrialObserver::ASYNCHRONOUS);
TestFieldTrialObserver observer;
int default_group = -1;
scoped_refptr<FieldTrial> trial =
CreateFieldTrial(kTrialName, 100, kDefaultGroupName, &default_group);
Expand All @@ -950,7 +973,7 @@ TEST_F(FieldTrialTest, ObserveDisabled) {
TEST_F(FieldTrialTest, ObserveForcedDisabled) {
const char kTrialName[] = "TrialToObserve3";

TestFieldTrialObserver observer(TestFieldTrialObserver::ASYNCHRONOUS);
TestFieldTrialObserver observer;
int default_group = -1;
scoped_refptr<FieldTrial> trial =
CreateFieldTrial(kTrialName, 100, kDefaultGroupName, &default_group);
Expand Down Expand Up @@ -1046,14 +1069,13 @@ TEST_F(FieldTrialTest, CreateSimulatedFieldTrial) {
{ 0.95, kDefaultGroupName },
};

for (size_t i = 0; i < base::size(test_cases); ++i) {
TestFieldTrialObserver observer(TestFieldTrialObserver::ASYNCHRONOUS);
scoped_refptr<FieldTrial> trial(
FieldTrial::CreateSimulatedFieldTrial(kTrialName, 100, kDefaultGroupName,
test_cases[i].entropy_value));
for (auto& test_case : test_cases) {
TestFieldTrialObserver observer;
scoped_refptr<FieldTrial> trial(FieldTrial::CreateSimulatedFieldTrial(
kTrialName, 100, kDefaultGroupName, test_case.entropy_value));
trial->AppendGroup("A", 80);
trial->AppendGroup("B", 10);
EXPECT_EQ(test_cases[i].expected_group, trial->group_name());
EXPECT_EQ(test_case.expected_group, trial->group_name());

// Field trial shouldn't have been registered with the list.
EXPECT_FALSE(FieldTrialList::TrialExists(kTrialName));
Expand Down

0 comments on commit da23db5

Please sign in to comment.