Wi-Fi Sync: Ensure profile networks are loaded before initializing

This CL:
1. Does not initialize the network cache until the user's networks
have been loaded.
2. Queues calls to GetAllSyncableNetworks until the network cache
has been loaded

Bug: 1154341
Change-Id: I158cb29109c9f8de52ff09f1c82866931e371459
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600119
Commit-Queue: Claude van der Merwe <cvandermerwe@google.com>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841610}

chromeos/components/sync_wifi/local_network_collector_impl.cc

@@ -53,6 +53,13 @@ LocalNetworkCollectorImpl::~LocalNetworkCollectorImpl() = default;
 
 void LocalNetworkCollectorImpl::GetAllSyncableNetworks(
     ProtoListCallback callback) {
+  if (!is_mojo_networks_loaded_) {
+    after_networks_are_loaded_callback_queue_.push(
+        base::BindOnce(&LocalNetworkCollectorImpl::GetAllSyncableNetworks,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+    return;
+  }
+
   std::string request_guid = InitializeRequest();
   request_guid_to_list_callback_[request_guid] = std::move(callback);
 
@@ -273,6 +280,13 @@ void LocalNetworkCollectorImpl::OnRequestFinished(
 }
 
 void LocalNetworkCollectorImpl::OnNetworkStateListChanged() {
+  if (!NetworkHandler::Get()
+           ->network_state_handler()
+           ->IsProfileNetworksLoaded()) {
+    is_mojo_networks_loaded_ = false;
+    return;
+  }
+
   cros_network_config_->GetNetworkStateList(
       network_config::mojom::NetworkFilter::New(
           network_config::mojom::FilterType::kConfigured,
@@ -285,6 +299,11 @@ void LocalNetworkCollectorImpl::OnNetworkStateListChanged() {
 void LocalNetworkCollectorImpl::OnGetNetworkList(
     std::vector<network_config::mojom::NetworkStatePropertiesPtr> networks) {
   mojo_networks_ = std::move(networks);
+  is_mojo_networks_loaded_ = true;
+  while (!after_networks_are_loaded_callback_queue_.empty()) {
+    std::move(after_networks_are_loaded_callback_queue_.front()).Run();
+    after_networks_are_loaded_callback_queue_.pop();
+  }
 }
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/local_network_collector_impl.h

@@ -113,6 +113,9 @@ class LocalNetworkCollectorImpl
       request_guid_to_in_flight_networks_;
   base::flat_map<std::string, ProtoCallback> request_guid_to_single_callback_;
   base::flat_map<std::string, ProtoListCallback> request_guid_to_list_callback_;
+  base::queue<base::OnceCallback<void()>>
+      after_networks_are_loaded_callback_queue_;
+  bool is_mojo_networks_loaded_ = false;
 
   base::WeakPtrFactory<LocalNetworkCollectorImpl> weak_ptr_factory_{this};
 };

chromeos/components/sync_wifi/local_network_collector_impl_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/task_environment.h"
 #include "chromeos/components/sync_wifi/local_network_collector.h"
 #include "chromeos/components/sync_wifi/local_network_collector_impl.h"
@@ -63,6 +64,7 @@ class LocalNetworkCollectorImplTest : public testing::Test {
         remote_cros_network_config_.get());
     local_network_collector_->SetNetworkMetadataStore(
         NetworkHandler::Get()->network_metadata_store()->GetWeakPtr());
+    on_get_all_syncable_networks_count_ = 0;
   }
 
   void TearDown() override {
@@ -77,6 +79,7 @@ class LocalNetworkCollectorImplTest : public testing::Test {
     for (int i = 0; i < (int)result.size(); i++) {
       EXPECT_EQ(expected[i], DecodeHexString(result[i].hex_ssid()));
     }
+    on_get_all_syncable_networks_count_++;
   }
 
   void OnGetSyncableNetwork(
@@ -103,6 +106,9 @@ class LocalNetworkCollectorImplTest : public testing::Test {
   }
 
   NetworkTestHelper* helper() { return local_test_helper_.get(); }
+  size_t on_get_all_syncable_networks_count() {
+    return on_get_all_syncable_networks_count_;
+  }
 
  private:
   base::test::TaskEnvironment task_environment_;
@@ -112,6 +118,8 @@ class LocalNetworkCollectorImplTest : public testing::Test {
   mojo::Remote<chromeos::network_config::mojom::CrosNetworkConfig>
       remote_cros_network_config_;
 
+  size_t on_get_all_syncable_networks_count_;
+
   DISALLOW_COPY_AND_ASSIGN(LocalNetworkCollectorImplTest);
 };
 
@@ -129,6 +137,41 @@ TEST_F(LocalNetworkCollectorImplTest, TestGetAllSyncableNetworks) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_F(LocalNetworkCollectorImplTest,
+       TestGetAllSyncableNetworks_MojoNetworksUnitializedThenInitialized) {
+  const std::string kSharedUserDirectory =
+      NetworkProfileHandler::GetSharedProfilePath();
+  helper()->network_state_test_helper()->ClearProfiles();
+  // Add back shared profile path to simulate user on the login screen.
+  helper()->network_state_test_helper()->profile_test()->AddProfile(
+      /*profile_path=*/kSharedUserDirectory, /*userhash=*/std::string());
+  base::RunLoop().RunUntilIdle();
+
+  size_t on_get_all_syncable_networks_count_before =
+      on_get_all_syncable_networks_count();
+  local_network_collector()->GetAllSyncableNetworks(base::DoNothing());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(on_get_all_syncable_networks_count_before,
+            on_get_all_syncable_networks_count());
+
+  // Log users back in.
+  const char* kProfilePathUser =
+      helper()->network_state_test_helper()->ProfilePathUser();
+  const char* kUserHash = helper()->network_state_test_helper()->UserHash();
+  helper()->network_state_test_helper()->profile_test()->AddProfile(
+      /*profile_path=*/kProfilePathUser, /*userhash=*/kUserHash);
+  helper()->network_state_test_helper()->profile_test()->AddProfile(
+      /*profile_path=*/kUserHash, /*userhash=*/std::string());
+
+  std::vector<std::string> expected;
+  local_network_collector()->GetAllSyncableNetworks(
+      base::BindOnce(&LocalNetworkCollectorImplTest::OnGetAllSyncableNetworks,
+                     base::Unretained(this), expected));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(on_get_all_syncable_networks_count_before + 1,
+            on_get_all_syncable_networks_count());
+}
+
 TEST_F(LocalNetworkCollectorImplTest,
        TestGetAllSyncableNetworks_WithFiltering) {
   helper()->ConfigureWiFiNetwork(kFredSsid, /*is_secured=*/true,

chromeos/components/sync_wifi/network_test_helper.cc

@@ -76,7 +76,8 @@ void NetworkTestHelper::SetUp() {
   NetworkHandler::Get()->InitializePrefServices(&user_prefs_, &local_state_);
   network_state_helper_->ResetDevicesAndServices();
   network_state_helper_->profile_test()->AddProfile(
-      network_state_helper_->UserHash(), std::string());
+      /*profile_path=*/network_state_helper_->UserHash(),
+      /*userhash=*/std::string());
 
   base::RunLoop().RunUntilIdle();
 }

chromeos/dbus/shill/fake_shill_manager_client.cc

@@ -1148,6 +1148,14 @@ base::Value FakeShillManagerClient::GetEnabledServiceList() const {
   return new_service_list;
 }
 
+void FakeShillManagerClient::ClearProfiles() {
+  if (GetListProperty(shill::kProfilesProperty)->empty()) {
+    return;
+  }
+  GetListProperty(shill::kProfilesProperty)->ClearList();
+  CallNotifyObserversPropertyChanged(shill::kProfilesProperty);
+}
+
 void FakeShillManagerClient::ScanCompleted(const std::string& device_path) {
   VLOG(1) << "ScanCompleted: " << device_path;
   if (!device_path.empty()) {

chromeos/dbus/shill/fake_shill_manager_client.h

@@ -98,6 +98,7 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillManagerClient
   void SetSimulateConfigurationResult(
       FakeShillSimulatedResult configuration_result) override;
   base::Value GetEnabledServiceList() const override;
+  void ClearProfiles() override;
 
   // Constants used for testing.
   static const char kFakeEthernetNetworkGuid[];

chromeos/dbus/shill/shill_manager_client.h

@@ -121,6 +121,9 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillManagerClient {
     virtual void SetSimulateConfigurationResult(
         FakeShillSimulatedResult configuration_result) = 0;
 
+    // Clears profile list.
+    virtual void ClearProfiles() = 0;
+
    protected:
     virtual ~TestInterface() {}
   };

chromeos/network/network_state_handler.cc

@@ -200,6 +200,10 @@ const NetworkState* NetworkStateHandler::GetAvailableManagedWifiNetwork()
   return GetNetworkState(available_managed_network_path);
 }
 
+bool NetworkStateHandler::IsProfileNetworksLoaded() {
+  return is_profile_networks_loaded_;
+}
+
 bool NetworkStateHandler::OnlyManagedWifiNetworksAllowed() const {
   return allow_only_policy_networks_to_connect_ ||
          (allow_only_policy_networks_to_connect_if_available_ &&
@@ -1290,8 +1294,9 @@ void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
   }
 }
 
-void NetworkStateHandler::ProfileListChanged() {
+void NetworkStateHandler::ProfileListChanged(const base::Value& profile_list) {
   NET_LOG(EVENT) << "ProfileListChanged. Re-Requesting Network Properties";
+  ProcessIsUserLoggedIn(profile_list);
   for (ManagedStateList::iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
@@ -1568,6 +1573,10 @@ void NetworkStateHandler::ManagedStateListChanged(
       NotifyIfActiveNetworksChanged();
       NotifyNetworkListChanged();
       UpdateManagedWifiNetworkAvailable();
+      // ManagedStateListChanged only gets executed if all pending updates have
+      // completed. Profile networks are loaded if a user is logged in and all
+      // pending updates are complete.
+      is_profile_networks_loaded_ = is_user_logged_in_;
       return;
     case ManagedState::MANAGED_TYPE_DEVICE:
       std::string devices;
@@ -2110,4 +2119,14 @@ void NetworkStateHandler::SetDefaultNetworkValues(const std::string& path,
   default_network_is_metered_ = metered;
 }
 
+void NetworkStateHandler::ProcessIsUserLoggedIn(
+    const base::Value& profile_list) {
+  if (!profile_list.is_list()) {
+    return;
+  }
+  // The profile list contains the shared profile on the login screen. Once the
+  // user is logged in there is more than one profile in the profile list.
+  is_user_logged_in_ = profile_list.GetList().size() > 1;
+}
+
 }  // namespace chromeos

chromeos/network/network_state_handler.h

@@ -402,6 +402,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   // available.
   const NetworkState* GetAvailableManagedWifiNetwork() const;
 
+  // Returns true if a user is logged in and the networks for the logged in user
+  // have been loaded.
+  bool IsProfileNetworksLoaded();
+
   // Returns true if the AllowOnlyPolicyNetworksToConnect policy is enabled or
   // if the AllowOnlyPolicyNetworksToConnectIfAvailable policy is enabled and
   // there is a managed wifi network available.
@@ -431,7 +435,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
 
   // The list of profiles changed (i.e. a user has logged in). Re-request
   // properties for all services since they may have changed.
-  void ProfileListChanged() override;
+  void ProfileListChanged(const base::Value& profile_list) override;
 
   // Parses the properties for the network service or device. Mostly calls
   // managed->PropertyChanged(key, value) for each dictionary entry.
@@ -634,6 +638,9 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   // Metered.
   void SetDefaultNetworkValues(const std::string& path, bool metered);
 
+  // Determines whether the user is logged in and sets |is_user_logged_in_|.
+  void ProcessIsUserLoggedIn(const base::Value& profile_list);
+
   // Shill property handler instance, owned by this class.
   std::unique_ptr<internal::ShillPropertyHandler> shill_property_handler_;
 
@@ -703,6 +710,12 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   bool allow_only_policy_networks_to_connect_if_available_ = false;
   std::vector<std::string> blocked_hex_ssids_;
 
+  // After login the user's saved networks get updated asynchronously from
+  // shill. These variables indicate whether a user is logged in, and if the
+  // user's saved networks are done updating.
+  bool is_profile_networks_loaded_ = false;
+  bool is_user_logged_in_ = false;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandler);

chromeos/network/network_state_handler_unittest.cc

@@ -2269,4 +2269,40 @@ TEST_F(NetworkStateHandlerTest, Hostname) {
   EXPECT_TRUE(test_observer_->hostname().empty());
 }
 
+TEST_F(NetworkStateHandlerTest, IsProfileNetworksLoaded) {
+  // Ensure that the network list is the expected size.
+  const size_t kNumShillManagerClientStubImplServices = 4;
+  EXPECT_EQ(kNumShillManagerClientStubImplServices,
+            test_observer_->network_count());
+  // Add a non-visible network to a private profile.
+  const std::string profile = "/profile/profile1";
+  const std::string wifi_favorite_path = "/service/wifi_faviorite";
+  service_test_->AddService(wifi_favorite_path, "wifi_faviorite_guid",
+                            "wifi_faviorite", shill::kTypeWifi,
+                            shill::kStateIdle, false /* add_to_visible */);
+
+  NetworkStateHandler::NetworkStateList networks;
+
+  // Get networks before user is logged in.
+  network_state_handler_->GetNetworkListByType(
+      NetworkTypePattern::Default(), false /* configured_only */,
+      false /* visible_only */, 0 /* no limit */, &networks);
+  EXPECT_EQ(4u, networks.size());
+  EXPECT_FALSE(network_state_handler_->IsProfileNetworksLoaded());
+
+  // Simulate a user logging in.
+  profile_test_->AddProfile(profile, "" /* userhash */);
+  EXPECT_TRUE(profile_test_->AddService(profile, wifi_favorite_path));
+  // Wait for login to complete and profile networks to get loaded
+  UpdateManagerProperties();
+  // Get networks after user is logged in.
+  network_state_handler_->GetNetworkListByType(
+      NetworkTypePattern::Default(), false /* configured_only */,
+      false /* visible_only */, 0 /* no limit */, &networks);
+  base::RunLoop().RunUntilIdle();
+  // User is logged in and private network has been loaded.
+  EXPECT_TRUE(network_state_handler_->IsProfileNetworksLoaded());
+  EXPECT_EQ(5u, networks.size());
+}
+
 }  // namespace chromeos

chromeos/network/network_state_test_helper.cc

@@ -97,6 +97,11 @@ void NetworkStateTestHelper::ClearServices() {
   base::RunLoop().RunUntilIdle();
 }
 
+void NetworkStateTestHelper::ClearProfiles() {
+  profile_test_->ClearProfiles();
+  manager_test_->ClearProfiles();
+}
+
 void NetworkStateTestHelper::AddDevice(const std::string& device_path,
                                        const std::string& type,
                                        const std::string& name) {

chromeos/network/network_state_test_helper.h

@@ -46,6 +46,9 @@ class NetworkStateTestHelper {
   // Clears any fake services.
   void ClearServices();
 
+  // Clears the profile list.
+  void ClearProfiles();
+
   // Calls ShillDeviceClient::TestInterface::AddDevice and sets update_received
   // on the DeviceState.
   void AddDevice(const std::string& device_path,

chromeos/network/shill_property_handler.cc

@@ -378,7 +378,7 @@ void ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
     if (value.GetAsString(&prohibited_technologies))
       UpdateProhibitedTechnologies(prohibited_technologies);
   } else if (key == shill::kProfilesProperty) {
-    listener_->ProfileListChanged();
+    listener_->ProfileListChanged(value);
   } else if (key == shill::kCheckPortalListProperty) {
     std::string check_portal_list;
     if (value.GetAsString(&check_portal_list))

chromeos/network/shill_property_handler.h

@@ -56,7 +56,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ShillPropertyHandler
         const base::Value& properties) = 0;
 
     // Called when the list of profiles changes.
-    virtual void ProfileListChanged() = 0;
+    virtual void ProfileListChanged(const base::Value& profile_list) = 0;
 
     // Called when a property for a watched network service has changed.
     virtual void UpdateNetworkServiceProperty(

chromeos/network/shill_property_handler_unittest.cc

@@ -55,7 +55,12 @@ class TestListener : public internal::ShillPropertyHandler::Listener {
     initial_property_updates(GetTypeString(type))[path] += 1;
   }
 
-  void ProfileListChanged() override {}
+  void ProfileListChanged(const base::Value& profile_list) override {
+    if (!profile_list.is_list()) {
+      return;
+    }
+    profile_list_size_ = profile_list.GetList().size();
+  }
 
   void UpdateNetworkServiceProperty(const std::string& service_path,
                                     const std::string& key,
@@ -113,6 +118,7 @@ class TestListener : public internal::ShillPropertyHandler::Listener {
   }
   std::string hostname() { return hostname_; }
   int errors() { return errors_; }
+  int profile_list_size() { return profile_list_size_; }
 
  private:
   std::string GetTypeString(ManagedState::ManagedType type) {
@@ -159,6 +165,7 @@ class TestListener : public internal::ShillPropertyHandler::Listener {
   int technology_list_updates_;
   std::string hostname_;
   int errors_;
+  int profile_list_size_;
 };
 
 }  // namespace
@@ -292,6 +299,17 @@ TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerStub) {
   EXPECT_EQ(0, listener_->errors());
 }
 
+TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerProfileListChanged) {
+  EXPECT_EQ(1, listener_->profile_list_size());
+
+  const char kMountedUserDirectory[] = "/profile/chronos/shill";
+  // Simulate a user logging in. When a user logs in the mounted user directory
+  // path is added to the list of profile paths.
+  profile_test_->AddProfile(kMountedUserDirectory, /*user_hash=*/"");
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, listener_->profile_list_size());
+}
+
 TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerHostnameChanged) {
   EXPECT_TRUE(listener_->hostname().empty());
   const char kTestHostname[] = "Test Hostname";