bluetooth: mac: Improve classic device discovery and update

There are a couple of problems with the current implementation of
classic device discovery:

  1. IOBluetoothDeviceInquiry's cache is never cleaned which means that
     only the first discovery session will find the device. (Since the device
     is still in the cache future discovery session will not be notified of
     the device being found.) We could send notifications for all devices
     in the cache but then we could be notifying of devices that are no longer
     around.
  2. IOBluetoothDevice getLastInquiryUpdate returns nil even for devices
     that have just been discovered during an inquiry procedure. This causes
     us to immediately remove a device that we just discovered.
  3. IOBluetoothDevice pairedDevices sometimes returns devices that are not
     paired which causes us to add a non paired device.

Solutions:
  1. Clean cache every time StartDiscovery is called. This means new sessions
     will be notified of previously seen. Which allows us to implement
     the solution for 2.
  2. Now that we notify whenever we see a device, we can update our cross
     platform last_update_time_. Then when we are removing outdated devices
     we can check last_update_time_ to see if the device should be removed.
  3. Check [IOBluetoothDevice isPaired] before adding a device.

Also changes some VLOG(1)s to VLOG(3) since they were spamming the logs.

BUG=618650,638715

Review-Url: https://codereview.chromium.org/2282763004
Cr-Commit-Position: refs/heads/master@{#421069}

device/bluetooth/bluetooth_adapter.cc

@@ -122,13 +122,13 @@ const BluetoothDevice* BluetoothAdapter::GetDevice(
   std::string canonicalized_address =
       BluetoothDevice::CanonicalizeAddress(address);
   if (canonicalized_address.empty())
-    return NULL;
+    return nullptr;
 
   DevicesMap::const_iterator iter = devices_.find(canonicalized_address);
   if (iter != devices_.end())
     return iter->second;
 
-  return NULL;
+  return nullptr;
 }
 
 void BluetoothAdapter::AddPairingDelegate(
@@ -383,14 +383,16 @@ void BluetoothAdapter::RemoveTimedOutDevices() {
 
     bool device_expired =
         (base::Time::NowFromSystemTime() - last_update_time) > timeoutSec;
-    VLOG(1) << "device: " << device->GetAddress()
+    VLOG(3) << "device: " << device->GetAddress()
             << ", last_update: " << last_update_time
             << ", exp: " << device_expired;
 
     if (!device_expired) {
       ++it;
       continue;
     }
+
+    VLOG(1) << "Removing device: " << device->GetAddress();
     DevicesMap::iterator next = it;
     next++;
     std::unique_ptr<BluetoothDevice> removed_device =

device/bluetooth/bluetooth_adapter_mac.mm

@@ -469,12 +469,19 @@
   std::string device_address =
       BluetoothClassicDeviceMac::GetDeviceAddress(device);
 
+  BluetoothDevice* device_classic = GetDevice(device_address);
+
   // Only notify observers once per device.
-  if (devices_.count(device_address))
+  if (device_classic != nullptr) {
+    VLOG(3) << "Updating classic device: " << device_classic->GetAddress();
+    device_classic->UpdateTimestamp();
     return;
+  }
 
-  BluetoothDevice* device_classic = new BluetoothClassicDeviceMac(this, device);
+  device_classic = new BluetoothClassicDeviceMac(this, device);
   devices_.set(device_address, base::WrapUnique(device_classic));
+  VLOG(1) << "Adding new classic device: " << device_classic->GetAddress();
+
   FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                     DeviceAdded(this, device_classic));
 }
@@ -567,7 +574,11 @@
 void BluetoothAdapterMac::AddPairedDevices() {
   // Add any new paired devices.
   for (IOBluetoothDevice* device in [IOBluetoothDevice pairedDevices]) {
-    ClassicDeviceAdded(device);
+    // pairedDevices sometimes includes unknown devices that are not paired.
+    // Radar issue with id 2282763004 has been filed about it.
+    if ([device isPaired]) {
+      ClassicDeviceAdded(device);
+    }
   }
 }
 

device/bluetooth/bluetooth_classic_device_mac.h

@@ -73,7 +73,7 @@ class BluetoothClassicDeviceMac : public BluetoothDeviceMac {
       const GattConnectionCallback& callback,
       const ConnectErrorCallback& error_callback) override;
 
-  base::Time GetLastUpdateTime() const;
+  base::Time GetLastUpdateTime() const override;
 
   // Returns the Bluetooth address for the |device|. The returned address has a
   // normalized format (see below).

device/bluetooth/bluetooth_classic_device_mac.mm

@@ -62,7 +62,9 @@ BluetoothUUID ExtractUuid(IOBluetoothSDPDataElement* service_class_data) {
 BluetoothClassicDeviceMac::BluetoothClassicDeviceMac(
     BluetoothAdapterMac* adapter,
     IOBluetoothDevice* device)
-    : BluetoothDeviceMac(adapter), device_([device retain]) {}
+    : BluetoothDeviceMac(adapter), device_([device retain]) {
+  UpdateTimestamp();
+}
 
 BluetoothClassicDeviceMac::~BluetoothClassicDeviceMac() {
 }
@@ -255,8 +257,9 @@ BluetoothUUID ExtractUuid(IOBluetoothSDPDataElement* service_class_data) {
 }
 
 base::Time BluetoothClassicDeviceMac::GetLastUpdateTime() const {
-  return base::Time::FromDoubleT(
-      [[device_ getLastInquiryUpdate] timeIntervalSince1970]);
+  // getLastInquiryUpdate returns nil unpredictably so just use the
+  // cross platform implementation of last update time.
+  return last_update_time_;
 }
 
 int BluetoothClassicDeviceMac::GetHostTransmitPower(

device/bluetooth/bluetooth_device.cc

@@ -479,6 +479,10 @@ void BluetoothDevice::UpdateTimestamp() {
   last_update_time_ = base::Time::NowFromSystemTime();
 }
 
+base::Time BluetoothDevice::GetLastUpdateTime() const {
+  return last_update_time_;
+}
+
 // static
 int8_t BluetoothDevice::ClampPower(int power) {
   if (power < INT8_MIN) {

device/bluetooth/bluetooth_device.h

@@ -517,8 +517,12 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
   // empty string.
   static std::string CanonicalizeAddress(const std::string& address);
 
-  // Return the timestamp for when this device was last seen.
-  base::Time GetLastUpdateTime() const { return last_update_time_; }
+  // Update the last time this device was seen.
+  void UpdateTimestamp();
+
+  // Returns the time of the last call to UpdateTimestamp(), or base::Time() if
+  // it hasn't been called yet.
+  virtual base::Time GetLastUpdateTime() const;
 
   // Called by BluetoothAdapter when a new Advertisement is seen for this
   // device. This replaces previously seen Advertisement Data.
@@ -581,9 +585,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
 
   BluetoothDevice(BluetoothAdapter* adapter);
 
-  // Update the last time this device was seen.
-  void UpdateTimestamp();
-
   // Implements platform specific operations to initiate a GATT connection.
   // Subclasses must also call DidConnectGatt, DidFailToConnectGatt, or
   // DidDisconnectGatt immediately or asynchronously as the connection state

device/bluetooth/bluetooth_discovery_manager_mac.mm

@@ -31,7 +31,7 @@ - (id)initWithManager:(device::BluetoothDiscoveryManagerMacClassic*)manager;
 
 namespace device {
 
-// ementation of BluetoothDiscoveryManagerMac for Bluetooth classic device
+// Implementation of BluetoothDiscoveryManagerMac for Bluetooth classic device
 // discovery, using the IOBluetooth framework.
 class BluetoothDiscoveryManagerMacClassic
     : public BluetoothDiscoveryManagerMac {
@@ -58,6 +58,10 @@ bool StartDiscovery() override {
     DVLOG(1) << "Discovery requested";
     should_do_discovery_ = true;
 
+    // Clean the cache so that new discovery sessions find previously
+    // discovered devices as well.
+    [inquiry_ clearFoundDevices];
+
     if (inquiry_running_) {
       DVLOG(1) << "Device inquiry already running";
       return true;

device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm

@@ -93,7 +93,7 @@
     CBPeripheral* peripheral,
     NSDictionary* advertisementData,
     int rssi) {
-  VLOG(1) << "DiscoveredPeripheral";
+  VLOG(3) << "DiscoveredPeripheral";
   observer_->LowEnergyDeviceUpdated(peripheral, advertisementData, rssi);
 }