bluetooth: Call error callback when characteristics get destroyed during a GATT Event (mac)

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
index 553618e3f3d1956a67c996c3434804101e8ea85e..1f6326b22460000025421b0677c718722338673e 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
@@ -378,6 +378,43 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
+// TODO(crbug.com/621901): Enable test on all platforms.
+#if defined(OS_MACOSX)
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       ReadRemoteCharacteristic_Disconnected) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate(
+      BluetoothRemoteGattCharacteristic::PROPERTY_READ));
+
+  characteristic1_->ReadRemoteCharacteristic(
+      GetReadValueCallback(Call::NOT_EXPECTED),
+      GetGattErrorCallback(Call::EXPECTED));
+
+#if !defined(OS_MACOSX)
+  // On macOS we don't get any events after disconnection so no need to

[scheib, 2016/09/16 19:50:12]

If there's just 'no need to remember' then we might as well do it the same on
all platforms to simplify the test.

I'm guessing something goes 'bonk' on macOS, or we don't support
imulateGattCharacteristicRead(nullptr on macOS, and that's why you've done this.
If so, say so in the comment.

[ortuno, 2016/09/20 02:41:43]

Through experimentation and the bug report for this CL I found that once a
device disconnects we get no events for that device on macOS. This is the
opposite of what happens on Android. When you disconnect during a gatt
operation, an event is still dispatched for that gatt object. (I believe that is
the real cause of the race condition we've been seeing in which a characteristic
is destroyed before the gatt event arrives). I think we still want to test this
behavior on Android.
We could definitely support RememberCharacteristicForSubsequentAction but it
would be a non-significant refactor. I haven't seen any evidence that it's
something we actually want to test so I added a CHECK to make sure the
assumption that we will never get events after disconnection is satisfied. If we
start crashing then we know we need to handle that case and can refactor the mac
testing framework to test for it.

[scheib, 2016/09/20 03:53:16]

Thanks for explanation, please include in code the explanation of why mac is
ifdef'd (complexity that doesn't test a scenario we hit, FYI we CHECK that we
don't hit it), and that on non-mac we're testing the read event arriving after
disconnection. 

[ortuno, 2016/09/22 03:06:32]

Done.

+  // remember the characteristic for the read response.
+  RememberCharacteristicForSubsequentAction(characteristic1_);
+#endif
+
+  ASSERT_EQ(1u, adapter_->GetDevices().size());
+  SimulateGattDisconnection(adapter_->GetDevices()[0]);
+
+  EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,
+            last_gatt_error_code_);
+
+#if !defined(OS_MACOSX)
+  // On macOS we don't get any events after disconnection so no need to
+  // dispatch a read response.
+  std::vector<uint8_t> empty_vector;
+  SimulateGattCharacteristicRead(nullptr /* use remembered characteristic */,
+                                 empty_vector);
+#endif
+}
+#endif  // defined(OS_MACOSX)
+
 #if defined(OS_ANDROID) || defined(OS_WIN)
 // Tests WriteRemoteCharacteristic completing after Chrome objects are deleted.
 // macOS: Not applicable: This can never happen if CBPeripheral delegate is set
@@ -404,6 +441,42 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
+// TODO(crbug.com/621901): Enable test on all platforms.
+#if defined(OS_MACOSX)
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       WriteRemoteCharacteristic_Disconnected) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate(
+      BluetoothRemoteGattCharacteristic::PROPERTY_WRITE));
+
+  std::vector<uint8_t> empty_vector;
+  characteristic1_->WriteRemoteCharacteristic(
+      empty_vector, GetCallback(Call::NOT_EXPECTED),
+      GetGattErrorCallback(Call::EXPECTED));
+
+#if !defined(OS_MACOSX)
+  // On macOS we don't get any events after disconnection so no need to
+  // remember the characteristic for the write response.
+  RememberCharacteristicForSubsequentAction(characteristic1_);
+#endif  // !defined(OS_MACOSX)
+
+  ASSERT_EQ(1u, adapter_->GetDevices().size());
+  SimulateGattDisconnection(adapter_->GetDevices()[0]);
+
+  EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,
+            last_gatt_error_code_);
+
+#if !defined(OS_MACOSX)
+  // On macOS we don't get any events after disconnection so no need
+  // to dispatch a write response.
+  SimulateGattCharacteristicWrite(/* use remembered characteristic */ nullptr);
+#endif  // !defined(OS_MACOSX)
+}
+#endif  // defined(OS_MACOSX)
+
 #if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 // Tests ReadRemoteCharacteristic and GetValue with non-empty value buffer.
 TEST_F(BluetoothRemoteGattCharacteristicTest, ReadRemoteCharacteristic) {