Skip to content

Support OnDemandUpdate with multiple components at once. #23169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions browser/brave_browser_main_parts.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/command_line.h"
#include "brave/browser/browsing_data/brave_clear_browsing_data.h"
#include "brave/browser/ethereum_remote_client/buildflags/buildflags.h"
#include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h"
#include "brave/components/brave_rewards/common/rewards_flags.h"
#include "brave/components/brave_rewards/common/rewards_util.h"
#include "brave/components/brave_sync/features.h"
Expand All @@ -20,8 +21,10 @@
#include "brave/components/tor/buildflags/buildflags.h"
#include "brave/components/translate/core/common/brave_translate_features.h"
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_features.h"
#include "components/component_updater/component_updater_service.h"
#include "components/prefs/pref_service.h"
#include "components/sync/base/command_line_switches.h"
#include "components/translate/core/browser/translate_language_list.h"
Expand Down Expand Up @@ -60,16 +63,20 @@
#include "brave/browser/android/preferences/features.h"
#endif

#if BUILDFLAG(ENABLE_TOR) || !BUILDFLAG(IS_ANDROID)
#include "chrome/browser/browser_process.h"
#endif

#if BUILDFLAG(ETHEREUM_REMOTE_CLIENT_ENABLED) && BUILDFLAG(ENABLE_EXTENSIONS)
#include "brave/browser/extensions/brave_component_loader.h"
#include "chrome/browser/extensions/extension_service.h"
#include "extensions/browser/extension_system.h"
#endif

int BraveBrowserMainParts::PreMainMessageLoopRun() {
brave_component_updater::BraveOnDemandUpdater::GetInstance()
->RegisterOnDemandUpdater(
&g_browser_process->component_updater()->GetOnDemandUpdater());

return ChromeBrowserMainParts::PreMainMessageLoopRun();
}

void BraveBrowserMainParts::PreBrowserStart() {
#if BUILDFLAG(ENABLE_SPEEDREADER)
// Register() must be called after the SerializedNavigationDriver is
Expand Down
1 change: 1 addition & 0 deletions browser/brave_browser_main_parts.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class BraveBrowserMainParts : public ChromeBrowserMainParts {
BraveBrowserMainParts& operator=(const BraveBrowserMainParts&) = delete;
~BraveBrowserMainParts() override = default;

int PreMainMessageLoopRun() override;
void PreBrowserStart() override;
void PostBrowserStart() override;
void PreShutdown() override;
Expand Down
5 changes: 0 additions & 5 deletions browser/brave_browser_process_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#include "brave/common/brave_channel_info.h"
#include "brave/components/brave_ads/browser/component_updater/resource_component.h"
#include "brave/components/brave_component_updater/browser/brave_component_updater_delegate.h"
#include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h"
#include "brave/components/brave_component_updater/browser/local_data_files_service.h"
#include "brave/components/brave_referrals/browser/brave_referrals_service.h"
#include "brave/components/brave_shields/content/browser/ad_block_service.h"
Expand Down Expand Up @@ -53,7 +52,6 @@
#include "chrome/common/channel_info.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/pref_names.h"
#include "components/component_updater/component_updater_service.h"
#include "components/component_updater/timer_update_scheduler.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_security_policy.h"
Expand Down Expand Up @@ -153,9 +151,6 @@ void BraveBrowserProcessImpl::Init() {
content::ChildProcessSecurityPolicy::GetInstance()->RegisterWebSafeScheme(
ipfs::kIPNSScheme);
#endif
brave_component_updater::BraveOnDemandUpdater::GetInstance()
->RegisterOnDemandUpdateCallback(
base::BindRepeating(&component_updater::BraveOnDemandUpdate));
UpdateBraveDarkMode();
pref_change_registrar_.Add(
kBraveDarkMode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,13 @@

#include "src/chrome/browser/component_updater/component_updater_utils.cc"

#include "base/functional/callback.h"
#include "chrome/browser/browser_process.h"
#include "components/component_updater/component_updater_service.h"
#include "brave/components/brave_component_updater/browser/brave_on_demand_updater.h"

namespace component_updater {

void BraveOnDemandUpdate(const std::string& component_id) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function will be removed in a follow up PR. It's left for now to not overcomplicate the PR.

component_updater::ComponentUpdateService* cus =
g_browser_process->component_updater();
cus->GetOnDemandUpdater().OnDemandUpdate(
component_id, component_updater::OnDemandUpdater::Priority::FOREGROUND,
component_updater::Callback());
brave_component_updater::BraveOnDemandUpdater::GetInstance()->OnDemandUpdate(
component_id);
}

} // namespace component_updater
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@

namespace component_updater {

bool ComponentInstallerPolicy::IsBraveComponent() const {
return false;
}

void ComponentInstaller::Register(ComponentUpdateService* cus,
base::OnceClosure callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down Expand Up @@ -59,4 +63,8 @@ void ComponentInstaller::Register(
registered_version, max_previous_product_version);
}

bool ComponentInstaller::IsBraveComponent() const {
return installer_policy_->IsBraveComponent();
}

} // namespace component_updater
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_COMPONENT_UPDATER_COMPONENT_INSTALLER_H_
#define BRAVE_CHROMIUM_SRC_COMPONENTS_COMPONENT_UPDATER_COMPONENT_INSTALLER_H_

// Prevent CrxInstaller::OnUpdateError from being redefined by the below
// #define.
#include "components/update_client/update_client.h"

// We can't redefine Register() here because that would change two methods at
Expand All @@ -22,10 +20,16 @@
const base::Version& registered_version = base::Version(kNullVersion), \
const base::Version& max_previous_product_version = \
base::Version(kNullVersion)); \
bool IsBraveComponent() const override; \
void OnUpdateError

#define AllowUpdatesOnMeteredConnections \
IsBraveComponent() const; \
virtual bool AllowUpdatesOnMeteredConnections

#include "src/components/component_updater/component_installer.h" // IWYU pragma: export

#undef OnUpdateError
#undef AllowUpdatesOnMeteredConnections

#endif // BRAVE_CHROMIUM_SRC_COMPONENTS_COMPONENT_UPDATER_COMPONENT_INSTALLER_H_
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/* Copyright (c) 2024 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#include "components/component_updater/component_updater_service.h"

#include "base/notreached.h"
#include "components/component_updater/component_updater_service_internal.h"

#include "src/components/component_updater/component_updater_service.cc"

namespace component_updater {

void CrxUpdateService::OnDemandUpdate(const std::vector<std::string>& ids,
Priority priority,
Callback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

for (const auto& id : ids) {
if (!GetComponent(id)) {
if (callback) {
base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback),
update_client::Error::INVALID_ARGUMENT));
}
return;
}
}

auto crx_data_callback = base::BindOnce(&CrxUpdateService::GetCrxComponents,
base::Unretained(this));
auto update_complete_callback = base::BindOnce(
&CrxUpdateService::OnUpdateComplete, base::Unretained(this),
std::move(callback), base::TimeTicks::Now());

update_client_->Update(ids, std::move(crx_data_callback), {},
priority == Priority::FOREGROUND,
std::move(update_complete_callback));
}

void OnDemandUpdater::OnDemandUpdate(const std::vector<std::string>& ids,
Priority priority,
Callback callback) {
NOTREACHED_NORETURN();
}

} // namespace component_updater
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,25 @@ class BraveComponentUpdaterAndroid;
}
} // namespace chrome

namespace brave_shields {
class AdBlockComponentFiltersProvider;
namespace brave_component_updater {
class BraveOnDemandUpdater;
}

#define BRAVE_COMPONENT_UPDATER_SERVICE_H_ \
friend class ::IPFSDOMHandler; \
friend class ::chrome::android::BraveComponentUpdaterAndroid;

#define BRAVE_COMPONENT_UPDATER_SERVICE_H_ON_DEMAND_UPDATER \
private: \
friend void BraveOnDemandUpdate(const std::string&); \
friend class brave_shields::AdBlockComponentFiltersProvider; \
\
#define BRAVE_COMPONENT_UPDATER_SERVICE_H_ON_DEMAND_UPDATER \
private: \
friend class brave_component_updater::BraveOnDemandUpdater; \
\
virtual void OnDemandUpdate(const std::vector<std::string>& ids, \
Priority priority, Callback callback); \
\
public:

#include "src/components/component_updater/component_updater_service.h" // IWYU pragma: export

#undef BRAVE_COMPONENT_UPDATER_SERVICE_H_ON_DEMAND_UPDATER
#undef BRAVE_COMPONENT_UPDATER_SERVICE_H_

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/* Copyright (c) 2024 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_COMPONENT_UPDATER_COMPONENT_UPDATER_SERVICE_INTERNAL_H_
#define BRAVE_CHROMIUM_SRC_COMPONENTS_COMPONENT_UPDATER_COMPONENT_UPDATER_SERVICE_INTERNAL_H_

#include "components/component_updater/component_updater_service.h"

#define OnDemandUpdate \
OnDemandUpdate(const std::vector<std::string>& ids, Priority priority, \
Callback callback) override; \
void OnDemandUpdate

#include "src/components/component_updater/component_updater_service_internal.h" // IWYU pragma: export

#undef OnDemandUpdate

#endif // BRAVE_CHROMIUM_SRC_COMPONENTS_COMPONENT_UPDATER_COMPONENT_UPDATER_SERVICE_INTERNAL_H_
57 changes: 48 additions & 9 deletions chromium_src/components/update_client/update_checker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,13 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "components/update_client/update_checker.h"

#include <optional>

#include "base/ranges/algorithm.h"
#include "components/update_client/update_client.h"

#include "src/components/update_client/update_checker.cc"

#if BUILDFLAG(WIDEVINE_ARM64_DLL_FIX)
Expand All @@ -14,6 +19,19 @@

namespace update_client {

namespace {

bool IsBraveComponent(const Component* component) {
CHECK(component);
const auto& crx_component = component->crx_component();
if (!crx_component || !crx_component->installer) {
return false;
}
return crx_component->installer->IsBraveComponent();
}

} // namespace

SequentialUpdateChecker::SequentialUpdateChecker(
scoped_refptr<Configurator> config,
PersistedData* metadata)
Expand All @@ -39,6 +57,16 @@ void SequentialUpdateChecker::CheckForUpdates(
additional_attributes_ = additional_attributes;
update_check_callback_ = std::move(update_check_callback);

// Partition IDs for batch update checks. The order of
// `components_to_check_for_updates` doesn't matter to the caller, as
// post-update mapping is done via an Id->Component map, making this
// rearrangement safe.
base::ranges::stable_partition(
update_context_->components_to_check_for_updates,
[&](const std::string& id) {
return IsBraveComponent(update_context_->components[id].get());
});

for (const auto& id : update_context_->components_to_check_for_updates) {
remaining_ids_.push_back(id);
}
Expand All @@ -56,25 +84,36 @@ void SequentialUpdateChecker::CheckNext(
DCHECK(!remaining_ids_.empty());
DCHECK(update_context_);

const auto id = remaining_ids_.front();
remaining_ids_.pop_front();
// Support multiple checks in a single call, but only if they are all Brave.
std::vector<std::string> ids;
for (auto id_it = remaining_ids_.begin(); id_it != remaining_ids_.end();) {
const auto& component = update_context_->components[*id_it];
if (!ids.empty() && !IsBraveComponent(component.get())) {
break;
Copy link
Collaborator

@atuchin-m atuchin-m Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work well if remaining_ids_ contains only one Chromium component?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work well if remaining_ids_ contains only one Chromium component?

it should. This break triggers only if !ids.empty(), so at least one item must be added no matter what.

}
ids.push_back(*id_it);
id_it = remaining_ids_.erase(id_it);
}

scoped_refptr<UpdateContext> context = new UpdateContext(
update_context_->config, update_context_->crx_cache_,
update_context_->is_foreground, update_context_->is_install, {id},
update_context_->is_foreground, update_context_->is_install, ids,
update_context_->crx_state_change_callback,
update_context_->notify_observers_callback,
// We don't pass a context callback here because UpdateChecker doesn't use
// it. This is instead done by UpdateEngine, which calls us.
base::DoNothing(), update_context_->persisted_data,
/*is_update_check_only=*/false);

auto& component = context->components[id];
auto& crx_component = update_context_->components[id]->crx_component();
component->set_crx_component(*crx_component);
component->set_previous_version(crx_component->version);
component->set_previous_fp(crx_component->fingerprint);
context->components_to_check_for_updates.push_back(id);
DCHECK(!ids.empty());
for (const auto& id : ids) {
auto& component = context->components[id];
auto& crx_component = update_context_->components[id]->crx_component();
component->set_crx_component(*crx_component);
component->set_previous_version(crx_component->version);
component->set_previous_fp(crx_component->fingerprint);
context->components_to_check_for_updates.push_back(id);
}

update_checker_ = UpdateChecker::Create(config_, metadata_);

Expand Down
4 changes: 4 additions & 0 deletions chromium_src/components/update_client/update_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@

namespace update_client {

bool CrxInstaller::IsBraveComponent() const {
return false;
}

scoped_refptr<UpdateClient> UpdateClientFactory(
scoped_refptr<Configurator> config) {
VLOG(3) << "Brave UpdateClientFactory called";
Expand Down
5 changes: 5 additions & 0 deletions chromium_src/components/update_client/update_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@
#ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_UPDATE_CLIENT_UPDATE_CLIENT_H_
#define BRAVE_CHROMIUM_SRC_COMPONENTS_UPDATE_CLIENT_UPDATE_CLIENT_H_

#define GetInstalledFile(...) \
IsBraveComponent() const; \
virtual bool GetInstalledFile(__VA_ARGS__)

#define UpdateClientFactory \
UpdateClientFactory_ChromiumImpl(scoped_refptr<Configurator> config); \
scoped_refptr<UpdateClient> UpdateClientFactory

#include "src/components/update_client/update_client.h" // IWYU pragma: export

#undef GetInstalledFile
#undef UpdateClientFactory

#endif // BRAVE_CHROMIUM_SRC_COMPONENTS_UPDATE_CLIENT_UPDATE_CLIENT_H_
16 changes: 16 additions & 0 deletions components/brave_component_updater/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,19 @@ static_library("browser") {
"//crypto",
]
}

source_set("test_support") {
testonly = true

sources = [
"mock_on_demand_updater.cc",
"mock_on_demand_updater.h",
]

deps = [
":browser",
"//base",
"//components/component_updater",
"//testing/gmock",
]
}
Loading
Loading