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

Fix adblock default ruleset reparsing on startup #20585

Merged
merged 5 commits into from
Oct 18, 2023

Conversation

atuchin-m
Copy link
Collaborator

@atuchin-m atuchin-m commented Oct 18, 2023

Resolves brave/brave-browser#33718
Resolves brave/brave-browser#33532

The root of the issue is AdBlockFiltersProviderManager singleton. It subscribes to all the providers (in AdBlockFiltersProviderManager::AddProvider) => it gets notifications about an update in every provider.

Also, AdBlockFiltersProviderManager implements both AdBlockFiltersProvider and AdBlockFiltersProvider::Observer interfaces, acting like a decorator and proxying all the OnChanged to the listeners. The listeners are instances of SourceProviderObserver for the both engines (the manager is passed as AdBlockFiltersProvider* here
As a result, both engine observers get notifications about all the updates and rebuild the combined list of every update.

The PR fixes it in safe and straightforward way: adding to Observer::OnChange() extra information about what kind of source was updated. This allows us to filter out unnecessary notifications.

The PR:

  • implement a logic to suppress rebuilding the default engine on notifications about additional engine updates;
  • adds extra traces and UMA to track the number of engine rebuilding;
  • adds a browser test to verify the number of updates.

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:

  1. Verify that adblock engine works correctly (QA team);
  2. Verify the startup performance (perf team).

@atuchin-m atuchin-m self-assigned this Oct 18, 2023
@atuchin-m atuchin-m mentioned this pull request Oct 18, 2023
25 tasks
@@ -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_);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will prevent us from ocasionally calling deserializing from UI thread.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the OnDATLoaded method was actually removed in @antonok-edm's PR. I don't think we load dat files anymore

// 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(
Copy link
Collaborator Author

@atuchin-m atuchin-m Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify the expectations in format: {default engine updates, additional engine updates} in the begin of the test => the same of the end of the test

  1. (before the PR) {1, 1} => {3, 3}
  2. (after the PR) {1, 1} => {2, 2}
  3. (what we actually want in a long term perspective) {0, 0} => {1, 1}

UPD: changed the comment to match the latest changes.

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks ok, but I'd wait on @antonok-edm to merge

@atuchin-m atuchin-m enabled auto-merge (squash) October 18, 2023 20:43
@atuchin-m atuchin-m merged commit 8f12d5a into master Oct 18, 2023
15 checks passed
@atuchin-m atuchin-m deleted the fix-adblock-ruleset-reparsing branch October 18, 2023 21:27
@github-actions github-actions bot added this to the 1.61.x - Nightly milestone Oct 18, 2023
@atuchin-m atuchin-m restored the fix-adblock-ruleset-reparsing branch October 19, 2023 19:52
brave-builds added a commit that referenced this pull request Oct 19, 2023
brave-builds added a commit that referenced this pull request Oct 19, 2023
@kjozwiak
Copy link
Member

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.61.45 Chromium: 118.0.5993.96 (Official Build) nightly (64-bit)
-- | --
Revision | 31c36588a37ea88c2276d901ebd20dbf2344fde3
OS | Windows 11 Version 22H2 (Build 22621.2428)

Basically ensured that adblocking as working when running through #20137 (comment). QA will also run through some spot checks when we run through manual passes via 1.59.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[perf] Fix double rebuilding of default ruleset Add extra UMA and traces to adblock loading code
4 participants