UM: Fix async callback handling.
A bug in EvaluationContext::OnValueChangedOrPollTimeout() causes
scheduling of a reevaluation to fail. We fix it by copying the callback
pointer to a local variable and clearing it prior to invoking the
callback.
This also adds a check in the evaluation context unit tests code to
ensure that the EC under test is actually being destroyed; this is
useful to ensure that unit tests are not leaking EC references, e.g. by
form of pending main loop events that are holding EC handles in
closures.
BUG=chromium:391037
TEST=New unit test fails before fix, succeeds after it.
Change-Id: I63486b80525ec19d0cd399c52eea39d991a4ff53
Reviewed-on: https://chromium-review.googlesource.com/206538
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/evaluation_context_unittest.cc b/update_manager/evaluation_context_unittest.cc
index 9c51022..c60bcd6 100644
--- a/update_manager/evaluation_context_unittest.cc
+++ b/update_manager/evaluation_context_unittest.cc
@@ -16,6 +16,7 @@
#include "update_engine/update_manager/umtest_utils.h"
using base::Bind;
+using base::Closure;
using base::Time;
using base::TimeDelta;
using chromeos_update_engine::FakeClock;
@@ -26,6 +27,8 @@
using testing::StrictMock;
using testing::_;
+namespace chromeos_update_manager {
+
namespace {
void DoNothing() {}
@@ -39,9 +42,29 @@
return *value;
}
-} // namespace
+template<typename T>
+void ReadVar(scoped_refptr<EvaluationContext> ec, Variable<T>* var) {
+ ec->GetValue(var);
+}
-namespace chromeos_update_manager {
+// Runs |evaluation|; if the value pointed by |count_p| is greater than zero,
+// decrement it and schedule a reevaluation; otherwise, writes true to |done_p|.
+void EvaluateRepeatedly(Closure evaluation, scoped_refptr<EvaluationContext> ec,
+ int* count_p, bool* done_p) {
+ evaluation.Run();
+
+ // Schedule reevaluation if needed.
+ if (*count_p > 0) {
+ Closure closure = Bind(EvaluateRepeatedly, evaluation, ec, count_p, done_p);
+ ASSERT_TRUE(ec->RunOnValueChangeOrTimeout(closure))
+ << "Failed to schedule reevaluation, count_p=" << *count_p;
+ (*count_p)--;
+ } else {
+ *done_p = true;
+ }
+}
+
+} // namespace
class UmEvaluationContextTest : public ::testing::Test {
protected:
@@ -54,7 +77,19 @@
}
virtual void TearDown() {
- eval_ctx_ = NULL;
+ // Ensure that the evaluation context did not leak and is actually being
+ // destroyed.
+ if (eval_ctx_) {
+ base::WeakPtr<EvaluationContext> eval_ctx_weak_alias =
+ eval_ctx_->weak_ptr_factory_.GetWeakPtr();
+ UMTEST_ASSERT_NOT_NULL(eval_ctx_weak_alias.get());
+ eval_ctx_ = nullptr;
+ UMTEST_EXPECT_NULL(eval_ctx_weak_alias.get())
+ << "The evaluation context was not destroyed! This is likely a bug "
+ "in how the test was written, look for leaking handles to the EC, "
+ "possibly through closure objects.";
+ }
+
// Check that the evaluation context removed all the observers.
EXPECT_TRUE(fake_int_var_.observer_list_.empty());
EXPECT_TRUE(fake_async_var_.observer_list_.empty());
@@ -76,10 +111,10 @@
FakeVariable<string> fake_const_var_ = {"fake_const", kVariableModeConst};
FakeVariable<string> fake_poll_var_ = {"fake_poll",
TimeDelta::FromSeconds(1)};
- StrictMock<MockVariable<string>> mock_var_async_{"mock_var_async",
- kVariableModeAsync};
- StrictMock<MockVariable<string>> mock_var_poll_{"mock_var_poll",
- kVariableModePoll};
+ StrictMock<MockVariable<string>> mock_var_async_ {
+ "mock_var_async", kVariableModeAsync};
+ StrictMock<MockVariable<string>> mock_var_poll_ {
+ "mock_var_poll", kVariableModePoll};
};
TEST_F(UmEvaluationContextTest, GetValueFails) {
@@ -153,7 +188,7 @@
EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&DoNothing)));
}
-// Test that we don't schedule an event if there's no variable to wait for.
+// Test that reevaluation occurs when an async variable it depends on changes.
TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutWithVariablesTest) {
fake_async_var_.reset(new string("Async value"));
eval_ctx_->GetValue(&fake_async_var_);
@@ -203,7 +238,7 @@
EXPECT_FALSE(value);
}
-// Test that we don't schedule an event if there's no variable to wait for.
+// Test that reevaluation occurs when a polling timeout fires.
TEST_F(UmEvaluationContextTest, RunOnValueChangeOrTimeoutRunsFromTimeoutTest) {
fake_poll_var_.reset(new string("Polled value"));
eval_ctx_->GetValue(&fake_poll_var_);
@@ -217,6 +252,25 @@
EXPECT_TRUE(value);
}
+// Scheduling two reevaluations from the callback should succeed.
+TEST_F(UmEvaluationContextTest,
+ RunOnValueChangeOrTimeoutReevaluatesRepeatedly) {
+ fake_poll_var_.reset(new string("Polled value"));
+ Closure evaluation = Bind(ReadVar<string>, eval_ctx_, &fake_poll_var_);
+ int num_reevaluations = 2;
+ bool done = false;
+
+ // Run the evaluation once.
+ evaluation.Run();
+
+ // Schedule repeated reevaluations.
+ Closure closure = Bind(EvaluateRepeatedly, evaluation, eval_ctx_,
+ &num_reevaluations, &done);
+ ASSERT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(closure));
+ RunGMainLoopUntil(10000, Bind(&GetBoolean, &done));
+ EXPECT_EQ(0, num_reevaluations);
+}
+
// Test that we can delete the EvaluationContext while having pending events.
TEST_F(UmEvaluationContextTest, ObjectDeletedWithPendingEventsTest) {
fake_async_var_.reset(new string("Async value"));