[Nearby] Use SharedRemote with BT server socket

Previously the [Sync] mojo call to Accept was deadlocking. By switching
over to a ShareRemote and using a deadicated task runner we can avoid
the deadlock. ShareRemote is used to ensure the mojo is bound and run
on the same sequence even though the remote is not actually shared.

Fixed: 1133515
Change-Id: I44bc38ab8c3ddedcba02357c520d439ed7681c1b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439775
Commit-Queue: James Vecore <vecore@google.com>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812409}

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

@@ -4,6 +4,8 @@
 
 #include "chrome/services/sharing/nearby/platform_v2/bluetooth_server_socket.h"
 
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 #include "chrome/services/sharing/nearby/platform_v2/bluetooth_socket.h"
 #include "third_party/nearby/src/cpp/platform_v2/base/exception.h"
 
@@ -13,16 +15,13 @@ namespace chrome {
 
 BluetoothServerSocket::BluetoothServerSocket(
     mojo::PendingRemote<bluetooth::mojom::ServerSocket> server_socket)
-    : pending_server_socket_(std::move(server_socket)) {}
+    : task_runner_(
+          base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()})),
+      server_socket_(std::move(server_socket), task_runner_) {}
 
 BluetoothServerSocket::~BluetoothServerSocket() = default;
 
 std::unique_ptr<api::BluetoothSocket> BluetoothServerSocket::Accept() {
-  // We're now in the thread that BluetoothServerSocket primarily operates in.
-  // Bind |server_socket_| in this correct thread. See header documentation.
-  if (pending_server_socket_)
-    server_socket_.Bind(std::move(pending_server_socket_));
-
   bluetooth::mojom::AcceptConnectionResultPtr result;
   bool success = server_socket_->Accept(&result);
 
@@ -36,7 +35,6 @@ std::unique_ptr<api::BluetoothSocket> BluetoothServerSocket::Accept() {
 }
 
 Exception BluetoothServerSocket::Close() {
-  pending_server_socket_.reset();
   server_socket_.reset();
   return {Exception::kSuccess};
 }

chrome/services/sharing/nearby/platform_v2/bluetooth_server_socket.h

@@ -9,7 +9,7 @@
 
 #include "device/bluetooth/public/mojom/adapter.mojom.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
-#include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/bindings/shared_remote.h"
 #include "third_party/nearby/src/cpp/platform_v2/api/bluetooth_classic.h"
 
 namespace location {
@@ -32,14 +32,14 @@ class BluetoothServerSocket : public api::BluetoothServerSocket {
 
  private:
   // BluetoothServerSocket is created on the main thread, but its public methods
-  // are used on a separate dedicated thread. mojo::Remote objects (namely,
-  // |server_socket_|) must be bound on the same thread they are used on, to
-  // prevent deadlock. So, we hold onto this mojo::PendingRemote
-  // |pending_server_socket_| until Accept() is called, at which point
-  // |server_socket_| is bound with it (it is acceptable to pass a
-  // mojo::PendingRemote around multiple threads).
-  mojo::PendingRemote<bluetooth::mojom::ServerSocket> pending_server_socket_;
-  mojo::Remote<bluetooth::mojom::ServerSocket> server_socket_;
+  // are used on a separate dedicated thread. A mojo::ShareRemote object
+  // |server_socket_| is used to ensure that all calls on the mojo happen on
+  // the dedicated task runner/sequence regardless of the calling thread. This
+  // is necessary because bluetooth::mojom::ServerSocket.Accept() is a [Sync]
+  // mojo method and we have previously observed deadlock in the context of
+  // Nearby Connections without a SharedRemote implementation.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  mojo::SharedRemote<bluetooth::mojom::ServerSocket> server_socket_;
 };
 
 }  // namespace chrome