PolicyManager: Call all the ValueChanged() on the same main loop call.
This patch calls the ValueChanged() method on all the observers from
the same main loop calling, fixing a possible free after use if the
observer gets removed and deleted between the NotifyValueChanged()
call and the ValueChanged() calls.
BUG=chromium:355132
TEST=Unittest added.
Change-Id: I6365c3b86fe0b85502bfa81bbd18a86c55caa009
Reviewed-on: https://chromium-review.googlesource.com/192526
Reviewed-by: Alex Deymo <[email protected]>
Commit-Queue: Alex Deymo <[email protected]>
Tested-by: Alex Deymo <[email protected]>
diff --git a/policy_manager/variable.h b/policy_manager/variable.h
index 22cdd17..ab19f72 100644
--- a/policy_manager/variable.h
+++ b/policy_manager/variable.h
@@ -102,10 +102,8 @@
// Calls ValueChanged on all the observers.
void NotifyValueChanged() {
- for (auto& observer : observer_list_) {
- RunFromMainLoop(base::Bind(&BaseVariable::ObserverInterface::ValueChanged,
- base::Unretained(observer), this));
- }
+ RunFromMainLoop(base::Bind(&BaseVariable::OnValueChangedNotification,
+ base::Unretained(this)));
}
private:
@@ -113,6 +111,7 @@
friend class PmBaseVariableTest;
FRIEND_TEST(PmBaseVariableTest, RepeatedObserverTest);
FRIEND_TEST(PmBaseVariableTest, NotifyValueChangedTest);
+ FRIEND_TEST(PmBaseVariableTest, NotifyValueRemovesObserversTest);
BaseVariable(const std::string& name, VariableMode mode,
base::TimeDelta poll_interval)
@@ -120,6 +119,22 @@
poll_interval_(mode == kVariableModePoll ?
poll_interval : base::TimeDelta()) {}
+ void OnValueChangedNotification() {
+ // A ValueChanged() method can change the list of observers, for example
+ // removing itself and invalidating the iterator, so we create a snapshot
+ // of the observers first. Also, to support the case when *another* observer
+ // is removed, we check for them.
+ std::list<BaseVariable::ObserverInterface*> observer_list_copy(
+ observer_list_);
+
+ for (auto& observer : observer_list_copy) {
+ if (std::find(observer_list_.begin(), observer_list_.end(), observer) !=
+ observer_list_.end()) {
+ observer->ValueChanged(this);
+ }
+ }
+ }
+
// The default PollInterval in minutes.
static constexpr int kDefaultPollMinutes = 5;
diff --git a/policy_manager/variable_unittest.cc b/policy_manager/variable_unittest.cc
index 81e1e59..f68f7d3 100644
--- a/policy_manager/variable_unittest.cc
+++ b/policy_manager/variable_unittest.cc
@@ -106,6 +106,53 @@
// Check that all the observers are called.
EXPECT_EQ(2, observer1.calls_.size());
EXPECT_EQ(1, observer2.calls_.size());
+
+ var.RemoveObserver(&observer1);
+ var.RemoveObserver(&observer2);
+}
+
+class BaseVariableObserverRemover : public BaseVariable::ObserverInterface {
+ public:
+ BaseVariableObserverRemover() : calls_(0) {}
+
+ void ValueChanged(BaseVariable* variable) override {
+ for (auto& observer : remove_observers_) {
+ variable->RemoveObserver(observer);
+ }
+ calls_++;
+ }
+
+ void OnCallRemoveObserver(BaseVariable::ObserverInterface* observer) {
+ remove_observers_.push_back(observer);
+ }
+
+ int get_calls() { return calls_; }
+
+ private:
+ vector<BaseVariable::ObserverInterface*> remove_observers_;
+ int calls_;
+};
+
+// Tests that we can remove an observer from a Variable on the ValueChanged()
+// call to that observer.
+TEST(PmBaseVariableTest, NotifyValueRemovesObserversTest) {
+ DefaultVariable<int> var("var", kVariableModeAsync);
+ BaseVariableObserverRemover observer1;
+ BaseVariableObserverRemover observer2;
+
+ var.AddObserver(&observer1);
+ var.AddObserver(&observer2);
+
+ // Make each observer remove both observers on ValueChanged.
+ observer1.OnCallRemoveObserver(&observer1);
+ observer1.OnCallRemoveObserver(&observer2);
+ observer2.OnCallRemoveObserver(&observer1);
+ observer2.OnCallRemoveObserver(&observer2);
+
+ var.NotifyValueChanged();
+ RunGMainLoopMaxIterations(100);
+
+ EXPECT_EQ(1, observer1.get_calls() + observer2.get_calls());
}
} // namespace chromeos_policy_manager
