[CrOS MultiDevice] Clean up ConnectToDeviceOperationBase and add tests.

This CL expands upon [1], which was a quick fix to fix a crash in the
M-71 branch. This follow-up CL makes the class more robust by ensuring
that if connection priority is updated before a connection attempt is
made, the connection will be made with the updated priority when it is
performed.

Additionally, this CL adds a test for this functionality.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1338322

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

chromeos/services/secure_channel/connect_to_device_operation_base.h

@@ -7,6 +7,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/services/secure_channel/connect_to_device_operation.h"
 #include "chromeos/services/secure_channel/device_id_pair.h"
@@ -41,6 +42,7 @@ class ConnectToDeviceOperationBase
                                                     connection_priority),
         device_id_pair_(device_id_pair),
         task_runner_(task_runner),
+        pending_connection_attempt_priority_(connection_priority),
         weak_ptr_factory_(this) {
     // Attempt a connection; however, post this as a task to be run after the
     // constructor is finished. This ensures that the derived type is fully
@@ -49,25 +51,48 @@ class ConnectToDeviceOperationBase
         FROM_HERE,
         base::BindOnce(&ConnectToDeviceOperationBase<
                            FailureDetailType>::AttemptConnectionToDevice,
-                       weak_ptr_factory_.GetWeakPtr(), connection_priority));
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
   ~ConnectToDeviceOperationBase() override = default;
 
-  void AttemptConnectionToDevice(ConnectionPriority connection_priority) {
-    has_started_connection_attempt_ = true;
-    PerformAttemptConnectionToDevice(connection_priority);
+  void AttemptConnectionToDevice() {
+    // If there is no longer a pending connection attempt, this means that the
+    // attempt was canceled before it could be started.
+    if (!pending_connection_attempt_priority_)
+      return;
+
+    // Get the current priority, then reset the pending request.
+    ConnectionPriority priority = *pending_connection_attempt_priority_;
+    pending_connection_attempt_priority_.reset();
+
+    PerformAttemptConnectionToDevice(priority);
   }
 
   void CancelInternal() override {
-    if (has_started_connection_attempt_)
-      PerformCancellation();
+    // If the attempt is still pending, it has been canceled before it even
+    // was started. In this case, invalidate the pending request to ensure
+    // that it never ends up being attempted.
+    if (pending_connection_attempt_priority_) {
+      pending_connection_attempt_priority_.reset();
+      return;
+    }
+
+    // Otherwise, the attempt is already active, so cancel it directly.
+    PerformCancellation();
   }
 
   void UpdateConnectionPriorityInternal(
       ConnectionPriority connection_priority) override {
-    if (has_started_connection_attempt_)
-      PerformUpdateConnectionPriority(connection_priority);
+    // If the attempt is still pending, update the connection priorty so that
+    // when the attempt is started, the correct priority will be provided.
+    if (pending_connection_attempt_priority_) {
+      pending_connection_attempt_priority_ = connection_priority;
+      return;
+    }
+
+    // Otherwise, the attempt is already active, so update its priority.
+    PerformUpdateConnectionPriority(connection_priority);
   }
 
   virtual void PerformAttemptConnectionToDevice(
@@ -81,7 +106,7 @@ class ConnectToDeviceOperationBase
  private:
   const DeviceIdPair& device_id_pair_;
   scoped_refptr<base::TaskRunner> task_runner_;
-  bool has_started_connection_attempt_ = false;
+  base::Optional<ConnectionPriority> pending_connection_attempt_priority_;
   base::WeakPtrFactory<ConnectToDeviceOperationBase> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ConnectToDeviceOperationBase);

chromeos/services/secure_channel/connect_to_device_operation_base_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/test/test_simple_task_runner.h"
 #include "chromeos/services/secure_channel/device_id_pair.h"
@@ -34,30 +35,45 @@ class TestConnectToDeviceOperation
       const ConnectToDeviceOperation<std::string>::ConnectionFailedCallback&
           failure_callback,
       const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority) {
+      ConnectionPriority connection_priority,
+      bool should_attempt_connection_synchronously) {
     auto test_task_runner = base::MakeRefCounted<base::TestSimpleTaskRunner>();
     auto operation = base::WrapUnique(new TestConnectToDeviceOperation(
         std::move(success_callback), std::move(failure_callback),
         device_id_pair, connection_priority, test_task_runner));
-    test_task_runner->RunUntilIdle();
+
+    if (should_attempt_connection_synchronously)
+      test_task_runner->RunUntilIdle();
+
     return operation;
   }
 
   ~TestConnectToDeviceOperation() override = default;
 
+  void RunPendingTasks() { test_task_runner_->RunUntilIdle(); }
+
   bool has_attempted_connection() const { return has_attempted_connection_; }
   bool has_canceled_connection() const { return has_canceled_connection_; }
+  const base::Optional<ConnectionPriority>& current_connection_priority() {
+    return current_connection_priority_;
+  }
 
   // ConnectToDeviceOperationBase<std::string>:
   void PerformAttemptConnectionToDevice(
       ConnectionPriority connection_priority) override {
     has_attempted_connection_ = true;
+    current_connection_priority_ = connection_priority;
   }
 
-  void PerformCancellation() override { has_canceled_connection_ = true; }
+  void PerformCancellation() override {
+    has_canceled_connection_ = true;
+    current_connection_priority_.reset();
+  }
 
   void PerformUpdateConnectionPriority(
-      ConnectionPriority connection_priority) override {}
+      ConnectionPriority connection_priority) override {
+    current_connection_priority_ = connection_priority;
+  }
 
   // Make On{Successful|Failed}ConnectionAttempt() public for testing.
   using ConnectToDeviceOperation<std::string>::OnSuccessfulConnectionAttempt;
@@ -76,10 +92,13 @@ class TestConnectToDeviceOperation
                                                   std::move(failure_callback),
                                                   device_id_pair,
                                                   connection_priority,
-                                                  test_task_runner) {}
+                                                  test_task_runner),
+        test_task_runner_(test_task_runner) {}
 
+  scoped_refptr<base::TestSimpleTaskRunner> test_task_runner_;
   bool has_attempted_connection_ = false;
   bool has_canceled_connection_ = false;
+  base::Optional<ConnectionPriority> current_connection_priority_;
 };
 
 }  // namespace
@@ -91,7 +110,8 @@ class SecureChannelConnectToDeviceOperationBaseTest : public testing::Test {
 
   ~SecureChannelConnectToDeviceOperationBaseTest() override = default;
 
-  void CreateOperation(ConnectionPriority connection_priority) {
+  void CreateOperation(ConnectionPriority connection_priority,
+                       bool should_attempt_connection_synchronously = true) {
     test_operation_ = TestConnectToDeviceOperation::Create(
         base::BindOnce(&SecureChannelConnectToDeviceOperationBaseTest::
                            OnSuccessfulConnectionAttempt,
@@ -99,8 +119,10 @@ class SecureChannelConnectToDeviceOperationBaseTest : public testing::Test {
         base::BindRepeating(&SecureChannelConnectToDeviceOperationBaseTest::
                                 OnFailedConnectionAttempt,
                             base::Unretained(this)),
-        test_device_id_pair_, connection_priority);
-    EXPECT_TRUE(test_operation_->has_attempted_connection());
+        test_device_id_pair_, connection_priority,
+        should_attempt_connection_synchronously);
+    EXPECT_EQ(should_attempt_connection_synchronously,
+              test_operation_->has_attempted_connection());
   }
 
   TestConnectToDeviceOperation* test_operation() {
@@ -174,6 +196,39 @@ TEST_F(SecureChannelConnectToDeviceOperationBaseTest, Cancelation) {
   EXPECT_TRUE(test_operation()->has_canceled_connection());
 }
 
+TEST_F(SecureChannelConnectToDeviceOperationBaseTest,
+       UpdateConnectionPriorityBeforeAttemptStarted) {
+  CreateOperation(ConnectionPriority::kLow,
+                  false /* should_attempt_connection_synchronously */);
+
+  // Update the connection priority, then run pending tasks; the pending task
+  // should start the attempt.
+  test_operation()->UpdateConnectionPriority(ConnectionPriority::kMedium);
+  EXPECT_FALSE(test_operation()->has_attempted_connection());
+  test_operation()->RunPendingTasks();
+  EXPECT_TRUE(test_operation()->has_attempted_connection());
+  EXPECT_EQ(ConnectionPriority::kMedium,
+            test_operation()->current_connection_priority());
+
+  test_operation()->Cancel();
+}
+
+TEST_F(SecureChannelConnectToDeviceOperationBaseTest,
+       CancelBeforeAttemptStarted) {
+  CreateOperation(ConnectionPriority::kLow,
+                  false /* should_attempt_connection_synchronously */);
+
+  // Update the connection priority, cancel the attempt, then run pending tasks;
+  // the pending task should try to start the attempt but should fail since the
+  // task had already been canceled.
+  test_operation()->UpdateConnectionPriority(ConnectionPriority::kMedium);
+  EXPECT_FALSE(test_operation()->has_attempted_connection());
+  test_operation()->Cancel();
+  EXPECT_FALSE(test_operation()->has_attempted_connection());
+  test_operation()->RunPendingTasks();
+  EXPECT_FALSE(test_operation()->has_attempted_connection());
+}
+
 }  // namespace secure_channel
 
 }  // namespace chromeos