Skip to content
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

Unify Adblock engines into 1 engine #22127

Merged
merged 10 commits into from
Apr 4, 2024
Merged

Unify Adblock engines into 1 engine #22127

merged 10 commits into from
Apr 4, 2024

Conversation

cuba
Copy link
Contributor

@cuba cuba commented Feb 16, 2024

Resolves brave/brave-browser#36035
Resolves brave/brave-browser#36539

Two issues are bundled here

  1. Unify engines into two (standard/general and aggressive/additional)
  2. Load from dat files during launch. Once unified the engines have to change how we load from cache. It is natural to do this using dat files instead of text files with very little additional work. Loading cached engines from text files would result in unneeded work and unneeded testing.

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 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:

  1. Test the upgrade process ensures the engine is ready (i.e. youtube ad-blocking works immediately after upgrading). Note: Additional lists will not be ready right away after upgrading as it can cost up to 24s to perform this upgrade on slow devices. But future relaunches should see them ready.
  2. Test that youtube adblocking works during relaunch and heading directly to youtube (including additional lists such as cookie consent notices)
  3. Test https://dev-pages.brave.software/filtering/cosmetic-filtering.html
  4. Test https://dev-pages.brave.software/filtering/scriptlets.html
  5. Test https://dev-pages.brave.software/filtering/additional-lists.html
  6. Test https://dev-pages.bravesoftware.com/filtering/text-ads.html
  7. Test https://dev-pages.brave.software/filtering/scriptlets.html

Performance:

iPhone 8

Previous This
Launch ~11s ~3.3s (default) ~8s (all filter lists)
Post-launch ~2.2s (default) ~12s (all filter lists) ~0.2s
Screenshot Screenshot 2024-03-27 at 11 21 16 AM Screenshot 2024-03-28 at 3 19 10 PM

iPhone 15 pro

Previous This
Launch ~2s ~0.9s (default) ~2s (all filter lists)
Post-launch ~0.9s (default) ~4s (all filter lists) ~0.02s

@cuba cuba added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 labels Feb 16, 2024
@cuba cuba requested a review from a team as a February 16, 2024 17:42
@cuba cuba force-pushed the js/ios-list-catalog branch from dca908f to 8d44e09 Compare February 20, 2024 16:45
@cuba cuba force-pushed the js/unified-engines branch from 648cb90 to 6d2c303 Compare February 20, 2024 17:15
@cuba cuba force-pushed the js/ios-list-catalog branch from 8d44e09 to fa85bbf Compare February 21, 2024 12:19
@cuba cuba force-pushed the js/unified-engines branch from 6d2c303 to 956740d Compare February 21, 2024 12:21
@cuba cuba force-pushed the js/ios-list-catalog branch 2 times, most recently from c65dd4e to 65ddc47 Compare February 21, 2024 13:59
@cuba cuba force-pushed the js/ios-list-catalog branch 2 times, most recently from cde00d6 to 9efc40d Compare February 29, 2024 13:50
@cuba cuba force-pushed the js/unified-engines branch from 956740d to 24c829c Compare February 29, 2024 15:15
@cuba cuba force-pushed the js/ios-list-catalog branch from 9efc40d to 28e45f8 Compare March 2, 2024 13:48
@cuba cuba force-pushed the js/unified-engines branch from 24c829c to bc65632 Compare March 2, 2024 13:59
@cuba cuba force-pushed the js/ios-list-catalog branch from 28e45f8 to 266bb1f Compare March 3, 2024 12:42
@cuba cuba force-pushed the js/unified-engines branch from bc65632 to 960537b Compare March 3, 2024 12:43
@cuba cuba force-pushed the js/ios-list-catalog branch from 266bb1f to 5ebcbe7 Compare March 7, 2024 18:08
@cuba cuba force-pushed the js/unified-engines branch from 960537b to a5d85e9 Compare March 7, 2024 18:08
@cuba cuba force-pushed the js/ios-list-catalog branch from 5ebcbe7 to 492022d Compare March 18, 2024 18:28
@cuba cuba force-pushed the js/unified-engines branch from a5d85e9 to e2ff3ca Compare March 18, 2024 18:45
Base automatically changed from js/ios-list-catalog to master March 19, 2024 16:33
@cuba cuba force-pushed the js/unified-engines branch from e2ff3ca to ba7f085 Compare March 20, 2024 17:50
@cuba cuba force-pushed the js/unified-engines branch 5 times, most recently from b978520 to f19959d Compare March 28, 2024 11:05
@cuba cuba force-pushed the js/unified-engines branch 5 times, most recently from d14f85a to 0692a80 Compare March 29, 2024 00:05
@cuba cuba force-pushed the js/unified-engines branch 3 times, most recently from 869d863 to 6edb329 Compare March 30, 2024 13:27
@cuba cuba force-pushed the js/unified-engines branch from 8e929f7 to 4baeedc Compare March 30, 2024 14:55
@cuba cuba requested a review from kylehickinson April 3, 2024 18:37
Copy link
Contributor

github-actions bot commented Apr 3, 2024

[puLL-Merge] - brave/brave-core@22127

Description

This PR makes changes to the ad-block engine management to support grouped filter lists and resources components. It introduces new classes AdBlockEngineManager and AdBlockGroupsManager to handle compilation and management of ad-block engines. The changes allow for better organization and performance of the ad-blocking system.

Changes

Changes

  • AdBlockGroupsManager.swift: A new class that manages standard and aggressive ad-block engines, compiling them from grouped filter lists and resources. It interacts with AdBlockEngineManager, ContentBlockerManager and a SourceProvider to handle enabling/disabling sources and lazy-loading of engines.

  • AdBlockEngineManager.swift: A new class that manages a single grouped ad-block engine. It handles adding/removing filter list info, checking if compilation is needed, loading engines from cache, and serializing/deserializing engines.

  • GroupedAdBlockEngine.swift (renamed from CachedAdBlockEngine.swift): Updated to work with the new grouped engine format. Added EngineType enum to specify standard vs aggressive engines. Changed init and factory methods to take a FilterListGroup.

  • FilterListResourceDownloader.swift, FilterListCustomURLDownloader.swift: Updated to interact with the new AdBlockGroupsManager when filter lists are downloaded or removed.

  • Various files: Updated to use the new GroupedAdBlockEngine and AdBlockGroupsManager types. Removed usage of AdBlockStats which was replaced by the new managers.

  • Tests: Added new tests for AdBlockEngineManager and AdBlockGroupsManager. Updated existing tests for the refactored types.

Security Hotspots

  • The defaultCachedFolderName in the GroupedAdBlockEngine.EngineType extension uses hardcoded strings. Ensure these match up with intended cache folders. However, this doesn't pose a high security risk.

  • AdBlockEngineManager writes compiled filter data to the cache folder. Ensure the serialized data is properly validated when loading. However, this is already being done, so risk is low.

  • legacyCacheFiles(for:) in the default SourceProvider blindly returns any locally cached data as valid sources initially. However, this is just a temporary transitional method, so risk is limited.

Overall, the refactoring looks solid from a security standpoint. The main areas to validate are proper handling of the serialized engine cache, but no major issues stand out in the changeset. Nice work on the modernization of this code!

@cuba cuba merged commit 9214771 into master Apr 4, 2024
19 checks passed
@cuba cuba deleted the js/unified-engines branch April 4, 2024 12:21
@github-actions github-actions bot added this to the 1.66.x - Nightly milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache AdBlock DAT files client side [iOS] Merge all filter lists (including general ones) into one engine
2 participants