[CrOS Cellular] Add CellularInhibitor class

This class updates the "Inhibited" property of the Cellular device.
When some SIM-related operations are performed, properties of the
Cellular device can change to a temporary value and then change back.
To prevent churn in these properties, Shill provides the "Inhibited"
property to inhibit any scans.

This class is intended to be used when performing such actions to ensure
that these transient states never occur. Because this operation is
expected to be performed often, we encapsulate it within this class.

Note that this CL does not add any clients for the new class; this will
occur as part of follow-up CLs.

Bug: 1093185
Change-Id: I5776c5c4a47db3645951d639b2917d8c5769c45f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640314
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845823}

chromeos/network/BUILD.gn

@@ -38,6 +38,8 @@ component("network") {
   sources = [
     "auto_connect_handler.cc",
     "auto_connect_handler.h",
+    "cellular_inhibitor.cc",
+    "cellular_inhibitor.h",
     "cellular_metrics_logger.cc",
     "cellular_metrics_logger.h",
     "certificate_helper.cc",
@@ -238,6 +240,7 @@ source_set("unit_tests") {
   ]
   sources = [
     "auto_connect_handler_unittest.cc",
+    "cellular_inhibitor_unittest.cc",
     "cellular_metrics_logger_unittest.cc",
     "certificate_helper_unittest.cc",
     "client_cert_resolver_unittest.cc",

chromeos/network/cellular_inhibitor.cc

+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/network/cellular_inhibitor.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "chromeos/network/device_state.h"
+#include "chromeos/network/network_device_handler.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_type_pattern.h"
+#include "components/device_event_log/device_event_log.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+
+namespace chromeos {
+
+CellularInhibitor::CellularInhibitor() = default;
+
+CellularInhibitor::~CellularInhibitor() = default;
+
+void CellularInhibitor::Init(NetworkStateHandler* network_state_handler,
+                             NetworkDeviceHandler* network_device_handler) {
+  network_state_handler_ = network_state_handler;
+  network_device_handler_ = network_device_handler;
+}
+
+void CellularInhibitor::InhibitCellularScanning(SuccessCallback callback) {
+  SetInhibitProperty(/*new_inhibit_value=*/true, std::move(callback));
+}
+
+void CellularInhibitor::UninhibitCellularScanning(SuccessCallback callback) {
+  SetInhibitProperty(/*new_inhibit_value=*/false, std::move(callback));
+}
+
+const DeviceState* CellularInhibitor::GetCellularDevice() const {
+  return network_state_handler_->GetDeviceStateByType(
+      NetworkTypePattern::Cellular());
+}
+
+void CellularInhibitor::SetInhibitProperty(bool new_inhibit_value,
+                                           SuccessCallback callback) {
+  const DeviceState* cellular_device = GetCellularDevice();
+  if (!cellular_device) {
+    std::move(callback).Run(false);
+    return;
+  }
+
+  // If the new value is already set, return early.
+  if (cellular_device->inhibited() == new_inhibit_value) {
+    std::move(callback).Run(true);
+    return;
+  }
+
+  auto repeating_callback =
+      base::AdaptCallbackForRepeating(std::move(callback));
+  network_device_handler_->SetDeviceProperty(
+      cellular_device->path(), shill::kInhibitedProperty,
+      base::Value(new_inhibit_value),
+      base::BindOnce(&CellularInhibitor::OnSetPropertySuccess,
+                     weak_ptr_factory_.GetWeakPtr(), repeating_callback),
+      base::BindOnce(&CellularInhibitor::OnSetPropertyError,
+                     weak_ptr_factory_.GetWeakPtr(), repeating_callback,
+                     /*attempted_inhibit=*/new_inhibit_value));
+}
+
+void CellularInhibitor::OnSetPropertySuccess(
+    const base::RepeatingCallback<void(bool)>& success_callback) {
+  std::move(success_callback).Run(true);
+}
+
+void CellularInhibitor::OnSetPropertyError(
+    const base::RepeatingCallback<void(bool)>& success_callback,
+    bool attempted_inhibit,
+    const std::string& error_name,
+    std::unique_ptr<base::DictionaryValue> error_data) {
+  NET_LOG(ERROR) << (attempted_inhibit ? "Inhibit" : "Uninhibit")
+                 << "CellularScanning() failed: " << error_name;
+  std::move(success_callback).Run(false);
+}
+
+}  // namespace chromeos

chromeos/network/cellular_inhibitor.h

+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_NETWORK_CELLULAR_INHIBITOR_H_
+#define CHROMEOS_NETWORK_CELLULAR_INHIBITOR_H_
+
+#include "base/component_export.h"
+#include "base/memory/weak_ptr.h"
+#include "chromeos/network/network_handler_callbacks.h"
+
+namespace chromeos {
+
+class DeviceState;
+class NetworkStateHandler;
+class NetworkDeviceHandler;
+
+// Updates the "Inhibited" property of the Cellular device.
+//
+// When some SIM-related operations are performed, properties of the Cellular
+// device can change to a temporary value and then change back. To prevent churn
+// in these properties, Shill provides the "Inhibited" property to inhibit any
+// scans.
+//
+// This class is intended to be used when performing such actions to ensure that
+// these transient states never occur.
+class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularInhibitor {
+ public:
+  CellularInhibitor();
+  CellularInhibitor(const CellularInhibitor&) = delete;
+  CellularInhibitor& operator=(const CellularInhibitor&) = delete;
+  ~CellularInhibitor();
+
+  void Init(NetworkStateHandler* network_state_handler,
+            NetworkDeviceHandler* network_device_handler);
+
+  // Callback which returns true upon success or false upon failure.
+  using SuccessCallback = base::OnceCallback<void(bool)>;
+
+  // Sets or unsets the Inhibited property of the Cellular device.
+  void InhibitCellularScanning(SuccessCallback callback);
+  void UninhibitCellularScanning(SuccessCallback callback);
+
+ private:
+  const DeviceState* GetCellularDevice() const;
+
+  void SetInhibitProperty(bool new_inhibit_value, SuccessCallback callback);
+  void OnSetPropertySuccess(
+      const base::RepeatingCallback<void(bool)>& success_callback);
+  void OnSetPropertyError(
+      const base::RepeatingCallback<void(bool)>& success_callback,
+      bool attempted_inhibit,
+      const std::string& error_name,
+      std::unique_ptr<base::DictionaryValue> error_data);
+
+  NetworkStateHandler* network_state_handler_ = nullptr;
+  NetworkDeviceHandler* network_device_handler_ = nullptr;
+
+  base::WeakPtrFactory<CellularInhibitor> weak_ptr_factory_{this};
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_CELLULAR_INHIBITOR_H_

chromeos/network/cellular_inhibitor_unittest.cc

+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/network/cellular_inhibitor.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/test/task_environment.h"
+#include "base/values.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill/fake_shill_device_client.h"
+#include "chromeos/dbus/shill/shill_clients.h"
+#include "chromeos/dbus/shill/shill_manager_client.h"
+#include "chromeos/network/network_device_handler_impl.h"
+#include "chromeos/network/network_handler_callbacks.h"
+#include "chromeos/network/network_state_handler.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+
+namespace chromeos {
+namespace {
+
+const char kDefaultCellularDevicePath[] = "stub_cellular_device";
+
+enum class GetInhibitedPropertyResult { kTrue, kFalse, kOperationFailed };
+
+}  // namespace
+
+class CellularInhibitorTest : public testing::Test {
+ protected:
+  CellularInhibitorTest()
+      : task_environment_(
+            base::test::SingleThreadTaskEnvironment::MainThreadType::UI) {}
+  ~CellularInhibitorTest() override = default;
+
+  // testing::Test:
+  void SetUp() override {
+    shill_clients::InitializeFakes();
+    fake_device_client_ = ShillDeviceClient::Get();
+    fake_device_client_->GetTestInterface()->ClearDevices();
+
+    network_state_handler_ = NetworkStateHandler::InitializeForTest();
+    network_device_handler_ = NetworkDeviceHandler::InitializeForTesting(
+        network_state_handler_.get());
+
+    cellular_inhibitor_ = std::make_unique<CellularInhibitor>();
+    cellular_inhibitor_->Init(network_state_handler_.get(),
+                              network_device_handler_.get());
+  }
+
+  void TearDown() override {
+    cellular_inhibitor_.reset();
+    network_device_handler_.reset();
+    network_state_handler_.reset();
+    shill_clients::Shutdown();
+  }
+
+  void AddCellularDevice() {
+    ShillDeviceClient::TestInterface* device_test =
+        fake_device_client_->GetTestInterface();
+    device_test->AddDevice(kDefaultCellularDevicePath, shill::kTypeCellular,
+                           "cellular1");
+    base::RunLoop().RunUntilIdle();
+  }
+
+  bool InhibitCellularScanning() {
+    return SetInhibitProperty(/*new_inhibit_value=*/true);
+  }
+
+  bool UninhibitCellularScanning() {
+    return SetInhibitProperty(/*new_inhibit_value=*/false);
+  }
+
+  GetInhibitedPropertyResult GetInhibitedProperty() {
+    properties_.reset();
+    network_device_handler_->GetDeviceProperties(
+        kDefaultCellularDevicePath,
+        base::BindOnce(&CellularInhibitorTest::GetPropertiesCallback,
+                       base::Unretained(this)));
+    base::RunLoop().RunUntilIdle();
+
+    if (!properties_)
+      return GetInhibitedPropertyResult::kOperationFailed;
+
+    bool inhibited;
+    EXPECT_TRUE(properties_->GetBooleanWithoutPathExpansion(
+        shill::kInhibitedProperty, &inhibited));
+    return inhibited ? GetInhibitedPropertyResult::kTrue
+                     : GetInhibitedPropertyResult::kFalse;
+  }
+
+ private:
+  bool SetInhibitProperty(bool new_inhibit_value) {
+    base::RunLoop run_loop;
+    on_result_closure_ = run_loop.QuitClosure();
+
+    if (new_inhibit_value) {
+      cellular_inhibitor_->InhibitCellularScanning(
+          base::BindOnce(&CellularInhibitorTest::OnInhibitOrUninhibitResult,
+                         base::Unretained(this)));
+    } else {
+      cellular_inhibitor_->UninhibitCellularScanning(
+          base::BindOnce(&CellularInhibitorTest::OnInhibitOrUninhibitResult,
+                         base::Unretained(this)));
+    }
+    run_loop.Run();
+
+    return success_;
+  }
+
+  void OnInhibitOrUninhibitResult(bool success) {
+    success_ = success;
+    std::move(on_result_closure_).Run();
+  }
+
+  void GetPropertiesCallback(const std::string& device_path,
+                             base::Optional<base::Value> properties) {
+    if (!properties) {
+      properties_.reset();
+      return;
+    }
+
+    properties_ = base::DictionaryValue::From(
+        std::make_unique<base::Value>(std::move(*properties)));
+  }
+
+  base::test::SingleThreadTaskEnvironment task_environment_;
+
+  ShillDeviceClient* fake_device_client_ = nullptr;
+  std::unique_ptr<NetworkStateHandler> network_state_handler_;
+  std::unique_ptr<NetworkDeviceHandler> network_device_handler_;
+  std::unique_ptr<CellularInhibitor> cellular_inhibitor_;
+
+  bool success_;
+  std::unique_ptr<base::DictionaryValue> properties_;
+  base::OnceClosure on_result_closure_;
+};
+
+TEST_F(CellularInhibitorTest, Success) {
+  AddCellularDevice();
+
+  EXPECT_TRUE(InhibitCellularScanning());
+  EXPECT_EQ(GetInhibitedPropertyResult::kTrue, GetInhibitedProperty());
+
+  // Inhibit while already inhibited should succeed.
+  EXPECT_TRUE(InhibitCellularScanning());
+  EXPECT_EQ(GetInhibitedPropertyResult::kTrue, GetInhibitedProperty());
+
+  EXPECT_TRUE(UninhibitCellularScanning());
+  EXPECT_EQ(GetInhibitedPropertyResult::kFalse, GetInhibitedProperty());
+
+  // Uninhibit while already uninhibited should succeed.
+  EXPECT_TRUE(UninhibitCellularScanning());
+  EXPECT_EQ(GetInhibitedPropertyResult::kFalse, GetInhibitedProperty());
+}
+
+TEST_F(CellularInhibitorTest, Failure) {
+  // Do not add a Cellular device. This should cause commands below to fail,
+  // since the device cannot be inhibited if it does not exist.
+
+  EXPECT_FALSE(InhibitCellularScanning());
+  EXPECT_EQ(GetInhibitedPropertyResult::kOperationFailed,
+            GetInhibitedProperty());
+  EXPECT_FALSE(UninhibitCellularScanning());
+  EXPECT_EQ(GetInhibitedPropertyResult::kOperationFailed,
+            GetInhibitedProperty());
+}
+
+}  // namespace chromeos

chromeos/network/network_handler.cc

@@ -6,6 +6,7 @@
 
 #include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/network/auto_connect_handler.h"
+#include "chromeos/network/cellular_inhibitor.h"
 #include "chromeos/network/cellular_metrics_logger.h"
 #include "chromeos/network/client_cert_resolver.h"
 #include "chromeos/network/geolocation_handler.h"
@@ -34,6 +35,7 @@ NetworkHandler::NetworkHandler()
     : task_runner_(base::ThreadTaskRunnerHandle::Get()) {
   network_state_handler_.reset(new NetworkStateHandler());
   network_device_handler_.reset(new NetworkDeviceHandlerImpl());
+  cellular_inhibitor_.reset(new CellularInhibitor());
   network_profile_handler_.reset(new NetworkProfileHandler());
   network_configuration_handler_.reset(new NetworkConfigurationHandler());
   managed_network_configuration_handler_.reset(
@@ -59,6 +61,8 @@ NetworkHandler::~NetworkHandler() {
 void NetworkHandler::Init() {
   network_state_handler_->InitShillPropertyHandler();
   network_device_handler_->Init(network_state_handler_.get());
+  cellular_inhibitor_->Init(network_state_handler_.get(),
+                            network_device_handler_.get());
   network_profile_handler_->Init();
   network_configuration_handler_->Init(network_state_handler_.get(),
                                        network_device_handler_.get());
@@ -152,6 +156,10 @@ AutoConnectHandler* NetworkHandler::auto_connect_handler() {
   return auto_connect_handler_.get();
 }
 
+CellularInhibitor* NetworkHandler::cellular_inhibitor() {
+  return cellular_inhibitor_.get();
+}
+
 NetworkDeviceHandler* NetworkHandler::network_device_handler() {
   return network_device_handler_.get();
 }

chromeos/network/network_handler.h

@@ -17,6 +17,8 @@ class PrefService;
 namespace chromeos {
 
 class AutoConnectHandler;
+class CellularInhibitor;
+class CellularMetricsLogger;
 class ClientCertResolver;
 class GeolocationHandler;
 class ManagedNetworkConfigurationHandler;
@@ -34,7 +36,6 @@ class NetworkStateHandler;
 class NetworkSmsHandler;
 class ProhibitedTechnologiesHandler;
 class UIProxyConfigService;
-class CellularMetricsLogger;
 
 // Class for handling initialization and access to chromeos network handlers.
 // This class should NOT be used in unit tests. Instead, construct individual
@@ -76,6 +77,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkHandler {
   // explicit so that classes can be constructed explicitly in tests without
   // NetworkHandler.
   AutoConnectHandler* auto_connect_handler();
+  CellularInhibitor* cellular_inhibitor();
   NetworkStateHandler* network_state_handler();
   NetworkDeviceHandler* network_device_handler();
   NetworkProfileHandler* network_profile_handler();
@@ -103,6 +105,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkHandler {
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   std::unique_ptr<NetworkStateHandler> network_state_handler_;
   std::unique_ptr<NetworkDeviceHandlerImpl> network_device_handler_;
+  std::unique_ptr<CellularInhibitor> cellular_inhibitor_;
   std::unique_ptr<NetworkProfileHandler> network_profile_handler_;
   std::unique_ptr<NetworkConfigurationHandler> network_configuration_handler_;
   std::unique_ptr<ManagedNetworkConfigurationHandlerImpl>