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

chromeos/components/tether/ble_advertiser.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/components/tether/ble_advertiser.h"
 
 #include "base/bind.h"
 #include "chromeos/components/tether/ble_constants.h"
 #include "components/cryptauth/local_device_data_provider.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
 #include "components/cryptauth/remote_beacon_seed_fetcher.h"
 #include "components/proximity_auth/logging/logging.h"
 #include "device/bluetooth/bluetooth_advertisement.h"
 
 namespace chromeos {
 
 namespace tether {
 
 namespace {
 
 uint8_t kInvertedConnectionFlag = 0x01;
 
 }  // namespace
 
 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();
 }
 
 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()));
   }
 
   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) {
   DCHECK(adapter_.get() == adapter);
   AdvertiseIfPossible();
 }
 
 void BleAdvertiser::IndividualAdvertisement::AdvertisementReleased(
     device::BluetoothAdvertisement* advertisement) {
   DCHECK(advertisement_.get() == advertisement);
 
   // 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;
   }
 
   is_initializing_advertising_ = true;
 
   std::unique_ptr<device::BluetoothAdvertisement::Data> advertisement_data =
       base::MakeUnique<device::BluetoothAdvertisement::Data>(
           device::BluetoothAdvertisement::AdvertisementType::
               ADVERTISEMENT_TYPE_BROADCAST);
   advertisement_data->set_service_uuids(CreateServiceUuids());
   advertisement_data->set_service_data(CreateServiceData());
 
   adapter_->RegisterAdvertisement(
       std::move(advertisement_data),
       base::Bind(&IndividualAdvertisement::OnAdvertisementRegisteredCallback,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&IndividualAdvertisement::OnAdvertisementErrorCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 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_)
                << "\", Service data: " << advertisement_data_->DataInHex();
 }
 
 void BleAdvertiser::IndividualAdvertisement::OnAdvertisementErrorCallback(
     device::BluetoothAdvertisement::ErrorCode error_code) {
   is_initializing_advertising_ = false;
   PA_LOG(ERROR) << "Error registering advertisement. "
@@ skipping 51 matching lines @@
 BleAdvertiser::~BleAdvertiser() {}
 
 BleAdvertiser::BleAdvertiser(
     scoped_refptr<device::BluetoothAdapter> adapter,
     std::unique_ptr<cryptauth::ForegroundEidGenerator> eid_generator,
     const cryptauth::RemoteBeaconSeedFetcher* remote_beacon_seed_fetcher,
     const cryptauth::LocalDeviceDataProvider* local_device_data_provider)
     : 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) {
   if (device_id_to_advertisement_map_.size() >= kMaxConcurrentAdvertisements) {
     PA_LOG(ERROR) << "Attempted to register a device when the maximum number "
                   << "of devices have already been registered.";
     return false;
   }
 
   std::string local_device_public_key;
@@ skipping 25 matching lines @@
   std::unique_ptr<cryptauth::DataWithTimestamp> advertisement =
       eid_generator_->GenerateAdvertisement(local_device_public_key,
                                             remote_beacon_seeds);
   if (!advertisement) {
     PA_LOG(WARNING) << "Error generating advertisement for device with ID "
                     << remote_device.GetTruncatedDeviceIdForLogs() << ". "
                     << "Cannot advertise.";
     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;
 }
 
 bool BleAdvertiser::StopAdvertisingToDevice(
     const cryptauth::RemoteDevice& remote_device) {
   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