[CrOS PhoneHub] Call DisconnectFromEndpoint() when appropriate

When a NearbyConnectionBrokerImpl object is deleted, invoke
DisconnectFromEndpoint() if a connection had been established during the
lifetime of the object. Specifically, if OnConnectionInitiated() has
been called but OnDisconnected() has not, we disconnect.

Bug: 1149770, 1106937
Change-Id: I1c805475400d84d87092fbfafae20f0c48b7208c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545670Reviewed-by: James Vecore <vecore@google.com>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828827}

chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl.cc

@@ -28,6 +28,14 @@ using location::nearby::connections::mojom::PayloadPtr;
 using location::nearby::connections::mojom::PayloadTransferUpdatePtr;
 using location::nearby::connections::mojom::Status;
 
+void OnDisconnectFromEndpointResult(const std::string& endpoint_id,
+                                    Status status) {
+  if (status != Status::kSuccess) {
+    PA_LOG(WARNING) << "Failed to disconnect from endpoint with ID "
+                    << endpoint_id;
+  }
+}
+
 }  // namespace
 
 // static
@@ -85,7 +93,16 @@ NearbyConnectionBrokerImpl::NearbyConnectionBrokerImpl(
                      base::Unretained(this)));
 }
 
-NearbyConnectionBrokerImpl::~NearbyConnectionBrokerImpl() = default;
+NearbyConnectionBrokerImpl::~NearbyConnectionBrokerImpl() {
+  if (is_connection_active_) {
+    DCHECK(!remote_endpoint_id_.empty());
+    PA_LOG(VERBOSE) << "Disconnecting from endpoint with ID "
+                    << remote_endpoint_id_;
+    nearby_connections_->DisconnectFromEndpoint(
+        mojom::kServiceId, remote_endpoint_id_,
+        base::BindOnce(&OnDisconnectFromEndpointResult, remote_endpoint_id_));
+  }
+}
 
 void NearbyConnectionBrokerImpl::TransitionToStatus(
     ConnectionStatus connection_status) {
@@ -104,6 +121,7 @@ void NearbyConnectionBrokerImpl::OnEndpointDiscovered(
     DiscoveredEndpointInfoPtr info) {
   DCHECK_EQ(ConnectionStatus::kDiscoveringEndpoint, connection_status_);
 
+  DCHECK(!endpoint_id.empty());
   remote_endpoint_id_ = endpoint_id;
   TransitionToStatus(ConnectionStatus::kRequestingConnection);
 
@@ -196,6 +214,7 @@ void NearbyConnectionBrokerImpl::OnConnectionInitiated(
 
   DCHECK_EQ(ConnectionStatus::kRequestingConnection, connection_status_);
   TransitionToStatus(ConnectionStatus::kAcceptingConnection);
+  is_connection_active_ = true;
 
   nearby_connections_->AcceptConnection(
       mojom::kServiceId, remote_endpoint_id_,
@@ -241,6 +260,7 @@ void NearbyConnectionBrokerImpl::OnDisconnected(
   }
 
   PA_LOG(WARNING) << "Connection disconnected";
+  is_connection_active_ = false;
   TransitionToDisconnected();
 }
 

chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl.h

@@ -150,9 +150,13 @@ class NearbyConnectionBrokerImpl
 
   ConnectionStatus connection_status_ = ConnectionStatus::kUninitialized;
 
-  // Set once an endpoint is discovered.
+  // Starts empty, then set in OnEndpointDiscovered().
   std::string remote_endpoint_id_;
 
+  // Starts as false; set to true in OnConnectionInitiated() and back to false
+  // in OnDisconnected().
+  bool is_connection_active_ = false;
+
   base::WeakPtrFactory<NearbyConnectionBrokerImpl> weak_ptr_factory_{this};
 };
 

chrome/browser/chromeos/secure_channel/nearby_connection_broker_impl_unittest.cc

@@ -169,6 +169,13 @@ class NearbyConnectionBrokerImplTest : public testing::Test,
     NotifyConnectionAccepted();
   }
 
+  void InvokeDisconnectionCallback() {
+    base::RunLoop disconnect_run_loop;
+    on_disconnected_closure_ = disconnect_run_loop.QuitClosure();
+    connection_lifecycle_listener_->OnDisconnected(kEndpointId);
+    disconnect_run_loop.Run();
+  }
+
   void SendMessage(const std::string& message, bool success) {
     base::RunLoop send_message_run_loop;
     base::RunLoop send_message_response_run_loop;
@@ -256,6 +263,27 @@ class NearbyConnectionBrokerImplTest : public testing::Test,
     disconnect_run_loop.Run();
   }
 
+  void DeleteBroker(bool expected_to_disconnect) {
+    if (!expected_to_disconnect) {
+      EXPECT_CALL(mock_nearby_connections_, DisconnectFromEndpoint(_, _, _))
+          .Times(0);
+      broker_.reset();
+      return;
+    }
+
+    base::RunLoop run_loop;
+    EXPECT_CALL(mock_nearby_connections_, DisconnectFromEndpoint(_, _, _))
+        .WillOnce(Invoke(
+            [&](const std::string& service_id, const std::string& endpoint_id,
+                NearbyConnectionsMojom::StopDiscoveryCallback callback) {
+              std::move(callback).Run(Status::kSuccess);
+              run_loop.Quit();
+            }));
+
+    broker_.reset();
+    run_loop.Run();
+  }
+
  private:
   // mojom::NearbyMessageReceiver:
   void OnMessageReceived(const std::string& message) override {
@@ -296,25 +324,36 @@ TEST_F(NearbyConnectionBrokerImplTest, SendAndReceive) {
   SendMessage("test2", /*success=*/true);
   ReceiveMessage("test3");
   ReceiveMessage("test4");
+  DeleteBroker(/*expected_to_disconnect=*/true);
+}
+
+TEST_F(NearbyConnectionBrokerImplTest, DisconnectsUnexpectedly) {
+  SetUpFullConnection();
+  InvokeDisconnectionCallback();
+  DeleteBroker(/*expected_to_disconnect=*/false);
 }
 
 TEST_F(NearbyConnectionBrokerImplTest, ReceiveInvalidPayloadType) {
   SetUpFullConnection();
   ReceiveInvalidPayloadType();
+  DeleteBroker(/*expected_to_disconnect=*/true);
 }
 
 TEST_F(NearbyConnectionBrokerImplTest, FailToSend) {
   SetUpFullConnection();
   SendMessage("test", /*success=*/false);
+  DeleteBroker(/*expected_to_disconnect=*/true);
 }
 
 TEST_F(NearbyConnectionBrokerImplTest, FailDiscovery) {
   FailDiscovery();
+  DeleteBroker(/*expected_to_disconnect=*/false);
 }
 
 TEST_F(NearbyConnectionBrokerImplTest, FailRequestingConnection) {
   DiscoverEndpoint();
   InvokeRequestConnectionCallback(/*success=*/false);
+  DeleteBroker(/*expected_to_disconnect=*/false);
 }
 
 TEST_F(NearbyConnectionBrokerImplTest, FailAcceptingConnection) {
@@ -322,6 +361,7 @@ TEST_F(NearbyConnectionBrokerImplTest, FailAcceptingConnection) {
   InvokeRequestConnectionCallback(/*success=*/true);
   NotifyConnectionInitiated();
   InvokeAcceptConnectionCallback(/*success=*/false);
+  DeleteBroker(/*expected_to_disconnect=*/true);
 }
 
 }  // namespace secure_channel