shill: Fix a remove-while-iterating issue in HookTable.
HookTable::Run() previously iterated through the list of actions and
invoked the start callback of each action, which was not safe because an
action that completes inline could call HookTable::Remove() to remove
itself from the list. This CL fixes the issue by iterating through a
list of start callback of each action.
BUG=chromium:380526
TEST=Tested the following:
1. FEATURES=test emerge-$BOARD platform2`
2. Verify that shill executes the termination action upon suspend on a
DUT with a cellular device.
Change-Id: Ie32a567d8de1017ff1262ba5e625e5a7a00cf01f
Reviewed-on: https://chromium-review.googlesource.com/202538
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Ben Chan <benchan@chromium.org>
diff --git a/hook_table.cc b/hook_table.cc
index 418ce07..8d5f3ab 100644
--- a/hook_table.cc
+++ b/hook_table.cc
@@ -4,6 +4,7 @@
#include "shill/hook_table.h"
+#include <list>
#include <string>
#include <base/bind.h>
@@ -19,6 +20,7 @@
using base::Closure;
using base::ConstRef;
using base::Unretained;
+using std::list;
using std::string;
namespace shill {
@@ -26,7 +28,7 @@
HookTable::HookTable(EventDispatcher *event_dispatcher)
: event_dispatcher_(event_dispatcher) {}
-void HookTable::Add(const string &name, const base::Closure &start) {
+void HookTable::Add(const string &name, const Closure &start) {
Remove(name);
hook_table_.insert(
HookTableMap::value_type(name, HookAction(start)));
@@ -74,14 +76,21 @@
// Otherwise, if the first action completes inline, its call to
// ActionComplete() will cause the |done| callback to be invoked before the
// rest of the actions get started.
+ //
+ // An action that completes inline could call HookTable::Remove(), which
+ // modifies |hook_table_|. It is thus not safe to iterate through
+ // |hook_table_| to execute the actions. Instead, we keep a list of start
+ // callback of each action and iterate through that to invoke the callback.
+ list<Closure> action_start_callbacks;
for (auto &hook_entry : hook_table_) {
HookAction *action = &hook_entry.second;
+ action_start_callbacks.push_back(action->start);
action->started = true;
action->completed = false;
}
// Now start the actions.
- for (auto &hook_entry : hook_table_) {
- hook_entry.second.start.Run();
+ for (auto &callback : action_start_callbacks) {
+ callback.Run();
}
}
diff --git a/hook_table_unittest.cc b/hook_table_unittest.cc
index bc280f7..36cd231 100644
--- a/hook_table_unittest.cc
+++ b/hook_table_unittest.cc
@@ -26,9 +26,17 @@
namespace shill {
+namespace {
+
+const char kName[] = "test";
+const char kName1[] = "test1";
+const char kName2[] = "test2";
+const char kName3[] = "test3";
+
+} // namespace
+
class HookTableTest : public testing::Test {
public:
- static const char kName[];
MOCK_METHOD0(StartAction, void());
MOCK_METHOD0(StartAction2, void());
MOCK_METHOD1(DoneAction, void(const Error &));
@@ -45,8 +53,6 @@
HookTable hook_table_;
};
-const char HookTableTest::kName[] = "test";
-
TEST_F(HookTableTest, ActionCompletes) {
EXPECT_CALL(*this, StartAction());
EXPECT_CALL(*this, DoneAction(IsSuccess()));
@@ -66,6 +72,30 @@
hook_table->ActionComplete(name);
}
+ACTION_P2(CompleteActionAndRemoveAction, hook_table, name) {
+ hook_table->ActionComplete(name);
+ hook_table->Remove(name);
+}
+
+TEST_F(HookTableTest, ActionCompletesAndRemovesActionInDoneCallback) {
+ EXPECT_CALL(*this, StartAction())
+ .WillOnce(CompleteActionAndRemoveAction(&hook_table_, kName));
+ EXPECT_CALL(*this, StartAction2())
+ .WillOnce(CompleteAction(&hook_table_, kName2));
+ EXPECT_CALL(*this, DoneAction(IsSuccess()));
+ Closure start_cb = Bind(&HookTableTest::StartAction, Unretained(this));
+ Closure start2_cb = Bind(&HookTableTest::StartAction2, Unretained(this));
+ Callback<void(const Error &)> done_cb = Bind(&HookTableTest::DoneAction,
+ Unretained(this));
+ hook_table_.Add(kName, start_cb);
+ hook_table_.Add(kName2, start2_cb);
+ hook_table_.Run(0, done_cb);
+
+ // Ensure that the timeout callback got cancelled. If it did not get
+ // cancelled, done_cb will be run twice and make this test fail.
+ event_dispatcher_.DispatchPendingEvents();
+}
+
TEST_F(HookTableTest, ActionCompletesInline) {
// StartAction completes immediately before HookTable::Run() returns.
EXPECT_CALL(*this, StartAction())
@@ -102,9 +132,6 @@
}
TEST_F(HookTableTest, MultipleActionsAllSucceed) {
- const string kName1 = "test1";
- const string kName2 = "test2";
- const string kName3 = "test3";
Closure pending_cb;
const int kTimeout = 10;
EXPECT_CALL(*this, StartAction()).Times(2);
@@ -128,9 +155,6 @@
}
TEST_F(HookTableTest, MultipleActionsAndOneTimesOut) {
- const string kName1 = "test1";
- const string kName2 = "test2";
- const string kName3 = "test3";
Closure pending_cb;
const int kTimeout = 1;
EXPECT_CALL(*this, StartAction()).Times(3);
@@ -237,5 +261,4 @@
hook_table_.ActionComplete(kName);
}
-
} // namespace shill