From 82df613c129712a49a2fd201139e0153f0f65f46 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 11 Sep 2020 22:37:02 +0000 Subject: [PATCH] Explicitly cancel blocked requests Resolves brave/brave-browser#10063 --- .../ad_block_service_browsertest.cc | 45 +++++-------------- ...ave_ad_block_tp_network_delegate_helper.cc | 5 +-- .../net/brave_proxying_url_loader_factory.cc | 3 +- browser/net/brave_request_handler.cc | 2 +- browser/net/url_context.h | 5 ++- .../browser/ad_block_base_service.cc | 5 +-- .../browser/ad_block_base_service.h | 1 - .../ad_block_regional_service_manager.cc | 3 +- .../ad_block_regional_service_manager.h | 1 - .../browser/base_brave_shields_service.cc | 2 - .../browser/base_brave_shields_service.h | 1 - .../browser/tracking_protection_service.cc | 3 +- .../browser/tracking_protection_service.h | 3 +- 13 files changed, 24 insertions(+), 55 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 37718bea6f932..d21c2ab31cecd 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -275,7 +275,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); @@ -319,7 +319,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); @@ -371,7 +371,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); @@ -432,7 +432,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); @@ -455,7 +455,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')", @@ -479,7 +479,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')", @@ -503,7 +503,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); @@ -514,7 +514,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); @@ -538,7 +538,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); @@ -620,7 +620,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)); @@ -641,7 +641,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)); @@ -668,7 +668,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)); @@ -757,27 +757,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( diff --git a/browser/net/brave_ad_block_tp_network_delegate_helper.cc b/browser/net/brave_ad_block_tp_network_delegate_helper.cc index 83423bf2077a6..444bf09d048a0 100644 --- a/browser/net/brave_ad_block_tp_network_delegate_helper.cc +++ b/browser/net/brave_ad_block_tp_network_delegate_helper.cc @@ -31,21 +31,18 @@ void ShouldBlockAdOnTaskRunner(std::shared_ptr 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)) { + &did_match_exception, &ctx->mock_data_url)) { ctx->blocked_by = kAdBlocked; } else if (!did_match_exception && !g_brave_browser_process->ad_block_regional_service_manager() ->ShouldStartRequest(ctx->request_url, ctx->resource_type, tab_host, &did_match_exception, - &ctx->cancel_request_explicitly, &ctx->mock_data_url)) { ctx->blocked_by = kAdBlocked; } else if (!did_match_exception && !g_brave_browser_process->ad_block_custom_filters_service() ->ShouldStartRequest(ctx->request_url, ctx->resource_type, tab_host, &did_match_exception, - &ctx->cancel_request_explicitly, &ctx->mock_data_url)) { ctx->blocked_by = kAdBlocked; } diff --git a/browser/net/brave_proxying_url_loader_factory.cc b/browser/net/brave_proxying_url_loader_factory.cc index aec1b40a7246d..0bc10a448e7ed 100644 --- a/browser/net/brave_proxying_url_loader_factory.cc +++ b/browser/net/brave_proxying_url_loader_factory.cc @@ -362,10 +362,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, diff --git a/browser/net/brave_request_handler.cc b/browser/net/brave_request_handler.cc index 79b950d1a9470..7eb2d92f3306e 100644 --- a/browser/net/brave_request_handler.cc +++ b/browser/net/brave_request_handler.cc @@ -295,7 +295,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; diff --git a/browser/net/url_context.h b/browser/net/url_context.h index 8fb4155445142..413dc70203859 100644 --- a/browser/net/url_context.h +++ b/browser/net/url_context.h @@ -89,10 +89,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(); + } + // Default to invalid type for resource_type, so delegate helpers // can properly detect that the info couldn't be obtained. // TODO(iefremov): Replace with something like |WebRequestResourceType| to diff --git a/components/brave_shields/browser/ad_block_base_service.cc b/components/brave_shields/browser/ad_block_base_service.cc index ff59558fd1237..86f8ec6526371 100644 --- a/components/brave_shields/browser/ad_block_base_service.cc +++ b/components/brave_shields/browser/ad_block_base_service.cc @@ -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()); @@ -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; diff --git a/components/brave_shields/browser/ad_block_base_service.h b/components/brave_shields/browser/ad_block_base_service.h index 3c4a57a50197b..bcb632726e3e8 100644 --- a/components/brave_shields/browser/ad_block_base_service.h +++ b/components/brave_shields/browser/ad_block_base_service.h @@ -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); diff --git a/components/brave_shields/browser/ad_block_regional_service_manager.cc b/components/brave_shields/browser/ad_block_regional_service_manager.cc index a9ad4e6de5472..d2bb45190f98f 100644 --- a/components/brave_shields/browser/ad_block_regional_service_manager.cc +++ b/components/brave_shields/browser/ad_block_regional_service_manager.cc @@ -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) { diff --git a/components/brave_shields/browser/ad_block_regional_service_manager.h b/components/brave_shields/browser/ad_block_regional_service_manager.h index b4ba4c6166169..d4ac68bc3278b 100644 --- a/components/brave_shields/browser/ad_block_regional_service_manager.h +++ b/components/brave_shields/browser/ad_block_regional_service_manager.h @@ -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); diff --git a/components/brave_shields/browser/base_brave_shields_service.cc b/components/brave_shields/browser/base_brave_shields_service.cc index b82a3695fcb7e..1b900a74951a2 100644 --- a/components/brave_shields/browser/base_brave_shields_service.cc +++ b/components/brave_shields/browser/base_brave_shields_service.cc @@ -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; } diff --git a/components/brave_shields/browser/base_brave_shields_service.h b/components/brave_shields/browser/base_brave_shields_service.h index ad6f9a44d6e3f..f87f0c19e76d5 100644 --- a/components/brave_shields/browser/base_brave_shields_service.h +++ b/components/brave_shields/browser/base_brave_shields_service.h @@ -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: diff --git a/components/brave_shields/browser/tracking_protection_service.cc b/components/brave_shields/browser/tracking_protection_service.cc index 59a640ad7376a..6ff5633d3ff73 100644 --- a/components/brave_shields/browser/tracking_protection_service.cc +++ b/components/brave_shields/browser/tracking_protection_service.cc @@ -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. diff --git a/components/brave_shields/browser/tracking_protection_service.h b/components/brave_shields/browser/tracking_protection_service.h index 9d505dd2631b7..27d85d0414b3e 100644 --- a/components/brave_shields/browser/tracking_protection_service.h +++ b/components/brave_shields/browser/tracking_protection_service.h @@ -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,