Bug 1518730 - Wait for both A and AAAA responses to come back before notifying the listeners r=dragana

This way we preserve the behaviour of getaddrinfo, where both A and AAAA
responses come back at the same time.
Without this Firefox will always be biased, as the first request will usually
be resolved first. So if we requested IPv4 first, we'd mostly be using IPv4.
If we requested IPv6 first, normally we'll wait for the IPv4 response to come
back too, which is functionally equivalent to the new behaviour.
However, if the pref is set network.trr.early-AAAA;true then we'd use the IPv6
response immediately, possibly leading to a failed request if the IPv6
connection fails before we have an IPv4 address to fall back to.

A test for this patch was added in bug 1542561.

Depends on D33476

Differential Revision: https://phabricator.services.mozilla.com/D33477

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Valentin Gosu 2019-06-03 21:13:22 +00:00
parent c3ee743b2d
commit 269fbdbecf
4 changed files with 19 additions and 2 deletions

View File

@ -5536,6 +5536,10 @@ pref("network.trr.request-timeout", 1500);
pref("network.trr.early-AAAA", false);
// When true, it only sends AAAA when the system has IPv6 connectivity
pref("network.trr.skip-AAAA-when-not-supported", true);
// When true, the DNS request will wait for both A and AAAA responses
// (if both have been requested) before notifying the listeners.
// When true, it effectively cancels `network.trr.early-AAAA`
pref("network.trr.wait-for-A-and-AAAA", true);
// Explicitly disable ECS (EDNS Client Subnet, RFC 7871)
pref("network.trr.disable-ECS", true);
// After this many failed TRR requests in a row, consider TRR borked

View File

@ -260,6 +260,13 @@ nsresult TRRService::ReadPrefs(const char* name) {
mCheckIPv6Connectivity = tmp;
}
}
if (!name || !strcmp(name, TRR_PREF("wait-for-A-and-AAAA"))) {
bool tmp;
if (NS_SUCCEEDED(
Preferences::GetBool(TRR_PREF("wait-for-A-and-AAAA"), &tmp))) {
mWaitForAllResponses = tmp;
}
}
if (!name || !strcmp(name, kDisableIpv6Pref)) {
bool tmp;
if (NS_SUCCEEDED(Preferences::GetBool(kDisableIpv6Pref, &tmp))) {

View File

@ -36,6 +36,7 @@ class TRRService : public nsIObserver,
bool UseGET() { return mUseGET; }
bool EarlyAAAA() { return mEarlyAAAA; }
bool CheckIPv6Connectivity() { return mCheckIPv6Connectivity; }
bool WaitForAllResponses() { return mWaitForAllResponses; }
bool DisableIPv6() { return mDisableIPv6; }
bool DisableECS() { return mDisableECS; }
nsresult GetURI(nsCString& result);
@ -87,7 +88,8 @@ class TRRService : public nsIObserver,
Atomic<bool, Relaxed> mUseGET; // do DOH using GET requests (instead of POST)
Atomic<bool, Relaxed> mEarlyAAAA; // allow use of AAAA results before A is in
Atomic<bool, Relaxed> mCheckIPv6Connectivity; // check IPv6 connectivity
Atomic<bool, Relaxed> mDisableIPv6; // don't even try
Atomic<bool, Relaxed> mWaitForAllResponses; // Don't notify until all are in
Atomic<bool, Relaxed> mDisableIPv6; // don't even try
Atomic<bool, Relaxed> mDisableECS; // disable EDNS Client Subnet in requests
Atomic<uint32_t, Relaxed>
mDisableAfterFails; // this many fails in a row means failed TRR service

View File

@ -1657,7 +1657,6 @@ void nsHostResolver::AddToEvictionQ(nsHostRecord* rec) {
//
// CompleteLookup() checks if the resolving should be redone and if so it
// returns LOOKUP_RESOLVEAGAIN, but only if 'status' is not NS_ERROR_ABORT.
// takes ownership of AddrInfo parameter
nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
nsHostRecord* rec, nsresult status, AddrInfo* aNewRRSet, bool pb,
const nsACString& aOriginsuffix) {
@ -1730,6 +1729,11 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup(
return LOOKUP_OK;
}
if (gTRRService && gTRRService->WaitForAllResponses()) {
LOG(("CompleteLookup: waiting for all responses!\n"));
return LOOKUP_OK;
}
if (addrRec->mTrrA && (!gTRRService || !gTRRService->EarlyAAAA())) {
// This is an early AAAA with a pending A response. Allowed
// only by pref.