From e085c14e6c1a4503ad396c895f6ea41d3e922072 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Tue, 19 Mar 2024 15:23:09 -0700 Subject: [PATCH] fix cross-engine exceptions in standard blocking mode --- .../ad_block_service_browsertest.cc | 19 +++++++++++++++++++ .../content/browser/ad_block_service.cc | 18 +++++++++++------- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index d330ca784244..e6455707c1a6 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -458,6 +458,25 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CustomBlockDefaultException) { EXPECT_EQ(profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); } +// Load a page with an image blocked by custom filters, with a corresponding +// exception installed in the default filters, and make sure it is not blocked. +IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, + CustomBlockDefaultExceptionStandardMode) { + DisableAggressiveMode(); + + UpdateAdBlockInstanceWithRules("@@ad_banner.png"); + UpdateCustomAdBlockInstanceWithRules("*ad_banner.png"); + + GURL url = embedded_test_server()->GetURL(kAdBlockTestPage); + NavigateToURL(url); + content::WebContents* contents = web_contents(); + + ASSERT_EQ(true, EvalJs(contents, + "setExpectations(1, 0, 0, 0);" + "addImage('ad_banner.png')")); + EXPECT_EQ(profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); +} + // Load a page with an image which is not an ad, and make sure it is NOT // blocked. IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, diff --git a/components/brave_shields/content/browser/ad_block_service.cc b/components/brave_shields/content/browser/ad_block_service.cc index 20f64db5e23a..1011138aa4c2 100644 --- a/components/brave_shields/content/browser/ad_block_service.cc +++ b/components/brave_shields/content/browser/ad_block_service.cc @@ -139,17 +139,15 @@ adblock::BlockerResult AdBlockService::ShouldStartRequest( bool previously_matched_important) { DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence()); - adblock::BlockerResult fp_result = {}; - + adblock::BlockerResult fp_result = default_engine_->ShouldStartRequest( + url, resource_type, tab_host, previously_matched_rule, + previously_matched_exception, previously_matched_important); if (aggressive_blocking || base::FeatureList::IsEnabled( brave_shields::features::kBraveAdblockDefault1pBlocking) || !SameDomainOrHost( url, url::Origin::CreateFromNormalizedTuple("https", tab_host, 80), net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { - fp_result = default_engine_->ShouldStartRequest( - url, resource_type, tab_host, previously_matched_rule, - previously_matched_exception, previously_matched_important); // removeparam results from the default engine are ignored in default // blocking mode if (!aggressive_blocking) { @@ -158,14 +156,20 @@ adblock::BlockerResult AdBlockService::ShouldStartRequest( if (fp_result.important) { return fp_result; } + } else { + // if there's an exception from the default engine, it still needs to be + // considered by the additional engine + fp_result = {.has_exception = fp_result.has_exception}; } GURL request_url = fp_result.rewritten_url.has_value ? GURL(std::string(fp_result.rewritten_url.value)) : url; auto result = additional_filters_engine_->ShouldStartRequest( - request_url, resource_type, tab_host, previously_matched_rule, - previously_matched_exception, previously_matched_important); + request_url, resource_type, tab_host, + previously_matched_rule | fp_result.matched, + previously_matched_exception | fp_result.has_exception, + previously_matched_important | fp_result.important); result.matched |= fp_result.matched; result.has_exception |= fp_result.has_exception;