bluetooth: android: Confirm the notify session after the descriptor has been written.

device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc

 // Copyright 2015 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_remote_gatt_characteristic_android.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
@@ skipping 118 matching lines @@
 
 bool BluetoothRemoteGattCharacteristicAndroid::UpdateValue(
     const std::vector<uint8_t>& value) {
   NOTIMPLEMENTED();
   return false;
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::StartNotifySession(
     const NotifySessionCallback& callback,
     const ErrorCallback& error_callback) {
-  if (Java_ChromeBluetoothRemoteGattCharacteristic_startNotifySession(
-          AttachCurrentThread(), j_characteristic_.obj())) {
-    // TODO(crbug.com/569664): Wait until descriptor write completes before
-    // reporting success via calling |callback|.
-    scoped_ptr<device::BluetoothGattNotifySession> notify_session(
-        new BluetoothGattNotifySessionAndroid(instance_id_));
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(callback, base::Passed(&notify_session)));
-  } else {
+  if (!pending_start_notify_calls_.empty()) {

[ortuno, 2016/03/04 17:39:47]

Why are we allowing multiple notify session calls? I thought we would just fail
with IN PROGRESS. I'm OK with going for a queue but hopefully some thought have
been give as to how Stop is going to be implemented since this will clearly
increase the complexity.

[scheib, 2016/03/11 03:17:31]

Unlike a read or write that must happen serially, StartNotify is more like
StartDiscoverySession. Stop is called per BluetoothGattNotifySession and
decrements a reference count. Only when all references are released is the stop
carried out. I don't think there's much complexity here that we can't get out
of. 

+    pending_start_notify_calls_.push(std::make_pair(callback, error_callback));
+    return;
+  }
+
+  Properties properties = GetProperties();
+
+  bool hasNotify = properties & PROPERTY_NOTIFY;
+  bool hasIndicate = properties & PROPERTY_INDICATE;
+
+  if (!hasNotify && !hasIndicate) {
+    LOG(ERROR) << "Characteristic needs NOTIFY or INDICATE";
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(error_callback,
                    BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));

[ortuno, 2016/03/04 17:39:47]

I think this should be GATT_ERROR_NOT_SUPPORTED.

[scheib, 2016/03/11 03:17:31]

Done.

+    return;
   }
+
+  BluetoothGattDescriptor* descriptor = GetDescriptorForUUID(
+      BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid());
+
+  if (!descriptor) {
+    LOG(ERROR)
+        << "Could not find client characteristic configuration descriptor";
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));

[ortuno, 2016/03/04 17:39:47]

Same here. I think GATT_ERROR_NOT_SUPPORTED is more appropriate.

[scheib, 2016/03/11 03:17:31]

Done.

+    return;
+  }
+
+  if (!Java_ChromeBluetoothRemoteGattCharacteristic_setCharacteristicNotification(
+          AttachCurrentThread(), j_characteristic_.obj(), true)) {
+    LOG(ERROR) << "Error enabling characteristic notification";
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattServiceAndroid::GATT_ERROR_FAILED));
+    return;
+  }
+
+  std::vector<uint8_t> value;

[ortuno, 2016/03/04 17:39:47]

nit: value(2) // The Client Characteristic Configuration descriptor is 16bits
long

Also this would be nicer if we were allowed to use initialization syntax:

std::vector<uint8_t> value = hasNotify ? {1, 0} : {2, 0};

[scheib, 2016/03/11 03:17:31]

Done.

+  value.push_back(hasNotify ? 1 : 2);
+  value.push_back(0);
+
+  pending_start_notify_calls_.push(std::make_pair(callback, error_callback));
+  descriptor->WriteRemoteDescriptor(
+      value, base::Bind(&BluetoothRemoteGattCharacteristicAndroid::
+                            OnStartNotifySessionSuccess,
+                        base::Unretained(this)),

[ortuno, 2016/03/04 17:39:47]

Is this safe? I guess it is since the characteristic owns the descriptor.

[scheib, 2016/03/11 03:17:31]

Acknowledged.

+      base::Bind(
+          &BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionError,
+          base::Unretained(this)));
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::ReadRemoteCharacteristic(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
   if (read_pending_ || write_pending_) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(error_callback,
                               BluetoothGattService::GATT_ERROR_IN_PROGRESS));
     return;
@@ skipping 41 matching lines @@
 }
 
 void BluetoothRemoteGattCharacteristicAndroid::OnChanged(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     const JavaParamRef<jbyteArray>& value) {
   base::android::JavaByteArrayToByteVector(env, value, &value_);
   adapter_->NotifyGattCharacteristicValueChanged(this, value_);
 }
 
+void BluetoothRemoteGattCharacteristicAndroid::OnStartNotifySessionSuccess() {
+  while (!pending_start_notify_calls_.empty()) {

[scheib, 2016/03/02 06:15:20]

Protect against reentrant code by swapping the member vector to a local vector,
then looping over that.

[ortuno, 2016/03/04 17:39:47]

I just realized how easy it is to miss this. Do we have any tests for this?

[scheib, 2016/03/11 03:17:31]

Not yet -- we should. Here's a new issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=593554

+    PendingStartNotifyCall callbacks = pending_start_notify_calls_.front();
+    pending_start_notify_calls_.pop();
+    scoped_ptr<device::BluetoothGattNotifySession> notify_session(
+        new BluetoothGattNotifySessionAndroid(instance_id_));
+    callbacks.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);
+  }
+}
+
 void BluetoothRemoteGattCharacteristicAndroid::OnRead(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     int32_t status,
     const JavaParamRef<jbyteArray>& value) {
   read_pending_ = false;
 
   // Clear callbacks before calling to avoid reentrancy issues.
   ValueCallback read_callback = read_callback_;
   ErrorCallback read_error_callback = read_error_callback_;
@@ skipping 62 matching lines @@
 void BluetoothRemoteGattCharacteristicAndroid::EnsureDescriptorsCreated()
     const {
   if (!descriptors_.empty())
     return;
 
   Java_ChromeBluetoothRemoteGattCharacteristic_createDescriptors(
       AttachCurrentThread(), j_characteristic_.obj());
 }
 
 }  // namespace device