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

chromeos/components/tether/ble_advertiser.cc

Index: chromeos/components/tether/ble_advertiser.cc
diff --git a/chromeos/components/tether/ble_advertiser.cc b/chromeos/components/tether/ble_advertiser.cc
index f48087e0f72170617e8a6ff5bedad514faca016c..097899c520325f6efe24d51686707bd064c494c2 100644
--- a/chromeos/components/tether/ble_advertiser.cc
+++ b/chromeos/components/tether/ble_advertiser.cc
@@ -25,12 +25,17 @@ uint8_t kInvertedConnectionFlag = 0x01;
 BleAdvertiser::IndividualAdvertisement::IndividualAdvertisement(
     const std::string& device_id,
     scoped_refptr<device::BluetoothAdapter> adapter,
-    std::unique_ptr<cryptauth::DataWithTimestamp> advertisement_data)
+    std::unique_ptr<cryptauth::DataWithTimestamp> advertisement_data,
+    const base::Closure& on_advertisement_unregistered_callback,
+    std::unordered_set<std::string>* active_advertisement_device_ids_set)
     : device_id_(device_id),
       adapter_(adapter),
       advertisement_data_(std::move(advertisement_data)),
       is_initializing_advertising_(false),
       advertisement_(nullptr),
+      on_advertisement_unregistered_callback_(
+          on_advertisement_unregistered_callback),
+      active_advertisement_device_ids_set_(active_advertisement_device_ids_set),
       weak_ptr_factory_(this) {
   adapter_->AddObserver(this);
   AdvertiseIfPossible();
@@ -39,7 +44,7 @@ BleAdvertiser::IndividualAdvertisement::IndividualAdvertisement(
 BleAdvertiser::IndividualAdvertisement::~IndividualAdvertisement() {
   if (advertisement_) {
     advertisement_->Unregister(
-        base::Bind(&base::DoNothing),
+        on_advertisement_unregistered_callback_,
         base::Bind(&IndividualAdvertisement::OnAdvertisementUnregisterFailure,

[Ryan Hansberry, 2017/07/11 21:27:41]

I don't believe that this method is ever called with the code as-is because
IndividualAdvertisement is destroyed by the time this can be called back. So, I
propose we also inject an 'on_advertisement_unregistered_error_callback' to be
placed here (with a corresponding error callback in BleAdvertiser) -- that will
make the code here more consistent, and provides us logs in the bizarre case
that unregistration does fail. Thoughts?

[Kyle Horimoto, 2017/07/12 02:22:04]

Done.

                    weak_ptr_factory_.GetWeakPtr()));
   }
@@ -47,6 +52,13 @@ BleAdvertiser::IndividualAdvertisement::~IndividualAdvertisement() {
   adapter_->RemoveObserver(this);
 }
 
+void BleAdvertiser::IndividualAdvertisement::
+    OnPreviousAdvertisementUnregistered() {
+  DCHECK(active_advertisement_device_ids_set_->find(device_id_) ==
+         active_advertisement_device_ids_set_->end());
+  AdvertiseIfPossible();
+}
+
 void BleAdvertiser::IndividualAdvertisement::AdapterPoweredChanged(
     device::BluetoothAdapter* adapter,
     bool powered) {
@@ -60,13 +72,21 @@ void BleAdvertiser::IndividualAdvertisement::AdvertisementReleased(
 
   // If the advertisement was released, delete it and try again. Note that this
   // situation is not expected to occur under normal circumstances.
+  advertisement_->RemoveObserver(this);
   advertisement_ = nullptr;
+  active_advertisement_device_ids_set_->erase(device_id_);
+
   AdvertiseIfPossible();
 }
 
 void BleAdvertiser::IndividualAdvertisement::AdvertiseIfPossible() {
   if (!adapter_->IsPowered() || is_initializing_advertising_ ||
-      advertisement_) {
+      advertisement_ ||
+      active_advertisement_device_ids_set_->find(device_id_) !=
+          active_advertisement_device_ids_set_->end()) {
+    // It is not possible to advertise if the adapter is not powered. Likewise,
+    // we should not try to advertise if there is an advertisement already in
+    // progress.
     return;
   }
 
@@ -90,7 +110,11 @@ void BleAdvertiser::IndividualAdvertisement::AdvertiseIfPossible() {
 void BleAdvertiser::IndividualAdvertisement::OnAdvertisementRegisteredCallback(
     scoped_refptr<device::BluetoothAdvertisement> advertisement) {
   is_initializing_advertising_ = false;
+
   advertisement_ = advertisement;
+  advertisement_->AddObserver(this);
+  active_advertisement_device_ids_set_->insert(device_id_);
+
   PA_LOG(INFO) << "Advertisement registered. "
                << "Device ID: \""
                << cryptauth::RemoteDevice::TruncateDeviceIdForLogs(device_id_)
@@ -162,7 +186,8 @@ BleAdvertiser::BleAdvertiser(
     : adapter_(adapter),
       eid_generator_(std::move(eid_generator)),
       remote_beacon_seed_fetcher_(remote_beacon_seed_fetcher),
-      local_device_data_provider_(local_device_data_provider) {}
+      local_device_data_provider_(local_device_data_provider),
+      weak_ptr_factory_(this) {}
 
 bool BleAdvertiser::StartAdvertisingToDevice(
     const cryptauth::RemoteDevice& remote_device) {
@@ -208,9 +233,13 @@ bool BleAdvertiser::StartAdvertisingToDevice(
     return false;
   }
 
-  device_id_to_advertisement_map_[remote_device.GetDeviceId()] =
+  std::string device_id = remote_device.GetDeviceId();
+  device_id_to_advertisement_map_[device_id] =
       base::MakeUnique<IndividualAdvertisement>(
-          remote_device.GetDeviceId(), adapter_, std::move(advertisement));
+          remote_device.GetDeviceId(), adapter_, std::move(advertisement),
+          base::Bind(&BleAdvertiser::OnAdvertisementUnregistered,
+                     weak_ptr_factory_.GetWeakPtr(), device_id),
+          &active_advertisement_device_ids_set_);
   return true;
 }
 
@@ -219,6 +248,15 @@ bool BleAdvertiser::StopAdvertisingToDevice(
   return device_id_to_advertisement_map_.erase(remote_device.GetDeviceId()) > 0;
 }
 
+void BleAdvertiser::OnAdvertisementUnregistered(
+    const std::string& associated_device_id) {
+  active_advertisement_device_ids_set_.erase(associated_device_id);
+
+  auto it = device_id_to_advertisement_map_.find(associated_device_id);

[Ryan Hansberry, 2017/07/11 21:27:41]

nit: it's now easy to incorrectly conflate what
active_advertisement_device_ids_set_ and device_id_to_advertisement_map_ refer
to (they both seem to be talking about BluetoothAdvertisements when in reality
the first is referring to BluetoothAdvertisements and the latter to
IndividualAdvertisements). Do you mind renaming device_id_to_advertisement_map_
to device_id_to_individual_advertisement_map_?

[Kyle Horimoto, 2017/07/12 02:22:04]

Done.

+  if (it != device_id_to_advertisement_map_.end())
+    it->second->OnPreviousAdvertisementUnregistered();
+}
+
 }  // namespace tether
 
 }  // namespace chromeos