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.

Sign up for GitHub

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

Collapse HTML elements with blocked image/iframe requests #9144

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Collapse HTML elements with blocked image/iframe requests #9144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c94e99
collapse blocked requests
antonok-edm Jun 15, 2021
e91bfd4
add feature flag for element collapsing
antonok-edm Jun 15, 2021
5419b32
add tests for collapsed elements
antonok-edm Jun 16, 2021
fd1400e
add comments
antonok-edm Jun 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions browser/brave_shields/ad_block_service_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -1036,6 +1037,106 @@ 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();

EXPECT_EQ(true, EvalJs(contents,
"setExpectations(0, 1, 0, 0);"
"addImage('ad_banner.png')"));
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);

// 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"));
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a comment

}

// 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();

EXPECT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')"));
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);

// 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"));
}

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();

EXPECT_EQ(true, EvalJs(contents,
"setExpectations(0, 1, 0, 0);"
"addImage('ad_banner.png')"));
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);

// 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"));
}

// 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();

EXPECT_EQ(true, EvalJs(contents, "addFrame('ad_banner.png')"));
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);

// 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"));
}

// 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",
Expand Down
13 changes: 12 additions & 1 deletion browser/net/brave_proxying_url_loader_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -610,7 +611,17 @@ 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 (base::FeatureList::IsEnabled(
::brave_shields::features::kBraveAdblockCollapseBlockedElements) &&
ctx_->blocked_by == brave::kAdBlocked) {
collapse_status.should_collapse_initiator = true;
}

target_client_->OnComplete(collapse_status);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we please add a test?

Copy link
Collaborator Author

@antonok-edm antonok-edm Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I did look into this briefly - collapsed elements cannot be detected in JS, and the only API Chromium exposes for this is HTMLImageElement::IsCollapsed() (and similar for iframes), which is pretty deep in Blink.

Is it allowed to access the Blink elements in browsertests? Otherwise we could probably try some hacky JS where we check the size of a parent element, but it seems fragile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried for a bit to access IsCollapsed, but I don't see any convenient way to do it. I've used the JS clientHeight property instead, which I've found works here.

}

// Deletes |this|.
Expand Down
5 changes: 5 additions & 0 deletions chromium_src/chrome/browser/about_flags.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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, \
Expand Down
5 changes: 5 additions & 0 deletions chromium_src/chrome/browser/flag_descriptions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
2 changes: 2 additions & 0 deletions chromium_src/chrome/browser/flag_descriptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down
4 changes: 4 additions & 0 deletions components/brave_shields/common/features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ 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{
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add a comment?

"BraveAdblockCollapseBlockedElements", base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kBraveAdblockCosmeticFiltering{
"BraveAdblockCosmeticFiltering",
base::FEATURE_ENABLED_BY_DEFAULT};
Expand Down
1 change: 1 addition & 0 deletions components/brave_shields/common/features.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
17 changes: 17 additions & 0 deletions test/data/blocking.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down