Implement BluetoothGattNotifySession::Stop on Android, 2nd attempt

device/bluetooth/bluetooth_remote_gatt_characteristic.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
index fa96a9b984152d6c57ad23a0b17cd390ce434e7c..16ae1e5953f57e837a99dd944e02e804c6a9e456 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic.cc
@@ -4,17 +4,27 @@
 
 #include "device/bluetooth/bluetooth_remote_gatt_characteristic.h"
 
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "device/bluetooth/bluetooth_gatt_notify_session.h"
 #include "device/bluetooth/bluetooth_remote_gatt_descriptor.h"
 
 namespace device {
 
-BluetoothRemoteGattCharacteristic::BluetoothRemoteGattCharacteristic() {}
+BluetoothRemoteGattCharacteristic::BluetoothRemoteGattCharacteristic()
+    : weak_ptr_factory_(this) {}
 
-BluetoothRemoteGattCharacteristic::~BluetoothRemoteGattCharacteristic() {}
+BluetoothRemoteGattCharacteristic::~BluetoothRemoteGattCharacteristic() {
+  while (!pending_notify_commands_.empty()) {
+    pending_notify_commands_.front()->Cancel();
+  }
+}
 
 std::vector<BluetoothRemoteGattDescriptor*>
 BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID(
-    const BluetoothUUID& uuid) {
+    const BluetoothUUID& uuid) const {
   std::vector<BluetoothRemoteGattDescriptor*> descriptors;
   for (BluetoothRemoteGattDescriptor* descriptor : GetDescriptors()) {
     if (descriptor->GetUUID() == uuid) {
@@ -24,4 +34,290 @@ BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID(
   return descriptors;
 }
 
+base::WeakPtr<device::BluetoothRemoteGattCharacteristic>
+BluetoothRemoteGattCharacteristic::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+bool BluetoothRemoteGattCharacteristic::IsNotifying() const {
+  return !notify_sessions_.empty();
+}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommand::NotifySessionCommand(
+    const base::Closure& execute_callback,
+    const base::Closure& cancel_callback)
+    : execute_callback_(execute_callback), cancel_callback_(cancel_callback) {}
+
+BluetoothRemoteGattCharacteristic::NotifySessionCommand::
+    ~NotifySessionCommand() {}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommand::Execute() {
+  execute_callback_.Run();
+}
+
+void BluetoothRemoteGattCharacteristic::NotifySessionCommand::Cancel() {
+  cancel_callback_.Run();
+}
+
+void BluetoothRemoteGattCharacteristic::StartNotifySession(
+    const NotifySessionCallback& callback,
+    const ErrorCallback& error_callback) {
+  NotifySessionCommand* command = new NotifySessionCommand(
+      base::Bind(&BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession,
+                 base::Unretained(this), callback, error_callback),

[ortuno, 2016/08/09 21:57:56]

Unretained always raises some alarms. Either add a comment explaining why it's
safe to use Unretained or use a weakptr. This applies to all instances of
Unretained. You can add a comment at the top of the file explaining why it's
safe to use Unretained and just reference the comment where necessary.

[tommyt, 2016/08/10 12:07:18]

I've replaced all instances of base::Unretained(this) with GetWeakPtr().

+      base::Bind(&BluetoothRemoteGattCharacteristic::CancelStartNotifySession,
+                 base::Unretained(this),
+                 base::Bind(error_callback,
+                            BluetoothRemoteGattService::GATT_ERROR_FAILED)));
+
+  pending_notify_commands_.push(std::unique_ptr<NotifySessionCommand>(command));
+  if (pending_notify_commands_.size() == 1) {
+    command->Execute();
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::ExecuteStartNotifySession(
+    NotifySessionCallback callback,
+    ErrorCallback error_callback) {
+  // If the command that was resolved immediately before this command was run,
+  // this command should be resolved with the same result.
+  if (previous_command_type_ == NotifySessionCommand::COMMAND_START) {
+    if (previous_command_result_ == NotifySessionCommand::RESULT_SUCCESS) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(
+              &BluetoothRemoteGattCharacteristic::StartNotifySessionSuccess,
+              base::Unretained(this), callback));
+      return;
+    } else {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(
+              &BluetoothRemoteGattCharacteristic::StartNotifySessionError,
+              base::Unretained(this), error_callback,
+              previous_command_error_code_));
+      return;
+    }
+  }
+
+  Properties properties = GetProperties();

[ortuno, 2016/08/09 21:57:56]

optional nit: Would it make sense to put all this code that checks if
notifications are supported in a separate function? Maybe: std::pair<bool,
BluetoothRemoteGattService::GATT_ERROR> SupportsNotifyOrIndicate()?

[tommyt, 2016/08/10 12:07:18]

Personally, I only think there is an advantage to doing this if we need to check
this many places in the code. If that's not the case, I actually prefer to have
all the code in one place, as I think it makes it easier to see what happens.

In the latest patchset I have instead added a comment to each section of this
function, to explain what is done.

+
+  bool hasNotify = (properties & PROPERTY_NOTIFY) != 0;
+  bool hasIndicate = (properties & PROPERTY_INDICATE) != 0;
+
+  if (!hasNotify && !hasIndicate) {
+    LOG(ERROR) << "Characteristic needs NOTIFY or INDICATE";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::StartNotifySessionError,
+                   base::Unretained(this), error_callback,
+                   BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
+    return;
+  }
+
+  if (IsNotifying()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &BluetoothRemoteGattCharacteristic::StartNotifySessionSuccess,
+            base::Unretained(this), callback));
+    return;
+  }
+
+#if defined(OS_CHROMEOS) || defined(OS_LINUX) || defined(OS_MACOSX) || \

[ortuno, 2016/08/09 21:57:56]

Could you add a TODO explaining what needs to be done to remove this #if?
Ideally pointing to an issue for each platform.

[tommyt, 2016/08/10 12:07:18]

Done

+    defined(OS_WIN)
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&BluetoothRemoteGattCharacteristic::StartNotifySessionError,
+                 base::Unretained(this), error_callback,
+                 BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED));
+#else

[ortuno, 2016/08/09 21:57:56]

nit: //  !(defined(OS_CHROMEOS) && defined(OS_LINUX) && defined(OS_MACOSX) &&
defined(OS_WIN))

[tommyt, 2016/08/10 12:07:19]

Done

+  std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =
+      GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
+                               ClientCharacteristicConfigurationUuid());
+
+  if (ccc_descriptor.size() != 1u) {
+    LOG(ERROR) << "Found " << ccc_descriptor.size()
+               << " client characteristic configuration descriptors.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::StartNotifySessionError,
+                   base::Unretained(this), error_callback,
+                   (ccc_descriptor.size() == 0)
+                       ? BluetoothRemoteGattService::GATT_ERROR_NOT_SUPPORTED
+                       : BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  SubscribeToNotifications(
+      ccc_descriptor[0],
+      base::Bind(&BluetoothRemoteGattCharacteristic::StartNotifySessionSuccess,
+                 base::Unretained(this), callback),
+      base::Bind(&BluetoothRemoteGattCharacteristic::StartNotifySessionError,
+                 base::Unretained(this), error_callback));
+#endif

[ortuno, 2016/08/09 21:57:56]

nit: //  defined(OS_CHROMEOS) || defined(OS_LINUX) ...

[tommyt, 2016/08/10 12:07:19]

Done

+}
+
+void BluetoothRemoteGattCharacteristic::CancelStartNotifySession(
+    base::Closure callback) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(pending_notify_commands_.front());
+  pending_notify_commands_.pop();
+  previous_command_type_ = NotifySessionCommand::COMMAND_NONE;
+  callback.Run();
+}
+
+void BluetoothRemoteGattCharacteristic::StartNotifySessionSuccess(
+    NotifySessionCallback callback) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(pending_notify_commands_.front());
+
+  std::unique_ptr<device::BluetoothGattNotifySession> notify_session(
+      new BluetoothGattNotifySession(weak_ptr_factory_.GetWeakPtr()));
+  notify_sessions_.insert(notify_session.get());
+  callback.Run(std::move(notify_session));
+
+  pending_notify_commands_.pop();
+  if (!pending_notify_commands_.empty()) {
+    previous_command_type_ = NotifySessionCommand::COMMAND_START;
+    previous_command_result_ = NotifySessionCommand::RESULT_SUCCESS;
+    pending_notify_commands_.front()->Execute();
+  } else {
+    previous_command_type_ = NotifySessionCommand::COMMAND_NONE;
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::StartNotifySessionError(
+    ErrorCallback error_callback,
+    BluetoothRemoteGattService::GattErrorCode error) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(pending_notify_commands_.front());
+
+  error_callback.Run(error);
+
+  pending_notify_commands_.pop();
+  if (!pending_notify_commands_.empty()) {
+    previous_command_type_ = NotifySessionCommand::COMMAND_START;

[ortuno, 2016/08/09 21:57:56]

optional nit: Would passing these directly to Execute be cleaner?

[tommyt, 2016/08/10 12:07:18]

Maybe. I've done this in my latest patchset, and I think I like this a bit
better.

+    previous_command_result_ = NotifySessionCommand::RESULT_ERROR;
+    previous_command_error_code_ = error;
+    pending_notify_commands_.front()->Execute();
+  } else {
+    previous_command_type_ = NotifySessionCommand::COMMAND_NONE;
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::StopNotifySession(
+    BluetoothGattNotifySession* session,
+    const base::Closure& callback) {
+  NotifySessionCommand* command = new NotifySessionCommand(
+      base::Bind(&BluetoothRemoteGattCharacteristic::ExecuteStopNotifySession,
+                 base::Unretained(this), session, callback),
+      callback);

[ortuno, 2016/08/09 21:57:56]

nit: callback /* cancel_callback */);

[tommyt, 2016/08/10 12:07:18]

Done.

+
+  pending_notify_commands_.push(std::unique_ptr<NotifySessionCommand>(command));
+  if (pending_notify_commands_.size() == 1) {
+    command->Execute();
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::ExecuteStopNotifySession(
+    BluetoothGattNotifySession* session,

[ortuno, 2016/08/09 21:57:56]

It makes me nervous that we are passing a pointer that could have been deleted.
Do you think it would make sense to keep a count of the sessions rather than a
map? To know if the session belongs in the characteristic you could pass the
session's characteristic identifier.

[tommyt, 2016/08/10 12:07:18]

This session pointer is not dereferenced anywhere, it's just used as a key in
the notify_sessions_ set. Passing the characteristic identifier is not enough,
since we need to be able to remove the pointer from the notify_sessions_ set
when we're done.

The current code should be safe, seeing that we're not dereferencing these
pointers, but I do agree that keeping these raw pointers around is a bit like
leaving a loaded gun under the pillow. For now, I've added the unittest
StopNotifySession_SessionDeleted2 to test that deleting the sessions with
outstanding stop callbacks does not cause problems.

If we want to make this safer, then we may have to start using weak pointers to
the sessions. Some sort of a session ID that would uniquely identify a session
would probably also work.

+    base::Closure callback) {
+  std::set<BluetoothGattNotifySession*>::iterator session_iterator =
+      notify_sessions_.find(session);
+
+  // If the session does not even belong to this characteristic, we return an
+  // error right away.
+  if (session_iterator == notify_sessions_.end()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::StopNotifySessionError,
+                   base::Unretained(this), session, callback,
+                   BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  // If there are more active sessions, then we return right away.
+  if (notify_sessions_.size() > 1) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::StopNotifySessionSuccess,
+                   base::Unretained(this), session, callback));
+    return;
+  }
+
+  std::vector<BluetoothRemoteGattDescriptor*> ccc_descriptor =

[ortuno, 2016/08/09 21:57:56]

Do you need an "#if defined..." here?

[tommyt, 2016/08/10 12:07:19]

Done.

+      GetDescriptorsByUUID(BluetoothRemoteGattDescriptor::
+                               ClientCharacteristicConfigurationUuid());
+
+  if (ccc_descriptor.size() != 1u) {
+    LOG(ERROR) << "Found " << ccc_descriptor.size()
+               << " client characteristic configuration descriptors.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&BluetoothRemoteGattCharacteristic::StopNotifySessionError,
+                   base::Unretained(this), session, callback,
+                   BluetoothRemoteGattService::GATT_ERROR_FAILED));
+    return;
+  }
+
+  UnsubscribeFromNotifications(
+      ccc_descriptor[0],
+      base::Bind(&BluetoothRemoteGattCharacteristic::StopNotifySessionSuccess,
+                 base::Unretained(this), session, callback),
+      base::Bind(&BluetoothRemoteGattCharacteristic::StopNotifySessionError,
+                 base::Unretained(this), session, callback));
+}
+
+void BluetoothRemoteGattCharacteristic::CancelStopNotifySession(
+    base::Closure callback) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(pending_notify_commands_.front());
+  pending_notify_commands_.pop();
+  previous_command_type_ = NotifySessionCommand::COMMAND_NONE;
+  callback.Run();
+}
+
+void BluetoothRemoteGattCharacteristic::StopNotifySessionSuccess(
+    BluetoothGattNotifySession* session,
+    base::Closure callback) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(pending_notify_commands_.front());
+
+  notify_sessions_.erase(session);
+
+  callback.Run();
+
+  pending_notify_commands_.pop();
+  if (!pending_notify_commands_.empty()) {
+    previous_command_type_ = NotifySessionCommand::COMMAND_STOP;
+    previous_command_result_ = NotifySessionCommand::RESULT_SUCCESS;
+    pending_notify_commands_.front()->Execute();
+  } else {
+    previous_command_type_ = NotifySessionCommand::COMMAND_NONE;
+  }
+}
+
+void BluetoothRemoteGattCharacteristic::StopNotifySessionError(
+    BluetoothGattNotifySession* session,
+    base::Closure callback,
+    BluetoothRemoteGattService::GattErrorCode error) {
+  std::unique_ptr<NotifySessionCommand> command =
+      std::move(pending_notify_commands_.front());
+
+  notify_sessions_.erase(session);
+
+  callback.Run();
+
+  pending_notify_commands_.pop();
+  if (!pending_notify_commands_.empty()) {
+    previous_command_type_ = NotifySessionCommand::COMMAND_STOP;
+    previous_command_result_ = NotifySessionCommand::RESULT_ERROR;
+    previous_command_error_code_ = error;
+    pending_notify_commands_.front()->Execute();
+  } else {
+    previous_command_type_ = NotifySessionCommand::COMMAND_NONE;
+  }
+}
+
 }  // namespace device