From 7c94e99cdae9a0d0e1f666e26eefce1322ffa368 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Tue, 15 Jun 2021 12:01:53 -0700 Subject: [PATCH 1/4] collapse blocked requests --- browser/net/brave_proxying_url_loader_factory.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/browser/net/brave_proxying_url_loader_factory.cc b/browser/net/brave_proxying_url_loader_factory.cc index ca7153276e8c..9078308ecf94 100644 --- a/browser/net/brave_proxying_url_loader_factory.cc +++ b/browser/net/brave_proxying_url_loader_factory.cc @@ -610,7 +610,15 @@ void BraveProxyingURLLoaderFactory::InProgressRequest:: void BraveProxyingURLLoaderFactory::InProgressRequest::OnRequestError( const network::URLLoaderCompletionStatus& status) { if (!request_completed_) { - target_client_->OnComplete(status); + // Make a non-const copy of status so that |should_collapse_initiator| can + // be modified + network::URLLoaderCompletionStatus collapse_status(status); + + if (ctx_->blocked_by == brave::kAdBlocked) { + collapse_status.should_collapse_initiator = true; + } + + target_client_->OnComplete(collapse_status); } // Deletes |this|. From e91bfd4d3f03f592705bc2ddb872624520288a79 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Tue, 15 Jun 2021 13:40:48 -0700 Subject: [PATCH 2/4] add feature flag for element collapsing --- browser/net/brave_proxying_url_loader_factory.cc | 5 ++++- chromium_src/chrome/browser/about_flags.cc | 5 +++++ chromium_src/chrome/browser/flag_descriptions.cc | 5 +++++ chromium_src/chrome/browser/flag_descriptions.h | 2 ++ components/brave_shields/common/features.cc | 2 ++ components/brave_shields/common/features.h | 1 + 6 files changed, 19 insertions(+), 1 deletion(-) diff --git a/browser/net/brave_proxying_url_loader_factory.cc b/browser/net/brave_proxying_url_loader_factory.cc index 9078308ecf94..263d0e123770 100644 --- a/browser/net/brave_proxying_url_loader_factory.cc +++ b/browser/net/brave_proxying_url_loader_factory.cc @@ -14,6 +14,7 @@ #include "base/task/post_task.h" #include "brave/browser/net/brave_request_handler.h" #include "brave/components/brave_shields/browser/adblock_stub_response.h" +#include "brave/components/brave_shields/common/features.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" @@ -614,7 +615,9 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::OnRequestError( // be modified network::URLLoaderCompletionStatus collapse_status(status); - if (ctx_->blocked_by == brave::kAdBlocked) { + if (base::FeatureList::IsEnabled( + ::brave_shields::features::kBraveAdblockCollapseBlockedElements) && + ctx_->blocked_by == brave::kAdBlocked) { collapse_status.should_collapse_initiator = true; } diff --git a/chromium_src/chrome/browser/about_flags.cc b/chromium_src/chrome/browser/about_flags.cc index 4c1b9c4fe130..4f513022e339 100644 --- a/chromium_src/chrome/browser/about_flags.cc +++ b/chromium_src/chrome/browser/about_flags.cc @@ -25,6 +25,7 @@ #include "net/base/features.h" using brave_shields::features::kBraveAdblockCnameUncloaking; +using brave_shields::features::kBraveAdblockCollapseBlockedElements; using brave_shields::features::kBraveAdblockCosmeticFiltering; using brave_shields::features::kBraveAdblockCosmeticFilteringNative; using brave_shields::features::kBraveAdblockCspRules; @@ -135,6 +136,10 @@ using ntp_background_images::features::kBraveNTPSuperReferralWallpaper; flag_descriptions::kBraveAdblockCnameUncloakingName, \ flag_descriptions::kBraveAdblockCnameUncloakingDescription, kOsAll, \ FEATURE_VALUE_TYPE(kBraveAdblockCnameUncloaking)}, \ + {"brave-adblock-collapse-blocked-elements", \ + flag_descriptions::kBraveAdblockCollapseBlockedElementsName, \ + flag_descriptions::kBraveAdblockCollapseBlockedElementsDescription, \ + kOsAll, FEATURE_VALUE_TYPE(kBraveAdblockCollapseBlockedElements)}, \ {"brave-adblock-cosmetic-filtering", \ flag_descriptions::kBraveAdblockCosmeticFilteringName, \ flag_descriptions::kBraveAdblockCosmeticFilteringDescription, kOsAll, \ diff --git a/chromium_src/chrome/browser/flag_descriptions.cc b/chromium_src/chrome/browser/flag_descriptions.cc index cb9a007e9379..9f4c230f4cc0 100644 --- a/chromium_src/chrome/browser/flag_descriptions.cc +++ b/chromium_src/chrome/browser/flag_descriptions.cc @@ -25,6 +25,11 @@ const char kBraveAdblockCnameUncloakingName[] = "Enable CNAME uncloaking"; const char kBraveAdblockCnameUncloakingDescription[] = "Take DNS CNAME records into account when making network request blocking " "decisions."; +const char kBraveAdblockCollapseBlockedElementsName[] = + "Collapse HTML elements with blocked source attributes"; +const char kBraveAdblockCollapseBlockedElementsDescription[] = + "Cause iframe and img elements to be collapsed if the URL of their src " + "attribute is blocked"; const char kBraveAdblockCosmeticFilteringName[] = "Enable cosmetic filtering"; const char kBraveAdblockCosmeticFilteringDescription[] = "Enable support for cosmetic filtering"; diff --git a/chromium_src/chrome/browser/flag_descriptions.h b/chromium_src/chrome/browser/flag_descriptions.h index 59908f9ccce6..89b5af2aa987 100644 --- a/chromium_src/chrome/browser/flag_descriptions.h +++ b/chromium_src/chrome/browser/flag_descriptions.h @@ -17,6 +17,8 @@ extern const char kBraveNTPBrandedWallpaperDemoName[]; extern const char kBraveNTPBrandedWallpaperDemoDescription[]; extern const char kBraveAdblockCnameUncloakingName[]; extern const char kBraveAdblockCnameUncloakingDescription[]; +extern const char kBraveAdblockCollapseBlockedElementsName[]; +extern const char kBraveAdblockCollapseBlockedElementsDescription[]; extern const char kBraveAdblockCosmeticFilteringName[]; extern const char kBraveAdblockCosmeticFilteringNativeName[]; extern const char kBraveAdblockCosmeticFilteringDescription[]; diff --git a/components/brave_shields/common/features.cc b/components/brave_shields/common/features.cc index b6af2a6996d0..b8d5c5780c71 100644 --- a/components/brave_shields/common/features.cc +++ b/components/brave_shields/common/features.cc @@ -15,6 +15,8 @@ namespace features { // substituted for any canonical name found. const base::Feature kBraveAdblockCnameUncloaking{ "BraveAdblockCnameUncloaking", base::FEATURE_ENABLED_BY_DEFAULT}; +const base::Feature kBraveAdblockCollapseBlockedElements{ + "BraveAdblockCollapseBlockedElements", base::FEATURE_ENABLED_BY_DEFAULT}; const base::Feature kBraveAdblockCosmeticFiltering{ "BraveAdblockCosmeticFiltering", base::FEATURE_ENABLED_BY_DEFAULT}; diff --git a/components/brave_shields/common/features.h b/components/brave_shields/common/features.h index cace87e13f3d..18f3184700c7 100644 --- a/components/brave_shields/common/features.h +++ b/components/brave_shields/common/features.h @@ -13,6 +13,7 @@ struct Feature; namespace brave_shields { namespace features { extern const base::Feature kBraveAdblockCnameUncloaking; +extern const base::Feature kBraveAdblockCollapseBlockedElements; extern const base::Feature kBraveAdblockCosmeticFiltering; extern const base::Feature kBraveAdblockCosmeticFilteringNative; extern const base::Feature kBraveAdblockCspRules; From 5419b32dc060a9fbfd829e5bd7cdf301040f852f Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Wed, 16 Jun 2021 13:05:37 -0700 Subject: [PATCH 3/4] add tests for collapsed elements --- .../ad_block_service_browsertest.cc | 89 +++++++++++++++++++ test/data/blocking.html | 17 ++++ 2 files changed, 106 insertions(+) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index d5325fbfcd4f..05be4327aa2d 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -69,6 +69,7 @@ const char kRegionalAdBlockComponentTest64PublicKey[] = "LwIDAQAB"; using brave_shields::features::kBraveAdblockCnameUncloaking; +using brave_shields::features::kBraveAdblockCollapseBlockedElements; using brave_shields::features::kBraveAdblockCosmeticFiltering; using content::BrowserThread; @@ -1036,6 +1037,94 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TagPrefsControlTags) { AssertTagExists(brave_shields::kLinkedInEmbeds, false); } +// Load a page with a blocked image, and make sure it is collapsed. +IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CollapseBlockedImage) { + ASSERT_TRUE(InstallDefaultAdBlockExtension()); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + + GURL url = embedded_test_server()->GetURL(kAdBlockTestPage); + ui_test_utils::NavigateToURL(browser(), url); + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + ASSERT_EQ(true, EvalJs(contents, + "setExpectations(0, 1, 0, 0);" + "addImage('ad_banner.png')")); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); + + ASSERT_EQ(true, EvalJs(contents, + "let i = document.getElementsByClassName('adImage');" + "i[0].clientHeight === 0")); +} + +// Load a page with a blocked iframe, and make sure it is collapsed. +IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CollapseBlockedIframe) { + ASSERT_TRUE(InstallDefaultAdBlockExtension()); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + + GURL url = embedded_test_server()->GetURL(kAdBlockTestPage); + ui_test_utils::NavigateToURL(browser(), url); + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + ASSERT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')")); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); + + ASSERT_EQ(true, EvalJs(contents, + "let i = document.getElementsByClassName('adFrame');" + "i[0].clientHeight === 0")); +} + +class CollapseBlockedElementsFlagDisabledTest : public AdBlockServiceTest { + public: + CollapseBlockedElementsFlagDisabledTest() { + feature_list_.InitAndDisableFeature(kBraveAdblockCollapseBlockedElements); + } + + private: + base::test::ScopedFeatureList feature_list_; +}; + +// Load a page with a blocked image, and make sure it is not collapsed. +IN_PROC_BROWSER_TEST_F(CollapseBlockedElementsFlagDisabledTest, + DontCollapseBlockedImage) { + ASSERT_TRUE(InstallDefaultAdBlockExtension()); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + + GURL url = embedded_test_server()->GetURL(kAdBlockTestPage); + ui_test_utils::NavigateToURL(browser(), url); + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + ASSERT_EQ(true, EvalJs(contents, + "setExpectations(0, 1, 0, 0);" + "addImage('ad_banner.png')")); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); + + ASSERT_EQ(true, EvalJs(contents, + "let i = document.getElementsByClassName('adImage');" + "i[0].clientHeight !== 0")); +} + +// Load a page with a blocked iframe, and make sure it is not collapsed. +IN_PROC_BROWSER_TEST_F(CollapseBlockedElementsFlagDisabledTest, + DontCollapseBlockedIframe) { + ASSERT_TRUE(InstallDefaultAdBlockExtension()); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + + GURL url = embedded_test_server()->GetURL(kAdBlockTestPage); + ui_test_utils::NavigateToURL(browser(), url); + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + ASSERT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')")); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); + + ASSERT_EQ(true, EvalJs(contents, + "let i = document.getElementsByClassName('adFrame');" + "i[0].clientHeight !== 0")); +} + // Load a page with a script which uses a redirect data URL. IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectRulesAreRespected) { UpdateAdBlockInstanceWithRules("js_mock_me.js$redirect=noopjs", diff --git a/test/data/blocking.html b/test/data/blocking.html index 0634a8b8e4e9..e7a2b7904e6f 100644 --- a/test/data/blocking.html +++ b/test/data/blocking.html @@ -67,6 +67,23 @@ }); } +// Adds an iframe to the DOM with the specified src +function addFrame(src) { + return new Promise(resolve => { + const frame = document.createElement('iframe') + frame.src = src + frame.className = 'adFrame' + + frame.addEventListener('load', () => { + resolve(onLoad()) + }) + frame.addEventListener('error', () => { + resolve(onLoad()) + }) + document.body.appendChild(frame) + }); +} + function installBlockingServiceWorker() { return new Promise(resolve => { navigator.serviceWorker.addEventListener('message', msg => { From fd1400e240be3a648aebd262c3fd16b0b604f8e7 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 17 Jun 2021 08:18:04 -0700 Subject: [PATCH 4/4] add comments --- .../ad_block_service_browsertest.cc | 28 +++++++++++++------ components/brave_shields/common/features.cc | 2 ++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 05be4327aa2d..59e03dcfb66f 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -1047,12 +1047,15 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CollapseBlockedImage) { content::WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); - ASSERT_EQ(true, EvalJs(contents, + EXPECT_EQ(true, EvalJs(contents, "setExpectations(0, 1, 0, 0);" "addImage('ad_banner.png')")); EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); - ASSERT_EQ(true, EvalJs(contents, + // There is no way for JS to directly tell if an element has been collapsed, + // but the clientHeight property is zero for collapsed elements and nonzero + // otherwise. + EXPECT_EQ(true, EvalJs(contents, "let i = document.getElementsByClassName('adImage');" "i[0].clientHeight === 0")); } @@ -1067,10 +1070,13 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CollapseBlockedIframe) { content::WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); - ASSERT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')")); + EXPECT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')")); EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); - ASSERT_EQ(true, EvalJs(contents, + // There is no way for JS to directly tell if an element has been collapsed, + // but the clientHeight property is zero for collapsed elements and nonzero + // otherwise. + EXPECT_EQ(true, EvalJs(contents, "let i = document.getElementsByClassName('adFrame');" "i[0].clientHeight === 0")); } @@ -1096,12 +1102,15 @@ IN_PROC_BROWSER_TEST_F(CollapseBlockedElementsFlagDisabledTest, content::WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); - ASSERT_EQ(true, EvalJs(contents, + EXPECT_EQ(true, EvalJs(contents, "setExpectations(0, 1, 0, 0);" "addImage('ad_banner.png')")); EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); - ASSERT_EQ(true, EvalJs(contents, + // There is no way for JS to directly tell if an element has been collapsed, + // but the clientHeight property is zero for collapsed elements and nonzero + // otherwise. + EXPECT_EQ(true, EvalJs(contents, "let i = document.getElementsByClassName('adImage');" "i[0].clientHeight !== 0")); } @@ -1117,10 +1126,13 @@ IN_PROC_BROWSER_TEST_F(CollapseBlockedElementsFlagDisabledTest, content::WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); - ASSERT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')")); + EXPECT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')")); EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL); - ASSERT_EQ(true, EvalJs(contents, + // There is no way for JS to directly tell if an element has been collapsed, + // but the clientHeight property is zero for collapsed elements and nonzero + // otherwise. + EXPECT_EQ(true, EvalJs(contents, "let i = document.getElementsByClassName('adFrame');" "i[0].clientHeight !== 0")); } diff --git a/components/brave_shields/common/features.cc b/components/brave_shields/common/features.cc index b8d5c5780c71..bd52d8a11e8c 100644 --- a/components/brave_shields/common/features.cc +++ b/components/brave_shields/common/features.cc @@ -15,6 +15,8 @@ namespace features { // substituted for any canonical name found. const base::Feature kBraveAdblockCnameUncloaking{ "BraveAdblockCnameUncloaking", base::FEATURE_ENABLED_BY_DEFAULT}; +// When enabled, Brave will apply HTML element collapsing to all images and +// iframes that initiate a blocked network request. const base::Feature kBraveAdblockCollapseBlockedElements{ "BraveAdblockCollapseBlockedElements", base::FEATURE_ENABLED_BY_DEFAULT}; const base::Feature kBraveAdblockCosmeticFiltering{