Do not assume OobeUI is around when deleting AppLaunchController

Currently, AppLaunchController assumes that AppLaunchSplashScreenView,
owned by OobeUI, is around during AppLaunchController destruction. Both
AppLaunchController and OobeUI lifetime is controlled by
LoginDisplayHost, but OobeUI is technically owned by login window
(which gets created by LoginDisplayHost, and closed on LoginDisplayHost
destruction).

Whether AppLaunchController get deleted before or after
AppLaunchSplashScreenView largely depends on how the login window is
closed/destroyed - generally the window is destroyed asynchronously
when LoginDisplayHost goes out of scope, in which case AppLaunchController
is deleted before the splash screen view, but this does not seem to be
guaranteed.

For example, in AutoLaunchedNonKioskEnabledAppTest.NotLaunched test with
mash enabled, oobe_ui gets deleted synchronously when ScopedKeepAlive
owned by LoginDisplayHostCommon gets deleted (which happens before the
AppLaunchController is deleted) - ScopedKeepAlive going out of scope
causes MusClient to start closing all widgets.

To work around this have AppLaunchSplashScreenView notify its delegate
(i.e. AppLaunchController) when it gets deleted, so the delegate can stop
referencing it when that happens.

Technically, the linked bug could be fixed by ensuring that app launch
controller is deleted before ScopedKeepAlive, but that seems more
fragile, and it doesn't necessarily address all concerns (e.g. the case
where login window is removed for a reason other than login display host
destruction).

NOTE: The same applies to the ArcKioskController and
ArcKioskSplashScreenView.

Bug: 838992

Change-Id: I9b31293e791c1f8980e780e69f6061f87a968095
Reviewed-on: https://chromium-review.googlesource.com/1041485Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556181}

chrome/browser/chromeos/login/app_launch_controller.cc

@@ -157,7 +157,8 @@ AppLaunchController::AppLaunchController(const std::string& app_id,
 }
 
 AppLaunchController::~AppLaunchController() {
-  app_launch_splash_screen_view_->SetDelegate(nullptr);
+  if (app_launch_splash_screen_view_)
+    app_launch_splash_screen_view_->SetDelegate(nullptr);
 }
 
 void AppLaunchController::StartAppLaunch(bool is_auto_launch) {
@@ -303,6 +304,10 @@ void AppLaunchController::OnNetworkStateChanged(bool online) {
     MaybeShowNetworkConfigureUI();
 }
 
+void AppLaunchController::OnDeletingSplashScreenView() {
+  app_launch_splash_screen_view_ = nullptr;
+}
+
 void AppLaunchController::OnProfileLoaded(Profile* profile) {
   SYSLOG(INFO) << "Profile loaded... Starting app launch.";
   profile_ = profile;
@@ -344,8 +349,7 @@ void AppLaunchController::CleanUp() {
   startup_app_launcher_.reset();
   splash_wait_timer_.Stop();
 
-  if (host_)
-    host_->Finalize(base::OnceClosure());
+  host_->Finalize(base::OnceClosure());
 }
 
 void AppLaunchController::OnNetworkWaitTimedout() {
@@ -392,6 +396,9 @@ bool AppLaunchController::NeedOwnerAuthToConfigureNetwork() {
 }
 
 void AppLaunchController::MaybeShowNetworkConfigureUI() {
+  if (!app_launch_splash_screen_view_)
+    return;
+
   if (CanConfigureNetwork()) {
     if (NeedOwnerAuthToConfigureNetwork()) {
       if (network_config_requested_)
@@ -408,6 +415,9 @@ void AppLaunchController::MaybeShowNetworkConfigureUI() {
 }
 
 void AppLaunchController::ShowNetworkConfigureUIWhenReady() {
+  if (!app_launch_splash_screen_view_)
+    return;
+
   if (!profile_) {
     show_network_config_ui_after_profile_load_ = true;
     app_launch_splash_screen_view_->UpdateAppLaunchState(
@@ -422,6 +432,9 @@ void AppLaunchController::ShowNetworkConfigureUIWhenReady() {
 }
 
 void AppLaunchController::InitializeNetwork() {
+  if (!app_launch_splash_screen_view_)
+    return;
+
   // Show the network configuration dialog if network is not initialized
   // after a brief wait time.
   waiting_for_network_ = true;
@@ -434,7 +447,8 @@ void AppLaunchController::InitializeNetwork() {
 }
 
 bool AppLaunchController::IsNetworkReady() {
-  return app_launch_splash_screen_view_->IsNetworkReady();
+  return app_launch_splash_screen_view_ &&
+         app_launch_splash_screen_view_->IsNetworkReady();
 }
 
 bool AppLaunchController::ShouldSkipAppInstallation() {
@@ -442,6 +456,9 @@ bool AppLaunchController::ShouldSkipAppInstallation() {
 }
 
 void AppLaunchController::OnInstallingApp() {
+  if (!app_launch_splash_screen_view_)
+    return;
+
   app_launch_splash_screen_view_->UpdateAppLaunchState(
       AppLaunchSplashScreenView::APP_LAUNCH_STATE_INSTALLING_APPLICATION);
 
@@ -495,8 +512,10 @@ void AppLaunchController::OnReadyToLaunch() {
 
 void AppLaunchController::OnLaunchSucceeded() {
   SYSLOG(INFO) << "Kiosk launch succeeded, wait for app window.";
-  app_launch_splash_screen_view_->UpdateAppLaunchState(
-      AppLaunchSplashScreenView::APP_LAUNCH_STATE_WAITING_APP_WINDOW);
+  if (app_launch_splash_screen_view_) {
+    app_launch_splash_screen_view_->UpdateAppLaunchState(
+        AppLaunchSplashScreenView::APP_LAUNCH_STATE_WAITING_APP_WINDOW);
+  }
 
   DCHECK(!app_window_watcher_);
   app_window_watcher_.reset(new AppWindowWatcher(this, app_id_));
@@ -517,8 +536,8 @@ void AppLaunchController::OnLaunchFailed(KioskAppLaunchError::Error error) {
 
   // Saves the error and ends the session to go back to login screen.
   KioskAppLaunchError::Save(error);
-  chrome::AttemptUserExit();
   CleanUp();
+  chrome::AttemptUserExit();
 }
 
 bool AppLaunchController::IsShowingNetworkConfigScreen() {

chrome/browser/chromeos/login/app_launch_controller.h

@@ -97,6 +97,7 @@ class AppLaunchController : public AppLaunchSplashScreenView::Delegate,
   void OnCancelAppLaunch() override;
   void OnNetworkConfigRequested(bool requested) override;
   void OnNetworkStateChanged(bool online) override;
+  void OnDeletingSplashScreenView() override;
 
   // StartupAppLauncher::Delegate overrides:
   void InitializeNetwork() override;

chrome/browser/chromeos/login/arc_kiosk_controller.cc

@@ -34,7 +34,8 @@ ArcKioskController::ArcKioskController(LoginDisplayHost* host, OobeUI* oobe_ui)
       weak_ptr_factory_(this) {}
 
 ArcKioskController::~ArcKioskController() {
-  arc_kiosk_splash_screen_view_->SetDelegate(nullptr);
+  if (arc_kiosk_splash_screen_view_)
+    arc_kiosk_splash_screen_view_->SetDelegate(nullptr);
 }
 
 void ArcKioskController::StartArcKiosk(const AccountId& account_id) {
@@ -57,8 +58,8 @@ void ArcKioskController::CleanUp() {
   // Delegate is registered only when |profile_| is set.
   if (profile_)
     ArcKioskAppService::Get(profile_)->SetDelegate(nullptr);
-  if (host_)
-    host_->Finalize(base::OnceClosure());
+
+  host_->Finalize(base::OnceClosure());
 }
 
 void ArcKioskController::CloseSplashScreen() {
@@ -72,8 +73,8 @@ void ArcKioskController::OnAuthFailure(const AuthFailure& error) {
   LOG(ERROR) << "ARC Kiosk launch failed. Will now shut down, error="
              << error.GetErrorString();
   KioskAppLaunchError::Save(KioskAppLaunchError::ARC_AUTH_FAILED);
-  chrome::AttemptUserExit();
   CleanUp();
+  chrome::AttemptUserExit();
 }
 
 void ArcKioskController::OnAuthSuccess(const UserContext& user_context) {
@@ -124,14 +125,19 @@ void ArcKioskController::OnProfilePrepared(Profile* profile,
   // a profile load, so invalidate the delegate now.
   UserSessionManager::GetInstance()->DelegateDeleted(this);
   ArcKioskAppService::Get(profile_)->SetDelegate(this);
-  arc_kiosk_splash_screen_view_->UpdateArcKioskState(
-      ArcKioskSplashScreenView::ArcKioskState::WAITING_APP_LAUNCH);
+  if (arc_kiosk_splash_screen_view_) {
+    arc_kiosk_splash_screen_view_->UpdateArcKioskState(
+        ArcKioskSplashScreenView::ArcKioskState::WAITING_APP_LAUNCH);
+  }
 }
 
 void ArcKioskController::OnAppStarted() {
   DVLOG(1) << "ARC Kiosk launch succeeded, wait for app window.";
-  arc_kiosk_splash_screen_view_->UpdateArcKioskState(
-      ArcKioskSplashScreenView::ArcKioskState::WAITING_APP_WINDOW);
+
+  if (arc_kiosk_splash_screen_view_) {
+    arc_kiosk_splash_screen_view_->UpdateArcKioskState(
+        ArcKioskSplashScreenView::ArcKioskState::WAITING_APP_WINDOW);
+  }
 }
 
 void ArcKioskController::OnAppWindowLaunched() {
@@ -150,4 +156,8 @@ void ArcKioskController::OnCancelArcKioskLaunch() {
   chrome::AttemptUserExit();
 }
 
+void ArcKioskController::OnDeletingSplashScreenView() {
+  arc_kiosk_splash_screen_view_ = nullptr;
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/login/arc_kiosk_controller.h

@@ -62,11 +62,12 @@ class ArcKioskController : public LoginPerformer::Delegate,
 
   // ArcKioskSplashScreenView::Delegate implementation:
   void OnCancelArcKioskLaunch() override;
+  void OnDeletingSplashScreenView() override;
 
   // LoginDisplayHost owns itself.
   LoginDisplayHost* const host_;
   // Owned by OobeUI.
-  ArcKioskSplashScreenView* const arc_kiosk_splash_screen_view_;
+  ArcKioskSplashScreenView* arc_kiosk_splash_screen_view_;
   // Not owning here.
   Profile* profile_ = nullptr;
 

chrome/browser/chromeos/login/auto_launched_kiosk_browsertest.cc

@@ -542,9 +542,7 @@ class AutoLaunchedNonKioskEnabledAppTest : public AutoLaunchedKioskTest {
   DISALLOW_COPY_AND_ASSIGN(AutoLaunchedNonKioskEnabledAppTest);
 };
 
-// Disabled due to flaky failures: https://crbug.com/838992
-IN_PROC_BROWSER_TEST_F(AutoLaunchedNonKioskEnabledAppTest,
-                       DISABLED_NotLaunched) {
+IN_PROC_BROWSER_TEST_F(AutoLaunchedNonKioskEnabledAppTest, NotLaunched) {
   // Verify that Chrome hasn't already exited, e.g. in order to apply user
   // session flags.
   ASSERT_FALSE(termination_observer_->terminated());

chrome/browser/chromeos/login/screens/app_launch_splash_screen_view.h

@@ -36,6 +36,9 @@ class AppLaunchSplashScreenView {
     // is connected to the Internet.
     virtual void OnNetworkStateChanged(bool online) = 0;
 
+    // Invoked when the splash screen view is being deleted.
+    virtual void OnDeletingSplashScreenView() = 0;
+
    protected:
     virtual ~Delegate() {}
   };

chrome/browser/chromeos/login/screens/arc_kiosk_splash_screen_view.h

@@ -24,6 +24,9 @@ class ArcKioskSplashScreenView {
     // Invoked when the launch bailout shortcut key is pressed.
     virtual void OnCancelArcKioskLaunch() = 0;
 
+    // Invoked when the splash screen view gets being deleted.
+    virtual void OnDeletingSplashScreenView() = 0;
+
    protected:
     virtual ~Delegate() = default;
 

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

@@ -52,6 +52,8 @@ AppLaunchSplashScreenHandler::AppLaunchSplashScreenHandler(
 
 AppLaunchSplashScreenHandler::~AppLaunchSplashScreenHandler() {
   network_state_informer_->RemoveObserver(this);
+  if (delegate_)
+    delegate_->OnDeletingSplashScreenView();
 }
 
 void AppLaunchSplashScreenHandler::DeclareLocalizedValues(

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

@@ -28,7 +28,10 @@ ArcKioskSplashScreenHandler::ArcKioskSplashScreenHandler()
   set_call_js_prefix(kJsScreenPath);
 }
 
-ArcKioskSplashScreenHandler::~ArcKioskSplashScreenHandler() = default;
+ArcKioskSplashScreenHandler::~ArcKioskSplashScreenHandler() {
+  if (delegate_)
+    delegate_->OnDeletingSplashScreenView();
+}
 
 void ArcKioskSplashScreenHandler::DeclareLocalizedValues(
     ::login::LocalizedValuesBuilder* builder) {