[CrOS MultiDevice] Fix crash when cancelling connection attempt.

The crash was caused by attempting to cancel and/or update the priority
of a connection attempt which had not yet started. Starting a
connection is posted in an asynchronous task, so it is possible that
the connection attempt is canceled before it has been attempted. In
this case, we were erroneously assuming that the connection attempt
metadata had already been set when it actually had not.

This CL fixes this race condition by explicitly checking if the
connection has been attempted before performing any operation which
touches connection metadata.

Bug: 888073
Change-Id: I183202863f8e018b5dfdf77e1b208843217689e7
Reviewed-on: https://chromium-review.googlesource.com/c/1338322Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608517}

chromeos/services/secure_channel/ble_initiator_operation.cc

@@ -71,7 +71,7 @@ BleInitiatorOperation::BleInitiatorOperation(
 
 BleInitiatorOperation::~BleInitiatorOperation() = default;
 
-void BleInitiatorOperation::AttemptConnectionToDevice(
+void BleInitiatorOperation::PerformAttemptConnectionToDevice(
     ConnectionPriority connection_priority) {
   is_attempt_active_ = true;
   ble_connection_manager_->AttemptBleInitiatorConnection(

chromeos/services/secure_channel/ble_initiator_operation.h

@@ -59,7 +59,7 @@ class BleInitiatorOperation
       scoped_refptr<base::TaskRunner> task_runner);
 
   // ConnectToDeviceOperationBase<BleInitiatorFailureType>:
-  void AttemptConnectionToDevice(
+  void PerformAttemptConnectionToDevice(
       ConnectionPriority connection_priority) override;
   void PerformCancellation() override;
   void PerformUpdateConnectionPriority(

chromeos/services/secure_channel/ble_listener_operation.cc

@@ -72,7 +72,7 @@ BleListenerOperation::BleListenerOperation(
 
 BleListenerOperation::~BleListenerOperation() = default;
 
-void BleListenerOperation::AttemptConnectionToDevice(
+void BleListenerOperation::PerformAttemptConnectionToDevice(
     ConnectionPriority connection_priority) {
   is_attempt_active_ = true;
   ble_connection_manager_->AttemptBleListenerConnection(

chromeos/services/secure_channel/ble_listener_operation.h

@@ -60,7 +60,7 @@ class BleListenerOperation
       scoped_refptr<base::TaskRunner> task_runner);
 
   // ConnectToDeviceOperationBase<BleListenerFailureType>:
-  void AttemptConnectionToDevice(
+  void PerformAttemptConnectionToDevice(
       ConnectionPriority connection_priority) override;
   void PerformCancellation() override;
   void PerformUpdateConnectionPriority(

chromeos/services/secure_channel/connect_to_device_operation.h

@@ -49,7 +49,7 @@ class ConnectToDeviceOperation {
     }
 
     connection_priority_ = connection_priority;
-    PerformUpdateConnectionPriority(connection_priority);
+    UpdateConnectionPriorityInternal(connection_priority);
   }
 
   // Note: Canceling an ongoing connection attempt will not cause either of the
@@ -63,7 +63,7 @@ class ConnectToDeviceOperation {
     }
 
     has_finished_ = true;
-    PerformCancellation();
+    CancelInternal();
   }
 
   ConnectionPriority connection_priority() const {
@@ -78,9 +78,11 @@ class ConnectToDeviceOperation {
         failure_callback_(failure_callback),
         connection_priority_(connection_priority) {}
 
-  virtual void PerformCancellation() = 0;
-  virtual void PerformUpdateConnectionPriority(
-      ConnectionPriority connection_priority) = 0;
+  // Derived types can override these functions to implement cancellation and
+  // updating of priority.
+  virtual void CancelInternal() {}
+  virtual void UpdateConnectionPriorityInternal(
+      ConnectionPriority connection_priority) {}
 
   void OnSuccessfulConnectionAttempt(
       std::unique_ptr<AuthenticatedChannel> authenticated_channel) {

chromeos/services/secure_channel/connect_to_device_operation_base.h

@@ -54,7 +54,26 @@ class ConnectToDeviceOperationBase
 
   ~ConnectToDeviceOperationBase() override = default;
 
-  virtual void AttemptConnectionToDevice(
+  void AttemptConnectionToDevice(ConnectionPriority connection_priority) {
+    has_started_connection_attempt_ = true;
+    PerformAttemptConnectionToDevice(connection_priority);
+  }
+
+  void CancelInternal() override {
+    if (has_started_connection_attempt_)
+      PerformCancellation();
+  }
+
+  void UpdateConnectionPriorityInternal(
+      ConnectionPriority connection_priority) override {
+    if (has_started_connection_attempt_)
+      PerformUpdateConnectionPriority(connection_priority);
+  }
+
+  virtual void PerformAttemptConnectionToDevice(
+      ConnectionPriority connection_priority) = 0;
+  virtual void PerformCancellation() = 0;
+  virtual void PerformUpdateConnectionPriority(
       ConnectionPriority connection_priority) = 0;
 
   const DeviceIdPair& device_id_pair() const { return device_id_pair_; }
@@ -62,6 +81,7 @@ class ConnectToDeviceOperationBase
  private:
   const DeviceIdPair& device_id_pair_;
   scoped_refptr<base::TaskRunner> task_runner_;
+  bool has_started_connection_attempt_ = false;
   base::WeakPtrFactory<ConnectToDeviceOperationBase> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ConnectToDeviceOperationBase);

chromeos/services/secure_channel/connect_to_device_operation_base_unittest.cc

@@ -49,7 +49,7 @@ class TestConnectToDeviceOperation
   bool has_canceled_connection() const { return has_canceled_connection_; }
 
   // ConnectToDeviceOperationBase<std::string>:
-  void AttemptConnectionToDevice(
+  void PerformAttemptConnectionToDevice(
       ConnectionPriority connection_priority) override {
     has_attempted_connection_ = true;
   }

chromeos/services/secure_channel/fake_connect_to_device_operation.h

@@ -53,14 +53,14 @@ class FakeConnectToDeviceOperation
 
  private:
   // ConnectToDeviceOperation<FailureDetailType>:
-  void PerformCancellation() override {
+  void CancelInternal() override {
     canceled_ = true;
 
     if (cancel_callback_)
       std::move(cancel_callback_).Run();
   }
 
-  void PerformUpdateConnectionPriority(
+  void UpdateConnectionPriorityInternal(
       ConnectionPriority connection_priority) override {
     updated_priority_ = connection_priority;
   }