Implement HostnameResolver for the Suggest redirect feature - #9547
Conversation
Depending on the device's API version it'll determine which DnsLookup implementation to instantiate
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f696d2f9f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| override suspend fun lookup(network: Network, hostname: String): LookupResult { | ||
| return try { | ||
| val addresses = withContext(dispatcherProvider.io()) { | ||
| runInterruptible { network.getAllByName(hostname) } |
There was a problem hiding this comment.
On API 28, Network.getAllByName is a synchronous DNS call that is not guaranteed to stop when its worker thread is interrupted. runInterruptible only translates coroutine cancellation into a thread interrupt, and the surrounding withContext still waits for the call to finish, so a stalled DNS lookup can keep resolves() suspended well beyond its advertised two-second timeout. Run this lookup through a mechanism that lets the caller return at the deadline even if the underlying worker cannot be canceled.
Useful? React with 👍 / 👎.
| @Provides | ||
| fun provideHostnameResolver( | ||
| appBuildConfig: AppBuildConfig, | ||
| connectivityManager: ConnectivityManager, |
There was a problem hiding this comment.
q: the only ConnectivityManager provider in the graph is VpnAppModule.providesConnectivityManager in :vpn-impl, so browser code now leans on a binding the AppTP module owns. works today since :app depends on vpn-impl, but should we provide it here instead?
There was a problem hiding this comment.
That was my question as well :)
ConnectivityManager cannot be provided by both modules (BrowserModule and VpnAppModule). Dagger will detect the duplicate provision and fail the build. My question was what do you generally do in these cases in this codebase? A reasonable approach would be to find a common place to put it where both modules can access it.
A good candidate for this seems to be SystemComponentsModule. It also exposes a similar system service LocationManager. If you agree, I’ll move the ConnectivityManager provide method from VpnAppModule to SystemComponentsModule. I’ll reach out to the maintainers of that feature on how to verify the change, just in case.
| ): HostnameResolver { | ||
| val lookupTimeout = HostnameResolver.LOOKUP_TIMEOUT_MS.milliseconds | ||
| val dnsLookup = if (appBuildConfig.sdkInt >= 29) { | ||
| @Suppress("NewApi") |
There was a problem hiding this comment.
nit: the other five @Suppress("NewApi") in :app carry // we use appBuildConfig so the next reader knows why lint can't see the gate. worth adding the same here.
| import com.duckduckgo.app.browser.suggestredirect.DnsLookup.LookupResult.Failure | ||
| import com.duckduckgo.app.browser.suggestredirect.DnsLookup.LookupResult.Success | ||
| import com.duckduckgo.common.utils.DispatcherProvider | ||
| import com.wireguard.config.InetAddresses |
There was a problem hiding this comment.
q: no build file in the repo declares wireguard, so this class is on :app's classpath transitively, and the only other users are network-protection. what made InetAddresses.isHostname the right call over a local check? asking mainly because a netP dependency change would break the browser build.
There was a problem hiding this comment.
Correct.
Should we move this utility class to a common module (I believe this requires an API proposal though) or rather implement my own utility class for this? I figure the latter would be easier, but we may want to be consistent across the app on aspects such as hostname validation.
It’s mostly a question of what we value more. If we don’t feel so strong about how we perform hostname validation across the codebase, I’m happy to have a local implementation on the browser module for this feature. Let know and I’ll do the change.
There was a problem hiding this comment.
Done. I actually found PatternsCompat.DOMAIN_NAME & PatternsCompat.IP_ADDRESS from androidx.core.util, which I believe are a much better solution.
This way we don't rely on InetAddresses.isHostname, which is from a 3rd party transitive dependency (com.wireguard). Note that PatternsCompat.DOMAIN_NAME allows underscore on domain names on purpose, which InetAddresses.isHostname() did not.
This way, both the VPN module and the Browser module depend on a common DI module.
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1216807998862658/task/1217615568331049?focus=true
Tech Design URL: https://app.asana.com/1/137249556945/project/1216807998862658/task/1217494645371352?focus=true
Description
This PR adds the
HostnameResolverimplementation for the Suggest redirect feature, together with unit tests for it and integration tests for the twoDnsLookupimplementations (one for API 29+ and one for API 28), which are dependencies of the main component.The
HostnameResolvercomponent is wired up to DI but is not yet used in the app. The third PR, which culminates this feature's implementation will make use of it.UI changes
N/A
Note
Medium Risk
Introduces DNS lookups against the active network and relocates the app-scoped ConnectivityManager binding. The resolver is unused in product code, so runtime impact is limited until a follow-up.
Overview
Adds a
HostnameResolverthat reports whether a hostname resolves on the active network within 2s, for the upcoming Suggest redirect feature. It is wired in DI but not used yet.Lookups skip IP literals and invalid domains. API 29+ uses
DnsResolverwith cancellation; older APIs useNetwork.getAllByName.ConnectivityManageris now provided fromSystemComponentsModuleinstead of the VPN module.Reviewed by Cursor Bugbot for commit a5543f1. Bugbot is set up for automated code reviews on this repo. Configure here.