Bluetooth: macOS: Implementing read/write for descriptors

device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm

Index: device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
diff --git a/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm b/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
index 76cda0c349c19726f6917e40af48db3fd210a4a2..9983ade03b54c960e8b152bb18602cc179a6db38 100644
--- a/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
+++ b/device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm
@@ -4,17 +4,52 @@
 
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor_mac.h"
 
+#include "base/bind.h"
+#include "base/mac/foundation_util.h"

[tapted, 2017/04/05 00:38:37]

nit: #import

[jlebel, 2017/04/06 17:48:49]

Done.

 #include "base/strings/sys_string_conversions.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "device/bluetooth/bluetooth_adapter_mac.h"

[tapted, 2017/04/05 00:38:37]

#import

[jlebel, 2017/04/06 17:48:49]

Done.

 #include "device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h"

[tapted, 2017/04/05 00:38:37]

#import as well 

[jlebel, 2017/04/06 17:48:49]

Done.

 
+using base::mac::ObjCCast;
+
 namespace device {
 
+std::vector<uint8_t> VectorValueFromObjC(id objcValue) {

[tapted, 2017/04/05 00:38:37]

objcValue -> objc_value (or just value) - we only declare new things camelCase
between @foo/@end

[jlebel, 2017/04/06 17:48:49]

Done.

+  // According to
+  // https://developer.apple.com/reference/corebluetooth/cbdescriptor some
+  // descriptor values can be NSData, NSString or NSNumber.
+  std::vector<uint8_t> value;
+  if ([objcValue isKindOfClass:[NSString class]]) {
+    NSString* string = ObjCCast<NSString>(objcValue);

[tapted, 2017/04/05 00:38:37]

ObjCCast also does isKindOfClass, so there's a bit or repetition here. The
malloc and other stuff is all a bit scary. Really I think The neatest and safest
way to do this is to convert all types to NSData. I'd even suggest converting
numbers to strings first to remove questions around byte order and things like
that and just keep things simpler, but maybe that will break stuff.

NSData data = ObjCCast<NSData>(objc_value);
NSString* as_string = ObjCCast<NSString>(objc_value);
NSNumber* as_number = ObjCCast<NSNumber>(objc_value);

if (!data && !as_string && as_number) {
  // Simpler, but might not work:  as_string = [as_number stringValue];
  long descriptor = [as_number longValue];
  data = [NSData dataWithBytes:&descriptor length:sizeof(descriptor)];
}

if (!data && as_string)
  data = [as_string dataUsingEncoding:NSUTF8StringEncoding];

DCHECK(data) << "Unexpected .."
if (data) {
  value.resize([data length]);
  [data getBytes:value.data() length:value.size()];
}
return value;


The NS/CFNumber APIs are free to change data sizes when they're initialized, so
I'd verify that you really need stuff like CFNumberGetByteSize before using it
-- it's probably not useful.

[jlebel, 2017/04/06 17:48:49]

about the first "if":
I don't understand why we need to test data, as_string and as_number. I can't
imagine to have data and as_number set.

about DCHECK followed by "if (data)"
I've been told I should not test a value after a DCHECK(): either the DCHECK can
fail, so I need to remove the DCHECK(), or the DCHECK should never fails, so
there is no need to test right after.

I think it is really important to use CFNumberGetByteSize(). It would not make
sense for the developer to receive 8 bytes when the device sends only 2 bytes.

[tapted, 2017/04/07 04:40:43]

Yep - it would be an error if more than one is set. This was just about
organising the logic nicely.
Dealing with external APIs gives some leeway on this IMO. The code currently
captures all the documented possibilities, but we don't have a guarantee that
Apple introduce a new data type in the future. We want to detect that that
should be handled, but we don't want to introduce a security bug or crash users
machines.
See later comment.

+    const uint8_t* buffer = (const uint8_t*)(string.UTF8String);

[tapted, 2017/04/05 00:38:37]

note style guide prohibits C-style casts (use reinterpret_cast instead)

[jlebel, 2017/04/06 17:48:49]

Done.

+    value.assign(buffer, buffer + string.length);
+  } else if ([objcValue isKindOfClass:[NSNumber class]]) {
+    CFNumberRef number = base::mac::CFCast<CFNumberRef>(objcValue);
+    CFIndex byteSize = CFNumberGetByteSize(number);
+    CFNumberType type = CFNumberGetType(number);
+    uint8_t* buffer = (uint8_t*)malloc(byteSize);

[tapted, 2017/04/05 00:38:37]

memory leak? - nothing frees |buffer|

[jlebel, 2017/04/06 17:48:49]

Done.

+    CFNumberGetValue(number, type, buffer);
+    value.assign(buffer, buffer + byteSize);
+  } else if ([objcValue isKindOfClass:[NSData class]]) {
+    DCHECK([objcValue isKindOfClass:[NSData class]]);
+    NSData* data = ObjCCast<NSData>(objcValue);
+    const uint8_t* buffer = (const uint8_t*)(data.bytes);

[tapted, 2017/04/05 00:38:37]

data.bytes may return nil - this is really dangerous code.

[jlebel, 2017/04/06 17:48:49]

According to the documentation, -[NSData bytes] can return nil when there is no
data. So -[NSData length] should return 0. I think therefore there should be no
crash, since no bytes should be read from nil. Am I wrong?

[tapted, 2017/04/07 04:40:43]

Hm - maybe you're safe with C++ vectors. It scares me still though - passing
null to memcpy is undefined behaviour, even if the length is zero.

+    value.assign(buffer, buffer + data.length);
+  } else {
+    VLOG(1) << "Unexpected value: "
+            << base::SysNSStringToUTF8([objcValue description]);
+    NOTREACHED();
+  }
+  return value;
+}
+
 BluetoothRemoteGattDescriptorMac::BluetoothRemoteGattDescriptorMac(
     BluetoothRemoteGattCharacteristicMac* characteristic,
     CBDescriptor* descriptor)
     : gatt_characteristic_(characteristic),
-      cb_descriptor_(descriptor, base::scoped_policy::RETAIN) {
+      cb_descriptor_(descriptor, base::scoped_policy::RETAIN),
+      value_read_or_write_in_progress_(false) {
   uuid_ =
       BluetoothAdapterMac::BluetoothUUIDWithCBUUID([cb_descriptor_.get() UUID]);
   identifier_ = base::SysNSStringToUTF8(
@@ -40,7 +75,18 @@ const std::vector<uint8_t>& BluetoothRemoteGattDescriptorMac::GetValue() const {
   return value_;
 }
 
-BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {}
+BluetoothRemoteGattDescriptorMac::~BluetoothRemoteGattDescriptorMac() {
+  if (!read_value_callbacks_.first.is_null()) {

[ortuno, 2017/04/04 23:39:43]

We need tests like [1] to test that this is getting called. Branch point is
close so no need to it in this patch.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

[jlebel, 2017/04/06 17:48:49]

crbug.com/709066

+    std::pair<ValueCallback, ErrorCallback> callbacks;
+    callbacks.swap(read_value_callbacks_);
+    callbacks.second.Run(BluetoothGattService::GATT_ERROR_FAILED);
+  }
+  if (!write_value_callbacks_.first.is_null()) {
+    std::pair<base::Closure, ErrorCallback> callbacks;
+    callbacks.swap(write_value_callbacks_);
+    callbacks.second.Run(BluetoothGattService::GATT_ERROR_FAILED);
+  }
+}
 
 // Returns a pointer to the GATT characteristic that this characteristic
 // descriptor belongs to.
@@ -55,19 +101,89 @@ BluetoothRemoteGattDescriptorMac::GetCharacteristic() const {
 void BluetoothRemoteGattDescriptorMac::ReadRemoteDescriptor(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
-  NOTIMPLEMENTED();
+  if (value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Read failed, already in progress.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
+    return;
+  }
+  VLOG(1) << *this << ": Read value.";
+  value_read_or_write_in_progress_ = true;
+  read_value_callbacks_ = std::make_pair(callback, error_callback);
+  [GetCBPeripheral() readValueForDescriptor:cb_descriptor_];
 }
 
-// Sends a write request to a remote characteristic descriptor, to modify the
-// value of the descriptor with the new value |new_value|. |callback| is
-// called to signal success and |error_callback| for failures. This method
-// only applies to remote descriptors and will fail for those that are locally
-// hosted.
+// Sends a write request to a remote characteristic descriptor. |callback| is

[ortuno, 2017/04/04 23:39:43]

I think its weird to have these comments in the .mm file of a subclass. There is
one already in the base class' .h file. Just remove it.

[jlebel, 2017/04/06 17:48:49]

Done.

+// called to signal success or |error_callback| for failures.
 void BluetoothRemoteGattDescriptorMac::WriteRemoteDescriptor(
-    const std::vector<uint8_t>& new_value,
+    const std::vector<uint8_t>& value,
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
-  NOTIMPLEMENTED();
+  if (value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Write failed, already in progress.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
+    return;
+  }
+  VLOG(1) << *this << ": Write value.";
+  value_read_or_write_in_progress_ = true;
+  write_value_callbacks_ = std::make_pair(callback, error_callback);
+  base::scoped_nsobject<NSData> nsdata_value(
+      [[NSData alloc] initWithBytes:value.data() length:value.size()]);
+  [GetCBPeripheral() writeValue:nsdata_value forDescriptor:GetCBDescriptor()];
+}
+
+void BluetoothRemoteGattDescriptorMac::DidUpdateValueForDescriptor(
+    NSError* error) {
+  if (!value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Value updated, no read in progress.";
+    return;
+  }
+  std::pair<ValueCallback, ErrorCallback> callbacks;
+  callbacks.swap(read_value_callbacks_);
+  value_read_or_write_in_progress_ = false;
+  if (error) {
+    BluetoothGattService::GattErrorCode error_code =
+        BluetoothDeviceMac::GetGattErrorCodeFromNSError(error);
+    VLOG(1) << *this << ": Read value failed with error: "
+            << BluetoothAdapterMac::String(error)
+            << ", converted to error code: " << error_code;
+    callbacks.second.Run(error_code);
+    return;
+  }
+  VLOG(1) << *this << ": Value read.";
+  value_ = VectorValueFromObjC(cb_descriptor_.get().value);

[tapted, 2017/04/05 00:38:37]

we don't use ObjC dot notation for NSObjects in Chrome (only for structs like
NSSize). Instead of `cb_descriptor_.get().value` it should be `[cb_descriptor_
value]` so it's clear this is an ObjC message send.

(all the other places in this file that call ".get()" on scoped_nsobject should
be updated too)

[jlebel, 2017/04/06 17:48:49]

Done.

For the code I wrote in other files: crbug.com/709104

+  callbacks.first.Run(value_);
+}
+
+void BluetoothRemoteGattDescriptorMac::DidWriteValueForDescriptor(
+    NSError* error) {
+  if (!value_read_or_write_in_progress_) {
+    VLOG(1) << *this << ": Value written, no write in progress.";
+    return;
+  }
+  std::pair<base::Closure, ErrorCallback> callbacks;
+  callbacks.swap(write_value_callbacks_);
+  value_read_or_write_in_progress_ = false;
+  if (error) {
+    BluetoothGattService::GattErrorCode error_code =
+        BluetoothDeviceMac::GetGattErrorCodeFromNSError(error);
+    VLOG(1) << *this << ": Write value failed with error: "
+            << BluetoothAdapterMac::String(error)
+            << ", converted to error code: " << error_code;
+    callbacks.second.Run(error_code);
+    return;
+  }
+  VLOG(1) << *this << ": Value written.";
+  callbacks.first.Run();
+}
+
+CBPeripheral* BluetoothRemoteGattDescriptorMac::GetCBPeripheral() const {
+  return gatt_characteristic_->GetCBPeripheral();
 }
 
 CBDescriptor* BluetoothRemoteGattDescriptorMac::GetCBDescriptor() const {