[CrOS Tether] Do not register a new Bluetooth advertisement until any previous identical advertisem…

chromeos/components/tether/ble_advertiser_unittest.cc

Index: chromeos/components/tether/ble_advertiser_unittest.cc
diff --git a/chromeos/components/tether/ble_advertiser_unittest.cc b/chromeos/components/tether/ble_advertiser_unittest.cc
index d9676ce8785b0e0952451b0843e61b506d3ad381..2b2ff64c57148f2606fd34cd59a8ad3dbf733970 100644
--- a/chromeos/components/tether/ble_advertiser_unittest.cc
+++ b/chromeos/components/tether/ble_advertiser_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "chromeos/components/tether/ble_advertiser.h"
 
+#include "base/bind.h"
+#include "base/callback_forward.h"
 #include "base/logging.h"
 #include "chromeos/components/tether/ble_constants.h"
 #include "components/cryptauth/mock_foreground_eid_generator.h"
@@ -11,8 +13,8 @@
 #include "components/cryptauth/mock_remote_beacon_seed_fetcher.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
 #include "components/cryptauth/remote_device_test_util.h"
+#include "device/bluetooth/bluetooth_advertisement.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
-#include "device/bluetooth/test/mock_bluetooth_advertisement.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::Invoke;
@@ -80,6 +82,33 @@ class MockBluetoothAdapterWithAdvertisements
   ~MockBluetoothAdapterWithAdvertisements() override {}
 };
 
+class FakeBluetoothAdvertisement : public device::BluetoothAdvertisement {
+ public:
+  // |unregister_callback| should be called with the success callback passed to
+  // Unregister() whenever an Unregister() call occurs.
+  FakeBluetoothAdvertisement(
+      const base::Callback<
+          void(const device::BluetoothAdvertisement::SuccessCallback&)>&
+          unregister_callback)
+      : unregister_callback_(unregister_callback) {}
+
+  // BluetoothAdvertisement:
+  void Unregister(
+      const device::BluetoothAdvertisement::SuccessCallback& success_callback,
+      const device::BluetoothAdvertisement::ErrorCallback& error_callback)
+      override {
+    unregister_callback_.Run(success_callback);
+  }
+
+ private:
+  ~FakeBluetoothAdvertisement() override {}
+
+  base::Callback<void(const device::BluetoothAdvertisement::SuccessCallback&)>
+      unregister_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeBluetoothAdvertisement);
+};
+
 std::vector<cryptauth::DataWithTimestamp> GenerateFakeAdvertisements() {
   cryptauth::DataWithTimestamp advertisement1("advertisement1", 1000L, 2000L);
   cryptauth::DataWithTimestamp advertisement2("advertisement2", 2000L, 3000L);
@@ -163,7 +192,7 @@ class BleAdvertiserTest : public testing::Test {
     ble_advertiser_.reset();
 
     // All observers should have been removed.
-    EXPECT_FALSE(individual_advertisements_.size());
+    EXPECT_TRUE(individual_advertisements_.empty());
   }
 
   void OnAdapterAddObserver(device::BluetoothAdapter::Observer* observer) {
@@ -186,11 +215,11 @@ class BleAdvertiserTest : public testing::Test {
       scoped_refptr<RegisterAdvertisementArgs> args,
       const BleAdvertiser::IndividualAdvertisement* advertisement) {
     // First, verify that the service UUID list is correct.
-    EXPECT_EQ(static_cast<size_t>(1), args->service_uuids.size());
+    EXPECT_EQ(1u, args->service_uuids.size());
     EXPECT_EQ(std::string(kAdvertisingServiceUuid), args->service_uuids[0]);
 
     // Then, verify that the service data is correct.
-    EXPECT_EQ(static_cast<size_t>(1), args->service_data.size());
+    EXPECT_EQ(1u, args->service_data.size());
     std::vector<uint8_t> service_data_from_args =
         args->service_data[std::string(kAdvertisingServiceUuid)];
     EXPECT_EQ(service_data_from_args.size(),
@@ -218,8 +247,9 @@ class BleAdvertiserTest : public testing::Test {
   }
 
   void InvokeCallback(scoped_refptr<RegisterAdvertisementArgs> args) {
-    args->callback.Run(
-        make_scoped_refptr(new device::MockBluetoothAdvertisement));
+    args->callback.Run(make_scoped_refptr(new FakeBluetoothAdvertisement(
+        base::Bind(&BleAdvertiserTest::OnAdvertisementUnregistered,
+                   base::Unretained(this)))));
   }
 
   void ReleaseAdvertisement(
@@ -228,6 +258,18 @@ class BleAdvertiserTest : public testing::Test {
         individual_advertisement->advertisement_.get());
   }
 
+  void OnAdvertisementUnregistered(
+      const device::BluetoothAdvertisement::SuccessCallback& success_callback) {
+    unregister_callbacks_.push_back(success_callback);
+  }
+
+  void InvokeLastUnregisterCallbackAndRemove() {
+    DCHECK(!unregister_callbacks_.empty());
+    unregister_callbacks_[unregister_callbacks_.size() - 1].Run();
+    unregister_callbacks_.erase(unregister_callbacks_.begin() +
+                                unregister_callbacks_.size() - 1);
+  }
+
   std::unique_ptr<BleAdvertiser> ble_advertiser_;
 
   scoped_refptr<StrictMock<MockBluetoothAdapterWithAdvertisements>>
@@ -241,6 +283,8 @@ class BleAdvertiserTest : public testing::Test {
       register_advertisement_args_;
   std::vector<BleAdvertiser::IndividualAdvertisement*>
       individual_advertisements_;
+  std::vector<device::BluetoothAdvertisement::SuccessCallback>
+      unregister_callbacks_;
 
   const std::vector<cryptauth::RemoteDevice> fake_devices_;
   const std::vector<cryptauth::DataWithTimestamp> fake_advertisements_;
@@ -299,10 +343,10 @@ TEST_F(BleAdvertiserTest, AdapterPoweredOffWhenAdvertisementRegistered) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
 
   // RegisterAdvertisement() should not have been called.
-  EXPECT_FALSE(register_advertisement_args_.size());
+  EXPECT_TRUE(register_advertisement_args_.empty());
 }
 
 TEST_F(BleAdvertiserTest, RegisteringAdvertisementFails) {
@@ -315,8 +359,8 @@ TEST_F(BleAdvertiserTest, RegisteringAdvertisementFails) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
-  EXPECT_EQ(static_cast<size_t>(1), register_advertisement_args_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  EXPECT_EQ(1u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[0],
                            individual_advertisements_[0]);
 
@@ -334,8 +378,8 @@ TEST_F(BleAdvertiserTest, AdvertisementRegisteredSuccessfully) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
-  EXPECT_EQ(static_cast<size_t>(1), register_advertisement_args_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  EXPECT_EQ(1u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[0],
                            individual_advertisements_[0]);
 
@@ -344,7 +388,8 @@ TEST_F(BleAdvertiserTest, AdvertisementRegisteredSuccessfully) {
 
   // Now, unregister.
   EXPECT_TRUE(ble_advertiser_->StopAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_FALSE(individual_advertisements_.size());
+  InvokeLastUnregisterCallbackAndRemove();
+  EXPECT_TRUE(individual_advertisements_.empty());
 }
 
 TEST_F(BleAdvertiserTest, AdvertisementRegisteredSuccessfully_TwoDevices) {
@@ -358,8 +403,8 @@ TEST_F(BleAdvertiserTest, AdvertisementRegisteredSuccessfully_TwoDevices) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
-  EXPECT_EQ(static_cast<size_t>(1), register_advertisement_args_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  EXPECT_EQ(1u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[0],
                            individual_advertisements_[0]);
 
@@ -371,8 +416,8 @@ TEST_F(BleAdvertiserTest, AdvertisementRegisteredSuccessfully_TwoDevices) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[1]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[1]));
-  EXPECT_EQ(static_cast<size_t>(2), individual_advertisements_.size());
-  EXPECT_EQ(static_cast<size_t>(2), register_advertisement_args_.size());
+  EXPECT_EQ(2u, individual_advertisements_.size());
+  EXPECT_EQ(2u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[1],
                            individual_advertisements_[1]);
 
@@ -382,9 +427,11 @@ TEST_F(BleAdvertiserTest, AdvertisementRegisteredSuccessfully_TwoDevices) {
   // Now, unregister.
   EXPECT_TRUE(ble_advertiser_->StopAdvertisingToDevice(fake_devices_[0]));
   EXPECT_EQ(1u, individual_advertisements_.size());
+  InvokeLastUnregisterCallbackAndRemove();
 
   EXPECT_TRUE(ble_advertiser_->StopAdvertisingToDevice(fake_devices_[1]));
-  EXPECT_EQ(0u, individual_advertisements_.size());
+  EXPECT_TRUE(individual_advertisements_.empty());
+  InvokeLastUnregisterCallbackAndRemove();
 }
 
 TEST_F(BleAdvertiserTest, TooManyDevicesRegistered) {
@@ -424,17 +471,17 @@ TEST_F(BleAdvertiserTest, AdapterPowerChange_StartsOffThenTurnsOn) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
 
   // RegisterAdvertisement() should not have been called.
-  EXPECT_FALSE(register_advertisement_args_.size());
+  EXPECT_TRUE(register_advertisement_args_.empty());
 
   // Now, simulate power being changed. Since the power is now on,
   // RegisterAdvertisement() should have been called.
   individual_advertisements_[0]->AdapterPoweredChanged(mock_adapter_.get(),
                                                        true);
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
-  EXPECT_EQ(static_cast<size_t>(1), register_advertisement_args_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  EXPECT_EQ(1u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[0],
                            individual_advertisements_[0]);
 }
@@ -449,8 +496,8 @@ TEST_F(BleAdvertiserTest, AdvertisementReleased) {
       base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
 
   EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_EQ(static_cast<size_t>(1), individual_advertisements_.size());
-  EXPECT_EQ(static_cast<size_t>(1), register_advertisement_args_.size());
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  EXPECT_EQ(1u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[0],
                            individual_advertisements_[0]);
 
@@ -460,7 +507,7 @@ TEST_F(BleAdvertiserTest, AdvertisementReleased) {
   // Now, simulate the advertisement being released. A new advertisement should
   // have been created.
   ReleaseAdvertisement(individual_advertisements_[0]);
-  EXPECT_EQ(static_cast<size_t>(2), register_advertisement_args_.size());
+  EXPECT_EQ(2u, register_advertisement_args_.size());
   VerifyServiceDataMatches(register_advertisement_args_[1],
                            individual_advertisements_[0]);
 
@@ -469,7 +516,60 @@ TEST_F(BleAdvertiserTest, AdvertisementReleased) {
 
   // Now, unregister.
   EXPECT_TRUE(ble_advertiser_->StopAdvertisingToDevice(fake_devices_[0]));
-  EXPECT_FALSE(individual_advertisements_.size());
+  InvokeLastUnregisterCallbackAndRemove();
+  EXPECT_TRUE(individual_advertisements_.empty());
+}
+
+// Regression test for crbug.com/739883. This issue arises when the following
+// occurs:
+//   (1) BleAdvertiser starts advertising to device A.
+//   (2) BleAdvertiser stops advertising to device A. The advertisement starts
+//       its asynchyronous unregistration flow.
+//   (3) BleAdvertiser starts advertising to device A again, but the previous
+//       advertisement has not yet been fully unregistered.
+// Before the fix for crbug.com/739883, this would cause an error of type
+// ERROR_ADVERTISEMENT_ALREADY_EXISTS. However, the fix for this issue ensures
+// that the new advertisement in step (3) above does not start until the
+// previous one has been finished.
+TEST_F(BleAdvertiserTest, SameAdvertisementAdded_FirstHasNotBeenUnregistered) {
+  EXPECT_CALL(*mock_adapter_, AddObserver(_)).Times(2);
+  EXPECT_CALL(*mock_adapter_, RemoveObserver(_)).Times(2);
+  EXPECT_CALL(*mock_adapter_, IsPowered()).Times(3);
+  EXPECT_CALL(*mock_adapter_, RegisterAdvertisementWithArgsStruct(_)).Times(2);
+
+  mock_eid_generator_->set_advertisement(
+      base::MakeUnique<cryptauth::DataWithTimestamp>(fake_advertisements_[0]));
+
+  EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  EXPECT_EQ(1u, register_advertisement_args_.size());
+  VerifyServiceDataMatches(register_advertisement_args_[0],
+                           individual_advertisements_[0]);
+
+  InvokeCallback(register_advertisement_args_[0]);
+  VerifyAdvertisementHasStarted(individual_advertisements_[0]);
+
+  // Unregister, but do not invoke the last unregister callback.
+  EXPECT_TRUE(ble_advertiser_->StopAdvertisingToDevice(fake_devices_[0]));
+  EXPECT_TRUE(individual_advertisements_.empty());
+
+  // Start advertising again, to the same device. A new IndividualAdvertisement
+  // should have been created, but a call to RegisterAdvertisement() should NOT
+  // have occurred, since the previous advertisement has not yet been
+  // unregistered.
+  EXPECT_TRUE(ble_advertiser_->StartAdvertisingToDevice(fake_devices_[0]));
+  EXPECT_EQ(1u, individual_advertisements_.size());
+  // Should still be only one set of RegisterAdvertisement() args.
+  EXPECT_EQ(1u, register_advertisement_args_.size());
+
+  // Now, complete the previous unregistration.
+  InvokeLastUnregisterCallbackAndRemove();
+
+  // RegisterAdvertisement() should be called for the new advertisement.
+  EXPECT_EQ(2u, register_advertisement_args_.size());
+
+  VerifyServiceDataMatches(register_advertisement_args_[1],
+                           individual_advertisements_[0]);
 }
 
 }  // namespace tether