Tether: Create a workaround for BLE advertisements left dangling.

Because the registration of BluetoothAdvertisements is asynchronous,
it is possible for the object which registered it (in this case,
a BleAdvertiser::IndividualAdvertisement) to have been destroyed by
the time the BluetoothAdvertisement finished registering. Due to
crbug.com/741050, this results in a BluetoothAdvertisement without
an owner to unregister it eventually, leaving it permanently dangling.

This CL works around the issue (until it is resolved) by creating a
static method which is available when the registration callback
fires. If the IndividualAdvertisement is destroyed when registration
is finished, the BluetoothAdvertisement is unregistered.

Bug: 739883
Change-Id: I33f210f3bc89504ba670387460b3ffe2ba3fdcf1
Reviewed-on: https://chromium-review.googlesource.com/567561
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486543}

chromeos/components/tether/ble_advertiser.cc

@@ -20,8 +20,64 @@ namespace {
 
 uint8_t kInvertedConnectionFlag = 0x01;
 
+// Handles the unregistration of a BluetoothAdvertisement in the case that its
+// intended owner (an IndividualAdvertisement) was destroyed before successful
+// registration. See
+// BleAdvertiser::IndividualAdvertisement::OnAdvertisementRegistered().
+void OnAdvertisementUnregisteredAfterOwnerDestruction(
+    const std::string& associated_device_id) {
+  PA_LOG(ERROR) << "Unregistered advertisement for device ID: "
+                << cryptauth::RemoteDevice::TruncateDeviceIdForLogs(
+                       associated_device_id);
+}
+
+// Handles the failed unregistration of a BluetoothAdvertisement in the case
+// that its intended owner (an IndividualAdvertisement) was destroyed before
+// successful registration. See
+// BleAdvertiser::IndividualAdvertisement::OnAdvertisementRegistered().
+//
+// It is not expected that this function ever be called; that would indicate an
+// error in Bluetooth.
+void OnAdvertisementUnregisteredAfterOwnerDestructionFailure(
+    const std::string& associated_device_id,
+    device::BluetoothAdvertisement::ErrorCode error_code) {
+  PA_LOG(ERROR) << "Error unregistering advertisement. "
+                << "Device ID: \""
+                << cryptauth::RemoteDevice::TruncateDeviceIdForLogs(
+                       associated_device_id)
+                << "\", Error code: " << error_code;
+}
+
 }  // namespace
 
+// TODO (hansberry): Remove this workaround once crbug.com/741050 has been
+// resolved.
+// static
+void BleAdvertiser::IndividualAdvertisement::OnAdvertisementRegistered(
+    base::WeakPtr<BleAdvertiser::IndividualAdvertisement>
+        individual_advertisement,
+    const std::string& associated_device_id,
+    scoped_refptr<device::BluetoothAdvertisement> advertisement) {
+  // It's possible that the IndividualAdvertisement that registered this
+  // BluetoothAdvertisement has been destroyed before being able to own the
+  // BluetoothAdvertisement. If the IndividualAdvertisement still exists, simply
+  // give it the BluetoothAdvertisement it registered. If not, unregister the
+  // BluetoothAdvertisement, because otherwise it will remain registered
+  // forever.
+  if (individual_advertisement.get()) {
+    individual_advertisement->OnAdvertisementRegisteredCallback(advertisement);
+  } else {
+    PA_LOG(WARNING) << "BluetoothAdvertisement registered, but the "
+                    << "IndividualAdvertisement which registered it no longer "
+                    << "exists. Unregistering the BluetoothAdvertisement.";
+    advertisement->Unregister(
+        base::Bind(&OnAdvertisementUnregisteredAfterOwnerDestruction,
+                   associated_device_id),
+        base::Bind(&OnAdvertisementUnregisteredAfterOwnerDestructionFailure,
+                   associated_device_id));
+  }
+}
+
 BleAdvertiser::IndividualAdvertisement::IndividualAdvertisement(
     const std::string& device_id,
     scoped_refptr<device::BluetoothAdapter> adapter,
@@ -103,8 +159,8 @@ void BleAdvertiser::IndividualAdvertisement::AdvertiseIfPossible() {
 
   adapter_->RegisterAdvertisement(
       std::move(advertisement_data),
-      base::Bind(&IndividualAdvertisement::OnAdvertisementRegisteredCallback,
-                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&OnAdvertisementRegistered, weak_ptr_factory_.GetWeakPtr(),
+                 device_id_),
       base::Bind(&IndividualAdvertisement::OnAdvertisementErrorCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }

chromeos/components/tether/ble_advertiser.h

@@ -61,6 +61,12 @@ class BleAdvertiser {
       : public device::BluetoothAdapter::Observer,
         public device::BluetoothAdvertisement::Observer {
    public:
+    static void OnAdvertisementRegistered(
+        base::WeakPtr<BleAdvertiser::IndividualAdvertisement>
+            individual_advertisement,
+        const std::string& associated_device_id,
+        scoped_refptr<device::BluetoothAdvertisement> advertisement);
+
     IndividualAdvertisement(
         const std::string& device_id,
         scoped_refptr<device::BluetoothAdapter> adapter,

chromeos/components/tether/ble_advertiser_unittest.cc

@@ -572,6 +572,35 @@ TEST_F(BleAdvertiserTest, SameAdvertisementAdded_FirstHasNotBeenUnregistered) {
                            individual_advertisements_[0]);
 }
 
+TEST_F(BleAdvertiserTest,
+       AdvertisementRegisteredAfterIndividualAdvertisementWasDestroyed) {
+  EXPECT_CALL(*mock_adapter_, AddObserver(_)).Times(1);
+  EXPECT_CALL(*mock_adapter_, RemoveObserver(_)).Times(1);
+  EXPECT_CALL(*mock_adapter_, IsPowered()).Times(1);
+  EXPECT_CALL(*mock_adapter_, RegisterAdvertisementWithArgsStruct(_)).Times(1);
+
+  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]);
+
+  EXPECT_TRUE(ble_advertiser_->StopAdvertisingToDevice(fake_devices_[0]));
+  EXPECT_TRUE(individual_advertisements_.empty());
+
+  // Finish advertisement registration, without the IndividualAdvertisement
+  // which registered it existing.
+  InvokeCallback(register_advertisement_args_[0]);
+
+  // Verify that some mechanism recognized that the advertisement is not owned
+  // by any IndividualAdvertisement, and unregistered it. This verification will
+  // crash if Unregister() was not called on the advertisement.
+  InvokeLastUnregisterCallbackAndRemove();
+}
+
 }  // namespace tether
 
 }  // namespace chromeos