bluetooth: Add a new BluetoothAdapterClient instance and use it for G…

…etState

There are two parts to this change:

1. Create a new BluetoothAdapterClient instance. For this we add
   "alternate_bluetooth_adapter_client()" to the bundle of DBus clients.
   This new instance uses a separate DBus Connection through
   BluezDBusThreadManager. This ensures actions on one client won't
   affect the other client.

2. Implement BluetoothSystem::GetState using the new client.

Bug: 870192, 882771
Change-Id: I9faa92e8234b14dd374a04b4c9e9acbcfd7e6201
Reviewed-on: https://chromium-review.googlesource.com/c/1215427
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596520}

device/bluetooth/dbus/bluetooth_dbus_client_bundle.cc

@@ -57,6 +57,8 @@ BluetoothDBusClientBundle::BluetoothDBusClientBundle(bool use_fakes)
         BluetoothGattDescriptorClient::Create());
     bluetooth_gatt_manager_client_.reset(BluetoothGattManagerClient::Create());
     bluetooth_gatt_service_client_.reset(BluetoothGattServiceClient::Create());
+
+    alternate_bluetooth_adapter_client_.reset(BluetoothAdapterClient::Create());
   } else {
     bluetooth_adapter_client_.reset(new FakeBluetoothAdapterClient);
     bluetooth_le_advertising_manager_client_.reset(
@@ -75,6 +77,8 @@ BluetoothDBusClientBundle::BluetoothDBusClientBundle(bool use_fakes)
         new FakeBluetoothGattDescriptorClient);
     bluetooth_gatt_manager_client_.reset(new FakeBluetoothGattManagerClient);
     bluetooth_gatt_service_client_.reset(new FakeBluetoothGattServiceClient);
+
+    alternate_bluetooth_adapter_client_.reset(new FakeBluetoothAdapterClient);
   }
 }
 

device/bluetooth/dbus/bluetooth_dbus_client_bundle.h

@@ -86,6 +86,10 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDBusClientBundle {
     return bluetooth_profile_manager_client_.get();
   }
 
+  BluetoothAdapterClient* alternate_bluetooth_adapter_client() {
+    return alternate_bluetooth_adapter_client_.get();
+  }
+
  private:
   friend class BluezDBusManagerSetter;
 
@@ -109,6 +113,9 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDBusClientBundle {
   std::unique_ptr<BluetoothProfileManagerClient>
       bluetooth_profile_manager_client_;
 
+  // See "Alternate D-Bus Client" note in bluez_dbus_manager.h.
+  std::unique_ptr<BluetoothAdapterClient> alternate_bluetooth_adapter_client_;
+
   DISALLOW_COPY_AND_ASSIGN(BluetoothDBusClientBundle);
 };
 

device/bluetooth/dbus/bluez_dbus_manager.cc

@@ -43,8 +43,11 @@ namespace bluez {
 static BluezDBusManager* g_bluez_dbus_manager = nullptr;
 static bool g_using_bluez_dbus_manager_for_testing = false;
 
-BluezDBusManager::BluezDBusManager(dbus::Bus* bus, bool use_dbus_fakes)
+BluezDBusManager::BluezDBusManager(dbus::Bus* bus,
+                                   dbus::Bus* alternate_bus,
+                                   bool use_dbus_fakes)
     : bus_(bus),
+      alternate_bus_(alternate_bus),
       object_manager_support_known_(false),
       object_manager_supported_(false),
       weak_ptr_factory_(this) {
@@ -161,6 +164,11 @@ bluez::BluezDBusManager::GetBluetoothProfileManagerClient() {
   return client_bundle_->bluetooth_profile_manager_client();
 }
 
+BluetoothAdapterClient* BluezDBusManager::GetAlternateBluetoothAdapterClient() {
+  DCHECK(object_manager_support_known_);
+  return client_bundle_->alternate_bluetooth_adapter_client();
+}
+
 void BluezDBusManager::OnObjectManagerSupported(dbus::Response* response) {
   VLOG(1) << "Bluetooth supported. Initializing clients.";
   object_manager_supported_ = true;
@@ -215,6 +223,9 @@ void BluezDBusManager::InitializeClients() {
       GetSystemBus(), bluetooth_service_name);
   client_bundle_->bluetooth_profile_manager_client()->Init(
       GetSystemBus(), bluetooth_service_name);
+
+  client_bundle_->alternate_bluetooth_adapter_client()->Init(
+      alternate_bus_, bluetooth_service_name);
 }
 
 std::string BluezDBusManager::GetBluetoothServiceName() {
@@ -238,10 +249,19 @@ void BluezDBusManager::Initialize() {
   CHECK(!g_bluez_dbus_manager);
 
 #if defined(OS_CHROMEOS)
+  // On ChromeOS, BluetoothSystem needs a separate connection to Bluez, so we
+  // use BluezDBusThreadManager to get two different connections to the same
+  // services. This allows us to have two separate sets of clients in the same
+  // process.
+  BluezDBusThreadManager::Initialize();
+
   CreateGlobalInstance(chromeos::DBusThreadManager::Get()->GetSystemBus(),
+                       BluezDBusThreadManager::Get()->GetSystemBus(),
                        chromeos::DBusThreadManager::Get()->IsUsingFakes());
 #elif defined(OS_LINUX)
-  CreateGlobalInstance(BluezDBusThreadManager::Get()->GetSystemBus(),
+  // BluetoothSystem, the client that needs the extra connection, is not
+  // implemented on Linux, so no need for an extra Bus.
+  CreateGlobalInstance(BluezDBusThreadManager::Get()->GetSystemBus(), nullptr,
                        false /* use_dbus_stubs */);
 #endif
 }
@@ -251,16 +271,18 @@ std::unique_ptr<BluezDBusManagerSetter>
 bluez::BluezDBusManager::GetSetterForTesting() {
   if (!g_using_bluez_dbus_manager_for_testing) {
     g_using_bluez_dbus_manager_for_testing = true;
-    CreateGlobalInstance(nullptr, true);
+    CreateGlobalInstance(nullptr, nullptr, true);
   }
 
   return base::WrapUnique(new BluezDBusManagerSetter());
 }
 
 // static
-void BluezDBusManager::CreateGlobalInstance(dbus::Bus* bus, bool use_stubs) {
+void BluezDBusManager::CreateGlobalInstance(dbus::Bus* bus,
+                                            dbus::Bus* alternate_bus,
+                                            bool use_stubs) {
   CHECK(!g_bluez_dbus_manager);
-  g_bluez_dbus_manager = new BluezDBusManager(bus, use_stubs);
+  g_bluez_dbus_manager = new BluezDBusManager(bus, alternate_bus, use_stubs);
 }
 
 // static
@@ -274,8 +296,14 @@ void BluezDBusManager::Shutdown() {
   CHECK(g_bluez_dbus_manager);
   BluezDBusManager* dbus_manager = g_bluez_dbus_manager;
   g_bluez_dbus_manager = nullptr;
-  g_using_bluez_dbus_manager_for_testing = false;
   delete dbus_manager;
+
+#if defined(OS_CHROMEOS)
+  if (!g_using_bluez_dbus_manager_for_testing)
+    BluezDBusThreadManager::Shutdown();
+#endif
+
+  g_using_bluez_dbus_manager_for_testing = false;
   VLOG(1) << "BluezDBusManager Shutdown completed";
 }
 
@@ -364,4 +392,10 @@ void BluezDBusManagerSetter::SetBluetoothProfileManagerClient(
       ->client_bundle_->bluetooth_profile_manager_client_ = std::move(client);
 }
 
+void BluezDBusManagerSetter::SetAlternateBluetoothAdapterClient(
+    std::unique_ptr<BluetoothAdapterClient> client) {
+  bluez::BluezDBusManager::Get()
+      ->client_bundle_->alternate_bluetooth_adapter_client_ = std::move(client);
+}
+
 }  // namespace bluez

device/bluetooth/dbus/bluez_dbus_manager.h

@@ -53,6 +53,20 @@ class BluezDBusManagerSetter;
 // WeakPtrFactory when creating callbacks that run on UI thread. See
 // session_manager_client.cc for examples.
 //
+// Alternate D-Bus Client:
+//
+// BluezDBusManager is used by two separate clients. If both clients used the
+// same DBus connection to talk to BlueZ, then they could override each others'
+// state. For example, clients can start a scan with a set of filters; if
+// client #1 sets filter A, and then client #2 sets filter B, BlueZ would only
+// scan with filter B. BlueZ distinguishes between clients based on their D-Bus
+// connection, so if two clients with different connections try to start a scan
+// with two filters, BlueZ will merge these filters.
+//
+// For this reason, BluezDBusManager keeps two sets of the same client and uses
+// two separate D-Bus connections: "Bluetooth*Client" and
+// "AlternateBluetooth*Client".
+
 class DEVICE_BLUETOOTH_EXPORT BluezDBusManager {
  public:
   // Sets the global instance. Must be called before any calls to Get().
@@ -83,7 +97,7 @@ class DEVICE_BLUETOOTH_EXPORT BluezDBusManager {
 
   // Returns true once we know whether Object Manager is supported or not.
   // Until this method returns true, no classes should try to use the
-  // DBus Clients.
+  // D-Bus Clients.
   bool IsObjectManagerSupportKnown() { return object_manager_support_known_; }
 
   // Calls |callback| once we know whether Object Manager is supported or not.
@@ -110,17 +124,25 @@ class DEVICE_BLUETOOTH_EXPORT BluezDBusManager {
   BluetoothMediaTransportClient* GetBluetoothMediaTransportClient();
   BluetoothProfileManagerClient* GetBluetoothProfileManagerClient();
 
+  // See "Alternate D-Bus Client" note above.
+  BluetoothAdapterClient* GetAlternateBluetoothAdapterClient();
+
  private:
   friend class BluezDBusManagerSetter;
 
   // Creates a new BluezDBusManager using the DBusClients set in
-  // |client_bundle|.
-  explicit BluezDBusManager(dbus::Bus* bus, bool use_stubs);
+  // |client_bundle|. |alternate_bus| is used by a separate set of D-Bus
+  // clients; see "Alternate D-Bus Client" note above.
+  explicit BluezDBusManager(dbus::Bus* bus,
+                            dbus::Bus* alternate_bus,
+                            bool use_stubs);
   ~BluezDBusManager();
 
   // Creates a global instance of BluezDBusManager. Cannot be called more than
   // once.
-  static void CreateGlobalInstance(dbus::Bus* bus, bool use_stubs);
+  static void CreateGlobalInstance(dbus::Bus* bus,
+                                   dbus::Bus* alternate_bus,
+                                   bool use_stubs);
 
   void OnObjectManagerSupported(dbus::Response* response);
   void OnObjectManagerNotSupported(dbus::ErrorResponse* response);
@@ -133,6 +155,10 @@ class DEVICE_BLUETOOTH_EXPORT BluezDBusManager {
   std::string GetBluetoothServiceName();
 
   dbus::Bus* bus_;
+  // Separate D-Bus connection used by the "Alternate" set of D-Bus clients. See
+  // "Alternate D-Bus Client" note above.
+  dbus::Bus* alternate_bus_;
+
   std::unique_ptr<BluetoothDBusClientBundle> client_bundle_;
 
   base::Closure object_manager_support_known_callback_;
@@ -173,6 +199,9 @@ class DEVICE_BLUETOOTH_EXPORT BluezDBusManagerSetter {
   void SetBluetoothProfileManagerClient(
       std::unique_ptr<BluetoothProfileManagerClient> client);
 
+  void SetAlternateBluetoothAdapterClient(
+      std::unique_ptr<BluetoothAdapterClient> client);
+
  private:
   friend class BluezDBusManager;
 

services/device/bluetooth/BUILD.gn

@@ -23,6 +23,8 @@ source_set("bluetooth_system") {
 
   deps = [
     "//base",
+    "//dbus",
+    "//device/bluetooth",
   ]
 }
 
@@ -35,6 +37,8 @@ source_set("bluetooth_system_tests") {
 
   deps = [
     ":bluetooth_system",
+    "//dbus",
+    "//device/bluetooth",
     "//net",
     "//services/device:test_support",
     "//testing/gtest",

services/device/bluetooth/DEPS

@@ -0,0 +1,9 @@
+include_rules = [
+  "+dbus",
+]
+
+specific_include_rules = {
+  "bluetooth_system_unittest.cc": [
+    "+third_party/cros_system_api/dbus/service_constants.h"
+  ],
+}

services/device/bluetooth/bluetooth_system.cc

@@ -6,7 +6,11 @@
 
 #include <memory>
 #include <utility>
+#include <vector>
 
+#include "dbus/object_path.h"
+#include "device/bluetooth/dbus/bluetooth_adapter_client.h"
+#include "device/bluetooth/dbus/bluez_dbus_manager.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 
 namespace device {
@@ -23,4 +27,22 @@ BluetoothSystem::BluetoothSystem(mojom::BluetoothSystemClientPtr client) {
 
 BluetoothSystem::~BluetoothSystem() = default;
 
+void BluetoothSystem::GetState(GetStateCallback callback) {
+  std::vector<dbus::ObjectPath> object_paths =
+      GetBluetoothAdapterClient()->GetAdapters();
+  if (object_paths.empty()) {
+    std::move(callback).Run(State::kUnavailable);
+    return;
+  }
+
+  // TODO(crbug.com/870192): Return the state based on the adapter's state.
+  std::move(callback).Run(State::kPoweredOff);
+}
+
+bluez::BluetoothAdapterClient* BluetoothSystem::GetBluetoothAdapterClient() {
+  // Use AlternateBluetoothAdapterClient to avoid interfering with users of the
+  // regular BluetoothAdapterClient.
+  return bluez::BluezDBusManager::Get()->GetAlternateBluetoothAdapterClient();
+}
+
 }  // namespace device

services/device/bluetooth/bluetooth_system.h

@@ -8,6 +8,10 @@
 #include "base/macros.h"
 #include "services/device/public/mojom/bluetooth_system.mojom.h"
 
+namespace bluez {
+class BluetoothAdapterClient;
+}
+
 namespace device {
 
 class BluetoothSystem : public mojom::BluetoothSystem {
@@ -18,7 +22,12 @@ class BluetoothSystem : public mojom::BluetoothSystem {
   explicit BluetoothSystem(mojom::BluetoothSystemClientPtr client);
   ~BluetoothSystem() override;
 
+  // mojom::BluetoothSystem
+  void GetState(GetStateCallback callback) override;
+
  private:
+  bluez::BluetoothAdapterClient* GetBluetoothAdapterClient();
+
   mojom::BluetoothSystemClientPtr client_ptr_;
 
   DISALLOW_COPY_AND_ASSIGN(BluetoothSystem);