Explicitly cancel blocked requests

Resolves brave/brave-browser#10063
brave · Sep 18, 2020 · 82df613 · 82df613
1 parent 445b56a
commit 82df613
Show file tree

Hide file tree

Showing 13 changed files with 24 additions and 55 deletions.
diff --git 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
@@ -31,21 +31,18 @@ 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)) {
+          &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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,