Clear the BLE services list on disconnect.

device/bluetooth/bluetooth_device_unittest.cc

 // Copyright 2014 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 "device/bluetooth/bluetooth_device.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/run_loop.h"
@@ skipping 377 matching lines @@
   EXPECT_EQ(1, gatt_connection_attempts_);
   EXPECT_EQ(1, gatt_disconnection_attempts_);
   SimulateGattDisconnection(device);
   EXPECT_EQ(BluetoothDevice::ERROR_FAILED, last_connect_error_code_);
   for (BluetoothGattConnection* connection : gatt_connections_)
     EXPECT_FALSE(connection->IsConnected());
 }
 #endif  // defined(OS_ANDROID)
 
 #if defined(OS_ANDROID)
+// Calls CreateGattConnection & DisconnectGatt, then checks that gatt services

[ortuno, 2016/01/11 21:48:40]

You also need to add tests for bluez based systems in:

https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

[tommyt, 2016/01/12 08:42:35]

While it wouldn't be too hard to add equivalent unit tests to that bluez file,
those tests would be failing, as this functionality is not yet correctly
implemented on bluez. I think it would make more sense to implement both the
bluez tests and functionality as a separate change from this one.

[ortuno, 2016/01/12 22:28:49]

btw I'm not talking about clearing the services list but rather setting
services_discovered_ to false. Maybe I'm missing something but I don't think you
need to touch disconnect at all.

You would just need to make a two-line change:

Add "device_bluez->SetGattServicesDiscoveryComplete(false);" to
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...
if the device disconnected.

Removing the map without a proper implementation in Bluez leads to many bugs,
for example calling getPrimaryService() after all services have been discovered
would return a promise that never resolves.

[tommyt, 2016/01/13 09:42:30]

Hmm. The intention of my change was to fix something for android, and leave all
other platforms working the way they did before my change. Introducing this kind
of bug is of course not acceptable.

That being said, I have a hard time seeing: A) how my change could cause that
kind of bug, and B) how calling SetGattServicesDiscoveryComplete(false) on
disconnect would fix it. As far as I can see, calling getPrimaryService() after
all services have been discovered, should end up in the first clause (labelled
"1. & 2.") in BluetoothDispatcherHost::OnGetPrimaryService(). This should cause
AddToServicesMapAndSendGetPrimaryServiceSuccess() to be called, which should
resolve the promise right away.

The only change I did to the bluez code, was to add
device->SetGattServicesDiscoveryComplete(true) to make up for the
BluetoothDispatcherHost no longer keeping the discovery complete state in that
map. As far as I can see, whether this state is kept in a map or in a boolean
variable in each device, should not make any difference. What am I missing here?

[ortuno, 2016/01/13 18:22:01]

My bad that was an incorrect example. The issue that we currently have and I
would like to get fixed is the following:

1. Connect to a device; Its services are discovered and
gatt_services_discovered_ becomes true.
2. Disconnect.
3. Connect again. Services haven't been discovered yet but the variable is still
set to true so we hit case 3.

This can be solved if you set the variable to false when a device disconnects
the same way you do in android in onConnectionStateChanged.

I see your point in that you only want to fix Android but with that addition you
can fix Chrome OS and Linux in time for Chrome 49! Anyway, you are not
introducing any new bugs so this LGTM.

+// have been cleaned up.
+TEST_F(BluetoothTest, BluetoothGattConnection_DisconnectGatt_Cleanup) {

[scheib, 2016/01/11 18:40:09]

Thanks, would you add
+ Connecting again after a disconnect -- ensuring that a device can be connected
to again.
+ Verifying that the services discovered notification is sent
  See in this file other uses of:
  TestBluetoothAdapterObserver observer(adapter_);
  observer.gatt_services_discovered_count()

[tommyt, 2016/01/12 08:42:35]

Done.

+  InitWithFakeAdapter();
+  StartLowEnergyDiscoverySession();
+  BluetoothDevice* device = DiscoverLowEnergyDevice(3);
+
+  ResetEventCounts();
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattConnection(device);
+
+  std::vector<std::string> services;
+  services.push_back("00000000-0000-1000-8000-00805f9b34fb");
+  // 2 duplicate UUIDs creating 2 instances.
+  services.push_back("00000001-0000-1000-8000-00805f9b34fb");
+  services.push_back("00000001-0000-1000-8000-00805f9b34fb");
+  SimulateGattServicesDiscovered(device, services);
+  EXPECT_EQ(3u, device->GetGattServices().size());
+
+  device->DisconnectGatt();
+  SimulateGattDisconnection(device);
+  EXPECT_EQ(0u, device->GetGattServices().size());

[ortuno, 2016/01/11 21:48:40]

Also make sure IsGattServicesDiscovered == false.

[tommyt, 2016/01/12 08:42:35]

Done.

+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
 // Calls CreateGattConnection, but simulate errors connecting. Also, verifies
 // multiple errors should only invoke callbacks once.
 TEST_F(BluetoothTest, BluetoothGattConnection_ErrorAfterConnection) {
   InitWithFakeAdapter();
   StartLowEnergyDiscoverySession();
   BluetoothDevice* device = DiscoverLowEnergyDevice(3);
 
   ResetEventCounts();
   device->CreateGattConnection(GetGattConnectionCallback(Call::NOT_EXPECTED),
                                GetConnectErrorCallback(Call::EXPECTED));
@@ skipping 67 matching lines @@
   ResetEventCounts();
   SimulateGattConnection(device);
   EXPECT_EQ(1, gatt_discovery_attempts_);
 
   SimulateGattServicesDiscoveryError(device);
   EXPECT_EQ(0u, device->GetGattServices().size());
 }
 #endif  // defined(OS_ANDROID)
 
 }  // namespace device