Stop advertisement once after Cable discovery

The current BlueZ adapter implementation for BLE advertisement does not
unregister advertisements when BluetoothAdapter scoped referenece
pointer is destructed. This cause BluetoothAdapter::advertisements_
queue to increase indeterminately as BluetoothAdapter is a Chrome wide
singleton object. Manually unregister any ongoing advertisement once
CableDiscovery is destructed.

Bug: 846535
Change-Id: If7782132f99e580bfa5f6e9a0da2a116375e6b7c
Reviewed-on: https://chromium-review.googlesource.com/1072929
Commit-Queue: Jun Choi <[email protected]>
Reviewed-by: Tim Song <[email protected]>
Reviewed-by: Balazs Engedy <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#563003}(cherry picked from commit 7698eb3)
Reviewed-on: https://chromium-review.googlesource.com/1081181
Reviewed-by: Jun Choi <[email protected]>
Cr-Commit-Position: refs/branch-heads/3440@{#68}
Cr-Branched-From: 010ddcf-refs/heads/master@{#561733}

Author: Jun Choi

device/fido/fido_cable_discovery.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -128,9 +129,11 @@ FidoCableDiscovery::FidoCableDiscovery(
     std::vector<CableDiscoveryData> discovery_data)
     : discovery_data_(std::move(discovery_data)), weak_factory_(this) {}
 
-// Destruction of FidoCableDiscovery will unregister |advertisements_| on
-// best-effort basis.
-FidoCableDiscovery::~FidoCableDiscovery() = default;
+// This is a workaround for https://crbug.com/846522
+FidoCableDiscovery::~FidoCableDiscovery() {
+  for (auto advertisement : advertisements_)
+    advertisement.second->Unregister(base::DoNothing(), base::DoNothing());
+}
 
 void FidoCableDiscovery::DeviceAdded(BluetoothAdapter* adapter,
                                      BluetoothDevice* device) {

device/fido/fido_cable_discovery.h

@@ -56,6 +56,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
   ~FidoCableDiscovery() override;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(FidoCableDiscoveryTest,
+                           TestUnregisterAdvertisementUponDestruction);
+
   // BluetoothAdapter::Observer:
   void DeviceAdded(BluetoothAdapter* adapter, BluetoothDevice* device) override;
   void DeviceChanged(BluetoothAdapter* adapter,

device/fido/fido_cable_discovery_unittest.cc

@@ -11,9 +11,9 @@
 #include "base/stl_util.h"
 #include "build/build_config.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
+#include "device/bluetooth/bluetooth_advertisement.h"
 #include "device/bluetooth/test/bluetooth_test.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
-#include "device/bluetooth/test/mock_bluetooth_advertisement.h"
 #include "device/fido/fido_ble_device.h"
 #include "device/fido/fido_ble_uuids.h"
 #include "device/fido/fido_parsing_utils.h"
@@ -112,6 +112,16 @@ MATCHER_P2(IsAdvertisementContent,
   return true;
 }
 
+class CableMockBluetoothAdvertisement : public BluetoothAdvertisement {
+ public:
+  MOCK_METHOD2(Unregister,
+               void(const SuccessCallback& success_callback,
+                    const ErrorCallback& error_callback));
+
+ private:
+  ~CableMockBluetoothAdvertisement() override = default;
+};
+
 // Mock BLE adapter that abstracts out authenticator logic with the following
 // logic:
 //  - Responds to BluetoothAdapter::RegisterAdvertisement() by always invoking
@@ -150,18 +160,23 @@ class CableMockAdapter : public MockBluetoothAdapter {
   void ExpectRegisterAdvertisementWithResponse(
       bool simulate_success,
       base::span<const uint8_t> expected_client_eid,
-      base::StringPiece expected_uuid_formatted_client_eid) {
+      base::StringPiece expected_uuid_formatted_client_eid,
+      scoped_refptr<CableMockBluetoothAdvertisement> advertisement_ptr =
+          nullptr) {
+    if (!advertisement_ptr)
+      advertisement_ptr =
+          base::MakeRefCounted<CableMockBluetoothAdvertisement>();
+
     EXPECT_CALL(*this,
                 RegisterAdvertisement(
                     IsAdvertisementContent(expected_client_eid,
                                            expected_uuid_formatted_client_eid),
                     _, _))
         .WillOnce(::testing::WithArgs<1, 2>(
-            [simulate_success](const auto& success_callback,
-                               const auto& failure_callback) {
+            [simulate_success, advertisement_ptr](
+                const auto& success_callback, const auto& failure_callback) {
               simulate_success
-                  ? success_callback.Run(
-                        base::MakeRefCounted<MockBluetoothAdvertisement>())
+                  ? success_callback.Run(advertisement_ptr)
                   : failure_callback.Run(BluetoothAdvertisement::ErrorCode::
                                              INVALID_ADVERTISEMENT_ERROR_CODE);
             }));
@@ -338,4 +353,26 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) {
   scoped_task_environment_.RunUntilIdle();
 }
 
+TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) {
+  auto cable_discovery = CreateDiscovery();
+  CableMockBluetoothAdvertisement* advertisement =
+      new CableMockBluetoothAdvertisement();
+  EXPECT_CALL(*advertisement, Unregister(_, _)).Times(1);
+
+  ::testing::InSequence testing_sequence;
+  auto mock_adapter =
+      base::MakeRefCounted<::testing::NiceMock<CableMockAdapter>>();
+  mock_adapter->ExpectSuccessCallbackToSetPowered();
+  mock_adapter->ExpectRegisterAdvertisementWithResponse(
+      true /* simulate_success */, kClientEid, kUuidFormattedClientEid,
+      base::WrapRefCounted(advertisement));
+
+  BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter);
+  cable_discovery->Start();
+  scoped_task_environment_.RunUntilIdle();
+
+  EXPECT_EQ(1u, cable_discovery->advertisements_.size());
+  cable_discovery.reset();
+}
+
 }  // namespace device