bluetooth: Test & make StartNotifySession reentrant.

Follow up work from "bluetooth: android: Confirm the notify session after
the descriptor has been written." https://crrev.com/1712593002.

o Protects against reentrancy issue in OnStartNotifySessionSuccess
  and OnStartNotifySessionError by swapping callbacks container.
o Adds unit test for StartNotifySession failure condition.
o Adds unit test for StartNotifySession after GATT objects deleted.

BUG=584369

Review URL: https://codereview.chromium.org/1779083002

Cr-Commit-Position: refs/heads/master@{#383285}

7 files changed, 150 insertions, 29 deletions

diff --git a/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc b/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
index 3e4db9b..b27b073 100644
--- a/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
@@ -309,6 +309,9 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic_Empty) {
   EXPECT_EQ(1, gatt_write_characteristic_attempts_);
   SimulateGattCharacteristicWrite(characteristic1_);
 
+  // Duplicate write reported from OS shouldn't cause a problem:
+  SimulateGattCharacteristicWrite(characteristic1_);
+
   EXPECT_EQ(empty_vector, last_write_value_);
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
@@ -324,7 +327,7 @@ TEST_F(BluetoothGattCharacteristicTest, ReadRemoteCharacteristic_AfterDeleted) {
       GetGattErrorCallback(Call::NOT_EXPECTED));
 
   RememberCharacteristicForSubsequentAction(characteristic1_);
-  DeleteDevice(device_);
+  DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
 
   std::vector<uint8_t> empty_vector;
   SimulateGattCharacteristicRead(/* use remembered characteristic */ nullptr,
@@ -346,7 +349,7 @@ TEST_F(BluetoothGattCharacteristicTest,
       GetGattErrorCallback(Call::NOT_EXPECTED));
 
   RememberCharacteristicForSubsequentAction(characteristic1_);
-  DeleteDevice(device_);
+  DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
 
   SimulateGattCharacteristicWrite(/* use remembered characteristic */ nullptr);
   EXPECT_TRUE("Did not crash!");
@@ -538,7 +541,7 @@ TEST_F(BluetoothGattCharacteristicTest,
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
 
-  // TODO(crbug.com/591740): Remove if define for OS_ANDROID in this test.
+  // TODO(591740): Remove if define for OS_ANDROID in this test.
 }
 #endif  // defined(OS_ANDROID) || defined(OS_WIN)
 
@@ -916,6 +919,60 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
 #endif  // defined(OS_ANDROID)
 
 #if defined(OS_ANDROID)
+// Tests multiple StartNotifySessions pending and then an error.
+TEST_F(BluetoothGattCharacteristicTest, StartNotifySessionError_Multiple) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
+                                       GetGattErrorCallback(Call::EXPECTED));
+  characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
+                                       GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
+  EXPECT_EQ(0, callback_count_);
+  SimulateGattNotifySessionStartError(characteristic1_,
+                                      BluetoothGattService::GATT_ERROR_FAILED);
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(2, error_callback_count_);
+  ASSERT_EQ(0u, notify_sessions_.size());
+  EXPECT_EQ(BluetoothGattService::GATT_ERROR_FAILED, last_gatt_error_code_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests StartNotifySession completing after chrome objects are deleted.
+TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_AfterDeleted) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid()
+          .canonical_value());
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
+                                       GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
+  EXPECT_EQ(0, callback_count_);
+
+  RememberCharacteristicForSubsequentAction(characteristic1_);
+  RememberCCCDescriptorForSubsequentAction(characteristic1_);
+  DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
+
+  SimulateGattNotifySessionStarted(/* use remembered characteristic */ nullptr);
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(1, error_callback_count_);
+  ASSERT_EQ(0u, notify_sessions_.size());
+  EXPECT_EQ(BluetoothGattService::GATT_ERROR_FAILED, last_gatt_error_code_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
 // Tests Characteristic Value changes during a Notify Session.
 TEST_F(BluetoothGattCharacteristicTest, GattCharacteristicValueChanged) {
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
@@ -946,14 +1003,16 @@ TEST_F(BluetoothGattCharacteristicTest,
   ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
       /* properties: NOTIFY */ 0x10,
       /* expected_config_descriptor_value: NOTIFY */ 1));
+  TestBluetoothAdapterObserver observer(adapter_);
 
   RememberCharacteristicForSubsequentAction(characteristic1_);
-  DeleteDevice(device_);
+  DeleteDevice(device_);  // TODO(576906) delete only the characteristic.
 
   std::vector<uint8_t> empty_vector;
   SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr,
                                     empty_vector);
   EXPECT_TRUE("Did not crash!");
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
 }
 #endif  // defined(OS_ANDROID)
 
diff --git a/device/bluetooth/bluetooth_gatt_descriptor_unittest.cc b/device/bluetooth/bluetooth_gatt_descriptor_unittest.cc
index 190de7d..69bac49 100644
--- a/device/bluetooth/bluetooth_gatt_descriptor_unittest.cc
+++ b/device/bluetooth/bluetooth_gatt_descriptor_unittest.cc
@@ -94,7 +94,7 @@ TEST_F(BluetoothGattDescriptorTest, GetIdentifier) {
   BluetoothGattCharacteristic* char5 = service3->GetCharacteristics()[0];
   BluetoothGattCharacteristic* char6 = service3->GetCharacteristics()[1];
   // 6 descriptors (same UUID), 1 on each characteristic
-  // TODO(crbug.com/576900) Test multiple descriptors with same UUID on one
+  // TODO(576900) Test multiple descriptors with same UUID on one
   // characteristic.
   SimulateGattDescriptor(char1, uuid);
   SimulateGattDescriptor(char2, uuid);
@@ -202,6 +202,9 @@ TEST_F(BluetoothGattDescriptorTest, WriteRemoteDescriptor_Empty) {
   EXPECT_EQ(1, gatt_write_descriptor_attempts_);
   SimulateGattDescriptorWrite(descriptor1_);
 
+  // Duplicate write reported from OS shouldn't cause a problem:
+  SimulateGattDescriptorWrite(descriptor1_);
+
   EXPECT_EQ(empty_vector, last_write_value_);
 }
 #endif  // defined(OS_ANDROID)
@@ -215,7 +218,7 @@ TEST_F(BluetoothGattDescriptorTest, ReadRemoteDescriptor_AfterDeleted) {
                                      GetGattErrorCallback(Call::NOT_EXPECTED));
 
   RememberDescriptorForSubsequentAction(descriptor1_);
-  DeleteDevice(device_);
+  DeleteDevice(device_);  // TODO(576906) delete only the descriptor.
 
   std::vector<uint8_t> empty_vector;
   SimulateGattDescriptorRead(/* use remembered descriptor */ nullptr,
@@ -235,7 +238,7 @@ TEST_F(BluetoothGattDescriptorTest, WriteRemoteDescriptor_AfterDeleted) {
                                       GetGattErrorCallback(Call::NOT_EXPECTED));
 
   RememberDescriptorForSubsequentAction(descriptor1_);
-  DeleteDevice(device_);
+  DeleteDevice(device_);  // TODO(576906) delete only the descriptor.
 
   SimulateGattDescriptorWrite(/* use remembered descriptor */ nullptr);
   EXPECT_TRUE("Did not crash!");
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
index c4a73ff..6ad56f6 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc
@@ -48,6 +48,10 @@ BluetoothRemoteGattCharacteristicAndroid::
     ~BluetoothRemoteGattCharacteristicAndroid() {
   Java_ChromeBluetoothRemoteGattCharacteristic_onBluetoothRemoteGattCharacteristicAndroidDestruction(
       AttachCurrentThread(), j_characteristic_.obj());
+
+  if (pending_start_notify_calls_.size()) {
+    OnStartNotifySessionError(device::BluetoothGattService::GATT_ERROR_FAILED);
+  }
 }
 
 // static
@@ -137,7 +141,8 @@ void BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
     const NotifySessionCallback& callback,
     const ErrorCallback& error_callback) {
   if (!pending_start_notify_calls_.empty()) {
-    pending_start_notify_calls_.push(std::make_pair(callback, error_callback));
+    pending_start_notify_calls_.push_back(
+        std::make_pair(callback, error_callback));
     return;
   }
 
@@ -149,8 +154,8 @@ void BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
   if (!hasNotify && !hasIndicate) {
     LOG(ERROR) << "Characteristic needs NOTIFY or INDICATE";
     base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(error_callback, BluetoothGattService::GATT_ERROR_FAILED));
+        FROM_HERE, base::Bind(error_callback,
+                              BluetoothGattService::GATT_ERROR_NOT_SUPPORTED));
     return;
   }
 
@@ -178,11 +183,11 @@ void BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
     return;
   }
 
-  std::vector<uint8_t> value;
-  value.push_back(hasNotify ? 1 : 2);
-  value.push_back(0);
+  std::vector<uint8_t> value(2);
+  value[0] = hasNotify ? 1 : 2;
 
-  pending_start_notify_calls_.push(std::make_pair(callback, error_callback));
+  pending_start_notify_calls_.push_back(
+      std::make_pair(callback, error_callback));
   ccc_descriptor[0]->WriteRemoteDescriptor(
       value, base::Bind(&BluetoothRemoteGattCharacteristicAndroid::
                             OnStartNotifySessionSuccess,
@@ -250,21 +255,23 @@ void BluetoothRemoteGattCharacteristicAndroid::OnChanged(
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionSuccess() {
-  while (!pending_start_notify_calls_.empty()) {
-    PendingStartNotifyCall callbacks = pending_start_notify_calls_.front();
-    pending_start_notify_calls_.pop();
+  std::vector<PendingStartNotifyCall> reentrant_safe_callbacks;
+  reentrant_safe_callbacks.swap(pending_start_notify_calls_);
+
+  for (const auto& callback_pair : reentrant_safe_callbacks) {
     scoped_ptr<device::BluetoothGattNotifySession> notify_session(
         new BluetoothGattNotifySessionAndroid(instance_id_));
-    callbacks.first.Run(std::move(notify_session));
+    callback_pair.first.Run(std::move(notify_session));
   }
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionError(
     BluetoothGattService::GattErrorCode error) {
-  while (!pending_start_notify_calls_.empty()) {
-    PendingStartNotifyCall callbacks = pending_start_notify_calls_.front();
-    pending_start_notify_calls_.pop();
-    callbacks.second.Run(error);
+  std::vector<PendingStartNotifyCall> reentrant_safe_callbacks;
+  reentrant_safe_callbacks.swap(pending_start_notify_calls_);
+
+  for (auto const& callback_pair : reentrant_safe_callbacks) {
+    callback_pair.second.Run(error);
   }
 }
 
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.h b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.h
index 606b7d4..fa77764 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_android.h
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_android.h
@@ -7,8 +7,6 @@
 
 #include <stdint.h>
 
-#include <queue>
-
 #include "base/android/jni_android.h"
 #include "base/containers/scoped_ptr_hash_map.h"
 #include "base/macros.h"
@@ -134,7 +132,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothRemoteGattCharacteristicAndroid
   // StartNotifySession callbacks and pending state.
   typedef std::pair<NotifySessionCallback, ErrorCallback>
       PendingStartNotifyCall;
-  std::queue<PendingStartNotifyCall> pending_start_notify_calls_;
+  std::vector<PendingStartNotifyCall> pending_start_notify_calls_;
 
   // ReadRemoteCharacteristic callbacks and pending state.
   bool read_pending_ = false;
diff --git a/device/bluetooth/test/bluetooth_test.h b/device/bluetooth/test/bluetooth_test.h
index c840935..279b2e9 100644
--- a/device/bluetooth/test/bluetooth_test.h
+++ b/device/bluetooth/test/bluetooth_test.h
@@ -137,10 +137,29 @@ class BluetoothTestBase : public testing::Test {
   virtual void RememberCharacteristicForSubsequentAction(
       BluetoothGattCharacteristic* characteristic) {}
 
+  // Remembers |characteristic|'s Client Characteristic Configuration (CCC)
+  // descriptor's platform specific object to be used in a subsequent call to
+  // methods such as SimulateGattNotifySessionStarted. This enables tests where
+  // the platform attempts to reference descriptor objects after the Chrome
+  // objects have been deleted, e.g. with DeleteDevice.
+  virtual void RememberCCCDescriptorForSubsequentAction(
+      BluetoothGattCharacteristic* characteristic) {}
+
   // Simulates a Characteristic Set Notify success.
+  // If |characteristic| is null, acts upon the characteristic & CCC
+  // descriptor provided to RememberCharacteristicForSubsequentAction &
+  // RememberCCCDescriptorForSubsequentAction.
   virtual void SimulateGattNotifySessionStarted(
       BluetoothGattCharacteristic* characteristic) {}
 
+  // Simulates a Characteristic Set Notify error.
+  // If |characteristic| is null, acts upon the characteristic & CCC
+  // descriptor provided to RememberCharacteristicForSubsequentAction &
+  // RememberCCCDescriptorForSubsequentAction.
+  virtual void SimulateGattNotifySessionStartError(
+      BluetoothGattCharacteristic* characteristic,
+      BluetoothGattService::GattErrorCode error_code) {}
+
   // Simulates a Characteristic Set Notify operation failing synchronously once
   // for an unknown reason.
   virtual void SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
diff --git a/device/bluetooth/test/bluetooth_test_android.cc b/device/bluetooth/test/bluetooth_test_android.cc
index a3dfa4d..79961e8 100644
--- a/device/bluetooth/test/bluetooth_test_android.cc
+++ b/device/bluetooth/test/bluetooth_test_android.cc
@@ -174,21 +174,50 @@ void BluetoothTestAndroid::RememberCharacteristicForSubsequentAction(
       characteristic_android->GetJavaObject().obj());
 }
 
-void BluetoothTestAndroid::SimulateGattNotifySessionStarted(
+void BluetoothTestAndroid::RememberCCCDescriptorForSubsequentAction(
     BluetoothGattCharacteristic* characteristic) {
-  BluetoothGattDescriptor* descriptor =
+  remembered_ccc_descriptor_ =
       characteristic
           ->GetDescriptorsByUUID(
               BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid())
           .at(0);
-  BluetoothRemoteGattDescriptorAndroid* descriptor_android =
-      static_cast<BluetoothRemoteGattDescriptorAndroid*>(descriptor);
+  DCHECK(remembered_ccc_descriptor_);
+  RememberDescriptorForSubsequentAction(remembered_ccc_descriptor_);
+}
+
+void BluetoothTestAndroid::SimulateGattNotifySessionStarted(
+    BluetoothGattCharacteristic* characteristic) {
+  BluetoothRemoteGattDescriptorAndroid* descriptor_android = nullptr;
+  if (characteristic) {
+    descriptor_android = static_cast<BluetoothRemoteGattDescriptorAndroid*>(
+        characteristic
+            ->GetDescriptorsByUUID(BluetoothGattDescriptor::
+                                       ClientCharacteristicConfigurationUuid())
+            .at(0));
+  }
   Java_FakeBluetoothGattDescriptor_valueWrite(
       base::android::AttachCurrentThread(),
       descriptor_android ? descriptor_android->GetJavaObject().obj() : nullptr,
       0);  // android.bluetooth.BluetoothGatt.GATT_SUCCESS
 }
 
+void BluetoothTestAndroid::SimulateGattNotifySessionStartError(
+    BluetoothGattCharacteristic* characteristic,
+    BluetoothGattService::GattErrorCode error_code) {
+  BluetoothRemoteGattDescriptorAndroid* descriptor_android = nullptr;
+  if (characteristic) {
+    descriptor_android = static_cast<BluetoothRemoteGattDescriptorAndroid*>(
+        characteristic
+            ->GetDescriptorsByUUID(BluetoothGattDescriptor::
+                                       ClientCharacteristicConfigurationUuid())
+            .at(0));
+  }
+  Java_FakeBluetoothGattDescriptor_valueWrite(
+      base::android::AttachCurrentThread(),
+      descriptor_android ? descriptor_android->GetJavaObject().obj() : nullptr,
+      BluetoothRemoteGattServiceAndroid::GetAndroidErrorCode(error_code));
+}
+
 void BluetoothTestAndroid::
     SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
         BluetoothGattCharacteristic* characteristic) {
diff --git a/device/bluetooth/test/bluetooth_test_android.h b/device/bluetooth/test/bluetooth_test_android.h
index 8fbf61c..ddf39bf 100644
--- a/device/bluetooth/test/bluetooth_test_android.h
+++ b/device/bluetooth/test/bluetooth_test_android.h
@@ -44,8 +44,13 @@ class BluetoothTestAndroid : public BluetoothTestBase {
                                   int properties) override;
   void RememberCharacteristicForSubsequentAction(
       BluetoothGattCharacteristic* characteristic) override;
+  void RememberCCCDescriptorForSubsequentAction(
+      BluetoothGattCharacteristic* characteristic) override;
   void SimulateGattNotifySessionStarted(
       BluetoothGattCharacteristic* characteristic) override;
+  void SimulateGattNotifySessionStartError(
+      BluetoothGattCharacteristic* characteristic,
+      BluetoothGattService::GattErrorCode error_code) override;
   void SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(
       BluetoothGattCharacteristic* characteristic) override;
   void SimulateGattCharacteristicChanged(
@@ -151,6 +156,7 @@ class BluetoothTestAndroid : public BluetoothTestBase {
   base::android::ScopedJavaGlobalRef<jobject> j_fake_bluetooth_adapter_;
 
   int gatt_open_connections_ = 0;
+  BluetoothGattDescriptor* remembered_ccc_descriptor_ = nullptr;
 };
 
 // Defines common test fixture name. Use TEST_F(BluetoothTest, YourTestName).