Implement unified adblock catalog #19946

antonok-edm · 2023-08-30T18:29:38Z

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
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 wiki
- npm run lint, npm run presubmit wiki, 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 change makes the default adblock component available slightly later on first startup after install, as it requires an additional network round-trip for a component installation. It'd be good to test the following:

With a completely fresh install, open Brave and as quickly as humanly possible navigate to a page that is likely to have ads and/or cookie banners if Shields are down (i.e. definitely YouTube, maybe some others as well). Shields should still be working correctly. This should be the case on a wide range of internet connnections (slow/fast, high/low latency).

If there are any issues with this, it's possible to preload the default adblock component's ID and public key like we had before in a followup.

github-actions · 2023-08-30T18:34:24Z

components/brave_shields/browser/filter_list_catalog_entry.cc

+    if (i > UINT8_MAX) {
+      return false;
+    }
+    *field = (uint8_t)i;


_{reported by reviewdog 🐶}
[semgrep] Semgrep found a cast from int (signed) to uint8_t. For arithmetic use base::CheckAdd(value1, value2).AssignIfValid(&result)

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/c/cast-signed-to-unsigned.yaml

Cc @fmarier @thypon

I updated the check above to the following, although it doesn't appear in the preview here for some reason:

if (i < 0 || i > UINT8_MAX) { return false; }

@bridiver should we use some native helper here?

There's base::IsValueInRangeForNumericType<uint8_t>(i) but presumedly that won't help with the cast warning either. Closest I found for a one-liner was base::checked_cast<uint8_t>(value->GetInt()) but that CHECKs on out-of-range values instead of returning.

we definitely should not be doing a c-style cast anywhere, but base::checked_cast is probably what you want here. If you want to fail without a CHECK then call IsValueInRangeForNumericType first

https://chromium.googlesource.com/chromium/src/+/main/base/numerics/README.md

kylehickinson · 2023-08-30T20:46:58Z

ios/browser/api/brave_shields/adblock_filter_list_catalog_entry.mm

+@property(nonatomic, copy) bool hidden;
+@property(nonatomic, copy) bool default_enabled;
+@property(nonatomic, copy) bool first_party_protections;
+@property(nonatomic, copy) uint8_t permission_mask;


copy doesn't apply to primative types, it can be removed entirely and just be @property(nonatomic)

You didn't expose any of these properties in the header file so they aren't accessible on the iOS side

Camel-case for Obj-C property names and prefixed with is for bools (i.e. isHidden, isEnabledByDefault, isFirstPartyProtection, etc)

ah, exactly what I needed to know 🙏

updated

kylehickinson

iOS++

cc @cuba just to notify about upcoming non-breaking changes

bridiver · 2023-09-05T17:35:50Z

components/brave_shields/browser/ad_block_filters_provider_manager.cc

-      base::BindOnce(&AdBlockFiltersProviderManager::FinishCombinating,
-                     weak_factory_.GetWeakPtr(), std::move(cb)));
+  const auto collect_and_merge = base::BarrierClosure(
+      filters_providers.size(), base::BindOnce(std::move(cb)));


why are you adding another BindOnce to the callback here?

good point, fixed

bridiver · 2023-09-05T19:15:42Z

components/brave_shields/adblock/rs/src/filter_set.rs

+                .add_filter_list(
+                    std::str::from_utf8(rules.as_slice())?,
+                    ParseOptions {
+                        permissions: PermissionMask::from_bits(permission_mask),


permissions?

I've documented it in a few places including here and here; let me know if there's somewhere else you'd prefer to see more information.

https://github.com/brave/adblock-resources/pull/130/files#r1329289005

seems like you have an enum on one end, but not sure about the cpp end and definitely needs some docs, preferably in the code in the other place I commented

bridiver · 2023-09-18T21:10:15Z

browser/perf/brave_perf_features_processor.cc

-      g_brave_browser_process->ad_block_service()->regional_service_manager();
-  if (!regional_service_manager ||
-      !regional_service_manager->IsFilterListAvailable(
+  auto* component_service_manager =


component is already so overloaded, is there a better name we can use here?

😅 It is at least managing all the filter lists fetched via the component updater now. Previously it managed a weird subset of them, and the original "regional" naming was never 100% accurate for as long as I can remember.

Would CRXServiceManager be better?

see DM, I think maybe it's better to not expose this in the public api at all. I don't think callers of these methods need to care that there is a ComponentServiceManager that handles certain providers and a subscription manager that handles others, they just want to check IsFilterListAvailable or whatever the case may be

bridiver · 2023-09-18T21:22:42Z

components/brave_shields/browser/ad_block_engine.cc

+                                      const std::string& resources_json) {
+  auto result = adblock::engine_from_filter_set(std::move(filter_set));
+  if (result.result_kind != adblock::ResultKind::Success) {
+    LOG(ERROR) << "AdBlockEngine::OnFilterSetLoaded failed: "


Same as the other one, logging like this generally isn't useful so we would normally restrict this to a VLOG. We shouldn't just be logging errors into the main chrome log and actually I think even that is disabled by default maybe

bridiver · 2023-09-18T22:33:14Z

components/brave_shields/browser/ad_block_service.cc

          }
        },
-        adblock_engine_->AsWeakPtr(), deserialize_, std::move(dat_buf_),
-        resources_json);
+        adblock_engine_->AsWeakPtr(), *std::move(filter_set_), resources_json);


I think you might have something a bit weird here with *std::move(filter_set_). You want to move the underlying rust::Box<adblock::FilterSet>, not the absl::optional so I think it should be std::move(*filter_set_)

bridiver · 2023-09-18T22:45:52Z

components/brave_shields/browser/ad_block_service.cc

-  dat_buf_ = std::move(dat_buf);
+void AdBlockService::SourceProviderObserver::OnFilterSetLoaded(
+    rust::Box<adblock::FilterSet>* filter_set) {
+  filter_set_ = absl::make_optional(std::move(*filter_set));


This seems very odd and confusing. Taking ownership from a pointer is not the behavior you would expect. When using base::Owned the callback should not know or need to know that base::Owned was used and in this case it definitely would need to know that. I think you want auto* filter_set to be a unique_ptr, get a raw pointer reference using .get() before you create on_loaded_cb (and call std::move) and then pass that as base::Unretained to LoadFilterSetForEngine

I think this would also be simpler if you used unique_ptr instead of absl::optional which makes things a bit confusing here and also https://github.com/brave/brave-core/pull/19946/files#r1329348390

bridiver · 2023-09-19T19:23:44Z

components/brave_shields/browser/filter_list_catalog_entry.cc

@@ -70,6 +71,20 @@ bool GetStringVector(const base::Value* value,
  }
 }

+bool GetUint8(const base::Value* value, uint8_t* field) {


do we have any tests that cover this indirectly?

bridiver

looks good for merge to fix the bug and we'll follow-up with other changes

* The file was renamed from #19946

This reverts commit 2e17608, reversing changes made to 13c48d9.

antonok-edm requested review from bridiver and ShivanKaul August 30, 2023 18:29

antonok-edm requested a review from a team as a code owner August 30, 2023 18:29

antonok-edm self-assigned this Aug 30, 2023

github-actions bot reviewed Aug 30, 2023

View reviewed changes

github-actions bot added the needs-security-review label Aug 30, 2023

github-actions bot assigned fmarier and thypon Aug 30, 2023

This was referenced Aug 30, 2023

Unify default.json and regional.json into list_catalog.json brave/adblock-resources#130

Merged

Support unified adblock catalog brave/brave-core-crx-packager#687

Merged

antonok-edm force-pushed the unified-adblock-catalog branch from bd4762d to 3ba033e Compare August 30, 2023 20:29

antonok-edm requested a review from a team as a code owner August 30, 2023 20:29

antonok-edm force-pushed the unified-adblock-catalog branch from 3ba033e to f22f593 Compare August 30, 2023 20:37

kylehickinson requested changes Aug 30, 2023

View reviewed changes

antonok-edm force-pushed the unified-adblock-catalog branch 3 times, most recently from aa4acaf to 6c79625 Compare August 30, 2023 21:32

antonok-edm requested review from kylehickinson and cuba August 30, 2023 21:32

antonok-edm force-pushed the unified-adblock-catalog branch 2 times, most recently from b26d9be to 705f6de Compare August 30, 2023 22:13

kylehickinson approved these changes Aug 31, 2023

View reviewed changes

bridiver reviewed Sep 5, 2023

View reviewed changes

This was referenced Sep 6, 2023

[iOS] Use correct components for Adblock services brave/brave-browser#32799

Closed

Some Scriptlet Injections Filters [from uBlock] can/will crash Brave and not work correctly, because they are not parsed correctly in Brave's Adblocker brave/adblock-rust#307

Closed

antonok-edm force-pushed the unified-adblock-catalog branch 4 times, most recently from 06f0573 to e2fbd03 Compare September 13, 2023 17:47

reduce code duplication in SourceProviderObserver

91fd4a4

bridiver reviewed Sep 18, 2023

View reviewed changes

antonok-edm added 7 commits September 18, 2023 16:11

reload adblock engines via FilterSet

9656930

remove hacky empty dat_buf_ workaround

1a6ad1f

add methods for combinating FilterSets in all FiltersProviders

bd1a6f7

remove the now unused LoadDAT family of methods

2ec4c7c

support permission_mask field

45a8b34

seemingly unrelated formatting fixes

ca37c45

fix DebounceBrowserTest.DebounceBeforeDomainBlock

e274e6a

antonok-edm force-pushed the unified-adblock-catalog branch from 9f7a438 to e274e6a Compare September 19, 2023 00:19

antonok-edm requested a review from bridiver September 19, 2023 03:20

use std::unique_ptr instead of absl::optional

cfe8a82

antonok-edm force-pushed the unified-adblock-catalog branch from 0b9f8a0 to cfe8a82 Compare September 19, 2023 03:43

bridiver reviewed Sep 19, 2023

View reviewed changes

bridiver approved these changes Sep 19, 2023

View reviewed changes

antonok-edm merged commit 2e17608 into master Sep 19, 2023
15 checks passed

antonok-edm deleted the unified-adblock-catalog branch September 19, 2023 21:47

sangwoo108 mentioned this pull request Sep 20, 2023

Fix build #20227

Merged

25 tasks

sangwoo108 added a commit that referenced this pull request Sep 20, 2023

Fix build (#20227)

9a55916

* The file was renamed from #19946

SergeyZhukovsky mentioned this pull request Sep 25, 2023

ANR on AddDATBufferToFilterSet brave/brave-browser#33245

Closed

kjozwiak added this to the 1.60.x - Nightly milestone Sep 25, 2023

atuchin-m added a commit that referenced this pull request Oct 7, 2023

Revert "Merge pull request #19946 from brave/unified-adblock-catalog"

3cd2cf1

This reverts commit 2e17608, reversing changes made to 13c48d9.

atuchin-m mentioned this pull request Oct 7, 2023

Revert unified adblock catalog #20442

Merged

25 tasks

antonok-edm mentioned this pull request Oct 13, 2023

Unrevert unified adblock catalog w/ perf fixes brave/brave-browser#33621

Closed

This was referenced Nov 3, 2023

Problematic first party elements are not blocked by Shields in Standard mode brave/brave-browser#34081

Closed

Add first-party exception list to the correct adblock engine #20826

Merged

antonok-edm mentioned this pull request Jan 31, 2024

Unrevert unified adblock catalog #21556

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unified adblock catalog #19946

Implement unified adblock catalog #19946

antonok-edm commented Aug 30, 2023

github-actions bot Aug 30, 2023

antonok-edm Aug 30, 2023

thypon Aug 30, 2023

rillian Aug 31, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

kylehickinson Aug 30, 2023

kylehickinson Aug 30, 2023 •

edited

Loading

antonok-edm Aug 30, 2023

kylehickinson left a comment

bridiver Sep 5, 2023

antonok-edm Sep 13, 2023

bridiver Sep 5, 2023

antonok-edm Sep 13, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

antonok-edm Sep 18, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

bridiver Sep 18, 2023

bridiver Sep 19, 2023

bridiver left a comment

Implement unified adblock catalog #19946

Implement unified adblock catalog #19946

Conversation

antonok-edm commented Aug 30, 2023

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot Aug 30, 2023

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

kylehickinson Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson left a 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

bridiver left a comment

Choose a reason for hiding this comment

kylehickinson Aug 30, 2023 •

edited

Loading