[CrOS MultiDevice] Re-land crash fix for BleConnectionManagerImpl.

Originally landed as:
https://chromium-review.googlesource.com/c/1352855
Reverted as:
https://chromium-review.googlesource.com/c/chromium/src/+/1354327

First patch set includes original CL, and subsequent patches contain
updates to fix the failing test.

Original CL descrition:

When a SecureChannel disconnects due to authentication failures,
BleConnectionManagerImpl invokes the relevant failure callback, which
then causes ConnectionAttemptBase to cancel the connection attempt.

When the attempt is cancelled, the SecureChannel object is deleted via
BleConnectionManagerImpl::ProcessPotentialLingeringChannel(), but
BleConnectionManagerImpl::HandleSecureChannelDisconnection() assumed
that the object was still valid and tried to to remove itself as an
observer, causing a segfault.

The fix is to check whether the object has been deleted before
attempting to call RemoveObserver().

Bug: 905904, 909883
Change-Id: I770bff8f05865aeac8b7513d7193d0b7a40d056e
Reviewed-on: https://chromium-review.googlesource.com/c/1354333Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611937}

chromeos/services/secure_channel/ble_connection_manager_impl.cc

@@ -503,6 +503,15 @@ void BleConnectionManagerImpl::HandleSecureChannelDisconnection(
   // connection's latency should be reset and started anew.
   remote_device_id_to_timestamps_map_[remote_device_id]->Reset();
 
+  if (!DoesAuthenticatingChannelExist(remote_device_id)) {
+    PA_LOG(ERROR) << "BleConnectionManagerImpl::"
+                  << "HandleSecureChannelDisconnection(): Disconnected channel "
+                  << "not present in map. Remote device ID: "
+                  << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
+                         remote_device_id);
+    NOTREACHED();
+  }
+
   for (const auto& details : GetDetailsForRemoteDevice(remote_device_id)) {
     switch (details.connection_role()) {
       // Initiator role devices are notified of authentication errors as well as
@@ -525,10 +534,19 @@ void BleConnectionManagerImpl::HandleSecureChannelDisconnection(
     }
   }
 
+  // It is possible that the NotifyBle*Failure() calls above resulted in
+  // observers responding to the failure by canceling the connection attempt.
+  // If all attempts to |remote_device_id| were cancelled, the disconnected
+  // channel will have already been cleaned up via
+  // ProcessPotentialLingeringChannel().
+  auto it = remote_device_id_to_secure_channel_map_.find(remote_device_id);
+  if (it == remote_device_id_to_secure_channel_map_.end())
+    return;
+
   // Stop observing the disconnected channel and remove it from the map.
-  remote_device_id_to_secure_channel_map_[remote_device_id]
-      .first->RemoveObserver(this);
-  remote_device_id_to_secure_channel_map_.erase(remote_device_id);
+  SecureChannelWithRole& secure_channel_with_role = it->second;
+  secure_channel_with_role.first->RemoveObserver(this);
+  remote_device_id_to_secure_channel_map_.erase(it);
 
   // Since the previous connection failed, the connection attempts that were
   // paused in SetAuthenticatingChannel() need to be started up again. Note

chromeos/services/secure_channel/ble_connection_manager_impl_unittest.cc

@@ -468,7 +468,8 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
 
   void AttemptBleListenerConnection(const DeviceIdPair& device_id_pair,
                                     ConnectionPriority connection_priority,
-                                    bool expected_to_add_request) {
+                                    bool expected_to_add_request,
+                                    bool should_cancel_attempt_on_failure) {
     SetInRemoteDeviceIdToMetadataMap(
         device_id_pair, ConnectionRole::kListenerRole, connection_priority);
 
@@ -480,7 +481,8 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
             true /* created_via_background_advertisement */),
         base::BindRepeating(
             &SecureChannelBleConnectionManagerImplTest::OnBleListenerFailure,
-            base::Unretained(this), device_id_pair));
+            base::Unretained(this), device_id_pair,
+            should_cancel_attempt_on_failure));
 
     if (expected_to_add_request) {
       EXPECT_TRUE(fake_ble_scanner()->HasScanFilter(BleScanner::ScanFilter(
@@ -576,7 +578,9 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
   void SimulateSecureChannelDisconnection(
       const std::string& remote_device_id,
       bool fail_during_authentication,
-      cryptauth::FakeSecureChannel* fake_secure_channel) {
+      cryptauth::FakeSecureChannel* fake_secure_channel,
+      size_t num_initiator_attempts_canceled_from_disconnection = 0u,
+      size_t num_listener_attempts_canceled_from_disconnection = 0u) {
     size_t num_ble_initiator_failures_before_call =
         ble_initiator_failures_.size();
     size_t num_ble_listener_failures_before_call =
@@ -595,8 +599,12 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
 
     // Iterate through all pending requests to |remote_device_id|, ensuring that
     // all expected failures have been communicated back to the client.
-    size_t initiator_failures_index = num_ble_initiator_failures_before_call;
-    size_t listener_failure_index = num_ble_listener_failures_before_call;
+    size_t initiator_failures_index =
+        num_ble_initiator_failures_before_call +
+        num_initiator_attempts_canceled_from_disconnection;
+    size_t listener_failure_index =
+        num_ble_listener_failures_before_call +
+        num_listener_attempts_canceled_from_disconnection;
     for (const auto& tuple :
          remote_device_id_to_metadata_map_[remote_device_id]) {
       switch (std::get<1>(tuple)) {
@@ -771,16 +779,24 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
     if (!should_cancel_attempt_on_failure)
       return;
 
-    base::PostTask(FROM_HERE,
-                   base::BindOnce(&SecureChannelBleConnectionManagerImplTest::
-                                      CancelBleInitiatorConnectionAttempt,
-                                  base::Unretained(this), device_id_pair));
+    // Make a copy of the pair before canceling the attempt, since the reference
+    // points to memory owned by |manager_| which will be deleted.
+    DeviceIdPair device_id_pair_copy = device_id_pair;
+    CancelBleInitiatorConnectionAttempt(device_id_pair_copy);
   }
 
   void OnBleListenerFailure(const DeviceIdPair& device_id_pair,
+                            bool should_cancel_attempt_on_failure,
                             BleListenerFailureType failure_type) {
     ble_listener_failures_.push_back(
         std::make_pair(device_id_pair, failure_type));
+    if (!should_cancel_attempt_on_failure)
+      return;
+
+    // Make a copy of the pair before canceling the attempt, since the reference
+    // points to memory owned by |manager_| which will be deleted.
+    DeviceIdPair device_id_pair_copy = device_id_pair;
+    CancelBleListenerConnectionAttempt(device_id_pair_copy);
   }
 
   void SetInRemoteDeviceIdToMetadataMap(
@@ -934,13 +950,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kLow,
                                 true /* expected_to_add_request */,
                                 true /* should_cancel_attempt_on_failure */);
-
   SimulateBleFailureToGenerateAdvertisement(pair);
-
-  // Runs the cleanup routine that has been posted as a parallel task. It has
-  // not yet been run because |scoped_task_environment_| was instantiated with
-  // base::test::ScopedTaskEnvironment::ExecutionMode::QUEUED.
-  scoped_task_environment_.RunUntilIdle();
 }
 
 TEST_F(SecureChannelBleConnectionManagerImplTest,
@@ -962,6 +972,25 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
   CancelBleInitiatorConnectionAttempt(pair);
 }
 
+TEST_F(SecureChannelBleConnectionManagerImplTest,
+       OneRequest_Initiator_FailsAuthenticationThenCanceled_CleanupOnCallback) {
+  DeviceIdPair pair(test_devices()[1].GetDeviceId(),
+                    test_devices()[0].GetDeviceId());
+
+  AttemptBleInitiatorConnection(pair, ConnectionPriority::kLow,
+                                true /* expected_to_add_request */,
+                                true /* should_cancel_attempt_on_failure */);
+
+  cryptauth::FakeSecureChannel* fake_secure_channel =
+      SimulateConnectionEstablished(test_devices()[1],
+                                    ConnectionRole::kInitiatorRole);
+  SimulateSecureChannelDisconnection(
+      pair.remote_device_id(), true /* fail_during_authentication */,
+      fake_secure_channel,
+      1u /* num_initiator_attempts_canceled_from_disconnection */,
+      0u /* num_listener_attempts_canceled_from_disconnection */);
+}
+
 TEST_F(SecureChannelBleConnectionManagerImplTest,
        OneRequest_Initiator_GattFailureThenCanceled) {
   DeviceIdPair pair(test_devices()[1].GetDeviceId(),
@@ -1004,7 +1033,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   UpdateBleListenerConnectionPriority(pair, ConnectionPriority::kMedium,
                                       true /* expected_to_update_priority */);
   UpdateBleListenerConnectionPriority(pair, ConnectionPriority::kHigh,
@@ -1025,7 +1055,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1037,13 +1068,33 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
   CancelBleListenerConnectionAttempt(pair);
 }
 
+TEST_F(SecureChannelBleConnectionManagerImplTest,
+       OneRequest_Listener_FailsAuthenticationThenCanceled_CleanupOnCallback) {
+  DeviceIdPair pair(test_devices()[1].GetDeviceId(),
+                    test_devices()[0].GetDeviceId());
+
+  AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
+                               true /* expected_to_add_request */,
+                               true /* should_cancel_attempt_on_failure */);
+
+  cryptauth::FakeSecureChannel* fake_secure_channel =
+      SimulateConnectionEstablished(test_devices()[1],
+                                    ConnectionRole::kListenerRole);
+  SimulateSecureChannelDisconnection(
+      pair.remote_device_id(), true /* fail_during_authentication */,
+      fake_secure_channel,
+      0u /* num_initiator_attempts_canceled_from_disconnection */,
+      1u /* num_listener_attempts_canceled_from_disconnection */);
+}
+
 TEST_F(SecureChannelBleConnectionManagerImplTest,
        OneRequest_Listener_GattFailureThenCanceled) {
   DeviceIdPair pair(test_devices()[1].GetDeviceId(),
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1061,7 +1112,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1149,9 +1201,11 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                       test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair_1, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleListenerConnection(pair_2, ConnectionPriority::kMedium,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel_1 =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1179,9 +1233,11 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                       test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair_1, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleListenerConnection(pair_2, ConnectionPriority::kMedium,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel_1 =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1204,7 +1260,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1235,7 +1292,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1254,7 +1312,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1281,7 +1340,8 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1305,7 +1365,9 @@ TEST_F(SecureChannelBleConnectionManagerImplTest, ConnectionMetrics) {
 
   // Start a connection attempt and advance the clock.
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */);
+                               true /* expected_to_add_request */,
+                               false /* should_cancel_attempt_on_failure */);
+
   test_clock()->Advance(kScanToAdvertisementTime);
 
   // Simulate a connection being established, then disconnected.