[bt][gap] Clear callbacks before tearing down TestController tests
Delete callbacks that may contain dangling references to test data when
tearing down tests that use TestController.
Also move transaction count in BrEdrConnectionManagerTest from test
cases to the harness.
BT-671
Test: fx set x64 --variant="asan/bt-host-unittests"
fx run-test bluetooth_tests -t bt-host-unittests
(with local changes for BT-672 BT-673 TC-333)
Change-Id: I33d080a431345864bc5778b1a9755cc7edde3147
diff --git a/drivers/bluetooth/lib/gap/bredr_connection_manager_unittest.cc b/drivers/bluetooth/lib/gap/bredr_connection_manager_unittest.cc
index 5d0a4b0..7451afc 100644
--- a/drivers/bluetooth/lib/gap/bredr_connection_manager_unittest.cc
+++ b/drivers/bluetooth/lib/gap/bredr_connection_manager_unittest.cc
@@ -245,9 +245,14 @@
transport(), device_cache_.get(), data_domain_, true);
StartTestDevice();
+
+ test_device()->SetTransactionCallback([this] { transaction_count_++; },
+ async_get_default_dispatcher());
}
void TearDown() override {
+ // Don't trigger the transaction callback when cleaning up the manager.
+ test_device()->ClearTransactionCallback();
if (connection_manager_ != nullptr) {
// deallocating the connection manager disables connectivity.
test_device()->QueueCommandTransaction(
@@ -269,9 +274,11 @@
connection_manager_ = std::move(mgr);
}
+ RemoteDeviceCache* device_cache() const { return device_cache_.get(); }
+
data::testing::FakeDomain* data_domain() const { return data_domain_.get(); }
- RemoteDeviceCache* device_cache() const { return device_cache_.get(); }
+ int transaction_count() const { return transaction_count_; }
void QueueSuccessfulIncomingConn() const {
test_device()->QueueCommandTransaction(CommandTransaction(
@@ -298,6 +305,7 @@
std::unique_ptr<BrEdrConnectionManager> connection_manager_;
std::unique_ptr<RemoteDeviceCache> device_cache_;
fbl::RefPtr<data::testing::FakeDomain> data_domain_;
+ int transaction_count_ = 0;
FXL_DISALLOW_COPY_AND_ASSIGN(BrEdrConnectionManagerTest);
};
@@ -394,10 +402,6 @@
// Features page.
TEST_F(GAP_BrEdrConnectionManagerTest,
IncomingConnection_BrokenExtendedPageResponse) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
test_device()->QueueCommandTransaction(
CommandTransaction(kAcceptConnectionRequest,
{&kAcceptConnectionRequestRsp, &kConnectionComplete}));
@@ -421,7 +425,7 @@
RunLoopUntilIdle();
- EXPECT_EQ(6u, transactions);
+ EXPECT_EQ(6, transaction_count());
// When we deallocate the connection manager next, we should disconnect.
test_device()->QueueCommandTransaction(CommandTransaction(
@@ -437,16 +441,12 @@
RunLoopUntilIdle();
- EXPECT_EQ(9u, transactions);
+ EXPECT_EQ(9, transaction_count());
}
// Test: An incoming connection request should trigger an acceptance and an
// interrogation to discover capabilities.
TEST_F(GAP_BrEdrConnectionManagerTest, IncomingConnectionSuccess) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
EXPECT_EQ("", connmgr()->GetPeerId(kConnectionHandle));
QueueSuccessfulIncomingConn();
@@ -459,7 +459,7 @@
ASSERT_TRUE(dev);
EXPECT_EQ(dev->identifier(), connmgr()->GetPeerId(kConnectionHandle));
- EXPECT_EQ(6u, transactions);
+ EXPECT_EQ(6, transaction_count());
// When we deallocate the connection manager next, we should disconnect.
test_device()->QueueCommandTransaction(CommandTransaction(
@@ -475,7 +475,7 @@
RunLoopUntilIdle();
- EXPECT_EQ(9u, transactions);
+ EXPECT_EQ(9, transaction_count());
}
// Test: An incoming connection request should upgrade a known LE device with a
@@ -505,10 +505,6 @@
// Test: A remote disconnect should correctly remove the conneection.
TEST_F(GAP_BrEdrConnectionManagerTest, RemoteDisconnect) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
EXPECT_EQ("", connmgr()->GetPeerId(kConnectionHandle));
QueueSuccessfulIncomingConn();
@@ -521,7 +517,7 @@
ASSERT_TRUE(dev);
EXPECT_EQ(dev->identifier(), connmgr()->GetPeerId(kConnectionHandle));
- EXPECT_EQ(6u, transactions);
+ EXPECT_EQ(6, transaction_count());
// Remote end disconnects.
test_device()->SendCommandChannelPacket(kDisconnectionComplete);
@@ -540,7 +536,7 @@
RunLoopUntilIdle();
- EXPECT_EQ(8u, transactions);
+ EXPECT_EQ(8, transaction_count());
}
const auto kRemoteNameRequestCompleteFailed =
@@ -557,10 +553,6 @@
// - Receiving extra responses after a command fails will not fail
// - We don't query extended features if we don't receive an answer.
TEST_F(GAP_BrEdrConnectionManagerTest, IncommingConnectionFailedInterrogation) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
test_device()->QueueCommandTransaction(
CommandTransaction(kAcceptConnectionRequest,
{&kAcceptConnectionRequestRsp, &kConnectionComplete}));
@@ -582,7 +574,7 @@
RunLoopUntilIdle();
- EXPECT_EQ(5u, transactions);
+ EXPECT_EQ(5, transaction_count());
}
const auto kCapabilitiesRequest =
@@ -613,10 +605,6 @@
// TODO(jamuraa): returns correct capabilities when we have different
// requirements.
TEST_F(GAP_BrEdrConnectionManagerTest, CapabilityRequest) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
test_device()->QueueCommandTransaction(kCapabilitiesRequestReply,
{&kCapabilitiesRequestReplyRsp});
@@ -624,7 +612,7 @@
RunLoopUntilIdle();
- EXPECT_EQ(1u, transactions);
+ EXPECT_EQ(1, transaction_count());
}
const auto kUserConfirmationRequest = common::CreateStaticByteBuffer(
@@ -646,10 +634,6 @@
// Test: sends replies to Confirmation Requests
TEST_F(GAP_BrEdrConnectionManagerTest, ConfirmationRequest) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
test_device()->QueueCommandTransaction(kConfirmationRequestReply,
{&kConfirmationRequestReplyRsp});
@@ -657,22 +641,18 @@
RunLoopUntilIdle();
- EXPECT_EQ(1u, transactions);
+ EXPECT_EQ(1, transaction_count());
}
// Test: if L2CAP gets a link error, we disconnect the connection
TEST_F(GAP_BrEdrConnectionManagerTest, DisconnectOnLinkError) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
QueueSuccessfulIncomingConn();
test_device()->SendCommandChannelPacket(kConnectionRequest);
RunLoopUntilIdle();
- EXPECT_EQ(6u, transactions);
+ EXPECT_EQ(6, transaction_count());
// When we deallocate the connection manager next, we should disconnect.
test_device()->QueueCommandTransaction(CommandTransaction(
@@ -682,7 +662,7 @@
RunLoopUntilIdle();
- EXPECT_EQ(7u, transactions);
+ EXPECT_EQ(7, transaction_count());
test_device()->QueueCommandTransaction(
CommandTransaction(kReadScanEnable, {&kReadScanEnableRspBoth}));
@@ -693,21 +673,17 @@
RunLoopUntilIdle();
- EXPECT_EQ(9u, transactions);
+ EXPECT_EQ(9, transaction_count());
}
TEST_F(GAP_BrEdrConnectionManagerTest, ConnectedDeviceTimeout) {
- size_t transactions = 0;
- test_device()->SetTransactionCallback([&transactions] { transactions++; },
- async_get_default_dispatcher());
-
QueueSuccessfulIncomingConn();
test_device()->SendCommandChannelPacket(kConnectionRequest);
RunLoopUntilIdle();
- EXPECT_EQ(6u, transactions);
+ EXPECT_EQ(6, transaction_count());
auto* dev = device_cache()->FindDeviceByAddress(kTestDevAddr);
ASSERT_TRUE(dev);
diff --git a/drivers/bluetooth/lib/l2cap/channel_manager_unittest.cc b/drivers/bluetooth/lib/l2cap/channel_manager_unittest.cc
index 5ce3fea..a7fbdd0 100644
--- a/drivers/bluetooth/lib/l2cap/channel_manager_unittest.cc
+++ b/drivers/bluetooth/lib/l2cap/channel_manager_unittest.cc
@@ -56,6 +56,10 @@
}
void TearDown() override {
+ // Don't trigger the data callback when cleaning up the manager.
+ if (test_device()) {
+ test_device()->ClearDataCallback();
+ }
chanmgr_ = nullptr;
TestingBase::TearDown();
}
diff --git a/drivers/bluetooth/lib/testing/test_controller.cc b/drivers/bluetooth/lib/testing/test_controller.cc
index ad62277..f8d59c9 100644
--- a/drivers/bluetooth/lib/testing/test_controller.cc
+++ b/drivers/bluetooth/lib/testing/test_controller.cc
@@ -54,6 +54,11 @@
data_dispatcher_ = dispatcher;
}
+void TestController::ClearDataCallback() {
+ // Leave dispatcher set (if already set) to preserve its write-once-ness.
+ data_callback_ = nullptr;
+}
+
void TestController::SetTransactionCallback(
fit::closure callback,
async_dispatcher_t* dispatcher) {
@@ -66,6 +71,11 @@
transaction_dispatcher_ = dispatcher;
}
+void TestController::ClearTransactionCallback() {
+ // Leave dispatcher set (if already set) to preserve its write-once-ness.
+ transaction_callback_ = nullptr;
+}
+
void TestController::OnCommandPacketReceived(
const common::PacketView<hci::CommandHeader>& command_packet) {
uint16_t opcode = command_packet.header().opcode;
diff --git a/drivers/bluetooth/lib/testing/test_controller.h b/drivers/bluetooth/lib/testing/test_controller.h
index 15df8b2..c7e0ad7 100644
--- a/drivers/bluetooth/lib/testing/test_controller.h
+++ b/drivers/bluetooth/lib/testing/test_controller.h
@@ -58,13 +58,19 @@
const common::ByteBuffer& expected,
const std::vector<const common::ByteBuffer*>& replies);
- // Callback to invoke when a packet is received over the data channel.
+ // Callback to invoke when a packet is received over the data channel. Care
+ // should be taken to ensure that a callback with a reference to test case
+ // variables is not invoked when tearing down.
using DataCallback = fit::function<void(const common::ByteBuffer& packet)>;
void SetDataCallback(DataCallback callback, async_dispatcher_t* dispatcher);
+ void ClearDataCallback();
- // Callback invoked when a transaction completes.
+ // Callback invoked when a transaction completes. Care should be taken to
+ // ensure that a callback with a reference to test case variables is not
+ // invoked when tearing down.
void SetTransactionCallback(fit::closure callback,
async_dispatcher_t* dispatcher);
+ void ClearTransactionCallback();
private:
// FakeControllerBase overrides:
