Support CNAME adblocking by antonok-edm · Pull Request #6640 · brave/brave-core

antonok-edm · 2020-09-14T20:32:59Z

Submitter Checklist:

Test Plan:

The actual test plan

When visiting https://mathon.fr, there should be a request to a URL of a similar format[1] to https://xxxx.mathon.fr/xxxxxxxxxxx.js that is blocked and displayed under the Shields panel advanced view when expanding the "Ads and trackers blocked" dropdown counter. Further, there should not be a request to https://static.criteo.net/js/ld/ld.js in the list (or listed anywhere in the Dev Tools network tab).

here, x's are used to symbolize arbitrary alphanumeric characters

Some optional background reading :)

The website https://mathon.fr pulls a CNAME-cloaked script that triggers a few extra network requests. The subdomain and path may change arbitrarily, but during my testing, a script was requested from https://16ao.mathon.fr/frWWWMA6548.js. Using the dig commandline tool, we can find the full DNS records for 16ao.mathon.fr:

> dig 16ao.mathon.fr

; <<>> DiG 9.16.6 <<>> 16ao.mathon.fr
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 20370
;; flags: qr rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;16ao.mathon.fr.			IN	A

;; ANSWER SECTION:
16ao.mathon.fr.		147	IN	CNAME	mathon.eulerian.net.
mathon.eulerian.net.	7047	IN	CNAME	et5.eulerian.net.
et5.eulerian.net.	7047	IN	A	109.232.193.177              <- Canonical name

;; Query time: 0 msec
;; SERVER: ::1#53(::1)
;; WHEN: Thu Sep 17 16:09:00 EDT 2020
;; MSG SIZE  rcvd: 122

The canonical name for 16ao.mathon.fr is et5.eulerian.net, which is blocked by uBlock Origin's default lists. This prevents several other network requests from starting. As a result, the Shields panel actually shows fewer blocked requests (4 instead of 7 in my configuration).

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

antonok-edm · 2020-09-18T00:55:56Z

@iefremov This should be all set for review

browser/net/brave_ad_block_tp_network_delegate_helper.cc

bridiver · 2020-09-21T15:44:33Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+                      std::shared_ptr<BraveRequestInfo> ctx,
+                      const base::Optional<std::string> cname) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  scoped_refptr<base::SequencedTaskRunner> task_runner =


This isn't safe because AdblockCnameResolveHostClient is not owned by anything and so it could outlive anything including the ad_block_service

you could pass the task runner itself into the AdblockCnameResolveHostClient constructor since it's ref counted

bridiver · 2020-09-21T15:48:55Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

-                                        ctx));
+  auto* rph = content::RenderProcessHost::FromID(ctx->render_process_id);
+  if (!rph) {
+    // This case occurs for the top-level request. It's okay to ignore the


top-level requests don't have a render process host? That doesn't sound right and also if top-level requests can't be blocked, why are we calling ShouldBlockAdOnTaskRunner?

cc @iefremov

ctx->render_process_id comes up as -1 for top level requests from my testing, which causes the RenderProcessHost to be a nullptr.

resolved by getting required fields through GetWebContents

bridiver · 2020-09-21T16:04:47Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+                ctx, base::Optional<std::string>()),
+            base::BindOnce(&OnShouldBlockAdResult, next_callback, ctx));
+  } else {
+     new AdblockCnameResolveHostClient(next_callback, ctx);


rather than passing the next_callback, I would pass https://github.com/brave/brave-core/pull/6640/files#diff-5a17ec06c6e281e744cfa03e9d7561a6R164 so you don't have to duplicate it here https://github.com/brave/brave-core/pull/6640/files#diff-5a17ec06c6e281e744cfa03e9d7561a6R77

I would also partially bind https://github.com/brave/brave-core/pull/6640/files#diff-5a17ec06c6e281e744cfa03e9d7561a6R162 base::BindOnce(&ShouldBlockAdOnTaskRunner, ctx) and pass that as well so AdblockCnameResolveHostClient doesn't have to stay in sync with any changes here

We ended up keeping the logic only in OnGetCnameResult, renaming that to ShouldBlockAdWithOptionalCname, and calling it directly with the empty Optional if there's no render process host.

bridiver · 2020-09-21T16:08:25Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

@@ -56,16 +60,88 @@ void OnShouldBlockAdResult(const ResponseCallback& next_callback,
  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);


we should really be checking g_brave_browser_process->IsShuttingDown() here. This isn't new with your changes, but I think it's safer for the existing code. wdyt @iefremov ? I think next_callback.run could potentially crash here otherwise?

I think we are fine, until we hold any potentially dangling references - TaskRunners clean up their task queues on destruction. next_callback holds weak pointer to `BraveRequestHandler, so this is also safe.

iefremov · 2020-09-24T13:31:14Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+      base::BindOnce(&ShouldBlockAdWithOptionalCname, task_runner,
+                     next_callback, ctx);
+
+  new AdblockCnameResolveHostClient(std::move(cname_callback), ctx);


do you have any ideas or measurements about performance implications of this?

Not really, I don't had a good sense of how that can be done in this instance. I did look into writing some browsertests, but I couldn't see how to mock the DNS responses.

Happy to hear suggestions on this if you have any - but from manual testing, I couldn't notice any slowdown at all.

We can start with measuring the delay manually (check the time before ResolveHost and then in OnResult) and reporting it to UMA_HISTOGRAM_TIMES. Then after some browsing this value can be checked on brave://histograms. Would be nice to test an android apk.

as for browser tests, I see some examples using FakeHostResolver - did you check that?

hmm, I might need some more detailed instructions, this is all new to me.

Should histogram measurement code be left in and merged? Or just temporarily added for manual testing? Any good examples of where we've done this in the past?

As for browser tests, I'm not sure of the best way to force shields to use the fake resolver. It looks like chrome/browser/net/dns_probe_browsertest.cc uses this to set up a DnsProbeService with a fake resolver, and I can't tell what the equivalent approach would look like for this PR.

void DnsProbeBrowserTest::SetActiveBrowser(Browser* browser) { delaying_dns_probe_service_ = static_cast<DelayingDnsProbeService*>( DnsProbeServiceFactory::GetInstance()->SetTestingFactoryAndUse( browser->profile(), base::BindRepeating( &DelayingDnsProbeService::Create, base::BindRepeating(&DnsProbeBrowserTest::GetNetworkContext, base::Unretained(this)), base::BindRepeating( &DnsProbeBrowserTest::GetDnsConfigChangeManager, base::Unretained(this))))); // If currently watching a NetErrorTabHelper, stop doing so before start // watching another. if (monitored_tab_helper_) { monitored_tab_helper_->set_dns_probe_status_snoop_callback_for_testing( NetErrorTabHelper::DnsProbeStatusSnoopCallback()); } active_browser_ = browser; monitored_tab_helper_ = NetErrorTabHelper::FromWebContents( active_browser_->tab_strip_model()->GetActiveWebContents()); monitored_tab_helper_->set_dns_probe_status_snoop_callback_for_testing( BindRepeating(&DnsProbeBrowserTest::OnDnsProbeStatusSent, Unretained(this))); }

Should histogram measurement code be left in and merged?

yes, it can be useful in the future. Basically the whole Chromium codebase is full of histograms.
I've added some related ones like Brave.ProxyingURLLoader.TotalRequestTime or Brave.OnBeforeURLRequest_Handler

iefremov · 2020-09-24T13:34:03Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+
+  // Should not be called
+  void OnTextResults(const std::vector<std::string>& text_results) override {
+    DCHECK(false);


this is NOTREACHED()

iefremov · 2020-09-24T13:34:45Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+      std::move(cb_).Run(
+          base::Optional<std::string>(resolved_addresses->canonical_name()));
+    } else {
+      std::move(cb_).Run(base::Optional<std::string>());


base::nullopt or maybe just {} ?

cool, didn't know about these

iefremov · 2020-09-24T13:48:24Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+      return nullptr;
+    }
+    web_contents =
+        content::WebContents::FromRenderFrameHost(rfh);


can fit to the previous line?

iefremov · 2020-09-24T13:52:20Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+      ctx->request_url.host() != *canonical_name && *canonical_name != "") {
+    GURL::Replacements replacements = GURL::Replacements();
+    replacements.SetHost(canonical_name->c_str(),
+        url::Component(0, static_cast<int>(canonical_name->length())));


clang-format

iefremov · 2020-09-24T14:02:25Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+    auto* web_contents = GetWebContents(ctx->render_process_id,
+                                        ctx->render_frame_id,
+                                        ctx->frame_tree_node_id);
+    DCHECK(web_contents);


not sure this will work for service workers

@iefremov how might I fix that?

just replace with if

you're correct, I noticed that Twitch has this case occasionally. Should be fixed now.

iefremov · 2020-09-24T14:05:24Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+    DCHECK(web_contents);
+
+    content::BrowserContext* context = web_contents->GetBrowserContext();
+    DCHECK(context);


doesn't look necessary

iefremov · 2020-09-24T14:29:19Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

-    const ResponseCallback& next_callback,
-    std::shared_ptr<BraveRequestInfo> ctx) {
+  base::OnceCallback<void(base::Optional<std::string>)> cname_callback =
+      base::BindOnce(&ShouldBlockAdWithOptionalCname, task_runner,


I don't understand why do we need this callback. Can we just call ShouldBlockAdWithOptionalCname in OnComplete?
task_runner will be alive given that it is owned by ad_block_service which in turn is owned by g_browser_process.

Or we can bind an anonymous lambda here that would take get task_runner and post

Good call, we needed this previously in the (now removed) failure case when we couldn't create the resolver

I moved it into the AdblockCnameResolveHostClient constructor since you discovered we need to bind the ResponseCallback anyways.

bridiver

lgtm, but please handle @iefremov's concerns before merging

iefremov · 2020-09-29T04:49:36Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

 #include "ui/base/resource/resource_bundle.h"
+#include "url/url_canon.h"
+
+namespace {


typically anonymous namespace should be placed inside main namespace (i.e. brave)

iefremov · 2020-09-29T04:54:19Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+      const base::Optional<net::AddressList>& resolved_addresses) override {
+    if (result == net::OK && resolved_addresses) {
+      DCHECK(resolved_addresses.has_value() && !resolved_addresses->empty());
+      std::move(cb_).Run(


I think we can replace this with a direct call

task_runner->PostTaskAndReply( FROM_HERE, base::BindOnce(&ShouldBlockAdOnTaskRunner, ctx, cname), base::BindOnce(&OnShouldBlockAdResult, next_callback, ctx));

This way, you don't need to bind and store cb_, and ShouldBlockAdWithOptionalCname can also be eliminated. task_runner() can also be obtained right here. So you would need to only save next_callback in constructor

@bridiver said he wanted the task runner passed in, perhaps the two of you should discuss?

antonok-edm · 2020-09-30T06:44:12Z

@iefremov I did some browsing for ~30 minutes with the latest changes, compiled in Static. Some video streaming, some browsing both comments and posts on HN.

Histogram: Brave.ShieldsCNAMEBlocking.TotalResolutionTime recorded 5372 samples, mean = 2.1 (flags = 0x41)
0    ------------------------------------------------------------------------O (4839 = 90.1%)
1    ---O                                                                      (169 = 3.1%) {90.1%}
2    -O                                                                        (48 = 0.9%) {93.2%}
3    O                                                                         (17 = 0.3%) {94.1%}
4    O                                                                         (7 = 0.1%) {94.4%}
5    O                                                                         (11 = 0.2%) {94.6%}
6    O                                                                         (5 = 0.1%) {94.8%}
7    O                                                                         (5 = 0.1%) {94.9%}
8    O                                                                         (11 = 0.2%) {95.0%}
10   O                                                                         (15 = 0.3%) {95.2%}
12   O                                                                         (6 = 0.1%) {95.4%}
14   O                                                                         (22 = 0.4%) {95.6%}
17   O                                                                         (42 = 0.8%) {96.0%}
20   O                                                                         (16 = 0.3%) {96.7%}
24   O                                                                         (29 = 0.5%) {97.0%}
29   O                                                                         (16 = 0.3%) {97.6%}
34   O                                                                         (23 = 0.4%) {97.9%}
40   O                                                                         (44 = 0.8%) {98.3%}
48   O                                                                         (11 = 0.2%) {99.1%}
57   O                                                                         (8 = 0.1%) {99.3%}
68   O                                                                         (9 = 0.2%) {99.5%}
81   O                                                                         (4 = 0.1%) {99.6%}
96   O                                                                         (4 = 0.1%) {99.7%}
114  O                                                                         (3 = 0.1%) {99.8%}
135  O                                                                         (2 = 0.0%) {99.9%}
160  O                                                                         (2 = 0.0%) {99.9%}
190  O                                                                         (1 = 0.0%) {99.9%}
226  O                                                                         (0 = 0.0%) {99.9%}
268  O                                                                         (2 = 0.0%) {99.9%}
318  ... 
533  O                                                                         (1 = 0.0%) {100.0%}
633  ...

antonok-edm · 2020-10-02T21:23:53Z

Took some histogram measurements of Brave.ProxyingURLLoader.TotalRequestTime.

With CNAME adblocking

Histogram: Brave.ProxyingURLLoader.TotalRequestTime recorded 9431 samples, mean = 310.1 (flags = 0x41)
0      ---------------O                                                          (109 = 1.2%)
1      ------------------------------------------------------------------------O (530 = 5.6%) {1.2%}
2      ------------------------------------O                                     (264 = 2.8%) {6.8%}
3      ---------------------------O                                              (197 = 2.1%) {9.6%}
4      ------------------O                                                       (130 = 1.4%) {11.7%}
5      ---------O                                                                (67 = 0.7%) {13.0%}
6      -------O                                                                  (50 = 0.5%) {13.8%}
7      -------O                                                                  (55 = 0.6%) {14.3%}
8      ------O                                                                   (94 = 1.0%) {14.9%}
10     ------O                                                                   (93 = 1.0%) {15.9%}
12     -------O                                                                  (97 = 1.0%) {16.8%}
14     -------------O                                                            (298 = 3.2%) {17.9%}
17     -------------O                                                            (297 = 3.1%) {21.0%}
20     -----------------------O                                                  (685 = 7.3%) {24.2%}
24     --------------------------------O                                         (1180 = 12.5%) {31.4%}
29     ----------------O                                                         (575 = 6.1%) {44.0%}
34     ---------------O                                                          (545 = 5.8%) {50.1%}
40     ------------------------O                                                 (889 = 9.4%) {55.8%}
48     -------------O                                                            (495 = 5.2%) {65.3%}
57     ------O                                                                   (228 = 2.4%) {70.5%}
68     ----O                                                                     (163 = 1.7%) {72.9%}
81     ----O                                                                     (156 = 1.7%) {74.7%}
96     -----O                                                                    (182 = 1.9%) {76.3%}
114    ---O                                                                      (105 = 1.1%) {78.2%}
135    ---O                                                                      (114 = 1.2%) {79.4%}
160    -----O                                                                    (191 = 2.0%) {80.6%}
190    ----O                                                                     (135 = 1.4%) {82.6%}
226    --O                                                                       (68 = 0.7%) {84.0%}
268    -O                                                                        (33 = 0.3%) {84.7%}
318    -O                                                                        (26 = 0.3%) {85.1%}
378    -O                                                                        (36 = 0.4%) {85.4%}
449    O                                                                         (11 = 0.1%) {85.7%}
533    O                                                                         (16 = 0.2%) {85.9%}
633    -O                                                                        (27 = 0.3%) {86.0%}
752    O                                                                         (15 = 0.2%) {86.3%}
894    O                                                                         (11 = 0.1%) {86.5%}
1062   O                                                                         (10 = 0.1%) {86.6%}
1262   O                                                                         (16 = 0.2%) {86.7%}
1500   O                                                                         (12 = 0.1%) {86.9%}
1782   --------------------------------O                                         (1191 = 12.6%) {87.0%}
2117   O                                                                         (11 = 0.1%) {99.6%}
2516   -O                                                                        (19 = 0.2%) {99.7%}
2990   O                                                                         (2 = 0.0%) {99.9%}
3553   O                                                                         (1 = 0.0%) {100.0%}
4222   O                                                                         (1 = 0.0%) {100.0%}
5017   ... 
10000  O                                                                         (1 = 0.0%) {100.0%}

Without CNAME adblocking

Sample size here is ~36x larger.

Histogram: Brave.ProxyingURLLoader.TotalRequestTime recorded 339738 samples, mean = 1091.0 (flags = 0x41)
0      ---------------------------------------------------O                      (15832 = 4.7%)
1      ---------------------------------------O                                  (12088 = 3.6%) {4.7%}
2      ----------------------------------O                                       (10674 = 3.1%) {8.2%}
3      ---------------------------------------------O                            (14026 = 4.1%) {11.4%}
4      ------------------------------------------------------------------------O (22572 = 6.6%) {15.5%}
5      ---------------------------------------------------------------------O    (21587 = 6.4%) {22.1%}
6      ------------------------------------------O                               (13201 = 3.9%) {28.5%}
7      -------------------O                                                      (5908 = 1.7%) {32.4%}
8      ---------O                                                                (5357 = 1.6%) {34.1%}
10     -----O                                                                    (3406 = 1.0%) {35.7%}
12     -----O                                                                    (3315 = 1.0%) {36.7%}
14     ---------O                                                                (8229 = 2.4%) {37.7%}
17     -----------O                                                              (10290 = 3.0%) {40.1%}
20     -----------O                                                              (14216 = 4.2%) {43.1%}
24     --------O                                                                 (12650 = 3.7%) {47.3%}
29     --------O                                                                 (12119 = 3.6%) {51.0%}
34     ------O                                                                   (9319 = 2.7%) {54.6%}
40     -------O                                                                  (10444 = 3.1%) {57.3%}
48     ------O                                                                   (9139 = 2.7%) {60.4%}
57     -----O                                                                    (8289 = 2.4%) {63.1%}
68     -----O                                                                    (7097 = 2.1%) {65.5%}
81     ----O                                                                     (6246 = 1.8%) {67.6%}
96     -------O                                                                  (11424 = 3.4%) {69.5%}
114    -----O                                                                    (8318 = 2.4%) {72.8%}
135    ------O                                                                   (8907 = 2.6%) {75.3%}
160    ------O                                                                   (8965 = 2.6%) {77.9%}
190    ------O                                                                   (9077 = 2.7%) {80.5%}
226    ----O                                                                     (6804 = 2.0%) {83.2%}
268    ---O                                                                      (5010 = 1.5%) {85.2%}
318    --O                                                                       (3030 = 0.9%) {86.7%}
378    --O                                                                       (2626 = 0.8%) {87.6%}
449    --O                                                                       (2367 = 0.7%) {88.4%}
533    -O                                                                        (1419 = 0.4%) {89.0%}
633    -O                                                                        (959 = 0.3%) {89.5%}
752    -O                                                                        (905 = 0.3%) {89.7%}
894    -O                                                                        (1059 = 0.3%) {90.0%}
1062   -O                                                                        (1415 = 0.4%) {90.3%}
1262   -O                                                                        (1248 = 0.4%) {90.7%}
1500   -O                                                                        (1242 = 0.4%) {91.1%}
1782   ------O                                                                   (9284 = 2.7%) {91.5%}
2117   -O                                                                        (1079 = 0.3%) {94.2%}
2516   -O                                                                        (924 = 0.3%) {94.5%}
2990   O                                                                         (707 = 0.2%) {94.8%}
3553   O                                                                         (644 = 0.2%) {95.0%}
4222   O                                                                         (647 = 0.2%) {95.2%}
5017   O                                                                         (606 = 0.2%) {95.4%}
5961   O                                                                         (539 = 0.2%) {95.6%}
7083   O                                                                         (554 = 0.2%) {95.7%}
8416   O                                                                         (561 = 0.2%) {95.9%}
10000  ---------O                                                                (13414 = 3.9%) {96.1%}

Thoughts

With CNAME adblocking, the distribution is definitely skewed downwards and also noticeably bi-modal. Completion under 5 milliseconds(?) regresses from 22.1% to 13% of requests, but completion under 40 milliseconds(?) is roughly equal on both.

Given the previous measurements for Brave.ShieldsCNAMEBlocking.TotalResolutionTime, it's likely that the differences are accounted for outside of the DNS resolution itself - either by allocation of the AdblockCnameResolveHostClient, or by the increased workload for adblock-rust in cases where the canonical name must be checked.

Probably not related to this PR, but I am intrigued by the narrow peak at exactly 1782 - it's significant on the first histogram, but it's not negligible on the second histogram either.

antonok-edm · 2020-10-13T04:48:25Z

At @pes10k's suggestion, I've redone the performance measurements here using Puppeteer for a more fair comparison. These are both across the top 920 Alexa domains.

I used https://github.com/brave/format-chromium-histogram to generate these histograms using raw data from the Chrome DevTools Protocol.

With CNAME adblocking

Histogram: Brave.ProxyingURLLoader.TotalRequestTime recorded 122630 samples, mean = 344.4

0      -------O                                                                   (141 = 0.1%)
1      --------------------------------------------O                              (945 = 0.8%) {0.1%}
2      -------------------------------------------------O                         (1061 = 0.9%) {0.9%}
3      -------------------------------------------------------------------------O (1582 = 1.3%) {1.8%}
4      ----------------------------------------------------------------O          (1397 = 1.1%) {3.0%}
5      ------------------------------------------------O                          (1041 = 0.8%) {4.2%}
6      ------------------------------------O                                      (783 = 0.6%) {5.0%}
7      ---------------------------------O                                         (721 = 0.6%) {5.7%}
8      --------------------------O                                                (1119 = 0.9%) {6.3%}
10     ----------------------O                                                    (966 = 0.8%) {7.2%}
12     --------------------O                                                      (877 = 0.7%) {8.0%}
14     -------------------------O                                                 (1636 = 1.3%) {8.7%}
17     -------------------------------O                                           (1978 = 1.6%) {10.0%}
20     -----------------------------------O                                       (3067 = 2.5%) {11.6%}
24     --------------------------------------O                                    (4160 = 3.4%) {14.1%}
29     --------------------------------------O                                    (4091 = 3.3%) {17.5%}
34     -----------------------------------------------O                           (5105 = 4.2%) {20.8%}
40     ----------------------------------------------------------O                (6285 = 5.1%) {25.0%}
48     -------------------------------------------------------O                   (6041 = 4.9%) {30.1%}
57     ------------------------------------------------------------O              (6490 = 5.3%) {35.1%}
68     ------------------------------------------------------------O              (6505 = 5.3%) {40.4%}
81     ---------------------------------------------------------------------O     (7550 = 6.2%) {45.7%}
96     --------------------------------------------------------------------O      (7417 = 6.0%) {51.8%}
114    ---------------------------------------------------------O                 (6258 = 5.1%) {57.9%}
135    ---------------------------------------------------O                       (5531 = 4.5%) {63.0%}
160    ------------------------------------------O                                (4545 = 3.7%) {67.5%}
190    -----------------------------------O                                       (3790 = 3.1%) {71.2%}
226    ------------------------------------O                                      (3940 = 3.2%) {74.3%}
268    ---------------------------------O                                         (3567 = 2.9%) {77.5%}
318    ------------------------O                                                  (2568 = 2.1%) {80.4%}
378    ---------------------------O                                               (2857 = 2.3%) {82.5%}
449    -----------------------O                                                   (2498 = 2.0%) {84.8%}
533    ------------------O                                                        (1932 = 1.6%) {86.9%}
633    ---------------------O                                                     (2219 = 1.8%) {88.4%}
752    --------------------O                                                      (2098 = 1.7%) {90.2%}
894    -----------------O                                                         (1853 = 1.5%) {92.0%}
1062   --------------O                                                            (1526 = 1.2%) {93.5%}
1262   ------------O                                                              (1222 = 1.0%) {94.7%}
1500   ----------O                                                                (1002 = 0.8%) {95.7%}
1782   -------O                                                                   (734 = 0.6%) {96.5%}
2117   -------O                                                                   (733 = 0.6%) {97.1%}
2516   -------O                                                                   (720 = 0.6%) {97.7%}
2990   -----O                                                                     (546 = 0.4%) {98.3%}
3553   ----O                                                                      (404 = 0.3%) {98.7%}
4222   ---O                                                                       (286 = 0.2%) {99.1%}
5017   ---O                                                                       (281 = 0.2%) {99.3%}
5961   ---O                                                                       (284 = 0.2%) {99.5%}
7083   --O                                                                        (127 = 0.1%) {99.8%}
8416   -O                                                                         (82 = 0.1%) {99.9%}
10000  -O                                                                         (69 = 0.1%) {99.9%}

Without CNAME adblocking

Histogram: Brave.ProxyingURLLoader.TotalRequestTime recorded 121122 samples, mean = 365.0

0      -------O                                                                   (156 = 0.1%)
1      ---------------------------------------O                                   (917 = 0.8%) {0.1%}
2      -----------------------------------------------------O                     (1235 = 1.0%) {0.9%}
3      -------------------------------------------------------------------------O (1698 = 1.4%) {1.9%}
4      -------------------------------------------------------------O             (1426 = 1.2%) {3.3%}
5      ---------------------------------------------O                             (1045 = 0.9%) {4.5%}
6      ---------------------------------O                                         (755 = 0.6%) {5.3%}
7      -----------------------------O                                             (666 = 0.5%) {6.0%}
8      -----------------------O                                                   (1052 = 0.9%) {6.5%}
10     --------------------O                                                      (917 = 0.8%) {7.4%}
12     ------------------O                                                        (819 = 0.7%) {8.1%}
14     -----------------------O                                                   (1588 = 1.3%) {8.8%}
17     ---------------------------O                                               (1883 = 1.6%) {10.1%}
20     --------------------------------O                                          (2996 = 2.5%) {11.7%}
24     ----------------------------------O                                        (3980 = 3.3%) {14.2%}
29     -----------------------------------O                                       (4061 = 3.4%) {17.4%}
34     ------------------------------------------O                                (4942 = 4.1%) {20.8%}
40     -----------------------------------------------------O                     (6214 = 5.1%) {24.9%}
48     --------------------------------------------------O                        (5866 = 4.8%) {30.0%}
57     -----------------------------------------------------O                     (6152 = 5.1%) {34.9%}
68     -----------------------------------------------------O                     (6240 = 5.2%) {39.9%}
81     ---------------------------------------------------------------O           (7360 = 6.1%) {45.1%}
96     --------------------------------------------------------------O            (7215 = 6.0%) {51.2%}
114    -------------------------------------------------------O                   (6419 = 5.3%) {57.1%}
135    ------------------------------------------------O                          (5605 = 4.6%) {62.4%}
160    -------------------------------------O                                     (4327 = 3.6%) {67.0%}
190    -------------------------------O                                           (3593 = 3.0%) {70.6%}
226    -------------------------------O                                           (3630 = 3.0%) {73.6%}
268    -------------------------------O                                           (3594 = 3.0%) {76.6%}
318    ------------------------O                                                  (2716 = 2.2%) {79.5%}
378    ------------------------O                                                  (2779 = 2.3%) {81.8%}
449    ------------------------O                                                  (2773 = 2.3%) {84.1%}
533    -------------------O                                                       (2125 = 1.8%) {86.4%}
633    ------------------O                                                        (2050 = 1.7%) {88.1%}
752    ------------------O                                                        (2056 = 1.7%) {89.8%}
894    ----------------O                                                          (1803 = 1.5%) {91.5%}
1062   --------------O                                                            (1611 = 1.3%) {93.0%}
1262   -----------O                                                               (1230 = 1.0%) {94.3%}
1500   ---------O                                                                 (1010 = 0.8%) {95.4%}
1782   --------O                                                                  (890 = 0.7%) {96.2%}
2117   -------O                                                                   (723 = 0.6%) {96.9%}
2516   -----O                                                                     (582 = 0.5%) {97.5%}
2990   -----O                                                                     (538 = 0.4%) {98.0%}
3553   -----O                                                                     (495 = 0.4%) {98.4%}
4222   ---O                                                                       (341 = 0.3%) {98.9%}
5017   ---O                                                                       (291 = 0.2%) {99.1%}
5961   ---O                                                                       (277 = 0.2%) {99.4%}
7083   --O                                                                        (180 = 0.1%) {99.6%}
8416   --O                                                                        (133 = 0.1%) {99.8%}
10000  --O                                                                        (168 = 0.1%) {99.9%}

pes10k · 2020-10-13T14:15:51Z

@iefremov just some background; after seeing the initial numbers above, I thought it might be a sampling issue, and that in a heavily cached based system like DNS, the more queries you have, the more you drive the mean down to the min.

Anywhoo, just wanted to check in. Does it look like this is clear to merge then? :)

iefremov · 2020-10-13T16:46:39Z

yeah, well, then we are probably safe to merge. Though this looks like a good candidate for a/b test (which we don't do yet :))

antonok-edm self-assigned this Sep 14, 2020

iefremov self-requested a review September 15, 2020 05:23

antonok-edm force-pushed the cname-adblocking branch 2 times, most recently from b419aea to 142ce48 Compare September 17, 2020 19:37

antonok-edm marked this pull request as ready for review September 17, 2020 20:45

antonok-edm force-pushed the cname-adblocking branch from 1de933d to 418ecff Compare September 17, 2020 20:56

antonok-edm commented Sep 18, 2020

View reviewed changes

browser/net/brave_ad_block_tp_network_delegate_helper.cc Show resolved Hide resolved

antonok-edm requested a review from bridiver September 18, 2020 00:57

bridiver reviewed Sep 21, 2020

View reviewed changes

antonok-edm force-pushed the cname-adblocking branch 3 times, most recently from 50e7911 to eec6dc1 Compare September 21, 2020 19:36

iefremov reviewed Sep 24, 2020

View reviewed changes

antonok-edm force-pushed the cname-adblocking branch from 4fe6756 to 0d4219c Compare September 24, 2020 16:33

bridiver approved these changes Sep 24, 2020

View reviewed changes

iefremov reviewed Sep 29, 2020

View reviewed changes

antonok-edm force-pushed the cname-adblocking branch from 0d4219c to 51d38eb Compare September 30, 2020 06:43

antonok-edm force-pushed the cname-adblocking branch from 51d38eb to c65adfb Compare October 2, 2020 18:16

antonok-edm and others added 9 commits October 13, 2020 12:53

WIP CNAME adblocking

fad7a8e

Quick workaround for callback problems.

b7064ca

check the canonical URL through adblock-rust

192259f

move ShouldStartRequest methods into the main adblock engine

aca0e6f

deduplicate ShouldBlockAd logic

0f71bab

pass task runner to ShouldBlockAdWithOptionalCname

31b2c4b

run cname check even on top-level requests

99d411f

address @iefremov review comments

3b896ce

add histogram entry

89a4f48

antonok-edm force-pushed the cname-adblocking branch from c65adfb to 89a4f48 Compare October 13, 2020 16:53

antonok-edm requested a review from iefremov October 13, 2020 19:00

iefremov approved these changes Oct 14, 2020

View reviewed changes

antonok-edm merged commit faa3bec into master Oct 14, 2020

antonok-edm deleted the cname-adblocking branch October 14, 2020 14:27

antonok-edm added this to the 1.17.x - Nightly milestone Oct 14, 2020

antonok-edm mentioned this pull request Oct 14, 2020

Patch chrome.dns.resolve to support canonical_name flag brave/brave-browser#7117

Open

LaurenWags mentioned this pull request Nov 9, 2020

Support CNAME adblocking brave/brave-browser#11712

Closed

Dev-i-l mentioned this pull request Dec 3, 2020

Cname cloaking blocker bromite/bromite#833

Closed

		@@ -56,16 +60,88 @@ void OnShouldBlockAdResult(const ResponseCallback& next_callback,
		DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

Conversation

antonok-edm commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist:

Test Plan:

The actual test plan

Some optional background reading :)

Reviewer Checklist:

After-merge Checklist:

Uh oh!

antonok-edm commented Sep 18, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iefremov Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonok-edm Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iefremov Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridiver left a comment

Choose a reason for hiding this comment

Uh oh!

antonok-edm commented Sep 14, 2020 •

edited

Loading

iefremov Sep 24, 2020 •

edited

Loading

antonok-edm Sep 29, 2020 •

edited

Loading

iefremov Sep 29, 2020 •

edited

Loading

antonok-edm commented Oct 13, 2020 •

edited

Loading