[CrOS MultiDevice] Update ConnectionAttempt.

Now, ConnectionAttempt uses a single ConnectToDeviceOperation throughout
its lifetime, which drastically cleans up the code and removes the
ConnectToDeviceOperationFactory, which is not really needed.

This CL removes ConnectToDeviceOperationFactory entirely.

Bug: 824568, 752273
Change-Id: Iaf8182da258e864b1377aa94cefa1ed9440e8bd1
Reviewed-on: https://chromium-review.googlesource.com/1103337
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568181}

chromeos/services/secure_channel/BUILD.gn

@@ -51,8 +51,6 @@ static_library("secure_channel") {
     "client_connection_parameters_impl.h",
     "connect_to_device_operation.h",
     "connect_to_device_operation_base.h",
-    "connect_to_device_operation_factory.h",
-    "connect_to_device_operation_factory_base.h",
     "connection_attempt.h",
     "connection_attempt_base.h",
     "connection_attempt_delegate.h",
@@ -146,7 +144,6 @@ static_library("test_support") {
     "fake_client_connection_parameters.cc",
     "fake_client_connection_parameters.h",
     "fake_connect_to_device_operation.h",
-    "fake_connect_to_device_operation_factory.h",
     "fake_connection_attempt.h",
     "fake_connection_attempt_delegate.cc",
     "fake_connection_attempt_delegate.h",
@@ -201,7 +198,6 @@ source_set("unit_tests") {
     "ble_synchronizer_unittest.cc",
     "client_connection_parameters_impl_unittest.cc",
     "connect_to_device_operation_base_unittest.cc",
-    "connect_to_device_operation_factory_base_unittest.cc",
     "connection_attempt_base_unittest.cc",
     "error_tolerant_ble_advertisement_impl_unittest.cc",
     "multiplexed_channel_impl_unittest.cc",

chromeos/services/secure_channel/ble_initiator_operation.cc

@@ -37,15 +37,14 @@ std::unique_ptr<ConnectToDeviceOperation<BleInitiatorFailureType>>
 BleInitiatorOperation::Factory::BuildInstance(
     ConnectToDeviceOperation<BleInitiatorFailureType>::ConnectionSuccessCallback
         success_callback,
-    ConnectToDeviceOperation<BleInitiatorFailureType>::ConnectionFailedCallback
-        failure_callback,
+    const ConnectToDeviceOperation<
+        BleInitiatorFailureType>::ConnectionFailedCallback& failure_callback,
     const DeviceIdPair& device_id_pair,
     ConnectionPriority connection_priority,
-    base::OnceClosure destructor_callback,
     scoped_refptr<base::TaskRunner> task_runner) {
   return base::WrapUnique(new BleInitiatorOperation(
       std::move(success_callback), std::move(failure_callback), device_id_pair,
-      connection_priority, std::move(destructor_callback), task_runner));
+      connection_priority, task_runner));
 }
 
 BleInitiatorOperation::~BleInitiatorOperation() = default;
@@ -53,18 +52,16 @@ BleInitiatorOperation::~BleInitiatorOperation() = default;
 BleInitiatorOperation::BleInitiatorOperation(
     ConnectToDeviceOperation<BleInitiatorFailureType>::ConnectionSuccessCallback
         success_callback,
-    ConnectToDeviceOperation<BleInitiatorFailureType>::ConnectionFailedCallback
-        failure_callback,
+    const ConnectToDeviceOperation<
+        BleInitiatorFailureType>::ConnectionFailedCallback& failure_callback,
     const DeviceIdPair& device_id_pair,
     ConnectionPriority connection_priority,
-    base::OnceClosure destructor_callback,
     scoped_refptr<base::TaskRunner> task_runner)
     : ConnectToDeviceOperationBase<BleInitiatorFailureType>(
           std::move(success_callback),
           std::move(failure_callback),
           device_id_pair,
           connection_priority,
-          std::move(destructor_callback),
           task_runner) {}
 
 void BleInitiatorOperation::AttemptConnectionToDevice(

chromeos/services/secure_channel/ble_initiator_operation.h

@@ -30,11 +30,10 @@ class BleInitiatorOperation
     virtual std::unique_ptr<ConnectToDeviceOperation<BleInitiatorFailureType>>
     BuildInstance(ConnectToDeviceOperation<BleInitiatorFailureType>::
                       ConnectionSuccessCallback success_callback,
-                  ConnectToDeviceOperation<BleInitiatorFailureType>::
-                      ConnectionFailedCallback failure_callback,
+                  const ConnectToDeviceOperation<BleInitiatorFailureType>::
+                      ConnectionFailedCallback& failure_callback,
                   const DeviceIdPair& device_id_pair,
                   ConnectionPriority connection_priority,
-                  base::OnceClosure destructor_callback,
                   scoped_refptr<base::TaskRunner> task_runner =
                       base::ThreadTaskRunnerHandle::Get());
 
@@ -48,11 +47,10 @@ class BleInitiatorOperation
   BleInitiatorOperation(
       ConnectToDeviceOperation<
           BleInitiatorFailureType>::ConnectionSuccessCallback success_callback,
-      ConnectToDeviceOperation<
-          BleInitiatorFailureType>::ConnectionFailedCallback failure_callback,
+      const ConnectToDeviceOperation<
+          BleInitiatorFailureType>::ConnectionFailedCallback& failure_callback,
       const DeviceIdPair& device_id_pair,
       ConnectionPriority connection_priority,
-      base::OnceClosure destructor_callback,
       scoped_refptr<base::TaskRunner> task_runner);
 
   // ConnectToDeviceOperationBase<BleInitiatorFailureType>:

chromeos/services/secure_channel/ble_listener_operation.cc

@@ -37,15 +37,14 @@ std::unique_ptr<ConnectToDeviceOperation<BleListenerFailureType>>
 BleListenerOperation::Factory::BuildInstance(
     ConnectToDeviceOperation<BleListenerFailureType>::ConnectionSuccessCallback
         success_callback,
-    ConnectToDeviceOperation<BleListenerFailureType>::ConnectionFailedCallback
-        failure_callback,
+    const ConnectToDeviceOperation<
+        BleListenerFailureType>::ConnectionFailedCallback& failure_callback,
     const DeviceIdPair& device_id_pair,
     ConnectionPriority connection_priority,
-    base::OnceClosure destructor_callback,
     scoped_refptr<base::TaskRunner> task_runner) {
   return base::WrapUnique(new BleListenerOperation(
       std::move(success_callback), std::move(failure_callback), device_id_pair,
-      connection_priority, std::move(destructor_callback), task_runner));
+      connection_priority, task_runner));
 }
 
 BleListenerOperation::~BleListenerOperation() = default;
@@ -53,18 +52,16 @@ BleListenerOperation::~BleListenerOperation() = default;
 BleListenerOperation::BleListenerOperation(
     ConnectToDeviceOperation<BleListenerFailureType>::ConnectionSuccessCallback
         success_callback,
-    ConnectToDeviceOperation<BleListenerFailureType>::ConnectionFailedCallback
-        failure_callback,
+    const ConnectToDeviceOperation<
+        BleListenerFailureType>::ConnectionFailedCallback& failure_callback,
     const DeviceIdPair& device_id_pair,
     ConnectionPriority connection_priority,
-    base::OnceClosure destructor_callback,
     scoped_refptr<base::TaskRunner> task_runner)
     : ConnectToDeviceOperationBase<BleListenerFailureType>(
           std::move(success_callback),
           std::move(failure_callback),
           device_id_pair,
           connection_priority,
-          std::move(destructor_callback),
           task_runner) {}
 
 void BleListenerOperation::AttemptConnectionToDevice(

chromeos/services/secure_channel/ble_listener_operation.h

@@ -31,11 +31,10 @@ class BleListenerOperation
     BuildInstance(
         ConnectToDeviceOperation<
             BleListenerFailureType>::ConnectionSuccessCallback success_callback,
-        ConnectToDeviceOperation<
-            BleListenerFailureType>::ConnectionFailedCallback failure_callback,
+        const ConnectToDeviceOperation<
+            BleListenerFailureType>::ConnectionFailedCallback& failure_callback,
         const DeviceIdPair& device_id_pair,
         ConnectionPriority connection_priority,
-        base::OnceClosure destructor_callback,
         scoped_refptr<base::TaskRunner> task_runner =
             base::ThreadTaskRunnerHandle::Get());
 
@@ -49,11 +48,10 @@ class BleListenerOperation
   BleListenerOperation(
       ConnectToDeviceOperation<
           BleListenerFailureType>::ConnectionSuccessCallback success_callback,
-      ConnectToDeviceOperation<BleListenerFailureType>::ConnectionFailedCallback
-          failure_callback,
+      const ConnectToDeviceOperation<
+          BleListenerFailureType>::ConnectionFailedCallback& failure_callback,
       const DeviceIdPair& device_id_pair,
       ConnectionPriority connection_priority,
-      base::OnceClosure destructor_callback,
       scoped_refptr<base::TaskRunner> task_runner);
 
   // ConnectToDeviceOperationBase<BleListenerFailureType>:

chromeos/services/secure_channel/connect_to_device_operation.h

@@ -26,10 +26,11 @@ class ConnectToDeviceOperation {
  public:
   using ConnectionSuccessCallback =
       base::OnceCallback<void(std::unique_ptr<AuthenticatedChannel>)>;
-  using ConnectionFailedCallback = base::OnceCallback<void(FailureDetailType)>;
+  using ConnectionFailedCallback =
+      base::RepeatingCallback<void(FailureDetailType)>;
 
   virtual ~ConnectToDeviceOperation() {
-    if (!is_active_)
+    if (has_finished_)
       return;
 
     PA_LOG(ERROR) << "ConnectToDeviceOperation::~ConnectToDeviceOperation(): "
@@ -39,7 +40,7 @@ class ConnectToDeviceOperation {
 
   // Updates the priority for this operation.
   void UpdateConnectionPriority(ConnectionPriority connection_priority) {
-    if (!is_active_) {
+    if (has_finished_) {
       PA_LOG(ERROR) << "ConnectToDeviceOperation::UpdateConnectionPriority(): "
                     << "Connection priority update requested, but the "
                     << "operation was no longer active.";
@@ -54,14 +55,14 @@ class ConnectToDeviceOperation {
   // Note: Canceling an ongoing connection attempt will not cause either of the
   // success/failure callbacks passed to the constructor to be invoked.
   void Cancel() {
-    if (!is_active_) {
+    if (has_finished_) {
       PA_LOG(ERROR) << "ConnectToDeviceOperation::Cancel(): Tried to cancel "
                     << "operation after it had already finished.";
       NOTREACHED();
       return;
     }
 
-    is_active_ = false;
+    has_finished_ = true;
     PerformCancellation();
   }
 
@@ -71,10 +72,10 @@ class ConnectToDeviceOperation {
 
  protected:
   ConnectToDeviceOperation(ConnectionSuccessCallback success_callback,
-                           ConnectionFailedCallback failure_callback,
+                           const ConnectionFailedCallback& failure_callback,
                            ConnectionPriority connection_priority)
       : success_callback_(std::move(success_callback)),
-        failure_callback_(std::move(failure_callback)),
+        failure_callback_(failure_callback),
         connection_priority_(connection_priority) {}
 
   virtual void PerformCancellation() = 0;
@@ -83,31 +84,23 @@ class ConnectToDeviceOperation {
 
   void OnSuccessfulConnectionAttempt(
       std::unique_ptr<AuthenticatedChannel> authenticated_channel) {
-    if (!is_active_) {
+    if (has_finished_) {
       PA_LOG(ERROR) << "ConnectToDeviceOperation::"
                     << "OnSuccessfulConnectionAttempt(): Tried to "
                     << "complete operation after it had already finished.";
       return;
     }
 
-    is_active_ = false;
+    has_finished_ = true;
     std::move(success_callback_).Run(std::move(authenticated_channel));
   }
 
   void OnFailedConnectionAttempt(FailureDetailType failure_detail) {
-    if (!is_active_) {
-      PA_LOG(ERROR) << "ConnectToDeviceOperation::"
-                    << "OnFailedConnectionAttempt(): Tried to "
-                    << "complete operation after it had already finished.";
-      return;
-    }
-
-    is_active_ = false;
-    std::move(failure_callback_).Run(failure_detail);
+    failure_callback_.Run(failure_detail);
   }
 
  private:
-  bool is_active_ = true;
+  bool has_finished_ = false;
 
   ConnectionSuccessCallback success_callback_;
   ConnectionFailedCallback failure_callback_;

chromeos/services/secure_channel/connect_to_device_operation_base.h

@@ -34,14 +34,12 @@ class ConnectToDeviceOperationBase
           FailureDetailType>::ConnectionFailedCallback failure_callback,
       const DeviceIdPair& device_id_pair,
       ConnectionPriority connection_priority,
-      base::OnceClosure destructor_callback,
       scoped_refptr<base::TaskRunner> task_runner =
           base::ThreadTaskRunnerHandle::Get())
       : ConnectToDeviceOperation<FailureDetailType>(std::move(success_callback),
                                                     std::move(failure_callback),
                                                     connection_priority),
         device_id_pair_(device_id_pair),
-        destructor_callback_(std::move(destructor_callback)),
         task_runner_(task_runner),
         weak_ptr_factory_(this) {
     // Attempt a connection; however, post this as a task to be run after the
@@ -54,9 +52,7 @@ class ConnectToDeviceOperationBase
                        weak_ptr_factory_.GetWeakPtr(), connection_priority));
   }
 
-  ~ConnectToDeviceOperationBase() override {
-    std::move(destructor_callback_).Run();
-  }
+  ~ConnectToDeviceOperationBase() override = default;
 
   virtual void AttemptConnectionToDevice(
       ConnectionPriority connection_priority) = 0;
@@ -65,7 +61,6 @@ class ConnectToDeviceOperationBase
 
  private:
   const DeviceIdPair& device_id_pair_;
-  base::OnceClosure destructor_callback_;
   scoped_refptr<base::TaskRunner> task_runner_;
   base::WeakPtrFactory<ConnectToDeviceOperationBase> weak_ptr_factory_;
 

chromeos/services/secure_channel/connect_to_device_operation_base_unittest.cc

@@ -20,7 +20,6 @@ namespace secure_channel {
 
 namespace {
 
-const char kTestFailureReason[] = "testFailureReason";
 const char kTestRemoteDeviceId[] = "testRemoteDeviceId";
 const char kTestLocalDeviceId[] = "testLocalDeviceId";
 
@@ -32,16 +31,14 @@ class TestConnectToDeviceOperation
   static std::unique_ptr<TestConnectToDeviceOperation> Create(
       ConnectToDeviceOperation<std::string>::ConnectionSuccessCallback
           success_callback,
-      ConnectToDeviceOperation<std::string>::ConnectionFailedCallback
+      const ConnectToDeviceOperation<std::string>::ConnectionFailedCallback&
           failure_callback,
       const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority,
-      base::OnceClosure destructor_callback) {
+      ConnectionPriority connection_priority) {
     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, std::move(destructor_callback),
-        test_task_runner));
+        device_id_pair, connection_priority, test_task_runner));
     test_task_runner->RunUntilIdle();
     return operation;
   }
@@ -70,19 +67,16 @@ class TestConnectToDeviceOperation
   TestConnectToDeviceOperation(
       ConnectToDeviceOperation<std::string>::ConnectionSuccessCallback
           success_callback,
-      ConnectToDeviceOperation<std::string>::ConnectionFailedCallback
+      const ConnectToDeviceOperation<std::string>::ConnectionFailedCallback&
           failure_callback,
       const DeviceIdPair& device_id_pair,
       ConnectionPriority connection_priority,
-      base::OnceClosure destructor_callback,
       scoped_refptr<base::TestSimpleTaskRunner> test_task_runner)
-      : ConnectToDeviceOperationBase<std::string>(
-            std::move(success_callback),
-            std::move(failure_callback),
-            device_id_pair,
-            connection_priority,
-            std::move(destructor_callback),
-            test_task_runner) {}
+      : ConnectToDeviceOperationBase<std::string>(std::move(success_callback),
+                                                  std::move(failure_callback),
+                                                  device_id_pair,
+                                                  connection_priority,
+                                                  test_task_runner) {}
 
   bool has_attempted_connection_ = false;
   bool has_canceled_connection_ = false;
@@ -97,25 +91,15 @@ class SecureChannelConnectToDeviceOperationBaseTest : public testing::Test {
 
   ~SecureChannelConnectToDeviceOperationBaseTest() override = default;
 
-  // testing::Test:
-  void TearDown() override {
-    EXPECT_FALSE(destructor_callback_called_);
-    test_operation_.reset();
-    EXPECT_TRUE(destructor_callback_called_);
-  }
-
   void CreateOperation(ConnectionPriority connection_priority) {
     test_operation_ = TestConnectToDeviceOperation::Create(
         base::BindOnce(&SecureChannelConnectToDeviceOperationBaseTest::
                            OnSuccessfulConnectionAttempt,
                        base::Unretained(this)),
-        base::BindOnce(&SecureChannelConnectToDeviceOperationBaseTest::
-                           OnFailedConnectionAttempt,
-                       base::Unretained(this)),
-        test_device_id_pair_, connection_priority,
-        base::BindOnce(
-            &SecureChannelConnectToDeviceOperationBaseTest::OnOperationDeleted,
-            base::Unretained(this)));
+        base::BindRepeating(&SecureChannelConnectToDeviceOperationBaseTest::
+                                OnFailedConnectionAttempt,
+                            base::Unretained(this)),
+        test_device_id_pair_, connection_priority);
     EXPECT_TRUE(test_operation_->has_attempted_connection());
   }
 
@@ -141,14 +125,11 @@ class SecureChannelConnectToDeviceOperationBaseTest : public testing::Test {
     last_failure_detail_ = failure_detail;
   }
 
-  void OnOperationDeleted() { destructor_callback_called_ = true; }
-
   const base::test::ScopedTaskEnvironment scoped_task_environment_;
   const DeviceIdPair test_device_id_pair_;
 
   std::unique_ptr<AuthenticatedChannel> last_authenticated_channel_;
   std::string last_failure_detail_;
-  bool destructor_callback_called_ = false;
 
   std::unique_ptr<TestConnectToDeviceOperation> test_operation_;
 
@@ -177,8 +158,14 @@ TEST_F(SecureChannelConnectToDeviceOperationBaseTest, Success) {
 
 TEST_F(SecureChannelConnectToDeviceOperationBaseTest, Failure) {
   CreateOperation(ConnectionPriority::kLow);
-  test_operation()->OnFailedConnectionAttempt(kTestFailureReason);
-  EXPECT_EQ(kTestFailureReason, last_failure_detail());
+
+  test_operation()->OnFailedConnectionAttempt("failureReason1");
+  EXPECT_EQ("failureReason1", last_failure_detail());
+
+  test_operation()->OnFailedConnectionAttempt("failureReason2");
+  EXPECT_EQ("failureReason2", last_failure_detail());
+
+  test_operation()->Cancel();
 }
 
 TEST_F(SecureChannelConnectToDeviceOperationBaseTest, Cancelation) {

chromeos/services/secure_channel/connect_to_device_operation_factory.h

-// Copyright 2018 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.
-
-#ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECT_TO_DEVICE_OPERATION_FACTORY_H_
-#define CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECT_TO_DEVICE_OPERATION_FACTORY_H_
-
-#include "base/callback_forward.h"
-#include "base/macros.h"
-#include "chromeos/services/secure_channel/connect_to_device_operation.h"
-#include "chromeos/services/secure_channel/device_id_pair.h"
-#include "chromeos/services/secure_channel/public/cpp/shared/connection_priority.h"
-
-namespace chromeos {
-
-namespace secure_channel {
-
-// Factory for creating ConnectToDeviceOperation instances.
-template <typename FailureDetailType>
-class ConnectToDeviceOperationFactory {
- public:
-  virtual ~ConnectToDeviceOperationFactory() = default;
-
-  virtual std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>>
-  CreateOperation(
-      const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority,
-      typename ConnectToDeviceOperation<
-          FailureDetailType>::ConnectionSuccessCallback success_callback,
-      typename ConnectToDeviceOperation<
-          FailureDetailType>::ConnectionFailedCallback failure_callback) = 0;
-
- protected:
-  ConnectToDeviceOperationFactory() = default;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ConnectToDeviceOperationFactory);
-};
-
-}  // namespace secure_channel
-
-}  // namespace chromeos
-
-#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECT_TO_DEVICE_OPERATION_FACTORY_H_

chromeos/services/secure_channel/connect_to_device_operation_factory_base.h

-// Copyright 2018 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.
-
-#ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECT_TO_DEVICE_OPERATION_FACTORY_BASE_H_
-#define CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECT_TO_DEVICE_OPERATION_FACTORY_BASE_H_
-
-#include "base/bind.h"
-#include "base/callback_forward.h"
-#include "base/logging.h"
-#include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "chromeos/services/secure_channel/connect_to_device_operation_factory.h"
-#include "chromeos/services/secure_channel/device_id_pair.h"
-
-namespace chromeos {
-
-namespace secure_channel {
-
-// ConnectToDeviceOperationFactory implementation, which ensures that only one
-// operation can be active at a time.
-template <typename FailureDetailType>
-class ConnectToDeviceOperationFactoryBase
-    : public ConnectToDeviceOperationFactory<FailureDetailType> {
- public:
-  ~ConnectToDeviceOperationFactoryBase() override = default;
-
- protected:
-  ConnectToDeviceOperationFactoryBase(const DeviceIdPair& device_id_pair)
-      : device_id_pair_(device_id_pair), weak_ptr_factory_(this) {}
-
-  // Derived types should overload this function, passing the provided
-  // parameters to the constructor of a type derived from
-  // ConnectToDeviceOperationBase.
-  virtual std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>>
-  PerformCreateOperation(
-      const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority,
-      typename ConnectToDeviceOperation<
-          FailureDetailType>::ConnectionSuccessCallback success_callback,
-      typename ConnectToDeviceOperation<
-          FailureDetailType>::ConnectionFailedCallback failure_callback,
-      base::OnceClosure destructor_callback) = 0;
-
- private:
-  // ConnectToDeviceOperationFactory<FailureDetailType>:
-  std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>> CreateOperation(
-      const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority,
-      typename ConnectToDeviceOperation<
-          FailureDetailType>::ConnectionSuccessCallback success_callback,
-      typename ConnectToDeviceOperation<FailureDetailType>::
-          ConnectionFailedCallback failure_callback) override {
-    if (is_last_operation_active_) {
-      PA_LOG(ERROR) << "ConnectToDeviceOperationFactoryBase::CreateOperation():"
-                    << " Requested new operation before the previous one was "
-                    << "finished.";
-      NOTREACHED();
-      return nullptr;
-    }
-
-    is_last_operation_active_ = true;
-    return PerformCreateOperation(
-        device_id_pair_, connection_priority, std::move(success_callback),
-        std::move(failure_callback),
-        base::BindOnce(&ConnectToDeviceOperationFactoryBase<
-                           FailureDetailType>::OnPreviousOperationDeleted,
-                       weak_ptr_factory_.GetWeakPtr()));
-  }
-
-  void OnPreviousOperationDeleted() { is_last_operation_active_ = false; }
-
-  const DeviceIdPair device_id_pair_;
-  bool is_last_operation_active_ = false;
-
-  base::WeakPtrFactory<ConnectToDeviceOperationFactoryBase> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(ConnectToDeviceOperationFactoryBase);
-};
-
-}  // namespace secure_channel
-
-}  // namespace chromeos
-
-#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECT_TO_DEVICE_OPERATION_FACTORY_BASE_H_

chromeos/services/secure_channel/connect_to_device_operation_factory_base_unittest.cc

-// Copyright 2018 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 "chromeos/services/secure_channel/connect_to_device_operation_factory_base.h"
-
-#include <memory>
-
-#include "base/bind_helpers.h"
-#include "base/test/gtest_util.h"
-#include "chromeos/services/secure_channel/device_id_pair.h"
-#include "chromeos/services/secure_channel/fake_connect_to_device_operation.h"
-#include "chromeos/services/secure_channel/public/cpp/shared/authenticated_channel.h"
-#include "components/cryptauth/secure_channel.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace chromeos {
-
-namespace secure_channel {
-
-namespace {
-
-constexpr const ConnectionPriority kTestPriority = ConnectionPriority::kLow;
-
-const char kTestRemoteDeviceId[] = "testRemoteDeviceId";
-const char kTestLocalDeviceId[] = "testLocalDeviceId";
-
-// Since ConnectToDeviceOperationFactoryBase is templatized, a concrete
-// implementation is needed for its test.
-class TestConnectToDeviceOperationFactory
-    : public ConnectToDeviceOperationFactoryBase<std::string> {
- public:
-  TestConnectToDeviceOperationFactory(const DeviceIdPair& device_id_pair)
-      : ConnectToDeviceOperationFactoryBase<std::string>(device_id_pair),
-        device_id_pair_(device_id_pair) {}
-
-  ~TestConnectToDeviceOperationFactory() override = default;
-
-  void InvokePendingDestructorCallback() {
-    EXPECT_TRUE(last_destructor_callback_);
-    std::move(last_destructor_callback_).Run();
-  }
-
-  // ConnectToDeviceOperationFactoryBase<std::string>:
-  std::unique_ptr<ConnectToDeviceOperation<std::string>> PerformCreateOperation(
-      const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority,
-      typename ConnectToDeviceOperation<std::string>::ConnectionSuccessCallback
-          success_callback,
-      typename ConnectToDeviceOperation<std::string>::ConnectionFailedCallback
-          failure_callback,
-      base::OnceClosure destructor_callback) override {
-    // The previous destructor callback should have been invoked by the time
-    // this function runs.
-    EXPECT_FALSE(last_destructor_callback_);
-    last_destructor_callback_ = std::move(destructor_callback);
-
-    EXPECT_EQ(kTestPriority, connection_priority);
-    EXPECT_EQ(device_id_pair_, device_id_pair);
-
-    return std::make_unique<FakeConnectToDeviceOperation<std::string>>(
-        std::move(success_callback), std::move(failure_callback),
-        connection_priority);
-  }
-
- private:
-  const DeviceIdPair device_id_pair_;
-
-  base::OnceClosure last_destructor_callback_;
-};
-
-}  // namespace
-
-class SecureChannelConnectToDeviceOperationFactoryBaseTest
-    : public testing::Test {
- protected:
-  SecureChannelConnectToDeviceOperationFactoryBaseTest()
-      : test_device_id_pair_(kTestRemoteDeviceId, kTestLocalDeviceId) {}
-
-  ~SecureChannelConnectToDeviceOperationFactoryBaseTest() override = default;
-
-  void SetUp() override {
-    test_factory_ = std::make_unique<TestConnectToDeviceOperationFactory>(
-        test_device_id_pair_);
-  }
-
-  std::unique_ptr<ConnectToDeviceOperation<std::string>> CallCreateOperation() {
-    // Use a pointer to ConnectToDeviceOperationFactory, since
-    // ConnectToDeviceOperationFactoryBase makes CreateOperation private.
-    ConnectToDeviceOperationFactory<std::string>* factory = test_factory_.get();
-    return factory->CreateOperation(test_device_id_pair_, kTestPriority,
-                                    base::DoNothing() /* success_callback */,
-                                    base::DoNothing() /* failure_callback */);
-  }
-
-  void FinishActiveOperation(
-      std::unique_ptr<ConnectToDeviceOperation<std::string>>
-          operation_to_finish) {
-    EXPECT_TRUE(operation_to_finish);
-
-    // Use a pointer to FakeConnectToDeviceOperation, since
-    // ConnectToDeviceOperation makes OnSuccessfulConnectionAttempt() protected.
-    FakeConnectToDeviceOperation<std::string>* operation =
-        static_cast<FakeConnectToDeviceOperation<std::string>*>(
-            operation_to_finish.get());
-    operation->OnFailedConnectionAttempt(
-        "arbitraryFailureDetail" /* failure_detail */);
-
-    test_factory_->InvokePendingDestructorCallback();
-  }
-
- private:
-  const DeviceIdPair test_device_id_pair_;
-
-  std::unique_ptr<TestConnectToDeviceOperationFactory> test_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(
-      SecureChannelConnectToDeviceOperationFactoryBaseTest);
-};
-
-TEST_F(SecureChannelConnectToDeviceOperationFactoryBaseTest,
-       UseFactoryCorrectly) {
-  auto operation1 = CallCreateOperation();
-  EXPECT_TRUE(operation1);
-
-  // Invoke the destructor callback and try again.
-  FinishActiveOperation(std::move(operation1));
-  auto operation2 = CallCreateOperation();
-  EXPECT_TRUE(operation2);
-
-  // Invoke the destructor callback and try again.
-  FinishActiveOperation(std::move(operation2));
-  auto operation3 = CallCreateOperation();
-  EXPECT_TRUE(operation3);
-
-  FinishActiveOperation(std::move(operation3));
-}
-
-TEST_F(SecureChannelConnectToDeviceOperationFactoryBaseTest,
-       UseFactoryIncorrectly) {
-  // First operation should always complete successfully.
-  auto operation = CallCreateOperation();
-  EXPECT_TRUE(operation);
-
-  // The first operation's destructor callback has not yet been invoked, so
-  // creating another should fail.
-  EXPECT_DCHECK_DEATH(CallCreateOperation());
-
-  FinishActiveOperation(std::move(operation));
-}
-
-}  // namespace secure_channel
-
-}  // namespace chromeos

chromeos/services/secure_channel/connection_attempt.h

@@ -17,6 +17,7 @@
 #include "chromeos/services/secure_channel/connection_attempt_details.h"
 #include "chromeos/services/secure_channel/connection_details.h"
 #include "chromeos/services/secure_channel/pending_connection_request.h"
+#include "chromeos/services/secure_channel/pending_connection_request_delegate.h"
 
 namespace chromeos {
 
@@ -29,7 +30,7 @@ class AuthenticatedChannel;
 // more PendingConnectionRequests and notifies its delegate when the attempt has
 // succeeded or failed.
 template <typename FailureDetailType>
-class ConnectionAttempt {
+class ConnectionAttempt : public PendingConnectionRequestDelegate {
  public:
   // Extracts all of the ClientConnectionParameters owned by |attempt|'s
   // PendingConnectionRequests. This function deletes |attempt| as part of this
@@ -57,7 +58,7 @@ class ConnectionAttempt {
       return false;
     }
 
-    if (has_notified_delegate_) {
+    if (has_notified_delegate_of_success_) {
       PA_LOG(ERROR) << "ConnectionAttempt::AddPendingConnectionRequest(): "
                     << "Tried to add an additional request,but the attempt had "
                     << "already finished.";
@@ -88,21 +89,21 @@ class ConnectionAttempt {
 
   void OnConnectionAttemptSucceeded(
       std::unique_ptr<AuthenticatedChannel> authenticated_channel) {
-    if (has_notified_delegate_) {
+    if (has_notified_delegate_of_success_) {
       PA_LOG(ERROR) << "ConnectionAttempt::OnConnectionAttemptSucceeded(): "
                     << "Tried to alert delegate of a successful connection, "
                     << "but the attempt had already finished.";
       return;
     }
 
-    has_notified_delegate_ = true;
+    has_notified_delegate_of_success_ = true;
     delegate_->OnConnectionAttemptSucceeded(
         connection_attempt_details_.GetAssociatedConnectionDetails(),
         std::move(authenticated_channel));
   }
 
   void OnConnectionAttemptFinishedWithoutConnection() {
-    if (has_notified_delegate_) {
+    if (has_notified_delegate_of_success_) {
       PA_LOG(ERROR) << "ConnectionAttempt::"
                     << "OnConnectionAttemptFinishedWithoutConnection(): "
                     << "Tried to alert delegate of a failed connection, "
@@ -110,7 +111,6 @@ class ConnectionAttempt {
       return;
     }
 
-    has_notified_delegate_ = true;
     delegate_->OnConnectionAttemptFinishedWithoutConnection(
         connection_attempt_details_);
   }
@@ -119,7 +119,7 @@ class ConnectionAttempt {
   ConnectionAttemptDelegate* delegate_;
   const ConnectionAttemptDetails connection_attempt_details_;
 
-  bool has_notified_delegate_ = false;
+  bool has_notified_delegate_of_success_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ConnectionAttempt);
 };

chromeos/services/secure_channel/connection_attempt_base.h

@@ -5,22 +5,19 @@
 #ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECTION_ATTEMPT_BASE_H_
 #define CHROMEOS_SERVICES_SECURE_CHANNEL_CONNECTION_ATTEMPT_BASE_H_
 
-#include <unordered_map>
-
+#include "base/containers/flat_map.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/stl_util.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
 #include "chromeos/services/secure_channel/connect_to_device_operation.h"
-#include "chromeos/services/secure_channel/connect_to_device_operation_factory.h"
 #include "chromeos/services/secure_channel/connection_attempt.h"
 #include "chromeos/services/secure_channel/connection_attempt_details.h"
 #include "chromeos/services/secure_channel/connection_details.h"
 #include "chromeos/services/secure_channel/pending_connection_request.h"
-#include "chromeos/services/secure_channel/pending_connection_request_delegate.h"
+#include "chromeos/services/secure_channel/public/cpp/shared/authenticated_channel.h"
 #include "chromeos/services/secure_channel/public/cpp/shared/connection_priority.h"
 #include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
 
@@ -47,28 +44,29 @@ class AuthenticatedChannel;
 // ConnectionAttempt simply starts up a new operation and retries the
 // connection.
 template <typename FailureDetailType>
-class ConnectionAttemptBase : public ConnectionAttempt<FailureDetailType>,
-                              public PendingConnectionRequestDelegate {
+class ConnectionAttemptBase : public ConnectionAttempt<FailureDetailType> {
  protected:
   ConnectionAttemptBase(
-      std::unique_ptr<ConnectToDeviceOperationFactory<FailureDetailType>>
-          connect_to_device_operation_factory,
       ConnectionAttemptDelegate* delegate,
-      const ConnectionAttemptDetails& connection_attempt_details,
-      scoped_refptr<base::TaskRunner> task_runner =
-          base::ThreadTaskRunnerHandle::Get())
+      const ConnectionAttemptDetails& connection_attempt_details)
       : ConnectionAttempt<FailureDetailType>(delegate,
                                              connection_attempt_details),
-        connect_to_device_operation_factory_(
-            std::move(connect_to_device_operation_factory)),
-        task_runner_(task_runner),
         weak_ptr_factory_(this) {}
 
   ~ConnectionAttemptBase() override {
-    if (current_operation_)
-      current_operation_->Cancel();
+    if (operation_)
+      operation_->Cancel();
   }
 
+  virtual std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>>
+  CreateConnectToDeviceOperation(
+      const DeviceIdPair& device_id_pair,
+      ConnectionPriority connection_priority,
+      typename ConnectToDeviceOperation<
+          FailureDetailType>::ConnectionSuccessCallback success_callback,
+      const typename ConnectToDeviceOperation<
+          FailureDetailType>::ConnectionFailedCallback& failure_callback) = 0;
+
  private:
   // ConnectionAttempt<FailureDetailType>:
   void ProcessAddingNewConnectionRequest(
@@ -88,16 +86,26 @@ class ConnectionAttemptBase : public ConnectionAttempt<FailureDetailType>,
     id_to_request_map_[request->GetRequestId()] = std::move(request);
 
     // In the case that this ConnectionAttempt was just created and had not yet
-    // received a request yet, start up an operation.
+    // received a request yet, start up its operation.
     if (was_empty) {
-      StartNextConnectToDeviceOperation();
+      operation_ = CreateConnectToDeviceOperation(
+          this->connection_attempt_details().device_id_pair(),
+          GetHighestRemainingConnectionPriority(),
+          base::BindOnce(
+              &ConnectionAttemptBase<
+                  FailureDetailType>::OnConnectToDeviceOperationSuccess,
+              weak_ptr_factory_.GetWeakPtr()),
+          base::BindRepeating(
+              &ConnectionAttemptBase<
+                  FailureDetailType>::OnConnectToDeviceOperationFailure,
+              weak_ptr_factory_.GetWeakPtr()));
       return;
     }
 
     ConnectionPriority priority_after_add =
         GetHighestRemainingConnectionPriority();
     if (priority_before_add != priority_after_add)
-      current_operation_->UpdateConnectionPriority(priority_after_add);
+      operation_->UpdateConnectionPriority(priority_after_add);
   }
 
   std::vector<std::unique_ptr<ClientConnectionParameters>>
@@ -130,51 +138,21 @@ class ConnectionAttemptBase : public ConnectionAttempt<FailureDetailType>,
     ConnectionPriority priority_after_removal =
         GetHighestRemainingConnectionPriority();
     if (priority_before_removal != priority_after_removal)
-      current_operation_->UpdateConnectionPriority(priority_after_removal);
+      operation_->UpdateConnectionPriority(priority_after_removal);
 
     // If there are no longer any active entries, this attempt is finished.
     if (id_to_request_map_.empty())
       this->OnConnectionAttemptFinishedWithoutConnection();
   }
 
-  void StartNextConnectToDeviceOperation() {
-    DCHECK(!current_operation_);
-    current_operation_ = connect_to_device_operation_factory_->CreateOperation(
-        this->connection_attempt_details().device_id_pair(),
-        GetHighestRemainingConnectionPriority(),
-        base::BindOnce(
-            &ConnectionAttemptBase<
-                FailureDetailType>::OnConnectToDeviceOperationSuccess,
-            weak_ptr_factory_.GetWeakPtr()),
-        base::BindOnce(
-            &ConnectionAttemptBase<
-                FailureDetailType>::OnConnectToDeviceOperationFailure,
-            weak_ptr_factory_.GetWeakPtr()));
-  }
-
   void OnConnectToDeviceOperationSuccess(
       std::unique_ptr<AuthenticatedChannel> authenticated_channel) {
-    DCHECK(current_operation_);
-    current_operation_.reset();
+    DCHECK(operation_);
+    operation_.reset();
     this->OnConnectionAttemptSucceeded(std::move(authenticated_channel));
   }
 
   void OnConnectToDeviceOperationFailure(FailureDetailType failure_detail) {
-    DCHECK(current_operation_);
-    current_operation_.reset();
-
-    // After all requests have been notified of the failure (below), start a
-    // retry attempt. If all requests give up without a connection, this
-    // instance's delegate will be notified and will, in response, delete
-    // |this|. Thus, the retry attempt is posted as a task using a WeakPtr to
-    // ensure that a segfault will not occur.
-    task_runner_->PostTask(
-        FROM_HERE,
-        base::BindOnce(
-            &ConnectionAttemptBase<
-                FailureDetailType>::StartNextConnectToDeviceOperation,
-            weak_ptr_factory_.GetWeakPtr()));
-
     for (auto& map_entry : id_to_request_map_)
       map_entry.second->HandleConnectionFailure(failure_detail);
   }
@@ -188,17 +166,10 @@ class ConnectionAttemptBase : public ConnectionAttempt<FailureDetailType>,
     return highest_priority;
   }
 
-  std::unique_ptr<ConnectToDeviceOperationFactory<FailureDetailType>>
-      connect_to_device_operation_factory_;
-
-  std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>>
-      current_operation_;
-  std::unordered_map<
-      base::UnguessableToken,
-      std::unique_ptr<PendingConnectionRequest<FailureDetailType>>,
-      base::UnguessableTokenHash>
+  std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>> operation_;
+  base::flat_map<base::UnguessableToken,
+                 std::unique_ptr<PendingConnectionRequest<FailureDetailType>>>
       id_to_request_map_;
-  scoped_refptr<base::TaskRunner> task_runner_;
 
   base::WeakPtrFactory<ConnectionAttemptBase<FailureDetailType>>
       weak_ptr_factory_;

chromeos/services/secure_channel/fake_connect_to_device_operation.h

@@ -42,6 +42,10 @@ class FakeConnectToDeviceOperation
     destructor_callback_ = std::move(destructor_callback);
   }
 
+  void set_cancel_callback(base::OnceClosure cancel_callback) {
+    cancel_callback_ = std::move(cancel_callback);
+  }
+
   // Make On{Successful|Failed}ConnectionAttempt() public for testing.
   using ConnectToDeviceOperation<
       FailureDetailType>::OnSuccessfulConnectionAttempt;
@@ -49,7 +53,12 @@ class FakeConnectToDeviceOperation
 
  private:
   // ConnectToDeviceOperation<FailureDetailType>:
-  void PerformCancellation() override { canceled_ = true; }
+  void PerformCancellation() override {
+    canceled_ = true;
+
+    if (cancel_callback_)
+      std::move(cancel_callback_).Run();
+  }
 
   void PerformUpdateConnectionPriority(
       ConnectionPriority connection_priority) override {
@@ -59,6 +68,7 @@ class FakeConnectToDeviceOperation
   bool canceled_ = false;
   base::Optional<ConnectionPriority> updated_priority_;
   base::OnceClosure destructor_callback_;
+  base::OnceClosure cancel_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeConnectToDeviceOperation);
 };

chromeos/services/secure_channel/fake_connect_to_device_operation_factory.h

-// Copyright 2018 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.
-
-#ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_CONNECT_TO_DEVICE_OPERATION_FACTORY_H_
-#define CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_CONNECT_TO_DEVICE_OPERATION_FACTORY_H_
-
-#include "base/callback_forward.h"
-#include "base/macros.h"
-#include "base/optional.h"
-#include "chromeos/services/secure_channel/connect_to_device_operation_factory.h"
-#include "chromeos/services/secure_channel/device_id_pair.h"
-#include "chromeos/services/secure_channel/fake_connect_to_device_operation_factory.h"
-#include "chromeos/services/secure_channel/public/cpp/shared/connection_priority.h"
-
-namespace chromeos {
-
-namespace secure_channel {
-
-// Fake ConnectToDeviceOperationFactory implementation.
-template <typename FailureDetailType>
-class FakeConnectToDeviceOperationFactory
-    : public ConnectToDeviceOperationFactory<FailureDetailType> {
- public:
-  FakeConnectToDeviceOperationFactory() = default;
-  ~FakeConnectToDeviceOperationFactory() override = default;
-
-  FakeConnectToDeviceOperation<FailureDetailType>* last_created_instance() {
-    return last_created_instance_;
-  }
-
-  const base::Optional<ConnectionPriority>& last_created_priority() const {
-    return last_created_priority_;
-  }
-
-  size_t num_instances_created() const { return num_instances_created_; }
-
- private:
-  // ConnectToDeviceOperationFactory<FailureDetailType>:
-  std::unique_ptr<ConnectToDeviceOperation<FailureDetailType>> CreateOperation(
-      const DeviceIdPair& device_id_pair,
-      ConnectionPriority connection_priority,
-      typename ConnectToDeviceOperation<
-          FailureDetailType>::ConnectionSuccessCallback success_callback,
-      typename ConnectToDeviceOperation<FailureDetailType>::
-          ConnectionFailedCallback failure_callback) override {
-    auto instance =
-        std::make_unique<FakeConnectToDeviceOperation<FailureDetailType>>(
-            std::move(success_callback), std::move(failure_callback),
-            connection_priority);
-    last_created_instance_ = instance.get();
-    ++num_instances_created_;
-    return instance;
-  }
-
-  FakeConnectToDeviceOperation<FailureDetailType>* last_created_instance_ =
-      nullptr;
-  base::Optional<ConnectionPriority> last_created_priority_;
-  size_t num_instances_created_ = 0u;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeConnectToDeviceOperationFactory);
-};
-
-}  // namespace secure_channel
-
-}  // namespace chromeos
-
-#endif  // CHROMEOS_SERVICES_SECURE_CHANNEL_FAKE_CONNECT_TO_DEVICE_OPERATION_FACTORY_H_

chromeos/services/secure_channel/fake_connection_attempt.h

@@ -58,6 +58,12 @@ class FakeConnectionAttempt : public ConnectionAttempt<FailureDetailType> {
     id_to_request_map_[request->GetRequestId()] = std::move(request);
   }
 
+  // PendingConnectionRequestDelegate:
+  void OnRequestFinishedWithoutConnection(
+      const base::UnguessableToken& request_id,
+      PendingConnectionRequestDelegate::FailedConnectionReason reason)
+      override {}
+
   std::vector<std::unique_ptr<ClientConnectionParameters>>
   ExtractClientConnectionParameters() override {
     return std::move(client_data_for_extraction_);