[CrOS PhoneHub] Properly update tether status in Disconnect() case.

When OnActiveNetworksChanged() is called shortly after starting a
disconnect to a ConnectionStateType::kConnecting |tether_network_|, the
|tether_network_|'s ConnectionStateType may still remain in the
ConnectionStateType::kConnecting state. This may happen if on the phone,
hotspot is off but bluetooth tethering is on, and a connection attempt
is made, but the user does not acknowledge the notification to connect
on their phone, and subsequently decides to disconnect while
|tether_network_|'s ConnectionStateType is still
ConnectionStateType::kConnecting.

By the time OnDisconnectCompleted() is called, the connection state is
properly updated to ConnectionStateType::kDisconnected, so a fetch may
be necessary to promptly update |tether_network_|, as neither
OnActiveNetworksChanged() nor OnNetworkStateListChanged() may be
called shortly afterwards with the latest network information.

Fixed: 1143041
Bug: 1106937
Change-Id: Ieb5c6071cc661fed1a3310a24c57b1927348f9d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2510876
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823953}

chromeos/components/phonehub/BUILD.gn

@@ -197,6 +197,7 @@ source_set("unit_tests") {
     "//chromeos/services/device_sync/public/cpp:test_support",
     "//chromeos/services/multidevice_setup/public/cpp",
     "//chromeos/services/multidevice_setup/public/cpp:test_support",
+    "//chromeos/services/network_config:in_process_instance",
     "//chromeos/services/network_config/public/cpp:test_support",
     "//chromeos/services/secure_channel/public/cpp/client:test_support",
     "//components/prefs:test_support",

chromeos/components/phonehub/tether_controller_impl.cc

@@ -44,6 +44,13 @@ void TetherControllerImpl::TetherNetworkConnector::StartDisconnect(
   cros_network_config_->StartDisconnect(guid, std::move(callback));
 }
 
+void TetherControllerImpl::TetherNetworkConnector::GetNetworkStateList(
+    network_config::mojom::NetworkFilterPtr filter,
+    GetNetworkStateListCallback callback) {
+  cros_network_config_->GetNetworkStateList(std::move(filter),
+                                            std::move(callback));
+}
+
 TetherControllerImpl::TetherControllerImpl(
     PhoneModel* phone_model,
     MultiDeviceSetupClient* multidevice_setup_client)
@@ -197,7 +204,7 @@ void TetherControllerImpl::OnStartConnectCompleted(StartConnectResult result,
     return;
   }
 
-  // Note that OnVisibleTetherNetworkFetched() has not called
+  // Note that OnVisibleTetherNetworkFetched() may not have called
   // SetConnectDisconnectStatus() with kIdle at this point, so this should go
   // ahead and do it.
   SetConnectDisconnectStatus(ConnectDisconnectStatus::kIdle);
@@ -236,6 +243,14 @@ void TetherControllerImpl::OnDisconnectCompleted(bool success) {
 
   SetConnectDisconnectStatus(ConnectDisconnectStatus::kIdle);
 
+  // Fetch the tether network and its updated connection state, if it exists.
+  // By the time OnDisconnectCompleted() is called, the connection state is
+  // properly updated to ConnectionStateType::kDisconnected, so a fetch may be
+  // necessary to promptly update |tether_network_|, as neither
+  // OnActiveNetworksChanged() nor OnNetworkStateListChanged() may be called
+  // shortly afterwards with the latest network information.
+  FetchVisibleTetherNetwork();
+
   if (!success)
     PA_LOG(WARNING) << "Failed to disconnect tether network";
 }
@@ -243,9 +258,19 @@ void TetherControllerImpl::OnDisconnectCompleted(bool success) {
 void TetherControllerImpl::OnActiveNetworksChanged(
     std::vector<network_config::mojom::NetworkStatePropertiesPtr> networks) {
   // Active networks either changed externally (e.g via OS Settings or a new
-  // actve network added), or as a result of a call to AttemptConnection().
-  // This is needed for the case of ConnectionStateType::kConnecting in
-  // ComputeStatus().
+  // actve network added), or as a result of a call to AttemptConnection() or
+  // Disconnect(). This is needed for the case of
+  // ConnectionStateType::kConnecting in ComputeStatus().
+  //
+  // Note: When OnActiveNetworksChanged() is called shortly after starting a
+  // disconnect to a ConnectionStateType::kConnecting |tether_network_|, the
+  // |tether_network_|'s ConnectionStateType may still remain in the
+  // ConnectionStateType::kConnecting state. This may happen if on the phone,
+  // hotspot is off but bluetooth tethering is on, and a connection attempt is
+  // made, but the user does not acknowledge the notification to connect on
+  // their phone, and subsequently decides to disconnect while
+  // |tether_network_|'s ConnectionStateType is still
+  // ConnectionStateType::kConnecting.
   FetchVisibleTetherNetwork();
 }
 
@@ -292,7 +317,7 @@ void TetherControllerImpl::OnGetDeviceStateList(
 
 void TetherControllerImpl::FetchVisibleTetherNetwork() {
   // Return the connected, connecting, or connectable Tether network.
-  cros_network_config_->GetNetworkStateList(
+  connector_->GetNetworkStateList(
       network_config::mojom::NetworkFilter::New(FilterType::kVisible,
                                                 NetworkType::kTether,
                                                 /*limit=*/0),

chromeos/components/phonehub/tether_controller_impl.h

@@ -80,8 +80,8 @@ class TetherControllerImpl
     kDisconnecting = 4,
   };
 
-  // Connector that uses CrosNetworkConfig to connect and disconnect. This class
-  // is used for testing purposes.
+  // Connector that uses CrosNetworkConfig to connect, disconnect, and get the
+  // network state list. This class is used for testing purposes.
   class TetherNetworkConnector {
    public:
     using StartConnectCallback = base::OnceCallback<void(
@@ -90,6 +90,9 @@ class TetherControllerImpl
 
     using StartDisconnectCallback = base::OnceCallback<void(bool)>;
 
+    using GetNetworkStateListCallback = base::OnceCallback<void(
+        std::vector<network_config::mojom::NetworkStatePropertiesPtr>)>;
+
     TetherNetworkConnector();
     TetherNetworkConnector(const TetherNetworkConnector&) = delete;
     TetherNetworkConnector& operator=(const TetherNetworkConnector&) = delete;
@@ -99,6 +102,9 @@ class TetherControllerImpl
                               StartConnectCallback callback);
     virtual void StartDisconnect(const std::string& guid,
                                  StartDisconnectCallback callback);
+    virtual void GetNetworkStateList(
+        network_config::mojom::NetworkFilterPtr filter,
+        GetNetworkStateListCallback callback);
 
    private:
     mojo::Remote<network_config::mojom::CrosNetworkConfig> cros_network_config_;

chromeos/components/phonehub/tether_controller_impl_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/memory/weak_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/task_environment.h"
@@ -14,6 +15,7 @@
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_state_test_helper.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
+#include "chromeos/services/network_config/in_process_instance.h"
 #include "chromeos/services/network_config/public/cpp/cros_network_config_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -81,7 +83,10 @@ class TetherControllerImplTest : public testing::Test {
   class FakeTetherNetworkConnector
       : public TetherControllerImpl::TetherNetworkConnector {
    public:
-    FakeTetherNetworkConnector() = default;
+    FakeTetherNetworkConnector() {
+      chromeos::network_config::BindToInProcessInstance(
+          cros_network_config_.BindNewPipeAndPassReceiver());
+    }
     ~FakeTetherNetworkConnector() override = default;
 
     void StartConnect(const std::string& guid,
@@ -94,6 +99,31 @@ class TetherControllerImplTest : public testing::Test {
       start_disconnect_callback_ = std::move(callback);
     }
 
+    void GetNetworkStateList(network_config::mojom::NetworkFilterPtr filter,
+                             GetNetworkStateListCallback callback) override {
+      cros_network_config_->GetNetworkStateList(
+          std::move(filter),
+          base::BindOnce(
+              &FakeTetherNetworkConnector::OnVisibleTetherNetworkFetched,
+              weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+    }
+
+    void SetNextConnectionStateType(ConnectionStateType connection_state) {
+      connection_state_ = connection_state;
+    }
+
+    void OnVisibleTetherNetworkFetched(
+        GetNetworkStateListCallback callback,
+        std::vector<network_config::mojom::NetworkStatePropertiesPtr>
+            networks) {
+      if (connection_state_.has_value() && networks.size() == 1) {
+        networks[0]->connection_state = *connection_state_;
+        connection_state_ = base::nullopt;
+      }
+
+      std::move(callback).Run(std::move(networks));
+    }
+
     bool DoesPendingStartConnectCallbackExist() {
       return !start_connect_callback_.is_null();
     }
@@ -102,19 +132,28 @@ class TetherControllerImplTest : public testing::Test {
       return !start_disconnect_callback_.is_null();
     }
 
-    void InvokeStartConnectCallback(network_config::mojom::StartConnectResult
-                                        result = StartConnectResult::kSuccess,
+    void InvokeStartConnectCallbackWithFakeResult(
+        network_config::mojom::StartConnectResult result =
+            StartConnectResult::kSuccess,
         const std::string& message = "") {
       std::move(start_connect_callback_).Run(result, message);
     }
 
-    void InvokeDisconnectCallback(bool success = true) {
+    void InvokeDisconnectCallbackWithRealResults(std::string service_path) {
+      cros_network_config_->StartDisconnect(
+          service_path, std::move(start_disconnect_callback_));
+    }
+
+    void InvokeDisconnectCallbackWithFakeParams(bool success = true) {
       std::move(start_disconnect_callback_).Run(success);
     }
 
    private:
+    mojo::Remote<network_config::mojom::CrosNetworkConfig> cros_network_config_;
+    base::Optional<ConnectionStateType> connection_state_;
     StartConnectCallback start_connect_callback_;
     StartDisconnectCallback start_disconnect_callback_;
+    base::WeakPtrFactory<FakeTetherNetworkConnector> weak_ptr_factory_{this};
   };
 
   // testing::Test:
@@ -210,6 +249,12 @@ class TetherControllerImplTest : public testing::Test {
                                                    feature_state);
   }
 
+  void InvokeDisconnectCallbackWithRealResults() {
+    fake_tether_network_connector()->InvokeDisconnectCallbackWithRealResults(
+        service_path_);
+    base::RunLoop().RunUntilIdle();
+  }
+
   void InvokePendingSetFeatureEnabledStateCallback(
       bool success,
       bool expected_enabled = true) {
@@ -245,6 +290,36 @@ class TetherControllerImplTest : public testing::Test {
   std::unique_ptr<TetherControllerImpl> controller_;
 };
 
+TEST_F(TetherControllerImplTest,
+       DisconnectCompletesAfterOnActiveNetworksChanged) {
+  SetMultideviceSetupFeatureState(FeatureState::kEnabledByUser);
+
+  EnableTetherDevice();
+  AddVisibleTetherNetwork();
+
+  // Disconnect from a connecting state.
+  AttemptConnection();
+  EXPECT_EQ(GetStatus(), TetherController::Status::kConnecting);
+
+  SetTetherNetworkStateConnecting();
+  Disconnect();
+
+  // Simulate OnActiveNetworksChanged() being called after Disconnect()
+  // is requested when Bluetooth is on but hotspot is off, yielding a
+  // ConnectionStateType::kConnecting tether network instead of a
+  // ConnectionStateType::kDisconnected network.
+  fake_tether_network_connector()->SetNextConnectionStateType(
+      network_config::mojom::ConnectionStateType::kConnecting);
+  SetTetherNetworkStateDisconnected();
+  EXPECT_EQ(GetStatus(), TetherController::Status::kConnecting);
+
+  // Upon invoking the Disconnect callback, a refetch occurs.
+  EXPECT_TRUE(
+      fake_tether_network_connector()->DoesPendingDisconnectCallbackExist());
+  InvokeDisconnectCallbackWithRealResults();
+  EXPECT_EQ(GetStatus(), TetherController::Status::kConnectionAvailable);
+}
+
 TEST_F(TetherControllerImplTest, ExternalTetherChangesReflectToStatus) {
   EXPECT_EQ(GetStatus(), TetherController::Status::kIneligibleForFeature);
   SetMultideviceSetupFeatureState(FeatureState::kEnabledByUser);
@@ -308,7 +383,7 @@ TEST_F(TetherControllerImplTest, AttemptConnectDisconnect) {
 
   // Upon completing the connection, the status should no longer be connecting.
   EXPECT_EQ(GetStatus(), TetherController::Status::kConnecting);
-  fake_tether_network_connector()->InvokeStartConnectCallback();
+  fake_tether_network_connector()->InvokeStartConnectCallbackWithFakeResult();
   EXPECT_NE(GetStatus(), TetherController::Status::kConnecting);
 
   // Disconnect from a connected state.
@@ -316,7 +391,7 @@ TEST_F(TetherControllerImplTest, AttemptConnectDisconnect) {
   Disconnect();
   EXPECT_TRUE(
       fake_tether_network_connector()->DoesPendingDisconnectCallbackExist());
-  fake_tether_network_connector()->InvokeDisconnectCallback();
+  fake_tether_network_connector()->InvokeDisconnectCallbackWithFakeParams();
   SetTetherNetworkStateDisconnected();
 
   // Disconnect from a connecting state.
@@ -325,7 +400,7 @@ TEST_F(TetherControllerImplTest, AttemptConnectDisconnect) {
   Disconnect();
   EXPECT_TRUE(
       fake_tether_network_connector()->DoesPendingDisconnectCallbackExist());
-  fake_tether_network_connector()->InvokeDisconnectCallback();
+  fake_tether_network_connector()->InvokeDisconnectCallbackWithFakeParams();
 
   AttemptConnection();
   EXPECT_EQ(GetStatus(), TetherController::Status::kConnecting);
@@ -335,7 +410,7 @@ TEST_F(TetherControllerImplTest, AttemptConnectDisconnect) {
       fake_tether_network_connector()->DoesPendingDisconnectCallbackExist());
   AttemptConnection();
   EXPECT_EQ(GetStatus(), TetherController::Status::kConnecting);
-  fake_tether_network_connector()->InvokeDisconnectCallback();
+  fake_tether_network_connector()->InvokeDisconnectCallbackWithFakeParams();
   EXPECT_EQ(GetStatus(), TetherController::Status::kConnecting);
 
   // Disconnect from a disconnected state.