[CrOS Cellular] Fix crash when requesting Shill properties for stubs

NetworkStateHandler creates "stub" networks for eSIM profiles that are
tracked by Hermes but not Shill. In some cases, it was possible to
request properties for non-Shill networks, resulting in a crash.

This CL fixes this issue by ensuring that this never occurs for
non-Shill networks.

(cherry picked from commit fe52a00)

Fixed: 1197709
Change-Id: Ib91cfeb6d216d7859d86468ca25e70f22ec1af87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2818631
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#871542}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2822588
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4472@{#20}
Cr-Branched-From: 3d60439-refs/heads/master@{#870763}

chromeos/dbus/shill/fake_shill_service_client.cc

@@ -166,6 +166,8 @@ void FakeShillServiceClient::GetProperties(
     // Remove credentials that Shill wouldn't send.
     result_properties->RemoveKey(shill::kPassphraseProperty);
   } else {
+    DCHECK(!require_service_to_get_properties_);
+
     // This may happen if we remove services from the list.
     VLOG(2) << "Properties not found for: " << service_path.value();
   }
@@ -646,6 +648,11 @@ void FakeShillServiceClient::SetHoldBackServicePropertyUpdates(bool hold_back) {
                                                   std::move(property_update));
 }
 
+void FakeShillServiceClient::SetRequireServiceToGetProperties(
+    bool require_service_to_get_properties) {
+  require_service_to_get_properties_ = require_service_to_get_properties;
+}
+
 void FakeShillServiceClient::NotifyObserversPropertyChanged(
     const dbus::ObjectPath& service_path,
     const std::string& property) {

chromeos/dbus/shill/fake_shill_service_client.h

@@ -109,6 +109,8 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillServiceClient
   void SetConnectBehavior(const std::string& service_path,
                           const base::RepeatingClosure& behavior) override;
   void SetHoldBackServicePropertyUpdates(bool hold_back) override;
+  void SetRequireServiceToGetProperties(
+      bool require_service_to_get_properties) override;
 
  private:
   typedef base::ObserverList<ShillPropertyChangedObserver>::Unchecked
@@ -144,6 +146,10 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillServiceClient
   // |hold_back_service_property_updates_| was true.
   std::vector<base::OnceClosure> recorded_property_updates_;
 
+  // Whether or not this class should fail if GetProperties() is called for an
+  // unknown service.
+  bool require_service_to_get_properties_ = false;
+
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<FakeShillServiceClient> weak_ptr_factory_{this};

chromeos/dbus/shill/shill_service_client.h

@@ -106,6 +106,11 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillServiceClient {
     // |hold_back| == false, sends all recorded property updates.
     virtual void SetHoldBackServicePropertyUpdates(bool hold_back) = 0;
 
+    // Sets whether the fake should fail if requested to fetch properties for a
+    // service that is not known by Shill.
+    virtual void SetRequireServiceToGetProperties(
+        bool require_service_to_get_properties) = 0;
+
    protected:
     virtual ~TestInterface() {}
   };

chromeos/network/network_state_handler.cc

@@ -1096,8 +1096,8 @@ void NetworkStateHandler::RequestUpdateForNetwork(
     const std::string& service_path) {
   NetworkState* network = GetModifiableNetworkState(service_path);
   if (network) {
-    // Tether networks are not managed by Shill; do not request properties.
-    if (network->type() == kTypeTether)
+    // Do not request properties for networks which are not backed by Shill.
+    if (network->IsNonProfileType())
       return;
     // Do not request properties if a condition has already triggered a request.
     if (network->update_requested())
@@ -1308,6 +1308,11 @@ void NetworkStateHandler::ProfileListChanged(const base::Value& profile_list) {
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
+
+    // Do not request properties for networks which are not backed by Shill.
+    if (network->IsNonProfileType())
+      continue;
+
     shill_property_handler_->RequestProperties(
         ManagedState::MANAGED_TYPE_NETWORK, network->path());
   }

chromeos/network/network_state_handler.h

@@ -515,6 +515,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkStateHandler
   FRIEND_TEST_ALL_PREFIXES(NetworkStateHandlerTest, BlockedByPolicyOnlyManaged);
   FRIEND_TEST_ALL_PREFIXES(NetworkStateHandlerTest,
                            BlockedByPolicyOnlyManagedIfAvailable);
+  FRIEND_TEST_ALL_PREFIXES(NetworkStateHandlerTest, SyncStubCellularNetworks);
 
   // Implementation for GetNetworkListByType and GetActiveNetworkListByType.
   void GetNetworkListByTypeImpl(const NetworkTypePattern& type,

chromeos/network/network_state_handler_unittest.cc

@@ -2212,6 +2212,14 @@ TEST_F(NetworkStateHandlerTest, SyncStubCellularNetworks) {
   EXPECT_EQ(kStubCellularIccid, cellular->iccid());
   EXPECT_EQ(1u, test_observer_->network_list_changed_count());
 
+  // Set the test to fail if properties are requested from a service not backed
+  // by Shill, then update the profiles property and very that no test failure
+  // occurs. This verifies that stub networks do not have properties requested
+  // on profile change.
+  service_test_->SetRequireServiceToGetProperties(true);
+  network_state_handler_->shill_property_handler_->OnPropertyChanged(
+      shill::kProfilesProperty, base::Value(base::Value::Type::LIST));
+
   // Verify that StubCellularNetworksProvider can remove existing
   // networks.
   test_observer_->reset_change_counts();