Fix adblock default ruleset reparsing on startup (uplift to 1.59.x)

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"
@@ -271,8 +272,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();
@@ -463,6 +463,36 @@ 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(matuchin): 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 and vice versa.
+  histogram_tester_.ExpectTotalCount(
+      "Brave.Adblock.MakeEngineWithRules.Default", 2);
+  histogram_tester_.ExpectTotalCount(
+      "Brave.Adblock.MakeEngineWithRules.Additional", 2);
+}
+
 // Load a page with an ad image, and make sure it is blocked by the
 // regional blocker.
 IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByRegionalBlocker) {

components/brave_component_updater/browser/dat_file_util.cc

@@ -8,9 +8,10 @@
 #include <memory>
 #include <string>
 
-#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;
 }
 

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(

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

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;
   }

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);
   }
 }
 

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<void(bool deserialize,
                               const DATFileDataBuffer& dat_buf)>) = 0;
-  void NotifyObservers();
+  void NotifyObservers(bool is_for_default_engine);
 
  private:
   base::ObserverList<Observer> observers_;

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.

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);

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

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 another engine.
+    return;
+  }
   if (is_filter_provider_manager_) {
     static_cast<AdBlockFiltersProviderManager*>(filters_provider_.get())
         ->LoadDATBufferForEngine(

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;

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