From 634756fa6e2c3b9b5d1520fbc0ddb922f08534ab Mon Sep 17 00:00:00 2001 From: Mikhail Atuchin Date: Wed, 18 Oct 2023 11:46:48 +0100 Subject: [PATCH 1/5] Fix adblock ruleset reparsing --- .../browser/ad_block_component_filters_provider.cc | 2 +- .../browser/ad_block_custom_filters_provider.cc | 4 ++-- .../brave_shields/browser/ad_block_filters_provider.cc | 4 ++-- .../brave_shields/browser/ad_block_filters_provider.h | 4 ++-- .../browser/ad_block_filters_provider_manager.cc | 6 +++--- .../browser/ad_block_filters_provider_manager.h | 2 +- .../browser/ad_block_localhost_filters_provider.cc | 2 +- components/brave_shields/browser/ad_block_service.cc | 6 +++++- components/brave_shields/browser/ad_block_service.h | 2 +- .../browser/ad_block_subscription_filters_provider.cc | 2 +- 10 files changed, 19 insertions(+), 15 deletions(-) diff --git a/components/brave_shields/browser/ad_block_component_filters_provider.cc b/components/brave_shields/browser/ad_block_component_filters_provider.cc index 9bd489ddf2ee..d6fe5c53dfa0 100644 --- a/components/brave_shields/browser/ad_block_component_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_component_filters_provider.cc @@ -66,7 +66,7 @@ void AdBlockComponentFiltersProvider::OnComponentReady( const base::FilePath& path) { component_path_ = path; - NotifyObservers(); + NotifyObservers(engine_is_default_); } void AdBlockComponentFiltersProvider::LoadDATBuffer( diff --git a/components/brave_shields/browser/ad_block_custom_filters_provider.cc b/components/brave_shields/browser/ad_block_custom_filters_provider.cc index 51e35dafa0fa..bbe8d5a8a795 100644 --- a/components/brave_shields/browser/ad_block_custom_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_custom_filters_provider.cc @@ -54,7 +54,7 @@ bool AdBlockCustomFiltersProvider::UpdateCustomFilters( return false; local_state_->SetString(prefs::kAdBlockCustomFilters, custom_filters); - NotifyObservers(); + NotifyObservers(engine_is_default_); return true; } @@ -78,7 +78,7 @@ void AdBlockCustomFiltersProvider::LoadDATBuffer( void AdBlockCustomFiltersProvider::AddObserver( AdBlockFiltersProvider::Observer* observer) { AdBlockFiltersProvider::AddObserver(observer); - NotifyObservers(); + NotifyObservers(engine_is_default_); } } // namespace brave_shields diff --git a/components/brave_shields/browser/ad_block_filters_provider.cc b/components/brave_shields/browser/ad_block_filters_provider.cc index 4c20c9d840eb..23f23dcbe34f 100644 --- a/components/brave_shields/browser/ad_block_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_filters_provider.cc @@ -36,9 +36,9 @@ void AdBlockFiltersProvider::RemoveObserver( observers_.RemoveObserver(observer); } -void AdBlockFiltersProvider::NotifyObservers() { +void AdBlockFiltersProvider::NotifyObservers(bool is_for_default_engine) { for (auto& observer : observers_) { - observer.OnChanged(); + observer.OnChanged(is_for_default_engine); } } diff --git a/components/brave_shields/browser/ad_block_filters_provider.h b/components/brave_shields/browser/ad_block_filters_provider.h index 479c4a55776d..8b67abc77bdb 100644 --- a/components/brave_shields/browser/ad_block_filters_provider.h +++ b/components/brave_shields/browser/ad_block_filters_provider.h @@ -24,7 +24,7 @@ class AdBlockFiltersProvider { public: class Observer : public base::CheckedObserver { public: - virtual void OnChanged() = 0; + virtual void OnChanged(bool is_for_default_engine) = 0; }; explicit AdBlockFiltersProvider(bool engine_is_default); @@ -50,7 +50,7 @@ class AdBlockFiltersProvider { virtual void LoadDATBuffer( base::OnceCallback) = 0; - void NotifyObservers(); + void NotifyObservers(bool is_for_default_engine); private: base::ObserverList observers_; diff --git a/components/brave_shields/browser/ad_block_filters_provider_manager.cc b/components/brave_shields/browser/ad_block_filters_provider_manager.cc index ce0379431bcd..397877a88294 100644 --- a/components/brave_shields/browser/ad_block_filters_provider_manager.cc +++ b/components/brave_shields/browser/ad_block_filters_provider_manager.cc @@ -63,15 +63,15 @@ void AdBlockFiltersProviderManager::RemoveProvider( auto it = filters_providers.find(provider); DCHECK(it != filters_providers.end()); filters_providers.erase(it); - NotifyObservers(); + NotifyObservers(is_for_default_engine); } std::string AdBlockFiltersProviderManager::GetNameForDebugging() { return "AdBlockCustomFiltersProvider"; } -void AdBlockFiltersProviderManager::OnChanged() { - NotifyObservers(); +void AdBlockFiltersProviderManager::OnChanged(bool is_for_default_engine) { + NotifyObservers(is_for_default_engine); } // Use LoadDATBufferForEngine instead, for Filter Provider Manager. diff --git a/components/brave_shields/browser/ad_block_filters_provider_manager.h b/components/brave_shields/browser/ad_block_filters_provider_manager.h index a74cbffba576..f6c4b554b199 100644 --- a/components/brave_shields/browser/ad_block_filters_provider_manager.h +++ b/components/brave_shields/browser/ad_block_filters_provider_manager.h @@ -51,7 +51,7 @@ class AdBlockFiltersProviderManager : public AdBlockFiltersProvider, const DATFileDataBuffer& dat_buf)> cb); // AdBlockFiltersProvider::Observer - void OnChanged() override; + void OnChanged(bool is_default_engine) override; void AddProvider(AdBlockFiltersProvider* provider, bool is_for_default_engine); diff --git a/components/brave_shields/browser/ad_block_localhost_filters_provider.cc b/components/brave_shields/browser/ad_block_localhost_filters_provider.cc index 37b34f130d28..d13c1994b765 100644 --- a/components/brave_shields/browser/ad_block_localhost_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_localhost_filters_provider.cc @@ -50,7 +50,7 @@ void AdBlockLocalhostFiltersProvider::LoadDATBuffer( void AdBlockLocalhostFiltersProvider::AddObserver( AdBlockFiltersProvider::Observer* observer) { AdBlockFiltersProvider::AddObserver(observer); - NotifyObservers(); + NotifyObservers(engine_is_default_); } } // namespace brave_shields diff --git a/components/brave_shields/browser/ad_block_service.cc b/components/brave_shields/browser/ad_block_service.cc index 59f2e2cee4c2..a70fe6743029 100644 --- a/components/brave_shields/browser/ad_block_service.cc +++ b/components/brave_shields/browser/ad_block_service.cc @@ -96,7 +96,11 @@ AdBlockService::SourceProviderObserver::~SourceProviderObserver() { resource_provider_->RemoveObserver(this); } -void AdBlockService::SourceProviderObserver::OnChanged() { +void AdBlockService::SourceProviderObserver::OnChanged(bool is_default_engine) { + if (adblock_engine_->IsDefaultEngine() && !is_default_engine) { + // Skip updates of the additional engine for the default. + return; + } if (is_filter_provider_manager_) { static_cast(filters_provider_.get()) ->LoadDATBufferForEngine( diff --git a/components/brave_shields/browser/ad_block_service.h b/components/brave_shields/browser/ad_block_service.h index 4e2f79029e69..aea5b976f595 100644 --- a/components/brave_shields/browser/ad_block_service.h +++ b/components/brave_shields/browser/ad_block_service.h @@ -70,7 +70,7 @@ class AdBlockService { void OnDATLoaded(bool deserialize, const DATFileDataBuffer& dat_buf); // AdBlockFiltersProvider::Observer - void OnChanged() override; + void OnChanged(bool is_default_engine) override; // AdBlockResourceProvider::Observer void OnResourcesLoaded(const std::string& resources_json) override; diff --git a/components/brave_shields/browser/ad_block_subscription_filters_provider.cc b/components/brave_shields/browser/ad_block_subscription_filters_provider.cc index 67764923ec01..7b0d824754a9 100644 --- a/components/brave_shields/browser/ad_block_subscription_filters_provider.cc +++ b/components/brave_shields/browser/ad_block_subscription_filters_provider.cc @@ -51,7 +51,7 @@ void AdBlockSubscriptionFiltersProvider::OnDATFileDataReady( } void AdBlockSubscriptionFiltersProvider::OnListAvailable() { - NotifyObservers(); + NotifyObservers(engine_is_default_); } } // namespace brave_shields From dd36a7f63c415941b9d1f6d0eb0f1ece85429253 Mon Sep 17 00:00:00 2001 From: Mikhail Atuchin Date: Wed, 18 Oct 2023 14:56:13 +0100 Subject: [PATCH 2/5] Add UMA & traces --- .../browser/dat_file_util.cc | 6 ++- .../brave_shields/browser/ad_block_engine.cc | 40 ++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/components/brave_component_updater/browser/dat_file_util.cc b/components/brave_component_updater/browser/dat_file_util.cc index 9376d9db8850..ce8c9f55e8db 100644 --- a/components/brave_component_updater/browser/dat_file_util.cc +++ b/components/brave_component_updater/browser/dat_file_util.cc @@ -8,9 +8,10 @@ #include #include -#include "base/logging.h" #include "base/files/file_path.h" #include "base/files/file_util.h" +#include "base/logging.h" +#include "base/trace_event/trace_event.h" namespace { @@ -40,8 +41,11 @@ void GetDATFileData(const base::FilePath& file_path, namespace brave_component_updater { DATFileDataBuffer ReadDATFileData(const base::FilePath& dat_file_path) { + TRACE_EVENT_BEGIN1("brave.adblock", "ReadDATFileData", "path", + dat_file_path.MaybeAsASCII()); DATFileDataBuffer buffer; GetDATFileData(dat_file_path, &buffer); + TRACE_EVENT_END1("brave.adblock", "ReadDATFileData", "size", buffer.size()); return buffer; } diff --git a/components/brave_shields/browser/ad_block_engine.cc b/components/brave_shields/browser/ad_block_engine.cc index 5aa3f42b6551..a9b6c10690fc 100644 --- a/components/brave_shields/browser/ad_block_engine.cc +++ b/components/brave_shields/browser/ad_block_engine.cc @@ -12,7 +12,11 @@ #include "base/containers/contains.h" #include "base/json/json_reader.h" +#include "base/logging.h" +#include "base/metrics/histogram_functions.h" #include "base/strings/string_number_conversions.h" +#include "base/timer/elapsed_timer.h" +#include "base/trace_event/trace_event.h" #include "net/base/registry_controlled_domains/registry_controlled_domain.h" #include "url/origin.h" @@ -243,6 +247,7 @@ base::Value::List AdBlockEngine::HiddenClassIdSelectors( void AdBlockEngine::Load(bool deserialize, const DATFileDataBuffer& dat_buf, const std::string& resources_json) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); if (deserialize) { OnDATLoaded(dat_buf, resources_json); } else { @@ -275,7 +280,23 @@ void AdBlockEngine::AddKnownTagsToAdBlockInstance() { void AdBlockEngine::OnListSourceLoaded(const DATFileDataBuffer& filters, const std::string& resources_json) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + + base::ElapsedTimer timer; + TRACE_EVENT_BEGIN2("brave.adblock", "MakeEngineWithRules", "size", + filters.size(), "is_default_engine", is_default_engine_); + auto result = adblock::engine_with_rules(filters); + + TRACE_EVENT_END0("brave.adblock", "MakeEngineWithRules"); + if (is_default_engine_) { + base::UmaHistogramTimes("Brave.Adblock.MakeEngineWithRules.Default", + timer.Elapsed()); + } else { + base::UmaHistogramTimes("Brave.Adblock.MakeEngineWithRules.Additional", + timer.Elapsed()); + } + if (result.result_kind != adblock::ResultKind::Success) { LOG(ERROR) << "AdBlockEngine::OnListSourceLoaded failed: " << result.error_message.c_str(); @@ -286,13 +307,30 @@ void AdBlockEngine::OnListSourceLoaded(const DATFileDataBuffer& filters, void AdBlockEngine::OnDATLoaded(const DATFileDataBuffer& dat_buf, const std::string& resources_json) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + // An empty buffer will not load successfully. if (dat_buf.empty()) { return; } + base::ElapsedTimer timer; + TRACE_EVENT_BEGIN2("brave.adblock", "EngineDeserialize", "size", + dat_buf.size(), "is_default_engine", is_default_engine_); + auto client = adblock::new_engine(); - if (!client->deserialize(dat_buf)) { + const auto result = client->deserialize(dat_buf); + + TRACE_EVENT_END0("brave.adblock", "EngineDeserialize"); + if (is_default_engine_) { + base::UmaHistogramTimes("Brave.Adblock.EngineDeserialize.Default", + timer.Elapsed()); + } else { + base::UmaHistogramTimes("Brave.Adblock.EngineDeserialize.Additional", + timer.Elapsed()); + } + + if (!result) { LOG(ERROR) << "AdBlockEngine::OnDATLoaded deserialize failed"; return; } From 6ae4304a12dd7c45f46156b8cd0f10c29e5b289b Mon Sep 17 00:00:00 2001 From: Mikhail Atuchin Date: Wed, 18 Oct 2023 14:56:50 +0100 Subject: [PATCH 3/5] Add a browser test --- .../ad_block_service_browsertest.cc | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 2f6c49005ad3..d4da7559db41 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -15,6 +15,7 @@ #include "base/memory/raw_ptr.h" #include "base/path_service.h" #include "base/strings/stringprintf.h" +#include "base/test/metrics/histogram_tester.h" #include "base/test/scoped_feature_list.h" #include "base/test/thread_test_helper.h" #include "brave/browser/brave_browser_process.h" @@ -465,6 +466,40 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); } +class AdBlockServiceEngineUpdateCountTest : public AdBlockServiceTest { + protected: + const base::HistogramTester histogram_tester_; +}; + +// The test verifies the number of expected engine updating during normal +// startup. +// Warning: each engine updating is a CPU-heavy thing and degrades startup +// performance. +IN_PROC_BROWSER_TEST_F(AdBlockServiceEngineUpdateCountTest, + DefaultStartupWithCookieList) { + // The empty ruleset building until the components are loaded. + // TODO: remove that excessive work. + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Default", 1); + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Additional", 1); + + // Loads the default list first, then the additional cookie list. + ASSERT_TRUE( + InstallRegionalAdBlockExtension(brave_shields::kCookieListUuid, true)); + + // Only one new rebuild is expected. Loading the extra list must not + // trigger another rebuilding of the default engine. + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Default", 2); + + // Currently, the extra engine depends on the default, so we expect an extra + // unnecessary update. + // TODO: remove that dependency and change 3 => 2. + histogram_tester_.ExpectTotalCount( + "Brave.Adblock.MakeEngineWithRules.Additional", 3); +} + // Load a page with an ad image, and make sure it is blocked by the // regional blocker. IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByRegionalBlocker) { From 831fced3390cc0c6f2fdc82150b5657796bfcf6c Mon Sep 17 00:00:00 2001 From: Mikhail Atuchin Date: Wed, 18 Oct 2023 17:29:14 +0100 Subject: [PATCH 4/5] Fix lint --- browser/brave_shields/ad_block_service_browsertest.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index d4da7559db41..50e63fb4edac 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -478,7 +478,7 @@ class AdBlockServiceEngineUpdateCountTest : public AdBlockServiceTest { IN_PROC_BROWSER_TEST_F(AdBlockServiceEngineUpdateCountTest, DefaultStartupWithCookieList) { // The empty ruleset building until the components are loaded. - // TODO: remove that excessive work. + // TODO(matuchin): remove that excessive work. histogram_tester_.ExpectTotalCount( "Brave.Adblock.MakeEngineWithRules.Default", 1); histogram_tester_.ExpectTotalCount( @@ -495,7 +495,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceEngineUpdateCountTest, // Currently, the extra engine depends on the default, so we expect an extra // unnecessary update. - // TODO: remove that dependency and change 3 => 2. + // TODO(matuchin): remove that dependency and change 3 => 2. histogram_tester_.ExpectTotalCount( "Brave.Adblock.MakeEngineWithRules.Additional", 3); } From 086c8588bec127dadc6bf68884ad10c59566d4af Mon Sep 17 00:00:00 2001 From: Mikhail Atuchin Date: Wed, 18 Oct 2023 21:41:38 +0100 Subject: [PATCH 5/5] Skip non-matching updates --- browser/brave_shields/ad_block_service_browsertest.cc | 11 +++-------- components/brave_shields/browser/ad_block_service.cc | 4 ++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 50e63fb4edac..30cd52b3cc18 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -273,8 +273,7 @@ class EngineTestObserver : public brave_shields::AdBlockEngine::TestObserver { bool AdBlockServiceTest::InstallRegionalAdBlockExtension( const std::string& uuid, bool enable_list) { - // The regional adblock engines depend on the default engine for resource - // loads. + // Install the default engine first. EXPECT_TRUE(InstallDefaultAdBlockExtension()); auto* default_engine = g_brave_browser_process->ad_block_service()->default_engine_.get(); @@ -489,15 +488,11 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceEngineUpdateCountTest, InstallRegionalAdBlockExtension(brave_shields::kCookieListUuid, true)); // Only one new rebuild is expected. Loading the extra list must not - // trigger another rebuilding of the default engine. + // trigger another rebuilding of the default engine and vice versa. histogram_tester_.ExpectTotalCount( "Brave.Adblock.MakeEngineWithRules.Default", 2); - - // Currently, the extra engine depends on the default, so we expect an extra - // unnecessary update. - // TODO(matuchin): remove that dependency and change 3 => 2. histogram_tester_.ExpectTotalCount( - "Brave.Adblock.MakeEngineWithRules.Additional", 3); + "Brave.Adblock.MakeEngineWithRules.Additional", 2); } // Load a page with an ad image, and make sure it is blocked by the diff --git a/components/brave_shields/browser/ad_block_service.cc b/components/brave_shields/browser/ad_block_service.cc index a70fe6743029..d1f770af4fa6 100644 --- a/components/brave_shields/browser/ad_block_service.cc +++ b/components/brave_shields/browser/ad_block_service.cc @@ -97,8 +97,8 @@ AdBlockService::SourceProviderObserver::~SourceProviderObserver() { } void AdBlockService::SourceProviderObserver::OnChanged(bool is_default_engine) { - if (adblock_engine_->IsDefaultEngine() && !is_default_engine) { - // Skip updates of the additional engine for the default. + if (adblock_engine_->IsDefaultEngine() != is_default_engine) { + // Skip updates of another engine. return; } if (is_filter_provider_manager_) {