[DoH] Prevent DoH probes and canary domain check if pref disabled [chromium/src : main]
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
Hi bashi@, would you mind looking at this CL as well? Thank you!
Related details
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 3 comments![]( "Open in Gerrit")
`ResolveContext::doh_fallback_update_allowed()` returns true, we won'tfalse?
url = GURL(URLRequestMockDohJob::GetMockHttpsUrl("doh_test"));When `num_doh_servers` is 1, the template uses `doh_test_0` (line 714), but this `url` uses `doh_test`. If these produced different hostnames, the interceptor might not trigger. Since you fixed this logic in the `if (use_doh_fallback_upgrade)` branch (line 706), consider using similar logic here to keep it consistent and robust.
RunUntilIdle();Can we avoid using RunUntileIdle() in new tests? Is it difficult not using it? Ditto below usage.
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 3 comments![]( "Open in Gerrit")
`ResolveContext::doh_fallback_update_allowed()` returns true, we won'tAndrew Williamsfalse?
ah yes 😅 good catch! changed
url = GURL(URLRequestMockDohJob::GetMockHttpsUrl("doh_test"));When `num_doh_servers` is 1, the template uses `doh_test_0` (line 714), but this `url` uses `doh_test`. If these produced different hostnames, the interceptor might not trigger. Since you fixed this logic in the `if (use_doh_fallback_upgrade)` branch (line 706), consider using similar logic here to keep it consistent and robust.
ah good catch, I didn't notice this. I think this CL preserves the original functionality at least. Would you mind if I address changing this in a follow-up CL or do you think the change should be in this CL?
RunUntilIdle();Can we avoid using RunUntileIdle() in new tests? Is it difficult not using it? Ditto below usage.
The existing tests use `RunUntilIdle` extensively which is why I initially figured we could just have the new tests match the old ones, but in the latest patchset I've updated the new tests to avoid it.
For the cases where we actually have something to wait for, we use `base::test::RunUntil` now which is beneficial. For the cases where we are waiting just in case something happens but expect nothing to happen, the tests now use `FastForwardBy(runner->GetDelayUntilNextProbeForTest(0));` which if everything is working as expected is just `FastForwardBy(base::TimeDelta())`. I'm not sure if that's a great approach, since I'm wondering if it could theoretically have the same problems that as RunUntilIdle
FWIW these tests do pass the Flake Endorser check, though, so it doesn't seem like they will have issues in practice...
Related details
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
3 comments
url = GURL(URLRequestMockDohJob::GetMockHttpsUrl("doh_test"));Andrew WilliamsWhen `num_doh_servers` is 1, the template uses `doh_test_0` (line 714), but this `url` uses `doh_test`. If these produced different hostnames, the interceptor might not trigger. Since you fixed this logic in the `if (use_doh_fallback_upgrade)` branch (line 706), consider using similar logic here to keep it consistent and robust.
ah good catch, I didn't notice this. I think this CL preserves the original functionality at least. Would you mind if I address changing this in a follow-up CL or do you think the change should be in this CL?
Addressing in a follow-up CL is totally fine.
RunUntilIdle();Andrew WilliamsCan we avoid using RunUntileIdle() in new tests? Is it difficult not using it? Ditto below usage.
The existing tests use `RunUntilIdle` extensively which is why I initially figured we could just have the new tests match the old ones, but in the latest patchset I've updated the new tests to avoid it.
For the cases where we actually have something to wait for, we use `base::test::RunUntil` now which is beneficial. For the cases where we are waiting just in case something happens but expect nothing to happen, the tests now use `FastForwardBy(runner->GetDelayUntilNextProbeForTest(0));` which if everything is working as expected is just `FastForwardBy(base::TimeDelta())`. I'm not sure if that's a great approach, since I'm wondering if it could theoretically have the same problems that as RunUntilIdle
FWIW these tests do pass the Flake Endorser check, though, so it doesn't seem like they will have issues in practice...
Yeah, I agree with your points. I sometimes do `FastForwardBy(base::TimeDelta())`, which is effectively equivalent to `RunUntilIdle()`.
For now, I'm fine with either approach.
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[DoH] Prevent DoH probes and canary domain check if pref disabled
This CL makes it so that when
`ResolveContext::doh_fallback_update_allowed()` returns false, we won't
send DoH probes to the fallback DoH servers and we won't send the canary
domain check. This allows us to mitigate possible side effects for users
not eligible for the upcoming experiment to use a fallback DoH provider
for Enhanced Safe Browsing users.
Note that the current approach has the limitation that when
`doh_fallback_update_allowed()` transitions from false to true, we
won't trigger new DoH probes to be sent or new canary domain queries to
be sent. This is acceptable for the experiment since it's unlikely for
users to change their settings and the purpose of the experiment is only
to measure the performance impacts of this functionality across the
broader user population.
- M net/dns/canary_domain_service.cc
- M net/dns/canary_domain_service_unittest.cc
- M net/dns/dns_transaction.cc
- M net/dns/dns_transaction_unittest.cc
- M net/dns/resolve_context.cc
- M services/network/network_context_unittest.cc
Code-Review: +1 by Kenichi Ishibashi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |