Skip to content

Commit

Permalink
[APS] Only preload apps for new users, retrying errors on next startup
Browse files Browse the repository at this point in the history 
We now store two separate prefs for the APS first run process: whether
the process has been started (set immediately on first run for new
users), and whether the process has completed without errors. This
allows us to retry when the flow has been started but not completed (due
to network errors, crashes, or other issues).

Also adds a base::Feature for debugging purposes to force the preload
service to run on an existing profile.

Bug: b/259152331
Change-Id: Ibd3942284b5469c34c962b19014a5b00c5a96e20
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4054444
Auto-Submit: Tim Sergeant <tsergeant@chromium.org>
Commit-Queue: Tim Sergeant <tsergeant@chromium.org>
Reviewed-by: Melissa Zhang <melzhang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075664}
  • Loading branch information
@tgsergeant
tgsergeant authored and Chromium LUCI CQ committed Nov 25, 2022
1 parent 4b8224e commit 824244f
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 47 deletions.
47 changes: 36 additions & 11 deletions chrome/browser/apps/app_preload_service/app_preload_service.cc
View file
Expand Up @@ -9,8 +9,10 @@

#include "base/barrier_callback.h"
#include "base/containers/cxx20_erase_set.h"
#include "base/feature_list.h"
#include "base/functional/bind.h"
#include "base/logging.h"
#include "base/ranges/algorithm.h"
#include "chrome/browser/apps/app_preload_service/app_preload_service_factory.h"
#include "chrome/browser/apps/app_preload_service/device_info_manager.h"
#include "chrome/browser/apps/app_preload_service/preload_app_definition.h"
Expand All @@ -20,18 +22,21 @@
#include "components/prefs/pref_service.h"
#include "components/prefs/scoped_user_pref_update.h"
#include "components/services/app_service/public/cpp/app_types.h"
#include "components/user_manager/user_manager.h"

namespace {

// The pref dict is:
// {
// ...
// "apps.app_preload_service.state_manager": {
// first_run_completed: <bool>,
// "first_login_flow_started": <bool>,
// "first_login_flow_completed": <bool>
// },
// ...
// }

static constexpr char kFirstLoginFlowStartedKey[] = "first_login_flow_started";
static constexpr char kFirstLoginFlowCompletedKey[] =
"first_login_flow_completed";

Expand All @@ -44,16 +49,30 @@ static constexpr char kApsStateManager[] =
"apps.app_preload_service.state_manager";
} // namespace prefs

BASE_FEATURE(kAppPreloadServiceForceRun,
"AppPreloadServiceForceRun",
base::FEATURE_DISABLED_BY_DEFAULT);

AppPreloadService::AppPreloadService(Profile* profile)
: profile_(profile),
server_connector_(std::make_unique<AppPreloadServerConnector>()),
device_info_manager_(std::make_unique<DeviceInfoManager>(profile)),
web_app_installer_(std::make_unique<WebAppPreloadInstaller>(profile)) {
// Check to see if the service has been run before.
auto is_first_run = GetStateManager().FindBool(kFirstLoginFlowCompletedKey);
if (is_first_run == absl::nullopt) {
// the first run completed key has not been set, kick off the initial app
// installation flow.
// Preloads currently run for new users only. The "completed" pref is only set
// when preloads finish successfully, so preloads will be retried if they have
// been "started" but never "completed".
if (user_manager::UserManager::Get()->IsCurrentUserNew()) {
ScopedDictPrefUpdate(profile_->GetPrefs(), prefs::kApsStateManager)
->Set(kFirstLoginFlowStartedKey, true);
}

bool first_run_started =
GetStateManager().FindBool(kFirstLoginFlowStartedKey).value_or(false);
bool first_run_complete =
GetStateManager().FindBool(kFirstLoginFlowCompletedKey).value_or(false);

if ((first_run_started && !first_run_complete) ||
base::FeatureList::IsEnabled(kAppPreloadServiceForceRun)) {
device_info_manager_->GetDeviceInfo(
base::BindOnce(&AppPreloadService::StartAppInstallationForFirstLogin,
weak_ptr_factory_.GetWeakPtr()));
Expand Down Expand Up @@ -102,12 +121,18 @@ void AppPreloadService::OnGetAppsForFirstLoginCompleted(

void AppPreloadService::OnAllAppInstallationFinished(
const std::vector<bool>& results) {
// TODO(b/259152331): Handle failed app installs.
ScopedDictPrefUpdate(profile_->GetPrefs(), prefs::kApsStateManager)
->Set(kFirstLoginFlowCompletedKey, true);
OnFirstLoginFlowComplete(
base::ranges::all_of(results, [](bool b) { return b; }));
}

void AppPreloadService::OnFirstLoginFlowComplete(bool success) {
if (success) {
ScopedDictPrefUpdate(profile_->GetPrefs(), prefs::kApsStateManager)
->Set(kFirstLoginFlowCompletedKey, true);
}

if (check_first_pref_set_callback_) {
std::move(check_first_pref_set_callback_).Run();
if (installation_complete_callback_) {
std::move(installation_complete_callback_).Run(success);
}
}

Expand Down
25 changes: 17 additions & 8 deletions chrome/browser/apps/app_preload_service/app_preload_service.h
View file
Expand Up @@ -7,6 +7,7 @@

#include <memory>

#include "base/feature_list.h"
#include "base/functional/callback.h"
#include "base/gtest_prod_util.h"
#include "base/memory/raw_ptr.h"
Expand All @@ -29,6 +30,10 @@ class WebAppPreloadInstaller;

struct DeviceInfo;

// Debugging feature to always run the App Preload Service on startup, even if
// the Profile would not normally be eligible.
BASE_DECLARE_FEATURE(kAppPreloadServiceForceRun);

class AppPreloadService : public KeyedService {
public:
explicit AppPreloadService(Profile* profile);
Expand All @@ -41,20 +46,24 @@ class AppPreloadService : public KeyedService {
// Registers prefs used for state management of the App Preload Service.
static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);

void SetInstallationCompleteCallbackForTesting(
base::OnceCallback<void(bool)> callback) {
installation_complete_callback_ = std::move(callback);
}

private:
// This function begins the process to get a list of apps from the back end
// service, processes the list and installs the app list. This call should
// only be used the first time a profile is created on the device as this call
// installs a set of default and OEM apps.
void StartAppInstallationForFirstLogin(DeviceInfo device_info);

private:
friend class AppPreloadServiceTest;
FRIEND_TEST_ALL_PREFIXES(AppPreloadServiceTest, FirstLoginPrefSet);
FRIEND_TEST_ALL_PREFIXES(AppPreloadServiceTest, WebAppInstall);

// Processes the list of apps retrieved by the server connector.
void OnGetAppsForFirstLoginCompleted(std::vector<PreloadAppDefinition> apps);
void OnAllAppInstallationFinished(const std::vector<bool>& results);
// Called when the installation flow started by
// `StartAppInstallationForFirstLogin` is complete, with `success` indicating
// whether the overall flow was successful.
void OnFirstLoginFlowComplete(bool success);

const base::Value::Dict& GetStateManager() const;

Expand All @@ -64,9 +73,9 @@ class AppPreloadService : public KeyedService {
std::unique_ptr<WebAppPreloadInstaller> web_app_installer_;

// For testing
base::OnceClosure check_first_pref_set_callback_;
base::OnceCallback<void(bool)> installation_complete_callback_;

// |weak_ptr_factory_| must be the last member of this class.
// `weak_ptr_factory_` must be the last member of this class.
base::WeakPtrFactory<AppPreloadService> weak_ptr_factory_{this};
};

Expand Down
80 changes: 52 additions & 28 deletions chrome/browser/apps/app_preload_service/app_preload_service_unittest.cc
View file
Expand Up @@ -17,13 +17,16 @@
#include "chrome/browser/apps/app_preload_service/proto/app_provisioning.pb.h"
#include "chrome/browser/apps/app_service/app_service_proxy.h"
#include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
#include "chrome/browser/ash/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/web_applications/test/web_app_install_test_utils.h"
#include "chrome/browser/web_applications/web_app_helpers.h"
#include "chrome/common/chrome_features.h"
#include "chrome/test/base/testing_profile.h"
#include "components/prefs/pref_service.h"
#include "components/services/app_service/public/cpp/app_registry_cache.h"
#include "components/services/app_service/public/cpp/app_update.h"
#include "components/user_manager/scoped_user_manager.h"
#include "components/user_manager/user_manager.h"
#include "content/public/test/browser_task_environment.h"
#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
#include "services/network/test/test_url_loader_factory.h"
Expand All @@ -32,6 +35,7 @@

namespace {

constexpr char kFirstLoginFlowStartedKey[] = "first_login_flow_started";
constexpr char kFirstLoginFlowCompletedKey[] = "first_login_flow_completed";

constexpr char kApsStateManager[] = "apps.app_preload_service.state_manager";
Expand All @@ -45,26 +49,17 @@ const base::Value::Dict& GetStateManager(Profile* profile) {
namespace apps {

class AppPreloadServiceTest : public testing::Test {
public:
void VerifyFirstLoginPrefSet(base::OnceClosure on_complete) {
// We expect that the key has been set after the first login flow has been
// completed.
auto flow_completed =
GetStateManager(GetProfile()).FindBool(kFirstLoginFlowCompletedKey);
EXPECT_NE(flow_completed, absl::nullopt);
EXPECT_TRUE(flow_completed.value());

std::move(on_complete).Run();
}

protected:
AppPreloadServiceTest() {
AppPreloadServiceTest()
: scoped_user_manager_(std::make_unique<ash::FakeChromeUserManager>()) {
scoped_feature_list_.InitAndEnableFeature(features::kAppPreloadService);
}

void SetUp() override {
testing::Test::SetUp();

GetFakeUserManager()->set_current_user_new(true);

TestingProfile::Builder profile_builder;
profile_builder.SetSharedURLLoaderFactory(
base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
Expand All @@ -76,12 +71,18 @@ class AppPreloadServiceTest : public testing::Test {

Profile* GetProfile() { return profile_.get(); }

ash::FakeChromeUserManager* GetFakeUserManager() const {
return static_cast<ash::FakeChromeUserManager*>(
user_manager::UserManager::Get());
}

network::TestURLLoaderFactory url_loader_factory_;

private:
content::BrowserTaskEnvironment task_environment_;
base::test::ScopedFeatureList scoped_feature_list_;
std::unique_ptr<TestingProfile> profile_;
user_manager::ScopedUserManager scoped_user_manager_;
};

TEST_F(AppPreloadServiceTest, ServiceAccessPerProfile) {
Expand Down Expand Up @@ -121,28 +122,51 @@ TEST_F(AppPreloadServiceTest, ServiceAccessPerProfile) {
EXPECT_NE(otr_guest_service, service);
}

TEST_F(AppPreloadServiceTest, FirstLoginPrefSet) {
TEST_F(AppPreloadServiceTest, FirstLoginStartedPrefSet) {
// Ensure that the AppPreloadService is created.
AppPreloadService::Get(GetProfile());

auto flow_started =
GetStateManager(GetProfile()).FindBool(kFirstLoginFlowStartedKey);
auto flow_completed =
GetStateManager(GetProfile()).FindBool(kFirstLoginFlowCompletedKey);
// Since we're creating a new profile with no saved state, we expect the state
// to be "started", but not "completed".
EXPECT_TRUE(flow_started.has_value() && flow_started.value());
EXPECT_EQ(flow_completed, absl::nullopt);
}

TEST_F(AppPreloadServiceTest, FirstLoginCompletedPrefSetAfterSuccess) {
proto::AppProvisioningResponse response;
auto* app = response.add_apps_to_install();
app->set_name("Peanut Types");

url_loader_factory_.AddResponse(
AppPreloadServerConnector::GetServerUrl().spec(),
response.SerializeAsString());

base::test::TestFuture<bool> result;
auto* service = AppPreloadService::Get(GetProfile());
service->SetInstallationCompleteCallbackForTesting(result.GetCallback());
ASSERT_TRUE(result.Get());

// We expect that the key has been set after the first login flow has been
// completed.
auto flow_completed =
GetStateManager(GetProfile()).FindBool(kFirstLoginFlowCompletedKey);
// Since we're creating a new profile with no saved state, we expect the
// absence of the key.
EXPECT_EQ(flow_completed, absl::nullopt);
EXPECT_NE(flow_completed, absl::nullopt);
EXPECT_TRUE(flow_completed.value());
}

base::RunLoop run_loop;
auto* service = AppPreloadService::Get(GetProfile());
service->check_first_pref_set_callback_ =
base::BindOnce(&AppPreloadServiceTest::VerifyFirstLoginPrefSet,
base::Unretained(this), run_loop.QuitClosure());
TEST_F(AppPreloadServiceTest, FirstLoginExistingUserNotStarted) {
GetFakeUserManager()->set_current_user_new(false);
TestingProfile existing_user_profile;

// Ensure that the AppPreloadService is created.
AppPreloadService::Get(&existing_user_profile);

run_loop.Run();
auto flow_started = GetStateManager(&existing_user_profile)
.FindBool(kFirstLoginFlowStartedKey);
// Existing users should not start the first-login flow.
EXPECT_FALSE(flow_started.has_value());
}

TEST_F(AppPreloadServiceTest, WebAppInstall) {
Expand All @@ -161,10 +185,10 @@ TEST_F(AppPreloadServiceTest, WebAppInstall) {
AppPreloadServerConnector::GetServerUrl().spec(),
response.SerializeAsString());

base::test::TestFuture<void> result;
base::test::TestFuture<bool> result;
auto* service = AppPreloadService::Get(GetProfile());
service->check_first_pref_set_callback_ = result.GetCallback();
ASSERT_TRUE(result.Wait());
service->SetInstallationCompleteCallbackForTesting(result.GetCallback());
ASSERT_TRUE(result.Get());

auto app_id = web_app::GenerateAppId(absl::nullopt,
GURL("https://peanuttypes.com/app"));
Expand Down

0 comments on commit 824244f

Please sign in to comment.