From 2f7ea17e55296ab751955807ea410e24e597e2e0 Mon Sep 17 00:00:00 2001 From: Yuki Shiino Date: Thu, 29 Jul 2021 08:28:54 +0000 Subject: [PATCH] Revert "[CrOS Network] Refactor ESimManager to use CellularESimInstaller" This reverts commit cfd49cd8403df82433a5e250d2c76c7bb0f8fdbb. Reason for revert: Suspicious about causing test failures of EuiccTest.InstallProfileFromActivationCode https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/24408/test-results Original change's description: > [CrOS Network] Refactor ESimManager to use CellularESimInstaller > > This CL refactors the ESimManager to use the CellularESimInstaller for > installing profile from activation code. > > Bug: 1231305 > Change-Id: I0360afbb2bc3ab28179a44118004232a63006cdd > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3055531 > Reviewed-by: Azeem Arshad > Commit-Queue: Jason Zhang > Cr-Commit-Position: refs/heads/master@{#906467} Bug: 1231305 Change-Id: I1a677b34f6abb5e2d38a28f637975d8dddae6728 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3056328 Bot-Commit: Rubber Stamper Commit-Queue: Yuki Shiino Owners-Override: Yuki Shiino Cr-Commit-Position: refs/heads/master@{#906625} --- .../test_cellular_esim_profile_handler.cc | 17 +- .../test_cellular_esim_profile_handler.h | 9 - .../services/cellular_setup/esim_manager.cc | 3 - .../services/cellular_setup/esim_manager.h | 7 - .../services/cellular_setup/esim_test_base.cc | 8 +- .../services/cellular_setup/esim_test_base.h | 21 +- chromeos/services/cellular_setup/euicc.cc | 185 +++++++++++++++--- chromeos/services/cellular_setup/euicc.h | 31 ++- .../services/cellular_setup/euicc_unittest.cc | 76 ++++--- 9 files changed, 236 insertions(+), 121 deletions(-) diff --git a/chromeos/network/test_cellular_esim_profile_handler.cc b/chromeos/network/test_cellular_esim_profile_handler.cc index 7ad468af1869f..2931e16621a55 100644 --- a/chromeos/network/test_cellular_esim_profile_handler.cc +++ b/chromeos/network/test_cellular_esim_profile_handler.cc @@ -10,9 +10,7 @@ namespace chromeos { -TestCellularESimProfileHandler::TestCellularESimProfileHandler() - : enable_notify_profile_list_update_(true), - has_pending_notify_list_update_(false) {} +TestCellularESimProfileHandler::TestCellularESimProfileHandler() = default; TestCellularESimProfileHandler::~TestCellularESimProfileHandler() = default; @@ -27,15 +25,6 @@ void TestCellularESimProfileHandler::SetHasRefreshedProfilesForEuicc( refreshed_eids_.erase(eid); } -void TestCellularESimProfileHandler::SetEnableNotifyProfileListUpdate( - bool enable_notify_profile_list_update) { - enable_notify_profile_list_update_ = enable_notify_profile_list_update; - if (enable_notify_profile_list_update_ && has_pending_notify_list_update_) { - NotifyESimProfileListUpdated(); - has_pending_notify_list_update_ = false; - } -} - std::vector TestCellularESimProfileHandler::GetESimProfiles() { return esim_profile_states_; @@ -57,10 +46,6 @@ void TestCellularESimProfileHandler::OnHermesPropertiesUpdated() { esim_profile_states_ = new_profile_states; network_state_handler()->SyncStubCellularNetworks(); - if (!enable_notify_profile_list_update_) { - has_pending_notify_list_update_ = true; - return; - } NotifyESimProfileListUpdated(); } diff --git a/chromeos/network/test_cellular_esim_profile_handler.h b/chromeos/network/test_cellular_esim_profile_handler.h index cc46e6d3ddce2..5af6b84c075e2 100644 --- a/chromeos/network/test_cellular_esim_profile_handler.h +++ b/chromeos/network/test_cellular_esim_profile_handler.h @@ -22,13 +22,6 @@ class TestCellularESimProfileHandler : public CellularESimProfileHandler { void SetHasRefreshedProfilesForEuicc(const std::string& eid, bool has_refreshed); - // Enables or disables profile list update notification. - // When set to false, this class will disable triggering the - // NotifyESimProfileListUpdated() and when the next time it's set to true, it - // will call the NotifyESimProfileListUpdated() to fire any pending list - // update notification. - void SetEnableNotifyProfileListUpdate(bool enable_notify_profile_list_update); - // CellularESimProfileHandler: std::vector GetESimProfiles() override; bool HasRefreshedProfilesForEuicc(const std::string& eid) override; @@ -38,8 +31,6 @@ class TestCellularESimProfileHandler : public CellularESimProfileHandler { private: std::vector esim_profile_states_; base::flat_set refreshed_eids_; - bool enable_notify_profile_list_update_; - bool has_pending_notify_list_update_; }; } // namespace chromeos diff --git a/chromeos/services/cellular_setup/esim_manager.cc b/chromeos/services/cellular_setup/esim_manager.cc index fe8ffa5b0d4e0..b0447fba9ac4a 100644 --- a/chromeos/services/cellular_setup/esim_manager.cc +++ b/chromeos/services/cellular_setup/esim_manager.cc @@ -52,7 +52,6 @@ std::string ESimManager::GetRootSmdsAddress() { ESimManager::ESimManager() : ESimManager(NetworkHandler::Get()->cellular_connection_handler(), - NetworkHandler::Get()->cellular_esim_installer(), NetworkHandler::Get()->cellular_esim_profile_handler(), NetworkHandler::Get()->cellular_esim_uninstall_handler(), NetworkHandler::Get()->cellular_inhibitor(), @@ -61,14 +60,12 @@ ESimManager::ESimManager() ESimManager::ESimManager( CellularConnectionHandler* cellular_connection_handler, - CellularESimInstaller* cellular_esim_installer, CellularESimProfileHandler* cellular_esim_profile_handler, CellularESimUninstallHandler* cellular_esim_uninstall_handler, CellularInhibitor* cellular_inhibitor, NetworkConnectionHandler* network_connection_handler, NetworkStateHandler* network_state_handler) : cellular_connection_handler_(cellular_connection_handler), - cellular_esim_installer_(cellular_esim_installer), cellular_esim_profile_handler_(cellular_esim_profile_handler), cellular_esim_uninstall_handler_(cellular_esim_uninstall_handler), cellular_inhibitor_(cellular_inhibitor), diff --git a/chromeos/services/cellular_setup/esim_manager.h b/chromeos/services/cellular_setup/esim_manager.h index d5c73475c4375..9bbc36f4b6ded 100644 --- a/chromeos/services/cellular_setup/esim_manager.h +++ b/chromeos/services/cellular_setup/esim_manager.h @@ -22,7 +22,6 @@ class ObjectPath; namespace chromeos { class CellularConnectionHandler; -class CellularESimInstaller; class CellularESimUninstallHandler; class CellularInhibitor; class NetworkConnectionHandler; @@ -46,7 +45,6 @@ class ESimManager : public mojom::ESimManager, ESimManager(); ESimManager(CellularConnectionHandler* cellular_connection_handler, - CellularESimInstaller* cellular_esim_installer, CellularESimProfileHandler* cellular_esim_profile_handler, CellularESimUninstallHandler* cellular_esim_uninstall_handler, CellularInhibitor* cellular_inhibitor, @@ -80,10 +78,6 @@ class ESimManager : public mojom::ESimManager, // Notifies observers of changes to ESimProfile Lists. void NotifyESimProfileListChanged(Euicc* euicc); - CellularESimInstaller* cellular_esim_installer() { - return cellular_esim_installer_; - } - CellularESimProfileHandler* cellular_esim_profile_handler() { return cellular_esim_profile_handler_; } @@ -117,7 +111,6 @@ class ESimManager : public mojom::ESimManager, bool CreateEuiccIfNew(const dbus::ObjectPath& euicc_path); CellularConnectionHandler* cellular_connection_handler_; - CellularESimInstaller* cellular_esim_installer_; CellularESimProfileHandler* cellular_esim_profile_handler_; CellularESimUninstallHandler* cellular_esim_uninstall_handler_; CellularInhibitor* cellular_inhibitor_; diff --git a/chromeos/services/cellular_setup/esim_test_base.cc b/chromeos/services/cellular_setup/esim_test_base.cc index a28f2656a1f16..5d7767b2ccb22 100644 --- a/chromeos/services/cellular_setup/esim_test_base.cc +++ b/chromeos/services/cellular_setup/esim_test_base.cc @@ -12,7 +12,6 @@ #include "chromeos/dbus/shill/shill_clients.h" #include "chromeos/dbus/shill/shill_manager_client.h" #include "chromeos/network/cellular_connection_handler.h" -#include "chromeos/network/cellular_esim_installer.h" #include "chromeos/network/cellular_esim_uninstall_handler.h" #include "chromeos/network/cellular_inhibitor.h" #include "chromeos/network/fake_network_connection_handler.h" @@ -64,10 +63,6 @@ void ESimTestBase::SetUp() { cellular_connection_handler_->Init(network_state_handler_.get(), cellular_inhibitor_.get(), cellular_esim_profile_handler_.get()); - cellular_esim_installer_ = std::make_unique(); - cellular_esim_installer_->Init( - cellular_connection_handler_.get(), cellular_inhibitor_.get(), - network_connection_handler_.get(), network_state_handler_.get()); cellular_esim_uninstall_handler_ = std::make_unique(); cellular_esim_uninstall_handler_->Init( @@ -76,8 +71,7 @@ void ESimTestBase::SetUp() { network_state_handler_.get()); esim_manager_ = std::make_unique( - cellular_connection_handler_.get(), cellular_esim_installer_.get(), - cellular_esim_profile_handler_.get(), + cellular_connection_handler_.get(), cellular_esim_profile_handler_.get(), cellular_esim_uninstall_handler_.get(), cellular_inhibitor_.get(), network_connection_handler_.get(), network_state_handler_.get()); observer_ = std::make_unique(); diff --git a/chromeos/services/cellular_setup/esim_test_base.h b/chromeos/services/cellular_setup/esim_test_base.h index 802aa891c08f2..782a4fd844dac 100644 --- a/chromeos/services/cellular_setup/esim_test_base.h +++ b/chromeos/services/cellular_setup/esim_test_base.h @@ -13,15 +13,14 @@ namespace chromeos { -class CellularConnectionHandler; -class CellularESimInstaller; -class CellularESimUninstallHandler; class CellularInhibitor; -class FakeNetworkConnectionHandler; -class NetworkConfigurationHandler; -class NetworkDeviceHandler; class NetworkStateHandler; -class TestCellularESimProfileHandler; +class NetworkDeviceHandler; +class NetworkConfigurationHandler; +class FakeNetworkConnectionHandler; +class CellularConnectionHandler; +class CellularESimUninstallHandler; +class CellularESimProfileHandler; namespace cellular_setup { @@ -63,19 +62,13 @@ class ESimTestBase : public testing::Test { return network_state_handler_.get(); } - TestCellularESimProfileHandler* cellular_esim_profile_handler() { - return cellular_esim_profile_handler_.get(); - } - base::test::TaskEnvironment* task_environment() { return &task_environment_; } private: std::unique_ptr network_state_handler_; std::unique_ptr network_device_handler_; std::unique_ptr cellular_inhibitor_; - std::unique_ptr cellular_esim_installer_; - std::unique_ptr - cellular_esim_profile_handler_; + std::unique_ptr cellular_esim_profile_handler_; std::unique_ptr network_configuration_handler_; std::unique_ptr network_connection_handler_; std::unique_ptr diff --git a/chromeos/services/cellular_setup/euicc.cc b/chromeos/services/cellular_setup/euicc.cc index e9c4942222072..f7f7b652b13b9 100644 --- a/chromeos/services/cellular_setup/euicc.cc +++ b/chromeos/services/cellular_setup/euicc.cc @@ -12,7 +12,6 @@ #include "base/strings/strcat.h" #include "base/time/time.h" #include "chromeos/network/cellular_connection_handler.h" -#include "chromeos/network/cellular_esim_installer.h" #include "chromeos/network/cellular_esim_profile.h" #include "chromeos/network/cellular_inhibitor.h" #include "chromeos/network/hermes_metrics_util.h" @@ -41,6 +40,27 @@ constexpr base::TimeDelta kPendingProfileRefreshDelay = // Prefix for EID when encoded in QR Code. const char kEidQrCodePrefix[] = "EID:"; +// Measures the time from which this function is called to when |callback| +// is expected to run. The measured time difference should capture the time it +// took for a profile to be fully downloaded from a provided activation code. +Euicc::InstallProfileFromActivationCodeCallback +CreateTimedInstallProfileCallback( + Euicc::InstallProfileFromActivationCodeCallback callback) { + return base::BindOnce( + [](Euicc::InstallProfileFromActivationCodeCallback callback, + base::Time installation_start_time, mojom::ProfileInstallResult result, + mojo::PendingRemote esim_profile_pending_remote) + -> void { + std::move(callback).Run(result, std::move(esim_profile_pending_remote)); + if (result != mojom::ProfileInstallResult::kSuccess) + return; + UMA_HISTOGRAM_MEDIUM_TIMES( + "Network.Cellular.ESim.ProfileDownload.ActivationCode.Latency", + base::Time::Now() - installation_start_time); + }, + std::move(callback), base::Time::Now()); +} + // Measures the time from which this function is called to when |callback| // is expected to run. The measured time difference should capture the time it // took for a profile discovery request to complete. @@ -61,6 +81,13 @@ Euicc::RequestPendingProfilesCallback CreateTimedRequestPendingProfilesCallback( } } // namespace +// static +void Euicc::RecordInstallProfileViaQrCodeResult( + InstallProfileViaQrCodeResult result) { + base::UmaHistogramEnumeration( + "Network.Cellular.ESim.InstallViaQrCode.OperationResult", result); +} + // static void Euicc::RecordRequestPendingProfilesResult( RequestPendingProfilesResult result) { @@ -119,35 +146,17 @@ void Euicc::InstallProfileFromActivationCode( return; } - esim_manager_->cellular_esim_installer()->InstallProfileFromActivationCode( - activation_code, confirmation_code, path_, - base::BindOnce(&Euicc::OnESimInstallProfileResult, - weak_ptr_factory_.GetWeakPtr(), std::move(callback))); -} - -void Euicc::OnESimInstallProfileResult( - InstallProfileFromActivationCodeCallback callback, - HermesResponseStatus hermes_status, - absl::optional profile_path) { - mojom::ProfileInstallResult status = InstallResultFromStatus(hermes_status); - if (status != mojom::ProfileInstallResult::kSuccess) { - std::move(callback).Run(status, mojo::NullRemote()); - return; - } - - DCHECK(profile_path != absl::nullopt); - ESimProfile* esim_profile = GetProfileFromPath(profile_path.value()); - if (!esim_profile) { - // An ESimProfile may not exist for the newly created esim profile object - // path if ESimProfileHandler has not updated profile lists yet. Save the - // callback until an UpdateProfileList call creates an ESimProfile - // object for this path - install_calls_pending_create_.emplace(profile_path.value(), - std::move(callback)); - return; - } - std::move(callback).Run(mojom::ProfileInstallResult::kSuccess, - esim_profile->CreateRemote()); + // Try installing directly with activation code. + // TODO(crbug.com/1186682) Add a check for activation codes that are + // currently being installed to prevent multiple attempts for the same + // activation code. + NET_LOG(USER) << "Attempting installation with code " << activation_code; + esim_manager_->cellular_inhibitor()->InhibitCellularScanning( + CellularInhibitor::InhibitReason::kInstallingProfile, + base::BindOnce(&Euicc::PerformInstallProfileFromActivationCode, + weak_ptr_factory_.GetWeakPtr(), activation_code, + confirmation_code, + CreateTimedInstallProfileCallback(std::move(callback)))); } void Euicc::RequestPendingProfiles(RequestPendingProfilesCallback callback) { @@ -247,6 +256,122 @@ ESimProfile* Euicc::GetProfileFromPath(const dbus::ObjectPath& path) { return nullptr; } +void Euicc::PerformInstallProfileFromActivationCode( + const std::string& activation_code, + const std::string& confirmation_code, + InstallProfileFromActivationCodeCallback callback, + std::unique_ptr inhibit_lock) { + if (!inhibit_lock) { + NET_LOG(ERROR) << "Error inhibiting cellular device"; + RecordInstallProfileViaQrCodeResult( + InstallProfileViaQrCodeResult::kInhibitFailed); + std::move(callback).Run(mojom::ProfileInstallResult::kFailure, + mojo::NullRemote()); + return; + } + + HermesEuiccClient::Get()->InstallProfileFromActivationCode( + path_, activation_code, confirmation_code, + base::BindOnce(&Euicc::OnProfileInstallResult, + weak_ptr_factory_.GetWeakPtr(), std::move(callback), + std::move(inhibit_lock))); +} + +void Euicc::OnProfileInstallResult( + InstallProfileFromActivationCodeCallback callback, + std::unique_ptr inhibit_lock, + HermesResponseStatus status, + const dbus::ObjectPath* profile_path) { + hermes_metrics::LogInstallViaQrCodeResult(status); + + if (status != HermesResponseStatus::kSuccess) { + NET_LOG(ERROR) << "Error Installing profile status=" + << static_cast(status); + RecordInstallProfileViaQrCodeResult( + InstallProfileViaQrCodeResult::kHermesInstallFailed); + std::move(callback).Run(InstallResultFromStatus(status), + mojo::NullRemote()); + return; + } + + RecordInstallProfileViaQrCodeResult(InstallProfileViaQrCodeResult::kSuccess); + + install_calls_pending_connect_.emplace(*profile_path, std::move(callback)); + esim_manager_->cellular_connection_handler() + ->PrepareNewlyInstalledCellularNetworkForConnection( + path_, *profile_path, std::move(inhibit_lock), + base::BindOnce(&Euicc::OnNewProfileEnableSuccess, + weak_ptr_factory_.GetWeakPtr(), *profile_path), + base::BindOnce(&Euicc::OnPrepareCellularNetworkForConnectionFailure, + weak_ptr_factory_.GetWeakPtr(), *profile_path)); +} + +void Euicc::OnNewProfileEnableSuccess(const dbus::ObjectPath& profile_path, + const std::string& service_path) { + const NetworkState* network_state = + esim_manager_->network_state_handler()->GetNetworkState(service_path); + if (!network_state) { + HandleNewProfileEnableFailure(profile_path, + NetworkConnectionHandler::kErrorNotFound); + return; + } + + if (!network_state->IsConnectingOrConnected()) { + // The connection could fail but the user will be notified of connection + // failures separately. + esim_manager_->network_connection_handler()->ConnectToNetwork( + service_path, + /*success_callback=*/base::DoNothing(), + /*error_callback=*/base::DoNothing(), + /*check_error_state=*/false, ConnectCallbackMode::ON_STARTED); + } + + auto it = install_calls_pending_connect_.find(profile_path); + if (it == install_calls_pending_connect_.end()) { + NET_LOG(ERROR) << "ESim profile install callback missing after enable " + "success. profile_path=" + << profile_path.value(); + return; + } + InstallProfileFromActivationCodeCallback callback = std::move(it->second); + install_calls_pending_connect_.erase(it); + + ESimProfile* esim_profile = GetProfileFromPath(profile_path); + if (!esim_profile) { + // An ESimProfile may not exist for the newly created esim profile object + // path if ESimProfileHandler has not updated profile lists yet. Save the + // callback until an UpdateProfileList call creates an ESimProfile + // object for this path + install_calls_pending_create_.emplace(profile_path, std::move(callback)); + return; + } + std::move(callback).Run(mojom::ProfileInstallResult::kSuccess, + esim_profile->CreateRemote()); +} + +void Euicc::OnPrepareCellularNetworkForConnectionFailure( + const dbus::ObjectPath& profile_path, + const std::string& service_path, + const std::string& error_name) { + HandleNewProfileEnableFailure(profile_path, error_name); +} + +void Euicc::HandleNewProfileEnableFailure(const dbus::ObjectPath& profile_path, + const std::string& error_name) { + NET_LOG(ERROR) << "Error enabling newly created profile path=" + << profile_path.value() << " error_name=" << error_name; + + auto it = install_calls_pending_connect_.find(profile_path); + if (it == install_calls_pending_connect_.end()) { + NET_LOG(ERROR) + << "ESim profile install callback missing after enable failure"; + return; + } + std::move(it->second) + .Run(mojom::ProfileInstallResult::kFailure, mojo::NullRemote()); + install_calls_pending_connect_.erase(it); +} + void Euicc::PerformRequestPendingProfiles( RequestPendingProfilesCallback callback, std::unique_ptr inhibit_lock) { diff --git a/chromeos/services/cellular_setup/euicc.h b/chromeos/services/cellular_setup/euicc.h index 25d26b31f5417..1820c5be24e91 100644 --- a/chromeos/services/cellular_setup/euicc.h +++ b/chromeos/services/cellular_setup/euicc.h @@ -62,8 +62,20 @@ class Euicc : public mojom::Euicc { const mojom::EuiccPropertiesPtr& properties() { return properties_; } private: + FRIEND_TEST_ALL_PREFIXES(EuiccTest, InstallProfileFromActivationCode); FRIEND_TEST_ALL_PREFIXES(EuiccTest, RequestPendingProfiles); + // These values are persisted to logs. Entries should not be renumbered and + // numeric values should never be reused. + enum class InstallProfileViaQrCodeResult { + kSuccess = 0, + kInhibitFailed = 1, + kHermesInstallFailed = 2, + kMaxValue = kHermesInstallFailed + }; + static void RecordInstallProfileViaQrCodeResult( + InstallProfileViaQrCodeResult result); + // These values are persisted to logs. Entries should not be renumbered and // numeric values should never be reused. enum class RequestPendingProfilesResult { @@ -84,11 +96,20 @@ class Euicc : public mojom::Euicc { const std::string& confirmation_code, InstallProfileFromActivationCodeCallback callback, std::unique_ptr inhibit_lock); - void OnESimInstallProfileResult( + void OnProfileInstallResult( InstallProfileFromActivationCodeCallback callback, - HermesResponseStatus hermes_status, - absl::optional profile_path); + std::unique_ptr inhibit_lock, + HermesResponseStatus status, + const dbus::ObjectPath* object_path); + void OnNewProfileEnableSuccess(const dbus::ObjectPath& profile_path, + const std::string& service_path); void OnNewProfileConnectSuccess(const dbus::ObjectPath& profile_path); + void OnPrepareCellularNetworkForConnectionFailure( + const dbus::ObjectPath& profile_path, + const std::string& service_path, + const std::string& error_name); + void HandleNewProfileEnableFailure(const dbus::ObjectPath& profile_path, + const std::string& error_name); void PerformRequestPendingProfiles( RequestPendingProfilesCallback callback, std::unique_ptr inhibit_lock); @@ -120,6 +141,10 @@ class Euicc : public mojom::Euicc { // that are pending creation of a new ESimProfile object. std::map install_calls_pending_create_; + // Maps profile dbus paths to InstallProfileFromActivation method callbacks + // that are pending connection to the newly created network. + std::map + install_calls_pending_connect_; base::WeakPtrFactory weak_ptr_factory_{this}; }; diff --git a/chromeos/services/cellular_setup/euicc_unittest.cc b/chromeos/services/cellular_setup/euicc_unittest.cc index 099812f917fc5..87f45d83b3065 100644 --- a/chromeos/services/cellular_setup/euicc_unittest.cc +++ b/chromeos/services/cellular_setup/euicc_unittest.cc @@ -13,7 +13,6 @@ #include "chromeos/dbus/hermes/hermes_profile_client.h" #include "chromeos/network/fake_network_connection_handler.h" #include "chromeos/network/network_type_pattern.h" -#include "chromeos/network/test_cellular_esim_profile_handler.h" #include "chromeos/services/cellular_setup/esim_test_base.h" #include "chromeos/services/cellular_setup/esim_test_utils.h" #include "mojo/public/cpp/bindings/pending_remote.h" @@ -26,6 +25,8 @@ namespace { const char kInstallViaQrCodeHistogram[] = "Network.Cellular.ESim.InstallViaQrCode.Result"; +const char kInstallViaQrCodeOperationHistogram[] = + "Network.Cellular.ESim.InstallViaQrCode.OperationResult"; using InstallResultPair = std::pair>; @@ -68,7 +69,6 @@ class EuiccTest : public ESimTestBase { const mojo::Remote& euicc, const std::string& activation_code, const std::string& confirmation_code, - bool wait_for_create, bool wait_for_connect, bool fail_connect) { mojom::ProfileInstallResult out_install_result; @@ -87,22 +87,6 @@ class EuiccTest : public ESimTestBase { FastForwardProfileRefreshDelay(); - if (wait_for_create) { - base::RunLoop().RunUntilIdle(); - // We expect calling SetEnableNotifyProfileListUpdate(false) before - // testing wait_for_create = true scenario. Otherwise, the assertion - // will fail. - EXPECT_NE(mojom::ProfileInstallResult::kSuccess, out_install_result); - EXPECT_FALSE(out_esim_profile.is_valid()); - // Set the enable notify prolie list update back to true if it was false - // before and fire the pending notify profile list update and that should - // make the pending create callback complete. - cellular_esim_profile_handler()->SetEnableNotifyProfileListUpdate(true); - FastForwardProfileRefreshDelay(); - EXPECT_EQ(mojom::ProfileInstallResult::kSuccess, out_install_result); - EXPECT_TRUE(out_esim_profile.is_valid()); - } - if (wait_for_connect) { base::RunLoop().RunUntilIdle(); EXPECT_LE(1u, network_connection_handler()->connect_calls().size()); @@ -157,35 +141,63 @@ TEST_F(EuiccTest, GetProfileList) { TEST_F(EuiccTest, InstallProfileFromActivationCode) { base::HistogramTester histogram_tester; + mojo::Remote euicc = GetEuiccForEid(ESimTestBase::kTestEid); ASSERT_TRUE(euicc.is_bound()); HermesEuiccClient::TestInterface* euicc_test = HermesEuiccClient::Get()->GetTestInterface(); - // Verify that the callback is not completed if update profile list - // notification is not fired and save the callback until an update profile - // list gets called. - cellular_esim_profile_handler()->SetEnableNotifyProfileListUpdate(false); + // Verify that install errors return error code properly. + euicc_test->QueueHermesErrorStatus( + HermesResponseStatus::kErrorInvalidActivationCode); InstallResultPair result_pair = InstallProfileFromActivationCode( + euicc, /*activation_code=*/std::string(), + /*confirmation_code=*/std::string(), /*wait_for_connect=*/false, + /*fail_connect=*/false); + EXPECT_EQ(mojom::ProfileInstallResult::kErrorInvalidActivationCode, + result_pair.first); + EXPECT_FALSE(result_pair.second.is_valid()); + histogram_tester.ExpectBucketCount( + kInstallViaQrCodeHistogram, + HermesResponseStatus::kErrorInvalidActivationCode, + /*expected_count=*/1); + histogram_tester.ExpectBucketCount( + kInstallViaQrCodeOperationHistogram, + Euicc::InstallProfileViaQrCodeResult::kHermesInstallFailed, + /*expected_count=*/1); + + // Verify that connect failures are handled properly. + result_pair = InstallProfileFromActivationCode( euicc, euicc_test->GenerateFakeActivationCode(), - /*confirmation_code=*/std::string(), /*wait_for_create=*/true, - /*wait_for_connect=*/false, /*fail_connect=*/false); + /*confirmation_code=*/std::string(), /*wait_for_connect=*/true, + /*fail_connect=*/true); EXPECT_EQ(mojom::ProfileInstallResult::kSuccess, result_pair.first); - EXPECT_TRUE(result_pair.second.is_valid()); + ASSERT_TRUE(result_pair.second.is_valid()); histogram_tester.ExpectBucketCount(kInstallViaQrCodeHistogram, HermesResponseStatus::kSuccess, /*expected_count=*/1); + histogram_tester.ExpectBucketCount( + kInstallViaQrCodeOperationHistogram, + Euicc::InstallProfileViaQrCodeResult::kSuccess, + /*expected_count=*/1); // Verify that install succeeds when valid activation code is passed. result_pair = InstallProfileFromActivationCode( euicc, euicc_test->GenerateFakeActivationCode(), - /*confirmation_code=*/std::string(), /*wait_for_create=*/false, - /*wait_for_connect=*/false, /*fail_connect=*/false); + /*confirmation_code=*/std::string(), /*wait_for_connect=*/true, + /*fail_connect=*/false); EXPECT_EQ(mojom::ProfileInstallResult::kSuccess, result_pair.first); - EXPECT_TRUE(result_pair.second.is_valid()); + ASSERT_TRUE(result_pair.second.is_valid()); + + histogram_tester.ExpectTotalCount( + "Network.Cellular.ESim.ProfileDownload.ActivationCode.Latency", 2); histogram_tester.ExpectBucketCount(kInstallViaQrCodeHistogram, HermesResponseStatus::kSuccess, /*expected_count=*/2); + histogram_tester.ExpectBucketCount( + kInstallViaQrCodeOperationHistogram, + Euicc::InstallProfileViaQrCodeResult::kSuccess, + /*expected_count=*/2); } TEST_F(EuiccTest, InstallProfileAlreadyConnected) { @@ -201,8 +213,8 @@ TEST_F(EuiccTest, InstallProfileAlreadyConnected) { HermesEuiccClient::Get() ->GetTestInterface() ->GenerateFakeActivationCode(), - /*confirmation_code=*/std::string(), /*wait_for_create=*/false, - /*wait_for_connect=*/false, /*fail_connect=*/false); + /*confirmation_code=*/std::string(), /*wait_for_connect=*/false, + /*fail_connect=*/false); EXPECT_EQ(mojom::ProfileInstallResult::kSuccess, result_pair.first); } @@ -227,8 +239,8 @@ TEST_F(EuiccTest, InstallPendingProfileFromActivationCode) { InstallResultPair result_pair = InstallProfileFromActivationCode( euicc, dbus_properties->activation_code().value(), - /*confirmation_code=*/std::string(), /*wait_for_create=*/false, - /*wait_for_connect=*/true, /*fail_connect=*/false); + /*confirmation_code=*/std::string(), /*wait_for_connect=*/true, + /*fail_connect=*/false); EXPECT_EQ(mojom::ProfileInstallResult::kSuccess, result_pair.first); ASSERT_TRUE(result_pair.second.is_valid());