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

Jump to bottom

Explicitly cancel blocked requests #6632

Merged
merged 1 commit into from
Nov 2, 2020
Merged

Explicitly cancel blocked requests #6632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 12 additions & 33 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 @@ -273,7 +273,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByDefaultBlocker) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0, 0, 0);"
"setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('ad_banner.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -317,7 +317,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByCustomBlocker) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0, 0, 0);"
"setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('ad_banner.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -369,7 +369,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByRegionalBlocker) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0, 0, 0);"
"setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('ad_fr.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -430,7 +430,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0, 0, 0);"
"setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('v4_specific_banner.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -453,7 +453,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TwoSameAdsGetCountedAsOne) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 0, 1, 2, 0);"
"setExpectations(0, 0, 0, 1, 0, 2);"
"xhr('adbanner.js');"
"xhr('normal.js');"
"xhr('adbanner.js')",
Expand All @@ -477,7 +477,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TwoDiffAdsGetCountedAsTwo) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 0, 1, 2, 0);"
"setExpectations(0, 0, 0, 1, 0, 2);"
"xhr('adbanner.js?1');"
"xhr('normal.js');"
"xhr('adbanner.js?2')",
Expand All @@ -501,7 +501,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, NewTabContinuesToBlock) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 0, 0, 1, 0);"
"setExpectations(0, 0, 0, 0, 0, 1);"
"xhr('adbanner.js');",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -512,7 +512,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, NewTabContinuesToBlock) {

as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 0, 0, 1, 0);"
"setExpectations(0, 0, 0, 0, 0, 1);"
"xhr('adbanner.js');",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -536,7 +536,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SubFrame) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents->GetAllFrames()[1],
"setExpectations(0, 0, 0, 0, 1, 0);"
"setExpectations(0, 0, 0, 0, 0, 1);"
"xhr('adbanner.js?1');",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -618,7 +618,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
base::StringPrintf("setExpectations(0, 1, 0, 0, 0, 0);"
base::StringPrintf("setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
Expand All @@ -639,7 +639,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, BlockNYP) {
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
base::StringPrintf("setExpectations(0, 1, 0, 0, 0, 0);"
base::StringPrintf("setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
Expand All @@ -666,7 +666,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SocialButttonAdBLockTagTest) {
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
base::StringPrintf("setExpectations(0, 1, 0, 0, 0, 0);"
base::StringPrintf("setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
Expand Down Expand Up @@ -755,27 +755,6 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TagPrefsControlTags) {
AssertTagExists(brave_shields::kLinkedInEmbeds, false);
}

// Make sure that cancelrequest actually blocks
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CancelRequestOptionTest) {
UpdateAdBlockInstanceWithRules("logo.png$explicitcancel");
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
GURL tab_url = embedded_test_server()->GetURL("b.com", kAdBlockTestPage);
GURL resource_url =
embedded_test_server()->GetURL("example.com", "/logo.png");
ui_test_utils::NavigateToURL(browser(), tab_url);
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(
contents,
base::StringPrintf("setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);
}

// Load a page with a script which uses a redirect data URL.
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectRulesAreRespected) {
UpdateAdBlockInstanceWithRules(
Expand Down
4 changes: 2 additions & 2 deletions browser/net/brave_ad_block_tp_network_delegate_helper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ void ShouldBlockAdOnTaskRunner(std::shared_ptr<BraveRequestInfo> ctx,
std::string tab_host = ctx->tab_origin.host();
if (!g_brave_browser_process->ad_block_service()->ShouldStartRequest(
ctx->request_url, ctx->resource_type, tab_host, &did_match_exception,
&ctx->cancel_request_explicitly, &ctx->mock_data_url)) {
&ctx->mock_data_url)) {
ctx->blocked_by = kAdBlocked;
} else if (!did_match_exception && canonical_name.has_value() &&
ctx->request_url.host() != *canonical_name &&
Expand All @@ -73,7 +73,7 @@ void ShouldBlockAdOnTaskRunner(std::shared_ptr<BraveRequestInfo> ctx,

if (!g_brave_browser_process->ad_block_service()->ShouldStartRequest(
canonical_url, ctx->resource_type, tab_host, &did_match_exception,
&ctx->cancel_request_explicitly, &ctx->mock_data_url)) {
&ctx->mock_data_url)) {
ctx->blocked_by = kAdBlocked;
}
}
Expand Down
3 changes: 2 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 @@ -348,10 +348,11 @@ void BraveProxyingURLLoaderFactory::InProgressRequest::

// TODO(iefremov): Shorten
if (ctx_->blocked_by != brave::kNotBlocked) {
if (ctx_->cancel_request_explicitly) {
if (!ctx_->ShouldMockRequest()) {
OnRequestError(network::URLLoaderCompletionStatus(net::ERR_ABORTED));
return;
}

auto response = network::mojom::URLResponseHead::New();
std::string response_data;
brave_shields::MakeStubResponse(ctx_->mock_data_url, request_, &response,
Expand Down
2 changes: 1 addition & 1 deletion browser/net/brave_request_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ void BraveRequestHandler::RunNextCallback(
*ctx->new_url = GURL(ctx->new_url_spec);
}
if (ctx->blocked_by == brave::kAdBlocked) {
if (ctx->cancel_request_explicitly) {
if (!ctx->ShouldMockRequest()) {
RunCallbackForRequestIdentifier(ctx->request_identifier,
net::ERR_ABORTED);
return;
Expand Down
5 changes: 4 additions & 1 deletion browser/net/url_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,13 @@ struct BraveRequestInfo {
BraveNetworkDelegateEventType event_type = kUnknownEventType;
const base::ListValue* referral_headers_list = nullptr;
BlockedBy blocked_by = kNotBlocked;
bool cancel_request_explicitly = false;
std::string mock_data_url;
bool ipfs_local = true;

bool ShouldMockRequest() const {
return !mock_data_url.empty();
}

net::NetworkIsolationKey network_isolation_key = net::NetworkIsolationKey();

// Default to invalid type for resource_type, so delegate helpers
Expand Down
5 changes: 1 addition & 4 deletions components/brave_shields/browser/ad_block_base_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ bool AdBlockBaseService::ShouldStartRequest(
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url) {
DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence());

Expand All @@ -129,15 +128,13 @@ bool AdBlockBaseService::ShouldStartRequest(
url,
url::Origin::CreateFromNormalizedTuple("https", tab_host.c_str(), 80),
INCLUDE_PRIVATE_REGISTRIES);
// TODO(spinda): Remove explicit_cancel here when removed from adblock-rust.
bool explicit_cancel;
bool saved_from_exception;
if (ad_block_client_->matches(
url.spec(), url.host(), tab_host, is_third_party,
ResourceTypeToString(resource_type), &explicit_cancel,
&saved_from_exception, mock_data_url)) {
if (cancel_request_explicitly) {
*cancel_request_explicitly = explicit_cancel;
}
// We'd only possibly match an exception filter if we're returning true.
if (did_match_exception) {
*did_match_exception = false;
Expand Down
1 change: 0 additions & 1 deletion components/brave_shields/browser/ad_block_base_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ class AdBlockBaseService : public BaseBraveShieldsService {
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url) override;
void AddResources(const std::string& resources);
void EnableTag(const std::string& tag, bool enabled);
Expand Down
3 changes: 1 addition & 2 deletions components/brave_shields/browser/ad_block_regional_service_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,12 @@ bool AdBlockRegionalServiceManager::ShouldStartRequest(
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* matching_exception_filter,
bool* cancel_request_explicitly,
std::string* mock_data_url) {
base::AutoLock lock(regional_services_lock_);
for (const auto& regional_service : regional_services_) {
if (!regional_service.second->ShouldStartRequest(
url, resource_type, tab_host, matching_exception_filter,
cancel_request_explicitly, mock_data_url)) {
mock_data_url)) {
return false;
}
if (matching_exception_filter && *matching_exception_filter) {
Expand Down
1 change: 0 additions & 1 deletion components/brave_shields/browser/ad_block_regional_service_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ class AdBlockRegionalServiceManager {
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* matching_exception_filter,
bool* cancel_request_explicitly,
std::string* mock_data_url);
void EnableTag(const std::string& tag, bool enabled);
void AddResources(const std::string& resources);
Expand Down
10 changes: 3 additions & 7 deletions components/brave_shields/browser/ad_block_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,30 +77,26 @@ bool AdBlockService::ShouldStartRequest(
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url) {

if (!AdBlockBaseService::ShouldStartRequest(
url, resource_type, tab_host, did_match_exception,
cancel_request_explicitly, mock_data_url)) {
url, resource_type, tab_host, did_match_exception, mock_data_url)) {
return false;
}
if (did_match_exception && *did_match_exception) {
return true;
}

if (!regional_service_manager()->ShouldStartRequest(
url, resource_type, tab_host, did_match_exception,
cancel_request_explicitly, mock_data_url)) {
url, resource_type, tab_host, did_match_exception, mock_data_url)) {
return false;
}
if (did_match_exception && *did_match_exception) {
return true;
}

if (!custom_filters_service()->ShouldStartRequest(
url, resource_type, tab_host, did_match_exception,
cancel_request_explicitly, mock_data_url)) {
url, resource_type, tab_host, did_match_exception, mock_data_url)) {
return false;
}
if (did_match_exception && *did_match_exception) {
Expand Down
1 change: 0 additions & 1 deletion components/brave_shields/browser/ad_block_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class AdBlockService : public AdBlockBaseService {
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url) override;

AdBlockRegionalServiceManager* regional_service_manager();
Expand Down
2 changes: 0 additions & 2 deletions components/brave_shields/browser/base_brave_shields_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,10 @@ bool BaseBraveShieldsService::ShouldStartRequest(
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url) {
if (did_match_exception) {
*did_match_exception = false;
}
// Intentionally don't set cancel_request_explicitly
return true;
}

Expand Down
1 change: 0 additions & 1 deletion components/brave_shields/browser/base_brave_shields_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class BaseBraveShieldsService : public BraveComponent {
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception,
bool* cancel_request_explicitly,
std::string* mock_data_url);

protected:
Expand Down
3 changes: 1 addition & 2 deletions components/brave_shields/browser/tracking_protection_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,7 @@ bool TrackingProtectionService::ShouldStartRequest(
const GURL& url,
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* matching_exception_filter,
bool* cancel_request_explicitly) {
bool* matching_exception_filter) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
// There are no exceptions in the TP service, but exceptions are
// combined with brave/ad-block.
Expand Down
3 changes: 1 addition & 2 deletions components/brave_shields/browser/tracking_protection_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ class TrackingProtectionService : public LocalDataFilesObserver {
bool ShouldStartRequest(const GURL& spec,
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
bool* matching_exception_filter,
bool* cancel_request_explicitly);
bool* matching_exception_filter);

// implementation of LocalDataFilesObserver
void OnComponentReady(const std::string& component_id,
Expand Down