[bt][sco] Use unique_ptr/WeakPtr for ScoConnections instead of RefPtr
Model ownership of ScoConnection using std::unique_ptr and fxl::WeakPtr
instead of fbl::RefPtr. This makes the ownership model clearer and
reduces usage of fbl, which we can't use on Pigweed.
Bug: 100658
Test: fx test //src/connectivity/bluetooth/core/bt-host
Change-Id: Icce164fab33fa5733a237e77d88e738bd335999a
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/692091
Reviewed-by: Xo Wang <xow@google.com>
Commit-Queue: Ben Lawson <benlawson@google.com>
diff --git a/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.cc b/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.cc
index 4757c8f..51f17a3 100644
--- a/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.cc
+++ b/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.cc
@@ -243,7 +243,7 @@
ProfileServer::ScoConnectionServer::ScoConnectionServer(
fidl::InterfaceRequest<fuchsia::bluetooth::bredr::ScoConnection> request,
- fbl::RefPtr<bt::sco::ScoConnection> connection)
+ fxl::WeakPtr<bt::sco::ScoConnection> connection)
: ServerBase(this, std::move(request)), connection_(std::move(connection)) {
binding()->set_error_handler([this](zx_status_t) { Close(ZX_ERR_CANCELED); });
}
@@ -649,7 +649,7 @@
return;
}
- fbl::RefPtr<bt::sco::ScoConnection> connection = std::move(result.value().first);
+ fxl::WeakPtr<bt::sco::ScoConnection> connection = std::move(result.value().first);
const uint16_t max_tx_data_size = connection->max_tx_sdu_size();
fidl::InterfaceHandle<fidlbredr::ScoConnection> sco_handle;
diff --git a/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.h b/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.h
index 0dd38f4..defafbad 100644
--- a/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.h
+++ b/src/connectivity/bluetooth/core/bt-host/fidl/profile_server.h
@@ -46,7 +46,7 @@
class ScoConnectionServer final : public ServerBase<fuchsia::bluetooth::bredr::ScoConnection> {
public:
ScoConnectionServer(fidl::InterfaceRequest<fuchsia::bluetooth::bredr::ScoConnection> request,
- fbl::RefPtr<bt::sco::ScoConnection> connection);
+ fxl::WeakPtr<bt::sco::ScoConnection> connection);
~ScoConnectionServer() override;
void Activate(fit::callback<void()> on_closed);
void Read(ReadCallback callback) override;
@@ -55,7 +55,7 @@
private:
void TryRead();
void Close(zx_status_t epitaph);
- fbl::RefPtr<bt::sco::ScoConnection> connection_;
+ fxl::WeakPtr<bt::sco::ScoConnection> connection_;
fit::callback<void()> on_closed_;
// Non-null when a read request is waiting for an inbound packet.
fit::callback<void(fuchsia::bluetooth::bredr::RxPacketStatus, std::vector<uint8_t>)> read_cb_;
diff --git a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.cc b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.cc
index 2e15cae..2aa2bcb 100644
--- a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.cc
+++ b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.cc
@@ -29,13 +29,6 @@
});
}
-fbl::RefPtr<ScoConnection> ScoConnection::Create(
- std::unique_ptr<hci::Connection> connection, fit::closure deactivated_cb,
- hci_spec::SynchronousConnectionParameters parameters, hci::ScoDataChannel* channel) {
- return fbl::AdoptRef(
- new ScoConnection(std::move(connection), std::move(deactivated_cb), parameters, channel));
-}
-
ScoConnection::UniqueId ScoConnection::unique_id() const {
// HCI connection handles are unique per controller.
return handle();
diff --git a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.h b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.h
index 33c0d40..c7deff1d 100644
--- a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.h
+++ b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection.h
@@ -9,9 +9,6 @@
#include <queue>
-#include <fbl/ref_counted.h>
-#include <fbl/ref_ptr.h>
-
#include "src/connectivity/bluetooth/core/bt-host/common/byte_buffer.h"
#include "src/connectivity/bluetooth/core/bt-host/hci/connection.h"
#include "src/connectivity/bluetooth/core/bt-host/transport/sco_data_channel.h"
@@ -20,20 +17,19 @@
namespace bt::sco {
// ScoConnection is a wrapper around an owned SCO hci::Connection. It provides a
-// high-level interface to the underlying connection. This class implements the required ChannelT
-// template parameter methods of SocketChannelRelay and SocketFactory.
+// high-level interface to the underlying connection.
//
// This class is intended to be owned by a ScoConnectionManager.
-class ScoConnection final : public hci::ScoDataChannel::ConnectionInterface,
- public fbl::RefCounted<ScoConnection> {
+class ScoConnection final : public hci::ScoDataChannel::ConnectionInterface {
public:
// |connection| is the underlying connection and must have the link type kSCO or kESCO.
// |deactivated_cb| will be called when the connection has been Deactivated and should be
// destroyed.
- static fbl::RefPtr<ScoConnection> Create(std::unique_ptr<hci::Connection> connection,
- fit::closure deactivated_cb,
- hci_spec::SynchronousConnectionParameters parameters,
- hci::ScoDataChannel* channel);
+ ScoConnection(std::unique_ptr<hci::Connection> connection, fit::closure deactivated_cb,
+ hci_spec::SynchronousConnectionParameters parameters, hci::ScoDataChannel* channel);
+
+ // Destroying this object will disconnect the underlying HCI connection.
+ ~ScoConnection() override = default;
hci_spec::ConnectionHandle handle() const override { return handle_; }
@@ -68,6 +64,8 @@
// If an inbound packet is ready to be read, returns the packet. Otherwise, returns nullptr.
std::unique_ptr<hci::ScoDataPacket> Read();
+ fxl::WeakPtr<ScoConnection> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); }
+
// ScoDataChannel overrides:
hci_spec::SynchronousConnectionParameters parameters() override;
std::unique_ptr<hci::ScoDataPacket> GetNextOutboundPacket() override;
@@ -75,15 +73,6 @@
void OnHciError() override;
private:
- friend class fbl::RefPtr<ScoConnection>;
-
- explicit ScoConnection(std::unique_ptr<hci::Connection> connection, fit::closure deactivated_cb,
- hci_spec::SynchronousConnectionParameters parameters,
- hci::ScoDataChannel* channel);
-
- // Destroying this object will disconnect the underlying HCI connection.
- ~ScoConnection() = default;
-
// Common clean up logic for Close() and Deactivate(). Marks connection as inactive and closes the
// underlying connection.
void CleanUp();
diff --git a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.cc b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.cc
index 8f55ef7..f0cf866 100644
--- a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.cc
+++ b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.cc
@@ -85,8 +85,8 @@
// iterator, which would be invalidated by the removal.
while (connections_.size() > 0) {
auto pair = connections_.begin();
- auto handle = pair->first;
- auto conn = pair->second;
+ hci_spec::ConnectionHandle handle = pair->first;
+ ScoConnection* conn = pair->second.get();
conn->Close();
// Make sure we erase the connection if Close doesn't so the loop terminates.
@@ -117,7 +117,7 @@
cb(fitx::error(result.take_error()));
return;
}
- cb(fitx::ok(std::move(result.value().first)));
+ cb(fitx::ok(result.value().first));
});
}
@@ -186,15 +186,16 @@
};
hci_spec::SynchronousConnectionParameters conn_params =
in_progress_request_->parameters[in_progress_request_->current_param_index];
- auto conn = ScoConnection::Create(std::move(link), std::move(deactivated_cb), conn_params,
- transport_->sco_data_channel());
+ auto conn = std::make_unique<ScoConnection>(std::move(link), std::move(deactivated_cb),
+ conn_params, transport_->sco_data_channel());
+ fxl::WeakPtr<ScoConnection> conn_weak = conn->GetWeakPtr();
- auto [_, success] = connections_.try_emplace(connection_handle, conn);
+ auto [_, success] = connections_.try_emplace(connection_handle, std::move(conn));
ZX_ASSERT_MSG(success, "SCO connection already exists with handle %#.4x (peer: %s)",
connection_handle, bt_str(peer_id_));
CompleteRequest(
- fitx::ok(std::make_pair(std::move(conn), in_progress_request_->current_param_index)));
+ fitx::ok(std::make_pair(std::move(conn_weak), in_progress_request_->current_param_index)));
return hci::CommandChannel::EventCallbackResult::kContinue;
}
diff --git a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.h b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.h
index a52a158..b740fb7 100644
--- a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.h
+++ b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager.h
@@ -50,7 +50,7 @@
// |kCanceled| if a connection was never attempted, or |kFailed| if establishing a connection
// failed. Returns a handle that will cancel the request when dropped (if connection establishment
// has not started).
- using OpenConnectionResult = fitx::result<HostError, fbl::RefPtr<ScoConnection>>;
+ using OpenConnectionResult = fitx::result<HostError, fxl::WeakPtr<ScoConnection>>;
using OpenConnectionCallback = fit::callback<void(OpenConnectionResult)>;
RequestHandle OpenConnection(hci_spec::SynchronousConnectionParameters parameters,
OpenConnectionCallback callback);
@@ -64,7 +64,7 @@
// cancel the request when destroyed (if connection establishment has not started).
using AcceptConnectionResult =
fitx::result<HostError,
- std::pair<fbl::RefPtr<ScoConnection>, size_t /*index of parameters used*/>>;
+ std::pair<fxl::WeakPtr<ScoConnection>, size_t /*index of parameters used*/>>;
using AcceptConnectionCallback = fit::callback<void(AcceptConnectionResult)>;
RequestHandle AcceptConnection(std::vector<hci_spec::SynchronousConnectionParameters> parameters,
AcceptConnectionCallback callback);
@@ -73,7 +73,7 @@
using ScoRequestId = uint64_t;
using ConnectionResult =
fitx::result<HostError,
- std::pair<fbl::RefPtr<ScoConnection>, size_t /*index of parameters used*/>>;
+ std::pair<fxl::WeakPtr<ScoConnection>, size_t /*index of parameters used*/>>;
using ConnectionCallback = fit::callback<void(ConnectionResult)>;
class ConnectionRequest final {
@@ -146,7 +146,7 @@
std::optional<ConnectionRequest> in_progress_request_;
// Holds active connections.
- std::unordered_map<hci_spec::ConnectionHandle, fbl::RefPtr<ScoConnection>> connections_;
+ std::unordered_map<hci_spec::ConnectionHandle, std::unique_ptr<ScoConnection>> connections_;
// Handler IDs for registered events
std::vector<hci::CommandChannel::EventHandlerId> event_handler_ids_;
diff --git a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager_unittest.cc b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager_unittest.cc
index d379dbb..c2ea079 100644
--- a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager_unittest.cc
+++ b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_manager_unittest.cc
@@ -296,12 +296,13 @@
RunLoopUntilIdle();
ASSERT_TRUE(conn_result.has_value());
ASSERT_TRUE(conn_result->is_ok());
+ EXPECT_TRUE(conn_result->value());
EXPECT_CMD_PACKET_OUT(test_device(), testing::DisconnectPacket(kScoConnectionHandle));
DestroyManager();
RunLoopUntilIdle();
- // Ref should still be valid.
- EXPECT_TRUE(conn_result->value());
+ // WeakPtr should become invalid.
+ EXPECT_FALSE(conn_result->value());
}
TEST_F(ScoConnectionManagerTest, QueueThreeRequestsCancelsSecond) {
@@ -398,7 +399,7 @@
RunLoopUntilIdle();
ASSERT_TRUE(conn_result.has_value());
ASSERT_TRUE(conn_result->is_ok());
- fbl::RefPtr<ScoConnection> conn = std::move(conn_result->value());
+ fxl::WeakPtr<ScoConnection> conn = conn_result->value();
EXPECT_EQ(conn->handle(), kScoConnectionHandle);
auto disconn_status_packet =
diff --git a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_unittest.cc b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_unittest.cc
index 1abcecd..43efe97 100644
--- a/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_unittest.cc
+++ b/src/connectivity/bluetooth/core/bt-host/sco/sco_connection_unittest.cc
@@ -56,16 +56,16 @@
TestingBase::TearDown();
}
- virtual fbl::RefPtr<ScoConnection> CreateScoConnection(
+ virtual std::unique_ptr<ScoConnection> CreateScoConnection(
std::unique_ptr<hci::Connection> hci_conn) {
- return ScoConnection::Create(
+ return std::make_unique<ScoConnection>(
std::move(hci_conn), [this] { OnDeactivated(); },
hci_spec::SynchronousConnectionParameters(), /*channel=*/nullptr);
}
void OnDeactivated() { deactivated_cb_count_++; }
- auto sco_conn() { return sco_conn_; }
+ ScoConnection* sco_conn() { return sco_conn_.get(); }
auto hci_conn() { return hci_conn_; }
@@ -73,19 +73,19 @@
private:
size_t deactivated_cb_count_;
- fbl::RefPtr<ScoConnection> sco_conn_;
+ std::unique_ptr<ScoConnection> sco_conn_;
fxl::WeakPtr<hci::ScoConnection> hci_conn_;
};
class HciScoConnectionTest : public ScoConnectionTest {
public:
- fbl::RefPtr<ScoConnection> CreateScoConnection(
+ std::unique_ptr<ScoConnection> CreateScoConnection(
std::unique_ptr<hci::Connection> hci_conn) override {
constexpr hci_spec::SynchronousConnectionParameters hci_conn_params{
.input_data_path = hci_spec::ScoDataPath::kHci,
.output_data_path = hci_spec::ScoDataPath::kHci,
};
- return ScoConnection::Create(
+ return std::make_unique<ScoConnection>(
std::move(hci_conn), [this] { OnDeactivated(); }, hci_conn_params,
transport()->sco_data_channel());
}
@@ -93,7 +93,7 @@
class HciScoConnectionTestWithFakeScoChannel : public ScoConnectionTest {
public:
- fbl::RefPtr<ScoConnection> CreateScoConnection(
+ std::unique_ptr<ScoConnection> CreateScoConnection(
std::unique_ptr<hci::Connection> hci_conn) override {
channel_ = std::make_unique<hci::FakeScoDataChannel>(/*mtu=*/kHciScoMtu);
@@ -101,7 +101,7 @@
.input_data_path = hci_spec::ScoDataPath::kHci,
.output_data_path = hci_spec::ScoDataPath::kHci,
};
- return ScoConnection::Create(
+ return std::make_unique<ScoConnection>(
std::move(hci_conn), [this] { OnDeactivated(); }, hci_conn_params, channel_.get());
}
@@ -225,7 +225,7 @@
auto closed_cb = [&] { close_count++; };
std::vector<std::unique_ptr<hci::ScoDataPacket>> packets;
- auto rx_callback = [&packets, sco_conn = sco_conn().get()]() {
+ auto rx_callback = [&packets, sco_conn = sco_conn()->GetWeakPtr()]() {
std::unique_ptr<hci::ScoDataPacket> packet = sco_conn->Read();
ASSERT_TRUE(packet);
packets.push_back(std::move(packet));
@@ -364,7 +364,7 @@
// Queue a packet on a second connection.
auto hci_conn_1 = std::make_unique<hci::ScoConnection>(kConnectionHandle2, DeviceAddress(),
DeviceAddress(), transport()->WeakPtr());
- fbl::RefPtr<ScoConnection> sco_conn_1 = CreateScoConnection(std::move(hci_conn_1));
+ std::unique_ptr<ScoConnection> sco_conn_1 = CreateScoConnection(std::move(hci_conn_1));
size_t close_count_1 = 0;
auto closed_cb_1 = [&] { close_count_1++; };
EXPECT_TRUE(sco_conn_1->Activate(/*rx_callback=*/[] {}, std::move(closed_cb_1)));
diff --git a/src/connectivity/bluetooth/core/bt-host/socket/socket_factory_unittest.cc b/src/connectivity/bluetooth/core/bt-host/socket/socket_factory_unittest.cc
index e622796..c4e314c 100644
--- a/src/connectivity/bluetooth/core/bt-host/socket/socket_factory_unittest.cc
+++ b/src/connectivity/bluetooth/core/bt-host/socket/socket_factory_unittest.cc
@@ -52,10 +52,7 @@
fbl::RefPtr<l2cap::testing::FakeChannel> channel_;
};
-TEST_F(SocketFactoryTest, TemplatesCompile) {
- socket::SocketFactory<l2cap::Channel> l2cap_factory;
- socket::SocketFactory<sco::ScoConnection> sco_factory;
-}
+TEST_F(SocketFactoryTest, TemplatesCompile) { socket::SocketFactory<l2cap::Channel> l2cap_factory; }
TEST_F(SocketFactoryTest, CanCreateSocket) {
FactoryT socket_factory;
