[Kiosk] Fix flaky KioskTest.LaunchAppNetworkPortal

We did properly handle the case of losing network connection during
installation. We should close the network configure ui as soon as the
network appears to cover cases without an operator.

Bug: 1104169
Change-Id: Iee4cc7f342f417d51c152fd6d7ffa86e97affdf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302618Reviewed-by: Anqing Zhao <anqing@chromium.org>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790900}

chrome/browser/chromeos/login/kiosk_browsertest.cc

@@ -1012,8 +1012,7 @@ IN_PROC_BROWSER_TEST_F(KioskTest, LaunchAppNetworkDownConfigureNotAllowed) {
   WaitForAppLaunchSuccess();
 }
 
-// TODO(crbug.com/1104169): Fix flaky failures and enable this test.
-IN_PROC_BROWSER_TEST_F(KioskTest, DISABLED_LaunchAppNetworkPortal) {
+IN_PROC_BROWSER_TEST_F(KioskTest, LaunchAppNetworkPortal) {
   // Mock network could be configured without the owner password.
   ScopedCanConfigureNetwork can_configure_network(true, false);
 

chrome/browser/chromeos/login/kiosk_launch_controller.cc

@@ -233,6 +233,10 @@ KioskAppManagerBase::App KioskLaunchController::GetAppData() {
   }
 }
 
+bool KioskLaunchController::IsNetworkRequired() {
+  return network_required_;
+}
+
 void KioskLaunchController::CleanUp() {
   network_wait_timer_.Stop();
   splash_wait_timer_.Stop();
@@ -300,6 +304,10 @@ void KioskLaunchController::InitializeNetwork() {
   network_wait_timer_.Start(FROM_HERE, g_network_wait_time, this,
                             &KioskLaunchController::OnNetworkWaitTimedOut);
 
+  // When we are asked to initialize network, we should remember that this app
+  // requires network.
+  network_required_ = true;
+
   splash_screen_view_->UpdateAppLaunchState(
       AppLaunchSplashScreenView::APP_LAUNCH_STATE_PREPARING_NETWORK);
 
@@ -310,9 +318,6 @@ void KioskLaunchController::InitializeNetwork() {
 }
 
 void KioskLaunchController::OnNetworkWaitTimedOut() {
-  DCHECK_EQ(
-      app_state_,
-      AppState::INIT_NETWORK);  // Otherwise we should be installing the app.
   DCHECK_EQ(network_ui_state_, NetworkUIState::NOT_SHOWING);
 
   auto connection_type = network::mojom::ConnectionType::CONNECTION_UNKNOWN;
@@ -500,7 +505,7 @@ void KioskLaunchController::ShowNetworkConfigureUI() {
   splash_screen_view_->ShowNetworkConfigureUI();
 }
 
-void KioskLaunchController::CloseNetworkConfigureScreenIfTimedout() {
+void KioskLaunchController::CloseNetworkConfigureScreenIfOnline() {
   if (network_ui_state_ == NetworkUIState::SHOWING && network_wait_timedout_) {
     SYSLOG(INFO) << "We are back online, closing network configure screen.";
     splash_screen_view_->ToggleNetworkConfig(false);
@@ -519,10 +524,9 @@ void KioskLaunchController::OnNetworkConfigRequested() {
     case AppState::INSTALLING:
       // When requesting to show network configure UI, we should cancel current
       // installation and restart it as soon as the network is configured.
-      // This is identical to what happens when we lose network connection
-      // during installation
-      network_ui_state_ = NetworkUIState::NEED_TO_SHOW;
-      OnNetworkStateChanged(/*online*/ false);
+      app_state_ = AppState::INIT_NETWORK;
+      app_launcher_->RestartLauncher();
+      MaybeShowNetworkConfigureUI();
       break;
     case AppState::LAUNCHED:
       // We do nothing since the splash screen is soon to be destroyed.
@@ -545,17 +549,15 @@ void KioskLaunchController::OnNetworkStateChanged(bool online) {
     if (network_ui_state_ == NetworkUIState::NOT_SHOWING ||
         network_wait_timedout_) {
       network_wait_timer_.Stop();
-      CloseNetworkConfigureScreenIfTimedout();
+      CloseNetworkConfigureScreenIfOnline();
       app_launcher_->ContinueWithNetworkReady();
     }
   }
 
-  if (app_state_ == AppState::INSTALLING && !online) {
+  if (app_state_ == AppState::INSTALLING && network_required_ && !online) {
     SYSLOG(WARNING)
         << "Connection lost during installation, restarting launcher.";
-    app_state_ = AppState::INIT_NETWORK;
-    app_launcher_->RestartLauncher();
-    MaybeShowNetworkConfigureUI();
+    OnNetworkWaitTimedOut();
   }
 }
 

chrome/browser/chromeos/login/kiosk_launch_controller.h

@@ -119,6 +119,7 @@ class KioskLaunchController : public KioskProfileLoader::Delegate,
   void OnNetworkConfigFinished() override;
   void OnNetworkStateChanged(bool online) override;
   KioskAppManagerBase::App GetAppData() override;
+  bool IsNetworkRequired() override;
 
   // KioskAppLauncher::Delegate:
   void InitializeNetwork() override;
@@ -149,7 +150,7 @@ class KioskLaunchController : public KioskProfileLoader::Delegate,
   void MaybeShowNetworkConfigureUI();
   // Shows the network configuration dialog.
   void ShowNetworkConfigureUI();
-  void CloseNetworkConfigureScreenIfTimedout();
+  void CloseNetworkConfigureScreenIfOnline();
 
   void HandleWebAppInstallFailed();
 
@@ -173,6 +174,8 @@ class KioskLaunchController : public KioskProfileLoader::Delegate,
   // Whether app should be launched as soon as it is ready.
   bool launch_on_install_ = false;
   bool network_wait_timedout_ = false;
+  // Whether the network is required for the installation.
+  bool network_required_ = false;
 
   // Used to login into kiosk user profile.
   std::unique_ptr<KioskProfileLoader> kiosk_profile_loader_;

chrome/browser/chromeos/login/kiosk_launch_controller_browsertest.cc

@@ -242,7 +242,6 @@ IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
   launch_controls()->OnAppInstalling();
   ExpectState(AppState::INSTALLING, NetworkUIState::NOT_SHOWING);
 
-  EXPECT_CALL(*launcher(), RestartLauncher()).Times(1);
   SetOnline(false);
   launch_controls()->InitializeNetwork();
   ExpectState(AppState::INIT_NETWORK, NetworkUIState::SHOWING);

chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc

@@ -125,12 +125,6 @@ void AppLaunchSplashScreenHandler::UpdateAppLaunchState(AppLaunchState state) {
         l10n_util::GetStringUTF8(GetProgressMessageFromState(state_)));
   }
 
-  // When we are asked to initialize network, we should remember that this app
-  // requires network.
-  if (state_ == AppLaunchState::APP_LAUNCH_STATE_PREPARING_NETWORK) {
-    network_required_ = true;
-  }
-
   UpdateState(NetworkError::ERROR_REASON_UPDATE);
 }
 
@@ -145,7 +139,7 @@ void AppLaunchSplashScreenHandler::ShowNetworkConfigureUI() {
 
   // We should not block users when the network was not required by the
   // controller.
-  if (!network_required_) {
+  if (!delegate_->IsNetworkRequired()) {
     state = NetworkStateInformer::ONLINE;
   }
 

chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.h

@@ -42,6 +42,9 @@ class AppLaunchSplashScreenView {
 
     // Returns the data needed to be displayed on the splash screen.
     virtual KioskAppManagerBase::App GetAppData() = 0;
+
+    // Tells whether the network connection is required for app launch.
+    virtual bool IsNetworkRequired() = 0;
   };
 
   enum AppLaunchState {
@@ -133,8 +136,6 @@ class AppLaunchSplashScreenHandler
 
   // Whether network configure UI is being shown.
   bool network_config_shown_ = false;
-  // Whether the network is required in order to proceed with app launch.
-  bool network_required_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(AppLaunchSplashScreenHandler);
 };