Revert "[CrOS MultiDevice] Fix crash in BleConnectionManagerImpl."

This reverts commit 17b07c4c.

Reason for revert: Breaking chromeos_unittests, see https://crbug.com/909883

Original change's description:
> [CrOS MultiDevice] Fix crash in BleConnectionManagerImpl.
> 
> 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
> Change-Id: Ic593ef4047ff481e218c89028324eb0863131cdc
> Reviewed-on: https://chromium-review.googlesource.com/c/1352855
> Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#611781}

TBR=khorimoto@chromium.org,hansberry@chromium.org

Change-Id: I575b604a98318f778cd1f7d93c31e14806da2e3f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 905904
Reviewed-on: https://chromium-review.googlesource.com/c/1354327Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#611879}

chromeos/services/secure_channel/ble_connection_manager_impl.cc

@@ -503,15 +503,6 @@ 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
@@ -534,19 +525,10 @@ 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.
-  SecureChannelWithRole& secure_channel_with_role = it->second;
-  secure_channel_with_role.first->RemoveObserver(this);
-  remote_device_id_to_secure_channel_map_.erase(it);
+  remote_device_id_to_secure_channel_map_[remote_device_id]
+      .first->RemoveObserver(this);
+  remote_device_id_to_secure_channel_map_.erase(remote_device_id);
 
   // 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,8 +468,7 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
 
   void AttemptBleListenerConnection(const DeviceIdPair& device_id_pair,
                                     ConnectionPriority connection_priority,
-                                    bool expected_to_add_request,
-                                    bool should_cancel_attempt_on_failure) {
+                                    bool expected_to_add_request) {
     SetInRemoteDeviceIdToMetadataMap(
         device_id_pair, ConnectionRole::kListenerRole, connection_priority);
 
@@ -481,8 +480,7 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
             true /* created_via_background_advertisement */),
         base::BindRepeating(
             &SecureChannelBleConnectionManagerImplTest::OnBleListenerFailure,
-            base::Unretained(this), device_id_pair,
-            should_cancel_attempt_on_failure));
+            base::Unretained(this), device_id_pair));
 
     if (expected_to_add_request) {
       EXPECT_TRUE(fake_ble_scanner()->HasScanFilter(BleScanner::ScanFilter(
@@ -578,9 +576,7 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
   void SimulateSecureChannelDisconnection(
       const std::string& remote_device_id,
       bool fail_during_authentication,
-      cryptauth::FakeSecureChannel* fake_secure_channel,
-      size_t num_initiator_attempts_canceled_from_disconnection = 0u,
-      size_t num_listener_attempts_canceled_from_disconnection = 0u) {
+      cryptauth::FakeSecureChannel* fake_secure_channel) {
     size_t num_ble_initiator_failures_before_call =
         ble_initiator_failures_.size();
     size_t num_ble_listener_failures_before_call =
@@ -599,12 +595,8 @@ 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 +
-        num_initiator_attempts_canceled_from_disconnection;
-    size_t listener_failure_index =
-        num_ble_listener_failures_before_call +
-        num_listener_attempts_canceled_from_disconnection;
+    size_t initiator_failures_index = num_ble_initiator_failures_before_call;
+    size_t listener_failure_index = num_ble_listener_failures_before_call;
     for (const auto& tuple :
          remote_device_id_to_metadata_map_[remote_device_id]) {
       switch (std::get<1>(tuple)) {
@@ -779,18 +771,16 @@ class SecureChannelBleConnectionManagerImplTest : public testing::Test {
     if (!should_cancel_attempt_on_failure)
       return;
 
-    CancelBleInitiatorConnectionAttempt(device_id_pair);
+    base::PostTask(FROM_HERE,
+                   base::BindOnce(&SecureChannelBleConnectionManagerImplTest::
+                                      CancelBleInitiatorConnectionAttempt,
+                                  base::Unretained(this), device_id_pair));
   }
 
   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;
-
-    CancelBleListenerConnectionAttempt(device_id_pair);
   }
 
   void SetInRemoteDeviceIdToMetadataMap(
@@ -944,7 +934,13 @@ 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,
@@ -966,25 +962,6 @@ 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(),
@@ -1027,8 +1004,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
   UpdateBleListenerConnectionPriority(pair, ConnectionPriority::kMedium,
                                       true /* expected_to_update_priority */);
   UpdateBleListenerConnectionPriority(pair, ConnectionPriority::kHigh,
@@ -1049,8 +1025,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1062,33 +1037,13 @@ 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 */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1106,8 +1061,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1195,11 +1149,9 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                       test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair_1, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
   AttemptBleListenerConnection(pair_2, ConnectionPriority::kMedium,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel_1 =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1227,11 +1179,9 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                       test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair_1, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
   AttemptBleListenerConnection(pair_2, ConnectionPriority::kMedium,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel_1 =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1254,8 +1204,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1286,8 +1235,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1306,8 +1254,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
 
   cryptauth::FakeSecureChannel* fake_secure_channel =
       SimulateConnectionEstablished(test_devices()[1],
@@ -1334,8 +1281,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest,
                     test_devices()[0].GetDeviceId());
 
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
+                               true /* expected_to_add_request */);
   AttemptBleInitiatorConnection(pair, ConnectionPriority::kMedium,
                                 true /* expected_to_add_request */,
                                 false /* should_cancel_attempt_on_failure */);
@@ -1359,9 +1305,7 @@ TEST_F(SecureChannelBleConnectionManagerImplTest, ConnectionMetrics) {
 
   // Start a connection attempt and advance the clock.
   AttemptBleListenerConnection(pair, ConnectionPriority::kLow,
-                               true /* expected_to_add_request */,
-                               false /* should_cancel_attempt_on_failure */);
-
+                               true /* expected_to_add_request */);
   test_clock()->Advance(kScanToAdvertisementTime);
 
   // Simulate a connection being established, then disconnected.