Skip to content

Commit

Permalink
Remove calls to IsNetworkReady and IsShowingNetworkConfigScreen from …
Browse files Browse the repository at this point in the history 
…KioskAppInstaller

For Lacros we are looking to remove the dependency on the delegate in the ChromeAppKioskAppInstaller. To this end, this change is moving the two remaining calls out of the installer and into the StartupAppLauncher. There are some potential side effects in that network status doesn't get rechecked.

Change-Id: I6e2ebcb049b5bd6a0a9e5f8be2e0b93ba7d910f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3477234
Reviewed-by: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Anqing Zhao <anqing@chromium.org>
Commit-Queue: Ben Franz <bfranz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#998795}
  • Loading branch information
Ben Franz authored and Chromium LUCI CQ committed May 3, 2022
1 parent 0d48f9f commit 8c8d776
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 129 deletions.
110 changes: 41 additions & 69 deletions chrome/browser/ash/app_mode/startup_app_launcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,13 @@
#include "chrome/browser/chromeos/app_mode/chrome_kiosk_app_installer.h"
#include "chrome/browser/chromeos/app_mode/chrome_kiosk_app_launcher.h"
#include "chrome/browser/chromeos/app_mode/startup_app_launcher_update_checker.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_switches.h"
#include "components/crx_file/id_util.h"
#include "extensions/browser/app_window/app_window.h"
#include "extensions/browser/extension_prefs.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/common/constants.h"
#include "extensions/common/extension.h"
#include "extensions/common/file_util.h"
#include "extensions/common/manifest_handlers/kiosk_mode_info.h"
#include "extensions/common/manifest_handlers/offline_enabled_info.h"
#include "extensions/common/manifest_url_handlers.h"
#include "extensions/browser/app_window/app_window_registry.h"
#include "net/base/load_flags.h"

using extensions::Extension;

namespace ash {

namespace {
Expand All @@ -60,7 +49,28 @@ StartupAppLauncher::~StartupAppLauncher() {
}

void StartupAppLauncher::Initialize() {
MaybeInitializeNetwork();
DCHECK(state_ != LaunchState::kReadyToLaunch &&
state_ != LaunchState::kWaitingForWindow &&
state_ != LaunchState::kLaunchSucceeded);

if (delegate_->ShouldSkipAppInstallation()) {
OnInstallSuccess();
return;
}

// Wait until user has configured the network. We will come back into this
// class through ContinueWithNetworkReady.
if (delegate_->IsShowingNetworkConfigScreen()) {
state_ = LaunchState::kInitializingNetwork;
return;
}

// Update the offline enabled crx cache if the network is ready;
// or just install the app.
if (delegate_->IsNetworkReady())
ContinueWithNetworkReady();
else
BeginInstall();
}

void StartupAppLauncher::ContinueWithNetworkReady() {
Expand All @@ -69,8 +79,10 @@ void StartupAppLauncher::ContinueWithNetworkReady() {
<< static_cast<typename std::underlying_type<LaunchState>::type>(
state_);

if (state_ != LaunchState::kInitializingNetwork)
if (state_ != LaunchState::kInitializingNetwork &&
state_ != LaunchState::kNotStarted) {
return;
}

if (delegate_->ShouldSkipAppInstallation()) {
OnInstallSuccess();
Expand All @@ -97,51 +109,17 @@ void StartupAppLauncher::RestartLauncher() {
// If the installer is still running in the background, we don't need to
// restart the launch process. We will just wait until it completes and
// launches the kiosk app.
if (extensions::ExtensionSystem::Get(profile_)
->extension_service()
->pending_extension_manager()
->IsIdPending(app_id_)) {
if (installer_) {
SYSLOG(WARNING) << "Installer still running";
return;
}

MaybeInitializeNetwork();
}

void StartupAppLauncher::MaybeInitializeNetwork() {
DCHECK(state_ != LaunchState::kReadyToLaunch &&
state_ != LaunchState::kWaitingForWindow &&
state_ != LaunchState::kLaunchSucceeded);

const Extension* extension = GetPrimaryAppExtension();
bool crx_cached = KioskAppManager::Get()->HasCachedCrx(app_id_);
const bool requires_network =
(!extension && !crx_cached) ||
(extension &&
!extensions::OfflineEnabledInfo::IsOfflineEnabled(extension));

SYSLOG(INFO) << "MaybeInitializeNetwork"
<< ", requires_network=" << requires_network
<< ", network_ready=" << delegate_->IsNetworkReady();

state_ = LaunchState::kInitializingNetwork;

if (requires_network) {
delegate_->InitializeNetwork();
if (launcher_) {
SYSLOG(WARNING) << "Launcher is running";
return;
}

if (delegate_->ShouldSkipAppInstallation()) {
OnInstallSuccess();
return;
}

// Update the offline enabled crx cache if the network is ready;
// or just install the app.
if (delegate_->IsNetworkReady())
ContinueWithNetworkReady();
else
BeginInstall();
Initialize();
}

bool StartupAppLauncher::RetryWhenNetworkIsAvailable() {
Expand All @@ -166,8 +144,9 @@ void StartupAppLauncher::OnKioskExtensionDownloadFailed(

void StartupAppLauncher::OnKioskAppDataLoadStatusChanged(
const std::string& app_id) {
if (state_ != LaunchState::kWaitingForCache)
return;
DCHECK(state_ == LaunchState::kWaitingForCache);

kiosk_app_manager_observation_.Reset();

if (app_id != app_id_)
return;
Expand All @@ -178,18 +157,11 @@ void StartupAppLauncher::OnKioskAppDataLoadStatusChanged(
OnLaunchFailure(KioskAppLaunchError::Error::kUnableToDownload);
}

const extensions::Extension* StartupAppLauncher::GetPrimaryAppExtension()
const {
return extensions::ExtensionRegistry::Get(profile_)->GetInstalledExtension(
app_id_);
}

void StartupAppLauncher::BeginInstall() {
state_ = LaunchState::kInstallingApp;
delegate_->OnAppInstalling();
installer_ = std::make_unique<ChromeKioskAppInstaller>(
profile_, KioskAppManager::Get()->CreatePrimaryAppInstallData(app_id_),
delegate_);
profile_, KioskAppManager::Get()->CreatePrimaryAppInstallData(app_id_));
installer_->BeginInstall(base::BindOnce(
&StartupAppLauncher::OnInstallComplete, weak_ptr_factory_.GetWeakPtr()));
}
Expand All @@ -204,24 +176,24 @@ void StartupAppLauncher::OnInstallComplete(
case ChromeKioskAppInstaller::InstallResult::kSuccess:
OnInstallSuccess();
return;
case ChromeKioskAppInstaller::InstallResult::kUnableToInstall:
case ChromeKioskAppInstaller::InstallResult::kUnableToInstallPrimaryApp:
OnLaunchFailure(KioskAppLaunchError::Error::kUnableToInstall);
return;
case ChromeKioskAppInstaller::InstallResult::kNotKioskEnabled:
OnLaunchFailure(KioskAppLaunchError::Error::kNotKioskEnabled);
return;
case ChromeKioskAppInstaller::InstallResult::kNetworkMissing:
if (!RetryWhenNetworkIsAvailable())
OnLaunchFailure(KioskAppLaunchError::Error::kUnableToLaunch);
case ChromeKioskAppInstaller::InstallResult::kPrimaryAppNotCached:
case ChromeKioskAppInstaller::InstallResult::kUnableToInstallSecondaryApp:
if (!RetryWhenNetworkIsAvailable()) {
OnLaunchFailure(KioskAppLaunchError::Error::kUnableToInstall);
}
return;
}
}

void StartupAppLauncher::OnInstallSuccess() {
state_ = LaunchState::kReadyToLaunch;
// Updates to cached primary app crx will be ignored after this point, so
// there is no need to observe the kiosk app manager any longer.
kiosk_app_manager_observation_.Reset();

delegate_->OnAppPrepared();
}

Expand Down
8 changes: 0 additions & 8 deletions chrome/browser/ash/app_mode/startup_app_launcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,13 @@
#include <memory>
#include <string>

#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/scoped_observation.h"
#include "chrome/browser/ash/app_mode/kiosk_app_launcher.h"
#include "chrome/browser/ash/app_mode/kiosk_app_manager.h"
#include "chrome/browser/ash/app_mode/kiosk_app_manager_observer.h"
#include "chrome/browser/chromeos/app_mode/chrome_kiosk_app_installer.h"
#include "chrome/browser/chromeos/app_mode/chrome_kiosk_app_launcher.h"
#include "chrome/browser/extensions/install_observer.h"
#include "chrome/browser/extensions/install_tracker.h"
#include "extensions/browser/app_window/app_window_registry.h"

class Profile;
Expand All @@ -28,8 +25,6 @@ class AppWindowRegistry;

namespace ash {

class StartupAppLauncherUpdateChecker;

// Responsible for the startup of the app for Chrome App kiosk.
class StartupAppLauncher : public KioskAppLauncher,
public KioskAppManagerObserver,
Expand Down Expand Up @@ -73,15 +68,12 @@ class StartupAppLauncher : public KioskAppLauncher,
void OnLaunchSuccess();
void OnLaunchFailure(KioskAppLaunchError::Error error);

void MaybeInitializeNetwork();
bool RetryWhenNetworkIsAvailable();
void OnKioskAppDataLoadStatusChanged(const std::string& app_id);

// AppWindowRegistry::Observer:
void OnAppWindowAdded(extensions::AppWindow* app_window) override;

const extensions::Extension* GetPrimaryAppExtension() const;

// KioskAppManagerObserver overrides.
void OnKioskExtensionLoadedInCache(const std::string& app_id) override;
void OnKioskExtensionDownloadFailed(const std::string& app_id) override;
Expand Down
73 changes: 61 additions & 12 deletions chrome/browser/ash/app_mode/startup_app_launcher_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ class TestKioskLoaderVisitor
pending_update_urls_.erase(extension_id);
extensions::InstallTracker::Get(browser_context_)
->OnFinishCrxInstall(extension_id, false);
extension_service_->pending_extension_manager()->Remove(extension_id);
return true;
}

Expand Down Expand Up @@ -416,15 +417,9 @@ class StartupAppLauncherTest : public extensions::ExtensionServiceTestBase,
}

void InitializeLauncherWithNetworkReady() {
startup_app_launcher_->Initialize();
EXPECT_EQ(std::vector<LaunchState>({LaunchState::kInitializingNetwork}),
startup_launch_delegate_.launch_state_changes());
startup_launch_delegate_.ClearLaunchStateChanges();

startup_launch_delegate_.set_network_ready(true);
startup_app_launcher_->ContinueWithNetworkReady();
EXPECT_TRUE(startup_launch_delegate_.launch_state_changes().empty());
startup_launch_delegate_.ClearLaunchStateChanges();
startup_app_launcher_->Initialize();
EXPECT_THAT(startup_launch_delegate_.launch_state_changes(), ElementsAre());
}

[[nodiscard]] AssertionResult DownloadPrimaryApp(
Expand Down Expand Up @@ -1057,6 +1052,9 @@ TEST_F(StartupAppLauncherTest,

ASSERT_TRUE(FinishPrimaryAppInstall(primary_app_builder));

ASSERT_TRUE(
external_apps_loader_handler_->FailPendingInstall(kSecondaryAppId));

// After install is complete we should realize that the app needs to install
// secondary apps, so we need to get network set up
startup_launch_delegate_.WaitForLaunchStates(
Expand Down Expand Up @@ -1086,6 +1084,42 @@ TEST_F(StartupAppLauncherTest,
startup_launch_delegate_.launch_state_changes());
}

TEST_F(StartupAppLauncherTest,
OfflineInstallUncachedExtensionShouldForceNetwork) {
TestKioskExtensionBuilder primary_app_builder(Manifest::TYPE_PLATFORM_APP,
kTestPrimaryAppId);
primary_app_builder.set_version("1.0");
scoped_refptr<const extensions::Extension> primary_app =
primary_app_builder.Build();

startup_app_launcher_->Initialize();

EXPECT_THAT(startup_launch_delegate_.launch_state_changes(),
ElementsAre(LaunchState::kInstallingApp,
LaunchState::kInitializingNetwork));
startup_launch_delegate_.ClearLaunchStateChanges();

startup_launch_delegate_.set_network_ready(true);
startup_app_launcher_->ContinueWithNetworkReady();

ASSERT_TRUE(DownloadPrimaryApp(primary_app_builder));

EXPECT_THAT(startup_launch_delegate_.launch_state_changes(),
ElementsAre(LaunchState::kInstallingApp));
startup_launch_delegate_.ClearLaunchStateChanges();

ASSERT_TRUE(FinishPrimaryAppInstall(primary_app_builder));

EXPECT_THAT(startup_launch_delegate_.launch_state_changes(),
ElementsAre(LaunchState::kReadyToLaunch));
startup_launch_delegate_.ClearLaunchStateChanges();

startup_app_launcher_->LaunchApp();

EXPECT_THAT(startup_launch_delegate_.launch_state_changes(),
ElementsAre(LaunchState::kLaunchSucceeded));
}

TEST_F(StartupAppLauncherTest, IgnoreSecondaryAppsSecondaryApps) {
InitializeLauncherWithNetworkReady();

Expand Down Expand Up @@ -1126,7 +1160,7 @@ TEST_F(StartupAppLauncherTest, IgnoreSecondaryAppsSecondaryApps) {
EXPECT_TRUE(kiosk_app_session_initialized_);
}

TEST_F(StartupAppLauncherTest, SecondaryAppCrxInstallFailure) {
TEST_F(StartupAppLauncherTest, SecondaryAppCrxInstallFailureTriggersRetry) {
InitializeLauncherWithNetworkReady();

TestKioskExtensionBuilder primary_app_builder(Manifest::TYPE_PLATFORM_APP,
Expand All @@ -1141,10 +1175,25 @@ TEST_F(StartupAppLauncherTest, SecondaryAppCrxInstallFailure) {
ASSERT_TRUE(
external_apps_loader_handler_->FailPendingInstall(kSecondaryAppId));

EXPECT_EQ(KioskAppLaunchError::Error::kUnableToInstall,
startup_launch_delegate_.launch_error());
// The retry mechanism should trigger a new request to initialize the network
startup_launch_delegate_.WaitForLaunchStates(
{LaunchState::kInitializingNetwork});

EXPECT_FALSE(kiosk_app_session_initialized_);
startup_app_launcher_->ContinueWithNetworkReady();

ASSERT_TRUE(DownloadPrimaryApp(primary_app_builder));

startup_launch_delegate_.WaitForLaunchStates({LaunchState::kInstallingApp});
startup_launch_delegate_.ClearLaunchStateChanges();

ASSERT_EQ(std::set<std::string>({kSecondaryAppId}),
external_apps_loader_handler_->pending_update_urls());
TestKioskExtensionBuilder secondary_app_builder(Manifest::TYPE_PLATFORM_APP,
kSecondaryAppId);
secondary_app_builder.set_kiosk_enabled(false);
ASSERT_TRUE(FinishSecondaryExtensionInstall(secondary_app_builder));

startup_launch_delegate_.WaitForLaunchStates({LaunchState::kReadyToLaunch});
}

TEST_F(StartupAppLauncherTest,
Expand Down
Loading

0 comments on commit 8c8d776

Please sign in to comment.