[Bluetooth] Release ServerSocket resources before tear down.

Add ServerSocket::Disconnect() to release the underlying socket's
resources before destroying the reference, and make Nearby Connections
use and block on the new method when BluetoothServerSocket is torn down.

Fixed: 1151533
Change-Id: Ib52b66117a099afa2a843446b8aaca0b53ed2f31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617125Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842230}

chrome/services/sharing/nearby/platform/bluetooth_server_socket.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/services/sharing/nearby/platform/bluetooth_server_socket.h"
 
+#include "base/logging.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
 #include "chrome/services/sharing/nearby/platform/bluetooth_socket.h"
@@ -19,7 +20,9 @@ BluetoothServerSocket::BluetoothServerSocket(
           base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()})),
       server_socket_(std::move(server_socket), task_runner_) {}
 
-BluetoothServerSocket::~BluetoothServerSocket() = default;
+BluetoothServerSocket::~BluetoothServerSocket() {
+  Close();
+}
 
 std::unique_ptr<api::BluetoothSocket> BluetoothServerSocket::Accept() {
   bluetooth::mojom::AcceptConnectionResultPtr result;
@@ -35,7 +38,14 @@ std::unique_ptr<api::BluetoothSocket> BluetoothServerSocket::Accept() {
 }
 
 Exception BluetoothServerSocket::Close() {
-  server_socket_.reset();
+  if (server_socket_) {
+    if (server_socket_->Disconnect()) {
+      VLOG(1) << "Successfully tore down Nearby Bluetooth server socket.";
+    } else {
+      LOG(ERROR) << "Failed to tear down Nearby Bluetooth server socket.";
+    }
+    server_socket_.reset();
+  }
   return {Exception::kSuccess};
 }
 

chrome/services/sharing/nearby/platform/bluetooth_server_socket_unittest.cc

@@ -65,6 +65,9 @@ class FakeServerSocket : public bluetooth::mojom::ServerSocket {
   void Accept(AcceptCallback callback) override {
     std::move(callback).Run(std::move(accept_connection_result_));
   }
+  void Disconnect(DisconnectCallback callback) override {
+    std::move(callback).Run();
+  }
 
   bluetooth::mojom::AcceptConnectionResultPtr accept_connection_result_;
   base::OnceClosure on_destroy_callback_;

chrome/services/sharing/nearby/test_support/fake_adapter.cc

@@ -72,6 +72,9 @@ class FakeServerSocket : public mojom::ServerSocket {
   void Accept(AcceptCallback callback) override {
     std::move(callback).Run(/*result=*/nullptr);
   }
+  void Disconnect(DisconnectCallback callback) override {
+    std::move(callback).Run();
+  }
 };
 
 }  // namespace

device/bluetooth/public/mojom/adapter.mojom

@@ -92,8 +92,9 @@ interface DiscoverySession {
 };
 
 // Represents an open connection to a remote device. Releasing it will destroy
-// the underlying connection, but callers should prefer to let a call to
-// Disconnect() to finish first.
+// the underlying connection, but if callers want to try again soon, they should
+// call Disconnect() first and wait for completion to ensure that the resource
+// has been completely released.
 // Note: Methods which are declared [Sync] are for use by
 // //chrome/services/sharing/nearby; all other usage of their synchronous
 // signatures is strongly discouraged.
@@ -107,7 +108,9 @@ interface Socket {
 };
 
 // Represents a pending connecting from a remote device. Releasing it will
-// stop listening for an incoming connection.
+// stop listening for an incoming connection, but if callers want to start
+// listening again soon, they should call Disconnect() first and wait for
+// completion to ensure that the resource has been completely released.
 // Note: Methods which are declared [Sync] are for use by
 // //chrome/services/sharing/nearby; all other usage of their synchronous
 // signatures is strongly discouraged.
@@ -116,6 +119,13 @@ interface ServerSocket {
   // goes wrong, the returned |result| will be null.
   [Sync]
   Accept() => (AcceptConnectionResult? result);
+
+  // Use to gracefully close the underlying resource before destroying. The
+  // reply callback can be used to synchronize an attempt to re-initialize
+  // a server socket; attempting to listen on the same server socket on this
+  // device may fail with a busy error until the reply callback is invoked.
+  [Sync]
+  Disconnect() => ();
 };
 
 // Handles requests to either query Bluetooth adapter capabilities or state, or

device/bluetooth/server_socket.cc

@@ -36,6 +36,11 @@ void ServerSocket::Accept(AcceptCallback callback) {
                      weak_ptr_factory_.GetWeakPtr(), copyable_callback));
 }
 
+void ServerSocket::Disconnect(DisconnectCallback callback) {
+  DCHECK(server_socket_);
+  server_socket_->Disconnect(std::move(callback));
+}
+
 void ServerSocket::OnAccept(
     AcceptCallback callback,
     const device::BluetoothDevice* device,

device/bluetooth/server_socket.h

@@ -33,6 +33,7 @@ class ServerSocket : public mojom::ServerSocket {
 
   // mojom::ServerSocket:
   void Accept(AcceptCallback callback) override;
+  void Disconnect(DisconnectCallback callback) override;
 
  private:
   void OnAccept(AcceptCallback callback,

device/bluetooth/server_socket_unittest.cc

@@ -123,4 +123,9 @@ TEST_F(ServerSocketTest, TestAccept_Error) {
   EXPECT_FALSE(fake_bluetooth_server_socket_->HasAcceptArgs());
 }
 
+TEST_F(ServerSocketTest, TestDisconnect) {
+  server_socket_->Disconnect(base::DoNothing());
+  EXPECT_TRUE(fake_bluetooth_server_socket_->called_disconnect());
+}
+
 }  // namespace bluetooth