[Kiosk] Fix nullptr failures for Arc kiosk startup

We did not have proper checks for |app_launcher_| previously, causing
it to behave unexpectedly/crash for cases of ARC kiosk, when this
variable was not set. Now, we are setting this value properly and
there should be no failures.

Also making KioskLaunchController tests generic.

Bug: 1121911
Change-Id: I1acec49f251391c4060ba46725a754ed6de48cba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385379
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Reviewed-by: Anqing Zhao <anqing@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803437}

chrome/browser/chromeos/login/app_mode/kiosk_launch_controller.cc

@@ -86,6 +86,31 @@ void RecordKioskLaunchUMA(bool is_auto_launch) {
   }
 }
 
+// This is a not-owning wrapper around ArcKioskAppService which allows to be
+// plugged into a unique_ptr safely.
+// TODO(apotapchuk): Remove this when ARC kiosk is fully deprecated.
+class ArcKioskAppServiceWrapper : public KioskAppLauncher {
+ public:
+  ArcKioskAppServiceWrapper(ArcKioskAppService* service,
+                            KioskAppLauncher::Delegate* delegate)
+      : service_(service) {
+    service_->SetDelegate(delegate);
+  }
+
+  ~ArcKioskAppServiceWrapper() override { service_->SetDelegate(nullptr); }
+
+  // KioskAppLauncher:
+  void Initialize() override { service_->Initialize(); }
+  void ContinueWithNetworkReady() override {
+    service_->ContinueWithNetworkReady();
+  }
+  void RestartLauncher() override { service_->RestartLauncher(); }
+  void LaunchApp() override { service_->LaunchApp(); }
+
+ private:
+  ArcKioskAppService* const service_;
+};
+
 }  // namespace
 
 KioskLaunchController::KioskLaunchController(OobeUI* oobe_ui)
@@ -147,27 +172,29 @@ void KioskLaunchController::OnProfileLoaded(Profile* profile) {
   // Reset virtual keyboard to use IME engines in app profile early.
   ChromeKeyboardControllerClient::Get()->RebuildKeyboardIfEnabled();
 
-  switch (kiosk_app_id_.type) {
-    case KioskAppType::ARC_APP:
-      ArcKioskAppService::Get(profile_)->SetDelegate(this);
-      break;
-    case KioskAppType::CHROME_APP:
-      // Do not set update |app_launcher_| if has been set.
-      if (!app_launcher_)
+  // Do not set update |app_launcher_| if has been set.
+  if (!app_launcher_) {
+    switch (kiosk_app_id_.type) {
+      case KioskAppType::ARC_APP:
+        // ArcKioskAppService lifetime is bound to the profile, therefore
+        // wrap it into a separate object.
+        app_launcher_ = std::make_unique<ArcKioskAppServiceWrapper>(
+            ArcKioskAppService::Get(profile_), this);
+        break;
+      case KioskAppType::CHROME_APP:
         app_launcher_ = std::make_unique<StartupAppLauncher>(
             profile_, *kiosk_app_id_.app_id, this);
-      app_launcher_->Initialize();
-      break;
-    case KioskAppType::WEB_APP:
-      // Make keyboard config sync with the |VirtualKeyboardFeatures| policy.
-      ChromeKeyboardControllerClient::Get()->SetKeyboardConfigFromPref(true);
-      // Do not set update |app_launcher_| if has been set.
-      if (!app_launcher_)
+        break;
+      case KioskAppType::WEB_APP:
+        // Make keyboard config sync with the |VirtualKeyboardFeatures| policy.
+        ChromeKeyboardControllerClient::Get()->SetKeyboardConfigFromPref(true);
         app_launcher_ = std::make_unique<WebKioskAppLauncher>(
             profile, this, *kiosk_app_id_.account_id);
-      app_launcher_->Initialize();
-      break;
+        break;
+    }
   }
+
+  app_launcher_->Initialize();
   if (network_ui_state_ == NetworkUIState::NEED_TO_SHOW)
     ShowNetworkConfigureUI();
 }
@@ -242,9 +269,6 @@ void KioskLaunchController::CleanUp() {
   splash_wait_timer_.Stop();
 
   kiosk_profile_loader_.reset();
-
-  if (kiosk_app_id_.type == KioskAppType::ARC_APP)
-    ArcKioskAppService::Get(profile_)->SetDelegate(nullptr);
   // Can be null in tests.
   if (host_)
     host_->Finalize(base::OnceClosure());

chrome/browser/chromeos/login/app_mode/kiosk_launch_controller_browsertest.cc

@@ -2,12 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/public/cpp/keyboard/keyboard_controller.h"
+#include "chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
 #include "chrome/browser/chromeos/app_mode/kiosk_app_types.h"
 #include "chrome/browser/chromeos/app_mode/web_app/mock_web_kiosk_app_launcher.h"
 #include "chrome/browser/chromeos/login/app_mode/kiosk_launch_controller.h"
 #include "chrome/browser/chromeos/login/test/kiosk_test_helpers.h"
-#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client_test_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/webui/chromeos/login/fake_app_launch_splash_screen_handler.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -22,7 +21,9 @@ using testing::_;
 
 namespace chromeos {
 
-class KioskLaunchControllerTest : public InProcessBrowserTest {
+class KioskLaunchControllerTest
+    : public InProcessBrowserTest,
+      public ::testing::WithParamInterface<KioskAppType> {
  public:
   using AppState = KioskLaunchController::AppState;
   using NetworkUIState = KioskLaunchController::NetworkUIState;
@@ -42,12 +43,26 @@ class KioskLaunchControllerTest : public InProcessBrowserTest {
         KioskLaunchController::DisableWaitTimerAndLoginOperationsForTesting();
     controller_ = KioskLaunchController::CreateForTesting(
         view_.get(), std::move(app_launcher));
+
+    switch (GetParam()) {
+      case KioskAppType::ARC_APP:
+        kiosk_app_id_ = KioskAppId::ForArcApp(EmptyAccountId());
+        break;
+      case KioskAppType::CHROME_APP:
+        kiosk_app_id_ = KioskAppId::ForChromeApp(std::string());
+        KioskAppManager::Get()->AddAppForTest(std::string(), EmptyAccountId(),
+                                              GURL(), std::string());
+        break;
+      case KioskAppType::WEB_APP:
+        kiosk_app_id_ = KioskAppId::ForWebApp(EmptyAccountId());
+        break;
+    }
   }
 
   KioskLaunchController* controller() { return controller_.get(); }
 
-  WebKioskAppLauncher::Delegate* launch_controls() {
-    return static_cast<WebKioskAppLauncher::Delegate*>(controller_.get());
+  KioskAppLauncher::Delegate* launch_controls() {
+    return static_cast<KioskAppLauncher::Delegate*>(controller_.get());
   }
 
   KioskProfileLoader::Delegate* profile_controls() {
@@ -65,23 +80,6 @@ class KioskLaunchControllerTest : public InProcessBrowserTest {
     EXPECT_EQ(network_state, controller_->network_ui_state_);
   }
 
-  void ExpectKeyboardConfig() {
-    const keyboard::KeyboardConfig config =
-        ash::KeyboardController::Get()->GetKeyboardConfig();
-
-    // |auto_capitalize| is not controlled by the policy
-    // 'VirtualKeyboardFeatures', and its default value remains true.
-    EXPECT_TRUE(config.auto_capitalize);
-
-    // The other features are controlled by the policy
-    // 'VirtualKeyboardFeatures', and their default values should be false.
-    EXPECT_FALSE(config.auto_complete);
-    EXPECT_FALSE(config.auto_correct);
-    EXPECT_FALSE(config.handwriting);
-    EXPECT_FALSE(config.spell_check);
-    EXPECT_FALSE(config.voice_input);
-  }
-
   void FireSplashScreenTimer() { controller_->OnTimerFire(); }
 
   void SetOnline(bool online) {
@@ -94,7 +92,7 @@ class KioskLaunchControllerTest : public InProcessBrowserTest {
 
   FakeAppLaunchSplashScreenHandler* view() { return view_.get(); }
 
-  KioskAppId kiosk_app_id() { return KioskAppId::ForWebApp(EmptyAccountId()); }
+  KioskAppId kiosk_app_id() { return kiosk_app_id_; }
 
  private:
   ScopedCanConfigureNetwork can_configure_network_for_testing_{true, false};
@@ -103,9 +101,10 @@ class KioskLaunchControllerTest : public InProcessBrowserTest {
   std::unique_ptr<FakeAppLaunchSplashScreenHandler> view_;
   MockWebKioskAppLauncher* app_launcher_;  // owned by |controller_|.
   std::unique_ptr<KioskLaunchController> controller_;
+  KioskAppId kiosk_app_id_;
 };
 
-IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest, RegularFlow) {
+IN_PROC_BROWSER_TEST_P(KioskLaunchControllerTest, RegularFlow) {
   controller()->Start(kiosk_app_id(), false);
   ExpectState(AppState::CREATING_PROFILE, NetworkUIState::NOT_SHOWING);
 
@@ -128,11 +127,9 @@ IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest, RegularFlow) {
   launch_controls()->OnAppLaunched();
   ExpectState(AppState::LAUNCHED, NetworkUIState::NOT_SHOWING);
   EXPECT_TRUE(session_manager::SessionManager::Get()->IsSessionStarted());
-
-  ExpectKeyboardConfig();
 }
 
-IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest, AlreadyInstalled) {
+IN_PROC_BROWSER_TEST_P(KioskLaunchControllerTest, AlreadyInstalled) {
   controller()->Start(kiosk_app_id(), false);
   ExpectState(AppState::CREATING_PROFILE, NetworkUIState::NOT_SHOWING);
 
@@ -148,11 +145,9 @@ IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest, AlreadyInstalled) {
   launch_controls()->OnAppLaunched();
   ExpectState(AppState::LAUNCHED, NetworkUIState::NOT_SHOWING);
   EXPECT_TRUE(session_manager::SessionManager::Get()->IsSessionStarted());
-
-  ExpectKeyboardConfig();
 }
 
-IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
+IN_PROC_BROWSER_TEST_P(KioskLaunchControllerTest,
                        ConfigureNetworkBeforeProfile) {
   controller()->Start(kiosk_app_id(), false);
   ExpectState(AppState::CREATING_PROFILE, NetworkUIState::NOT_SHOWING);
@@ -180,11 +175,9 @@ IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
   launch_controls()->OnAppLaunched();
   ExpectState(AppState::LAUNCHED, NetworkUIState::NOT_SHOWING);
   EXPECT_TRUE(session_manager::SessionManager::Get()->IsSessionStarted());
-
-  ExpectKeyboardConfig();
 }
 
-IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
+IN_PROC_BROWSER_TEST_P(KioskLaunchControllerTest,
                        ConfigureNetworkDuringInstallation) {
   SetOnline(false);
   controller()->Start(kiosk_app_id(), false);
@@ -222,11 +215,9 @@ IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
   launch_controls()->OnAppLaunched();
   ExpectState(AppState::LAUNCHED, NetworkUIState::NOT_SHOWING);
   EXPECT_TRUE(session_manager::SessionManager::Get()->IsSessionStarted());
-
-  ExpectKeyboardConfig();
 }
 
-IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
+IN_PROC_BROWSER_TEST_P(KioskLaunchControllerTest,
                        ConnectionLostDuringInstallation) {
   controller()->Start(kiosk_app_id(), false);
   ExpectState(AppState::CREATING_PROFILE, NetworkUIState::NOT_SHOWING);
@@ -261,8 +252,11 @@ IN_PROC_BROWSER_TEST_F(KioskLaunchControllerTest,
   launch_controls()->OnAppLaunched();
   ExpectState(AppState::LAUNCHED, NetworkUIState::NOT_SHOWING);
   EXPECT_TRUE(session_manager::SessionManager::Get()->IsSessionStarted());
-
-  ExpectKeyboardConfig();
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         KioskLaunchControllerTest,
+                         testing::Values(KioskAppType::ARC_APP,
+                                         KioskAppType::CHROME_APP,
+                                         KioskAppType::WEB_APP));
 }  // namespace chromeos

chrome/browser/chromeos/login/app_mode/web_kiosk_browsertest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "ash/public/cpp/keyboard/keyboard_controller.h"
 #include "ash/public/cpp/login_screen_test_api.h"
 #include "ash/public/cpp/shelf_config.h"
 #include "ash/public/cpp/shelf_test_api.h"
@@ -17,6 +18,7 @@
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/ownership/fake_owner_settings_service.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
+#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client_test_helper.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/webui/chromeos/login/error_screen_handler.h"
@@ -132,6 +134,23 @@ class WebKioskTest : public OobeBaseTest {
     }
   }
 
+  void ExpectKeyboardConfig() {
+    const keyboard::KeyboardConfig config =
+        ash::KeyboardController::Get()->GetKeyboardConfig();
+
+    // |auto_capitalize| is not controlled by the policy
+    // 'VirtualKeyboardFeatures', and its default value remains true.
+    EXPECT_TRUE(config.auto_capitalize);
+
+    // The other features are controlled by the policy
+    // 'VirtualKeyboardFeatures', and their default values should be false.
+    EXPECT_FALSE(config.auto_complete);
+    EXPECT_FALSE(config.auto_correct);
+    EXPECT_FALSE(config.handwriting);
+    EXPECT_FALSE(config.spell_check);
+    EXPECT_FALSE(config.voice_input);
+  }
+
  private:
   NetworkPortalDetectorMixin network_portal_detector_{&mixin_host_};
   DeviceStateMixin device_state_mixin_{
@@ -254,4 +273,13 @@ IN_PROC_BROWSER_TEST_F(WebKioskTest, HiddenShelf) {
   EXPECT_FALSE(ash::ShelfTestApi().IsVisible());
 }
 
+IN_PROC_BROWSER_TEST_F(WebKioskTest, KeyboardConfigPolicy) {
+  SetOnline(true);
+  PrepareAppLaunch();
+  LaunchApp();
+  KioskSessionInitializedWaiter().Wait();
+
+  ExpectKeyboardConfig();
+}
+
 }  // namespace chromeos