pw_bluetooth_proxy: Remove static mutex on l2cap channel

Now the underlying bug in Vector is fixed we can remove this workaround.

Bug: 381942905
Change-Id: Ic66361eecb12625915fefef23547b5b4ac3a8aee
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/252852
Reviewed-by: Ben Lawson <benlawson@google.com>
Docs-Not-Needed: Austin Foxley <afoxley@google.com>
Reviewed-by: David Rees <drees@google.com>
Lint: Lint 🤖 <android-build-ayeaye@system.gserviceaccount.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed-service-accounts.iam.gserviceaccount.com>
Pigweed-Auto-Submit: Austin Foxley <afoxley@google.com>

pw_bluetooth_proxy/l2cap_channel.cc

@@ -27,6 +27,15 @@
 
 namespace pw::bluetooth::proxy {
 
+void L2capChannel::MoveLockedFields(L2capChannel& other) {
+  std::lock_guard lock(send_queue_mutex_);
+  std::lock_guard other_lock(other.send_queue_mutex_);
+  send_queue_ = std::move(other.send_queue_);
+  notify_on_dequeue_ = other.notify_on_dequeue_;
+  l2cap_channel_manager_.ReleaseChannel(other);
+  l2cap_channel_manager_.RegisterChannel(*this);
+}
+
 L2capChannel::L2capChannel(L2capChannel&& other)
     : l2cap_channel_manager_(other.l2cap_channel_manager_),
       connection_handle_(other.connection_handle()),
@@ -36,15 +45,7 @@ L2capChannel::L2capChannel(L2capChannel&& other)
       queue_space_available_fn_(std::move(other.queue_space_available_fn_)),
       payload_from_controller_fn_(
           std::move(other.payload_from_controller_fn_)) {
-  // All L2capChannels share a static mutex, so only one lock needs to be
-  // acquired here.
-  // TODO: https://pwbug.dev/381942905 - Once mutex no longer needs to be
-  // static, elide this operator or acquire a lock on both channels' mutexes.
-  std::lock_guard lock(global_send_queue_mutex_);
-  send_queue_ = std::move(other.send_queue_);
-  notify_on_dequeue_ = other.notify_on_dequeue_;
-  l2cap_channel_manager_.ReleaseChannel(other);
-  l2cap_channel_manager_.RegisterChannel(*this);
+  MoveLockedFields(other);
 }
 
 L2capChannel& L2capChannel::operator=(L2capChannel&& other) {
@@ -56,17 +57,9 @@ L2capChannel& L2capChannel::operator=(L2capChannel&& other) {
   transport_ = other.transport();
   local_cid_ = other.local_cid();
   remote_cid_ = other.remote_cid();
-  payload_from_controller_fn_ = std::move(other.payload_from_controller_fn_);
   queue_space_available_fn_ = std::move(other.queue_space_available_fn_);
-  // All L2capWriteChannels share a static mutex, so only one lock needs to be
-  // acquired here.
-  // TODO: https://pwbug.dev/369849508 - Once mutex is no longer static,
-  // elide this operator or acquire a lock on both channels' mutexes.
-  std::lock_guard lock(global_send_queue_mutex_);
-  send_queue_ = std::move(other.send_queue_);
-  notify_on_dequeue_ = other.notify_on_dequeue_;
-  l2cap_channel_manager_.ReleaseChannel(other);
-  l2cap_channel_manager_.RegisterChannel(*this);
+  payload_from_controller_fn_ = std::move(other.payload_from_controller_fn_);
+  MoveLockedFields(other);
   return *this;
 }
 
@@ -78,7 +71,7 @@ L2capChannel::~L2capChannel() {
 Status L2capChannel::QueuePacket(H4PacketWithH4&& packet) {
   Status status;
   {
-    std::lock_guard lock(global_send_queue_mutex_);
+    std::lock_guard lock(send_queue_mutex_);
     if (send_queue_.full()) {
       status = Status::Unavailable();
       notify_on_dequeue_ = true;
@@ -95,7 +88,7 @@ std::optional<H4PacketWithH4> L2capChannel::DequeuePacket() {
   std::optional<H4PacketWithH4> packet;
   bool should_notify = false;
   {
-    std::lock_guard lock(global_send_queue_mutex_);
+    std::lock_guard lock(send_queue_mutex_);
     if (!send_queue_.empty()) {
       packet.emplace(std::move(send_queue_.front()));
       send_queue_.pop();
@@ -200,7 +193,7 @@ void L2capChannel::ReportPacketsMayBeReadyToSend() {
 }
 
 void L2capChannel::ClearQueue() {
-  std::lock_guard lock(global_send_queue_mutex_);
+  std::lock_guard lock(send_queue_mutex_);
   send_queue_.clear();
 }
 

pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h

@@ -132,8 +132,7 @@ class L2capChannel : public IntrusiveForwardList<L2capChannel>::Item {
   // Alert `L2capChannelManager` that queued packets may be ready to send.
   // When calling this method, ensure no locks are held that are also acquired
   // in `Dequeue()` overrides.
-  void ReportPacketsMayBeReadyToSend()
-      PW_LOCKS_EXCLUDED(global_send_queue_mutex_);
+  void ReportPacketsMayBeReadyToSend() PW_LOCKS_EXCLUDED(send_queue_mutex_);
 
   // Remove all packets from queue.
   void ClearQueue();
@@ -154,6 +153,10 @@ class L2capChannel : public IntrusiveForwardList<L2capChannel>::Item {
   // TODO: https://pwbug.dev/349700888 - Make capacity configurable.
   static constexpr size_t kQueueCapacity = 5;
 
+  // Helper for move constructor and move assignment.
+  void MoveLockedFields(L2capChannel& other)
+      PW_LOCKS_EXCLUDED(send_queue_mutex_);
+
   L2capChannelManager& l2cap_channel_manager_;
 
   // ACL connection handle.
@@ -174,26 +177,18 @@ class L2capChannel : public IntrusiveForwardList<L2capChannel>::Item {
   // `L2capChannelManager` and channel may concurrently call functions that
   // access queue.
   //
-  // TODO: https://pwbug.dev/381942905 - This mutex is static to avoid it being
-  // destroyed when an L2capChannel is erased from a container. When an
-  // L2capChannel is erased, it is std::destroyed then overwritten by the
-  // subsequent L2capChannel in the container via the move assignment
-  // operator. After this, the previously destroyed L2capChannel object is
-  // again in an operational state, so its mutex needs to remain valid. This is
-  // a bug in pw::Vector::erase(). Once this behavior is fixed, we can remove
-  // the static to avoid cross-channel contention.
-  inline static sync::Mutex global_send_queue_mutex_;
+  sync::Mutex send_queue_mutex_;
 
   // Stores Tx L2CAP packets.
   InlineQueue<H4PacketWithH4, kQueueCapacity> send_queue_
-      PW_GUARDED_BY(global_send_queue_mutex_);
+      PW_GUARDED_BY(send_queue_mutex_);
 
   // Callback to notify writers after queue space opens up.
   Function<void()> queue_space_available_fn_;
 
   // True if the last queue attempt didn't have space. Will be cleared on
   // successful dequeue.
-  bool notify_on_dequeue_ PW_GUARDED_BY(global_send_queue_mutex_) = false;
+  bool notify_on_dequeue_ PW_GUARDED_BY(send_queue_mutex_) = false;
 
   //-------
   //  Rx: