Skip to content

Commit

Permalink
iwa: Use jitter for update discovery timer
Browse files Browse the repository at this point in the history 
Similar to Chrome Extension updates, we add 20% jitter to the update
discovery timer for IWA updates, so that it becomes less likely for
severs to be overloaded if a lot of people start Chrome(OS) at the
same time.

To achieve this, we replace the `base::RepeatingTimer` with a
`base::CancelableCallback` that we regularly schedule with 20% jitter.

This also fixes a small bug where the install manager observation was
only started when automatic updates were enabled - however, now that
updates can also be triggered manually from chrome://web-app-internals
and potentially other sources, we need to make sure that
`OnWebAppUninstalled` is actually called.

Fixed: b:305004160
Bug: 1444407
Change-Id: Iaddec9963b9e8f7e8a5200a131df1e7d50876a50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935851
Reviewed-by: Andrew Rayskiy <greengrape@google.com>
Commit-Queue: Christian Flach <cmfcmf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212157}
  • Loading branch information
@cmfcmf
cmfcmf authored and Chromium LUCI CQ committed Oct 19, 2023
1 parent e3c99e5 commit 2256e75
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 76 deletions.
121 changes: 87 additions & 34 deletions chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <type_traits>

#include "base/callback_list.h"
#include "base/cancelable_callback.h"
#include "base/check.h"
#include "base/containers/circular_deque.h"
#include "base/containers/cxx20_erase_map.h"
Expand All @@ -18,6 +19,7 @@
#include "base/location.h"
#include "base/memory/raw_ref.h"
#include "base/memory/weak_ptr.h"
#include "base/rand_util.h"
#include "base/ranges/algorithm.h"
#include "base/scoped_observation.h"
#include "base/task/sequenced_task_runner.h"
Expand Down Expand Up @@ -51,6 +53,7 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/isolated_web_apps_policy.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/abseil-cpp/absl/types/variant.h"
#include "url/gurl.h"

Expand Down Expand Up @@ -125,8 +128,8 @@ IsolatedWebAppUpdateManager::IsolatedWebAppUpdateManager(
// sessions.
!profile.IsGuestSession() &&
// Web Apps are not a thing in off the record profiles, but have this
// here just in case - we also wouldn't want to update IWAs in
// incognito windows.
// here just in case - we also wouldn't want to automatically update
// IWAs in incognito windows.
!profile.IsOffTheRecord() &&
#if BUILDFLAG(IS_CHROMEOS)
base::FeatureList::IsEnabled(
Expand All @@ -152,9 +155,6 @@ void IsolatedWebAppUpdateManager::Start() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

has_started_ = true;
if (!automatic_updates_enabled_) {
return;
}
install_manager_observation_.Observe(&provider_->install_manager());

if (!IsAnyIwaInstalled()) {
Expand Down Expand Up @@ -217,25 +217,19 @@ void IsolatedWebAppUpdateManager::DelayedStart() {
task_queue_.MaybeStartNextTask();

QueueUpdateDiscoveryTasks();
MaybeStartUpdateDiscoveryTimer();
}

void IsolatedWebAppUpdateManager::Shutdown() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

// Stop all potentially ongoing tasks and avoid scheduling new tasks.
install_manager_observation_.Reset();
update_discovery_timer_.Stop();
next_update_discovery_check_.Reset();
task_queue_.Clear();
update_apply_waiters_.clear();
}

base::Value IsolatedWebAppUpdateManager::AsDebugValue() const {
base::TimeDelta next_update_check =
update_discovery_timer_.desired_run_time() - base::TimeTicks::Now();
double next_update_check_in_minutes =
next_update_check.InSecondsF() / base::Time::kSecondsPerMinute;

base::Value::List update_apply_waiters;
for (const auto& [app_id, waiter] : update_apply_waiters_) {
update_apply_waiters.Append(waiter->AsDebugValue());
Expand All @@ -247,11 +241,8 @@ base::Value IsolatedWebAppUpdateManager::AsDebugValue() const {
.Set("update_discovery_frequency_in_minutes",
update_discovery_frequency_.InSecondsF() /
base::Time::kSecondsPerMinute)
.Set("update_discovery_timer",
base::Value::Dict()
.Set("running", update_discovery_timer_.IsRunning())
.Set("next_update_check_in_minutes",
next_update_check_in_minutes))
.Set("next_update_discovery_check",
next_update_discovery_check_.AsDebugValue())
.Set("task_queue", task_queue_.AsDebugValue())
.Set("update_apply_waiters", std::move(update_apply_waiters)));
}
Expand Down Expand Up @@ -300,24 +291,22 @@ void IsolatedWebAppUpdateManager::SetEnableAutomaticUpdatesForTesting(

void IsolatedWebAppUpdateManager::OnWebAppInstalled(
const webapps::AppId& app_id) {
MaybeStartUpdateDiscoveryTimer();
MaybeScheduleUpdateDiscoveryCheck();
}

void IsolatedWebAppUpdateManager::OnWebAppUninstalled(
const webapps::AppId& app_id,
webapps::WebappUninstallSource uninstall_source) {
update_apply_waiters_.erase(app_id);
task_queue_.ClearNonStartedTasksOfApp(app_id);
MaybeStopUpdateDiscoveryTimer();
MaybeResetScheduledUpdateDiscoveryCheck();
}

size_t IsolatedWebAppUpdateManager::DiscoverUpdatesNow() {
// If the update discovery timer is running, reset it, so that the next
// timer-based update discovery happens in `update_discovery_frequency_` time
// after this method is called.
if (update_discovery_timer_.IsRunning()) {
update_discovery_timer_.Reset();
}
// If an update discovery check is already scheduled, reset it, so that the
// next update discovery happens based on `update_discovery_frequency_` time
// after `QueueUpdateDiscoveryTasks` is called.
next_update_discovery_check_.Reset();
return QueueUpdateDiscoveryTasks();
}

Expand Down Expand Up @@ -410,25 +399,28 @@ size_t IsolatedWebAppUpdateManager::QueueUpdateDiscoveryTasks() {

task_queue_.MaybeStartNextTask();

MaybeScheduleUpdateDiscoveryCheck();

return num_new_tasks;
}

void IsolatedWebAppUpdateManager::MaybeStartUpdateDiscoveryTimer() {
if (!update_discovery_timer_.IsRunning() && IsAnyIwaInstalled()) {
update_discovery_timer_.Start(
FROM_HERE, update_discovery_frequency_,
base::BindRepeating(
void IsolatedWebAppUpdateManager::MaybeScheduleUpdateDiscoveryCheck() {
if (automatic_updates_enabled_ &&
!next_update_discovery_check_.IsScheduled() && IsAnyIwaInstalled()) {
next_update_discovery_check_.ScheduleWithJitter(
update_discovery_frequency_,
base::BindOnce(
base::IgnoreResult(
&IsolatedWebAppUpdateManager::QueueUpdateDiscoveryTasks),
// Ok to use `base::Unretained` here because `this` owns
// `update_discovery_timer_`.
// `next_update_check_`.
base::Unretained(this)));
}
}

void IsolatedWebAppUpdateManager::MaybeStopUpdateDiscoveryTimer() {
if (update_discovery_timer_.IsRunning() && !IsAnyIwaInstalled()) {
update_discovery_timer_.Stop();
void IsolatedWebAppUpdateManager::MaybeResetScheduledUpdateDiscoveryCheck() {
if (next_update_discovery_check_.IsScheduled() && !IsAnyIwaInstalled()) {
next_update_discovery_check_.Reset();
}
}

Expand Down Expand Up @@ -531,6 +523,67 @@ void IsolatedWebAppUpdateManager::OnLocalUpdateApplyTaskCreated(
.Then(std::move(callback)));
}

IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::
NextUpdateDiscoveryCheck() = default;

IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::
~NextUpdateDiscoveryCheck() = default;

void IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::ScheduleWithJitter(
const base::TimeDelta& base_delay,
base::OnceClosure callback) {
// 20% jitter (between 0.8 and 1.2)
double jitter_factor = base::RandDouble() * 0.4 + 0.8;
base::TimeDelta delay = base_delay * jitter_factor;

auto cancelable_callback = std::make_unique<base::CancelableOnceClosure>(
base::BindOnce(&NextUpdateDiscoveryCheck::Reset,
// Okay to use `base::Unretained` here, since `this`
// owns `next_check_`.
base::Unretained(this))
.Then(std::move(callback)));

next_check_ = {
{base::TimeTicks::Now() + delay, std::move(cancelable_callback)}};
base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
FROM_HERE, next_check_->second->callback(), delay);
}

absl::optional<base::TimeTicks>
IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::GetScheduledTime()
const {
if (next_check_.has_value()) {
return next_check_->first;
}
return absl::nullopt;
}

bool IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::IsScheduled()
const {
return next_check_.has_value();
}

void IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::Reset() {
// This will cancel any scheduled callbacks, because it deletes the
// `base::CancelableOnceCallback`.
next_check_.reset();
}

base::Value
IsolatedWebAppUpdateManager::NextUpdateDiscoveryCheck::AsDebugValue() const {
if (!next_check_.has_value()) {
return base::Value("not scheduled");
}

base::TimeDelta next_update_check =
next_check_->first - base::TimeTicks::Now();
double next_update_check_in_minutes =
next_update_check.InSecondsF() / base::Time::kSecondsPerMinute;

return base::Value(base::Value::Dict().Set("next_update_check_in_minutes",
next_update_check_in_minutes));
}

IsolatedWebAppUpdateManager::TaskQueue::TaskQueue(
IsolatedWebAppUpdateManager& update_manager)
: update_manager_(update_manager) {}
Expand Down
37 changes: 29 additions & 8 deletions chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <memory>

#include "base/callback_list.h"
#include "base/cancelable_callback.h"
#include "base/containers/circular_deque.h"
#include "base/containers/flat_map.h"
#include "base/files/file_error_or.h"
Expand All @@ -19,7 +20,6 @@
#include "base/memory/weak_ptr.h"
#include "base/scoped_observation.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "base/types/pass_key.h"
#include "base/values.h"
#include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_location.h"
Expand All @@ -28,6 +28,7 @@
#include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_discovery_task.h"
#include "chrome/browser/web_applications/web_app_install_manager_observer.h"
#include "components/webapps/common/web_app_id.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

class GURL;
class Profile;
Expand Down Expand Up @@ -111,10 +112,6 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
const webapps::AppId& app_id,
webapps::WebappUninstallSource uninstall_source) override;

const base::RepeatingTimer& GetUpdateDiscoveryTimerForTesting() const {
return update_discovery_timer_;
}

// Used to queue update discovery tasks manually from the
// chrome://web-app-internals page. Returns the number of tasks queued.
size_t DiscoverUpdatesNow();
Expand All @@ -128,6 +125,10 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
base::OnceCallback<void(base::expected<base::Version, std::string>)>
callback);

absl::optional<base::TimeTicks> GetNextUpdateDiscoveryTimeForTesting() const {
return next_update_discovery_check_.GetScheduledTime();
}

private:
// This queue manages update discovery and apply tasks. Tasks can be added to
// the queue via its `Push` methods. The queue will never start a new task on
Expand Down Expand Up @@ -215,8 +216,8 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
base::flat_map<web_package::SignedWebBundleId, GURL>
GetForceInstalledBundleIdToUpdateManifestUrlMap();

void MaybeStartUpdateDiscoveryTimer();
void MaybeStopUpdateDiscoveryTimer();
void MaybeScheduleUpdateDiscoveryCheck();
void MaybeResetScheduledUpdateDiscoveryCheck();

void CreateUpdateApplyWaiter(
const IsolatedWebAppUrlInfo& url_info,
Expand Down Expand Up @@ -260,8 +261,28 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {

bool has_started_ = false;

class NextUpdateDiscoveryCheck {
public:
NextUpdateDiscoveryCheck();
~NextUpdateDiscoveryCheck();

void ScheduleWithJitter(const base::TimeDelta& base_delay,
base::OnceClosure callback);

absl::optional<base::TimeTicks> GetScheduledTime() const;
bool IsScheduled() const;
void Reset();

base::Value AsDebugValue() const;

private:
absl::optional<std::pair<base::TimeTicks,
std::unique_ptr<base::CancelableOnceClosure>>>
next_check_;
};

base::TimeDelta update_discovery_frequency_;
base::RepeatingTimer update_discovery_timer_;
NextUpdateDiscoveryCheck next_update_discovery_check_;

TaskQueue task_queue_;

Expand Down

0 comments on commit 2256e75

Please sign in to comment.