arc: net: Prevent Service property update loops

To ensure that ARC would consistently get IP properties from the host
for all networks, change-id I24d26d6492f133e0c38949c8c3b4403687bfac21
(https://chromium-review.googlesource.com/c/chromium/src/+/2028881)
added a delayed property update request to shill when ArcNetHostImpl
cannot retrieve IP properties for a Shill connected Service.

However, one such request will trigger multiple property updates from
Shill and will retrigger the same codepath in ArcNetHostImpl that asked
for the property update request. This amplification can lead to an
infinite loop between Chrome and Shill incurring high CPU usage on the
system when ArcNetHostImpl will keep failing to find the IP properties
from the ONC properties or from the Device cached state.

This patch prevents this loop from happening at a high frequency by
making sure that at most one delayed property update request is
scheduled. A follow-up patch will add exponential backoff for the retry
delays.

It was reported in crbug/1054676 to happen consistently for the case of
native VPNs backed by a WiFi connection (i.e it does not happen with
VPNs backed by ethernet). The reason why ArcNetHostImpl fails to find IP
properties in that specific scenario is still unknown and will be
investigated separately.

BUG=1054676
TEST=Compiled, connected L2TP VPNs, observed that logs patterns seen in
crbug/1054676 do not happen and that cpu% of shill, chronos, and
system_server stays at normal levels.

Change-Id: I2a39ba9c8d14cf42b6b0dbdd10c885bf5be09cfa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078372
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746192}

components/arc/net/arc_net_host_impl.cc

@@ -36,10 +36,10 @@
 namespace {
 
 constexpr int kGetNetworksListLimit = 100;
-// Millisecond delay before asking for a network property update when no IP
+// Delay in millisecond before asking for a network property update when no IP
 // configuration can be retrieved for a network.
 constexpr base::TimeDelta kNetworkPropertyUpdateDelay =
-    base::TimeDelta::FromMilliseconds(3000);
+    base::TimeDelta::FromMilliseconds(5000);
 
 chromeos::NetworkStateHandler* GetStateHandler() {
   return chromeos::NetworkHandler::Get()->network_state_handler();
@@ -54,10 +54,6 @@ chromeos::NetworkConnectionHandler* GetNetworkConnectionHandler() {
   return chromeos::NetworkHandler::Get()->network_connection_handler();
 }
 
-void RequestUpdateForNetwork(const std::string& service_path) {
-  GetStateHandler()->RequestUpdateForNetwork(service_path);
-}
-
 bool IsDeviceOwner() {
   // Check whether the logged-in Chrome OS user is allowed to add or remove WiFi
   // networks. The user account state changes immediately after boot. There is a
@@ -369,18 +365,6 @@ arc::mojom::NetworkConfigurationPtr TranslateONCConfiguration(
     AddDeviceProperties(mojo.get(), network_state->device_path());
   }
 
-  if (mojo->ip_configs->empty()) {
-    LOG(WARNING) << "No IP configuration for " << network_state->path();
-    // A newly connected network may not immediately have any usable IP config
-    // object if IPv4 dhcp or IPv6 autoconf have not completed yet. Schedule
-    // with a few seconds delay a forced property update for that service to
-    // ensure the IP configuration is sent to ARC.
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE,
-        base::BindOnce(&RequestUpdateForNetwork, network_state->path()),
-        kNetworkPropertyUpdateDelay);
-  }
-
   return mojo;
 }
 
@@ -1119,9 +1103,42 @@ void ArcNetHostImpl::ActiveNetworksChanged(
 
   std::vector<arc::mojom::NetworkConfigurationPtr> network_configurations =
       TranslateNetworkStates(arc_vpn_service_path_, active_networks);
+
+  // A newly connected network may not immediately have any usable IP config
+  // object if IPv4 dhcp or IPv6 autoconf have not completed yet. Schedule
+  // with a few seconds delay a forced property update for that service to
+  // ensure the IP configuration is sent to ARC. Ensure that at most one such
+  // request is scheduled for a given service.
+  for (const auto& network : network_configurations) {
+    if (!network->ip_configs->empty())
+      continue;
+
+    if (!network->service_name)
+      continue;
+
+    const std::string& path = network->service_name.value();
+    if (pending_service_property_requests_.insert(path).second) {
+      LOG(WARNING) << "No IP configuration for " << path;
+      // TODO(hugobenichi): add exponential backoff for the case when IP
+      // configuration stays unavailable.
+      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE,
+          base::BindOnce(&ArcNetHostImpl::RequestUpdateForNetwork,
+                         weak_factory_.GetWeakPtr(), path),
+          kNetworkPropertyUpdateDelay);
+    }
+  }
+
   net_instance->ActiveNetworksChanged(std::move(network_configurations));
 }
 
+void ArcNetHostImpl::RequestUpdateForNetwork(const std::string& service_path) {
+  // TODO(hugobenichi): skip the request if the IP configuration for this
+  // service has been received since then and ARC has been notified about it.
+  pending_service_property_requests_.erase(service_path);
+  GetStateHandler()->RequestUpdateForNetwork(service_path);
+}
+
 void ArcNetHostImpl::NetworkListChanged() {
   // This is invoked any time the list of services is reordered or changed.
   // During the transition when a new service comes online, it will

components/arc/net/arc_net_host_impl.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 #include <memory>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -152,11 +153,17 @@ class ArcNetHostImpl : public KeyedService,
       const std::string& error_name,
       std::unique_ptr<base::DictionaryValue> error_data);
 
+  // Request properties of the Service corresponding to |service_path|.
+  void RequestUpdateForNetwork(const std::string& service_path);
+
   ArcBridgeService* const arc_bridge_service_;  // Owned by ArcServiceManager.
 
   // True if the chrome::NetworkStateHandler is currently being observed for
   // state changes.
   bool observing_network_state_ = false;
+  // Contains all service paths for which a property update request is
+  // currently scheduled.
+  std::set<std::string> pending_service_property_requests_;
 
   std::string cached_service_path_;
   std::string cached_guid_;