Decouple adblock engines from component loading methods #10994

antonok-edm · 2021-11-11T00:55:28Z

Resolves brave/brave-browser#19366

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

This is a fairly large refactoring that shouldn't change any existing behavior. The automated unit and browser tests cover most core functionality, but it would be great to go over test plans related to previously-merged adblock-related PRs and ensure that things still work the same way. In particular, that updated adblock rules are applied, whether that be the default set of rules, regional lists, custom filters, or custom list subscriptions.

bridiver · 2021-11-23T20:24:47Z

components/brave_shields/browser/ad_block_custom_filters_source_provider.cc

+void AdBlockCustomFiltersSourceProvider::Load(
+    base::OnceCallback<void(bool deserialize, const DATFileDataBuffer& dat_buf)>
+        cb) {
+  content::GetUIThreadTaskRunner({})->PostTaskAndReplyWithResult(


this is still called on more than one thread which is not safe. Also usage of base::Unretained is not safe here. Each class should be called on one and only one thread. The only exception is that a class can be created on one thread and then used on a different thread after that. I think it's best to add thread checks to enforce that

also avoid the use of named task runners like UIThread

bridiver · 2021-11-23T20:26:49Z

components/brave_shields/browser/ad_block_default_source_provider.cc

+    const base::FilePath& path) {
+  component_path_ = path;
+
+  base::ThreadPool::PostTaskAndReplyWithResult(


I would break these out into separate method calls and add comments to clarify what is happening

bridiver · 2021-11-23T20:29:17Z

components/brave_shields/browser/ad_block_test_source_provider.cc

@@ -0,0 +1,45 @@
+/* Copyright (c) 2021 The Brave Authors. All rights reserved.


file name should match the class name

components/adblock_rust_ffi/src/lib.rs

bridiver · 2021-12-01T00:59:14Z

components/brave_shields/browser/ad_block_regional_source_provider.cc

+void AdBlockRegionalSourceProvider::Load(
+    base::OnceCallback<void(bool deserialize, const DATFileDataBuffer& dat_buf)>
+        cb) {
+  if (component_path_.empty()) {


it's not ok to just ignore the callback

bridiver · 2021-12-01T01:00:53Z

components/brave_shields/browser/ad_block_resource_provider.h

+ public:
+  class Observer : public base::CheckedObserver {
+   public:
+    virtual void OnNewResourcesAvailable(const std::string& resources_json) = 0;


these aren't "new" resources, I would call this OnLoaded or something

components/brave_shields/browser/ad_block_source_provider.h

bridiver · 2021-12-01T01:07:43Z

components/brave_shields/browser/ad_block_engine_service.cc

@@ -253,49 +266,85 @@ void AdBlockBaseService::OnGetDATFileData(base::OnceClosure callback,
    return;
  }
  GetTaskRunner()->PostTask(


this shouldn't be using a task runner for anything anymore. AdBlockEngineService should only run on one thread

bridiver · 2021-12-01T01:12:52Z

components/brave_shields/browser/ad_block_default_source_provider.cc

+AdBlockDefaultSourceProvider::AdBlockDefaultSourceProvider(
+    component_updater::ComponentUpdateService* cus,
+    base::RepeatingCallback<void(const std::string& regional_catalog)>
+        regional_catalog_available_cb)


why is there a separate callback for this?

bridiver · 2021-12-01T01:19:34Z

components/brave_shields/browser/ad_block_source_provider.h

+                              const DATFileDataBuffer& dat_buf)>) = 0;
+
+ protected:
+  void ProvideNewDAT(const DATFileDataBuffer& dat_buf);


The convention for this would be OnDATLoaded, but I'm confused about why ListSource is also a DATFileDataBuffer

We have to pass the list source across the Rust FFI as a buffer as well because it's a potentially untrusted/invalid input. The FFI will attempt to parse it to a string, and handle it correspondingly if it doesn't parse correctly.

bridiver · 2021-12-01T01:20:27Z

components/brave_shields/browser/ad_block_source_provider.h

+ public:
+  class Observer : public base::CheckedObserver {
+   public:
+    virtual void OnNewDATAvailable(const DATFileDataBuffer& dat_buf) = 0;


same as below, this should be OnDATLoaded or OnDATReady

bridiver · 2021-12-01T01:34:00Z

components/brave_shields/browser/ad_block_service.cc

+      component_update_service_(cus),
+      task_runner_(task_runner),
+      subscription_service_manager_(std::move(subscription_service_manager)) {
+  default_source_provider_ =


the source provider should be owned by browser_process_impl and passed in to the AdBlockService

well, it doesn't necessarily need to be owned by browser_process_impl right now, but it should be created in browser_process_impl and passed in

bridiver · 2021-12-01T01:35:31Z

components/brave_shields/browser/ad_block_service.cc

+  default_source_provider_ =
+      std::make_unique<brave_shields::AdBlockDefaultSourceProvider>(
+          component_update_service_,
+          base::BindRepeating(&AdBlockService::OnRegionalCatalogFileDataReady,


AdBlockService should register as an observer for the SourceProvider

bridiver · 2021-12-01T01:38:01Z

components/brave_shields/browser/ad_block_service.cc

  return custom_filters_service_.get();
 }

+brave_shields::AdBlockCustomFiltersSourceProvider*
+AdBlockService::custom_filters_source_provider() {


nothing outside of this class should ever need access to the custom_filters_source_provider

bridiver · 2021-12-01T01:39:35Z

components/brave_shields/browser/domain_block_navigation_throttle.cc

@@ -61,27 +60,28 @@ std::unique_ptr<DomainBlockNavigationThrottle>
 DomainBlockNavigationThrottle::MaybeCreateThrottleFor(
    content::NavigationHandle* navigation_handle,
    AdBlockService* ad_block_service,
-    AdBlockCustomFiltersService* ad_block_custom_filters_service,
+    AdBlockCustomFiltersSourceProvider* ad_block_custom_filters_source_provider,


this is definitely not right, why are we passing in the source provider?

It's the same reason as #10994 (comment).

I think you're missing what I'm saying, the SourceProvider shouldn't be passed to other classes, they should only deal with the service.

I guess my reasoning is that there isn't a CustomFiltersService in the current implementation; just the engine and SourceProvider. The method to add the new custom filter could be added to AdBlockService in the current implementation, or adding CustomFiltersService to handle it could work too.

bridiver · 2021-12-01T01:44:48Z

components/brave_shields/browser/domain_block_controller_client.cc

-        ad_block_custom_filters_service_->GetCustomFilters();
-    ad_block_custom_filters_service_->UpdateCustomFilters(
+        ad_block_custom_filters_source_provider_->GetCustomFilters();
+    ad_block_custom_filters_source_provider_->UpdateCustomFilters(


we're actually modifying the filter to disable the rule? That's definitely not great, but a separate issue from this PR. Can you open a ticket for that? In any case these methods should be called on the service, not the source provider and DomainBlockControllerClient shouldn't have to know anything about the rules format. We should just be calling ad_block_custom_filters_service_->DisableRule(request_url_.host()) or something along those lines

Filed a followup at brave/brave-browser#19863

but both this class and DomainBlockNavigationThrottle should only ever talk to the service and shouldn't know anything about the underlying source provider so you should pass the service here and add methods to pass through to the source provider

bridiver · 2021-12-01T01:50:37Z

components/brave_shields/browser/ad_block_engine_service.cc

@@ -104,16 +104,17 @@ std::string ResourceTypeToString(blink::mojom::ResourceType resource_type) {

 namespace brave_shields {

-AdBlockBaseService::AdBlockBaseService(BraveComponent::Delegate* delegate)
-    : BaseBraveShieldsService(delegate),
+AdBlockEngineService::AdBlockEngineService(


I think you should drop Service here, it's very confusing

bridiver · 2021-12-01T01:54:27Z

components/brave_shields/browser/ad_block_default_source_provider.cc

+  if (cus) {
+    RegisterAdBlockDefaultComponent(
+        cus,
+        base::BindRepeating(&AdBlockDefaultSourceProvider::OnComponentReady,


this causes the SourceProvider to be called on more than one thread because OnComponentReady is called on the UI thread and Load is called from the adblock thread

bridiver · 2021-12-01T01:56:55Z

components/brave_shields/browser/ad_block_engine_service.cc

 }

 ///////////////////////////////////////////////////////////////////////////////

+std::unique_ptr<AdBlockEngineService> AdBlockEngineServiceFactory(


this method serves no useful purpose

bridiver · 2021-12-01T01:58:14Z

components/brave_shields/browser/ad_block_default_source_provider.h

+          regional_catalog_available_cb);
+  ~AdBlockDefaultSourceProvider() override;
+
+  void Load(base::OnceCallback<


don't overload method names that do different things. One of these loads a dat file and the other loads resources

they are technically overridden from AdBlockSourceProvider and AdBlockResourceProvider respectively; they make sense in context there but maybe not when overloaded side-by-side. I can rename to LoadDATBuffer and LoadResources respectively

iefremov · 2022-02-16T11:34:31Z

components/brave_shields/browser/ad_block_regional_service_manager.cc

  initialized_ = true;
 }

-AdBlockRegionalServiceManager::~AdBlockRegionalServiceManager() {}
+AdBlockRegionalServiceManager::~AdBlockRegionalServiceManager() {
+  catalog_provider_->RemoveObserver(this);


pls prefer ScopedObservation, for future reference

This method was only useful for the old extension-based cosmetic filtering implementation, and has started causing some tests to fail on the newer C++ implementation.

antonok-edm requested review from bridiver, iefremov, ShivanKaul and pilgrim-brave November 11, 2021 00:55

antonok-edm self-assigned this Nov 11, 2021

antonok-edm requested a review from a team as a code owner November 11, 2021 00:55

antonok-edm removed the request for review from ShivanKaul November 11, 2021 05:33

antonok-edm force-pushed the sourceprovider-adblock branch 2 times, most recently from aa460f7 to 89125c8 Compare November 11, 2021 18:25

bridiver reviewed Nov 23, 2021

View reviewed changes

components/adblock_rust_ffi/src/lib.rs Outdated Show resolved Hide resolved

antonok-edm force-pushed the sourceprovider-adblock branch from 89125c8 to 6ec06d4 Compare November 29, 2021 20:25

bridiver reviewed Dec 1, 2021

View reviewed changes

components/brave_shields/browser/ad_block_source_provider.h Outdated Show resolved Hide resolved

bridiver reviewed Dec 1, 2021

View reviewed changes

antonok-edm force-pushed the sourceprovider-adblock branch 2 times, most recently from f3015a9 to 761c204 Compare February 16, 2022 01:07

iefremov reviewed Feb 16, 2022

View reviewed changes

antonok-edm force-pushed the sourceprovider-adblock branch 2 times, most recently from 790525e to e2b5b59 Compare February 17, 2022 19:43

bridiver and others added 16 commits February 17, 2022 12:45

add httpse component installer

3153432

delete on correct task runner

c9a05e6

create engine to handle task runner vs UI thread

345c566

initialize engine right away

2578955

fix tests

7648cba

rename things according to review feedback

2718626

remove dead code

d8a2360

add regional catalog provider interface

8ae094f

call LoadRegionalCatalog in AdBlockRegionalServiceManager::Init

331afde

fix thread issues with new classes

6ceedb7

use the new engine

9ad25d7

add deleter for all adblock services

e4a1acc

fix destruction of providers

1ed764d

review feedback

bf82da8

final fixes

64d01d6

fix sequence checking in tests

391d032

antonok-edm force-pushed the sourceprovider-adblock branch from e2b5b59 to 391d032 Compare February 17, 2022 20:54

antonok-edm added 2 commits February 17, 2022 14:58

remove outdated WaitForBraveExtensionShieldsDataReady method

a60b50d

This method was only useful for the old extension-based cosmetic filtering implementation, and has started causing some tests to fail on the newer C++ implementation.

use a PrefChangeRegistrar for DefaultCookieListFlagEnabledTest

70c6a33

antonok-edm merged commit 28e41a4 into master Feb 18, 2022

antonok-edm deleted the sourceprovider-adblock branch February 18, 2022 02:37

antonok-edm added this to the 1.37.x - Nightly milestone Feb 18, 2022

antonok-edm mentioned this pull request Feb 18, 2022

Fix remaining threading issues in adblock code brave/brave-browser#21186

Closed

LaurenWags mentioned this pull request Mar 3, 2022

Decouple adblock engines from component loading methods brave/brave-browser#19366

Closed

goodov mentioned this pull request Sep 27, 2022

Non thread-safe PostTasks in AdBlockBaseService brave/brave-browser#15309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple adblock engines from component loading methods #10994

Decouple adblock engines from component loading methods #10994

antonok-edm commented Nov 11, 2021

bridiver Nov 23, 2021

bridiver Dec 1, 2021

bridiver Nov 23, 2021

bridiver Nov 23, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021 •

edited

Loading

bridiver Dec 1, 2021

antonok-edm Dec 2, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

antonok-edm Dec 1, 2021

bridiver Dec 8, 2021

antonok-edm Dec 9, 2021

bridiver Dec 1, 2021

antonok-edm Dec 1, 2021

bridiver Dec 8, 2021

iefremov Feb 14, 2022

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

bridiver Dec 1, 2021

antonok-edm Dec 2, 2021 •

edited

Loading

iefremov Feb 16, 2022

		@@ -0,0 +1,45 @@
		/* Copyright (c) 2021 The Brave Authors. All rights reserved.

Decouple adblock engines from component loading methods #10994

Decouple adblock engines from component loading methods #10994

Conversation

antonok-edm commented Nov 11, 2021

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonok-edm Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Dec 1, 2021 •

edited

Loading

antonok-edm Dec 2, 2021 •

edited

Loading