From c206347e885c7122dab8d27ef48ebcfb57445d94 Mon Sep 17 00:00:00 2001 From: Nihanth Subramanya Date: Thu, 4 Feb 2021 17:34:27 +0000 Subject: [PATCH] Bug 1689113 - Don't skip TRR unless confirmation explicitly failed. r=necko-reviewers,valentin Differential Revision: https://phabricator.services.mozilla.com/D103170 --- modules/libpref/init/StaticPrefList.yaml | 10 +- netwerk/dns/TRRService.cpp | 42 +++++++-- netwerk/dns/TRRService.h | 2 +- netwerk/dns/nsHostResolver.cpp | 3 +- netwerk/test/unit/test_trr.js | 111 +++++++++++++++++------ testing/xpcshell/moz-http2/moz-http2.js | 12 ++- 6 files changed, 138 insertions(+), 42 deletions(-) diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 90db78dbe1c6..2a353671c03f 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -494,7 +494,7 @@ type: RelaxedAtomicBool value: true mirror: always - + - name: apz.gtk.touchpad_pinch.enabled type: RelaxedAtomicBool value: false @@ -8702,6 +8702,14 @@ value: false mirror: always +# If we should wait for TRR service confirmation to complete before enabling +# TRR for lookups when fallback is enabled. Confirmation is always skipped when +# global mode is TRR-only (no fallback). +- name: network.trr.wait-for-confirmation + type: RelaxedAtomicBool + value: false + mirror: always + # Use GET (rather than POST) - name: network.trr.useGET type: RelaxedAtomicBool diff --git a/netwerk/dns/TRRService.cpp b/netwerk/dns/TRRService.cpp index 49649867c6fb..d76096cfe623 100644 --- a/netwerk/dns/TRRService.cpp +++ b/netwerk/dns/TRRService.cpp @@ -230,14 +230,16 @@ void TRRService::SetDetectedTrrURI(const nsACString& aURI) { mURISetByDetection = MaybeSetPrivateURI(aURI); } -bool TRRService::Enabled(nsIRequest::TRRMode aMode) { - if (mMode == nsIDNSService::MODE_TRROFF) { +bool TRRService::Enabled(nsIRequest::TRRMode aRequestMode) { + if (mMode == nsIDNSService::MODE_TRROFF || + aRequestMode == nsIRequest::TRR_DISABLED_MODE) { return false; } + if (mConfirmationState == CONFIRM_INIT && (!StaticPrefs::network_trr_wait_for_portal() || mCaptiveIsPassed || (mMode == nsIDNSService::MODE_TRRONLY || - aMode == nsIRequest::TRR_ONLY_MODE))) { + aRequestMode == nsIRequest::TRR_ONLY_MODE))) { LOG(("TRRService::Enabled => CONFIRM_TRYING\n")); mConfirmationState = CONFIRM_TRYING; } @@ -245,14 +247,38 @@ bool TRRService::Enabled(nsIRequest::TRRMode aMode) { if (mConfirmationState == CONFIRM_TRYING) { LOG(("TRRService::Enabled MaybeConfirm()\n")); MaybeConfirm(); + if (mMode == nsIDNSService::MODE_TRRONLY) { + MOZ_ASSERT(mConfirmationState == CONFIRM_OK, + "Global mode is trr-only, but confirmation failed?"); + } } - if (mConfirmationState != CONFIRM_OK) { - LOG(("TRRService::Enabled mConfirmationState=%d mCaptiveIsPassed=%d\n", - (int)mConfirmationState, (int)mCaptiveIsPassed)); + LOG(("TRRService::Enabled mConfirmationState=%d mCaptiveIsPassed=%d\n", + (int)mConfirmationState, (int)mCaptiveIsPassed)); + + if (mConfirmationState == CONFIRM_OK) { + return true; } - return (mConfirmationState == CONFIRM_OK); + if (StaticPrefs::network_trr_wait_for_confirmation()) { + return false; + } + + if ((aRequestMode == nsIRequest::TRR_DEFAULT_MODE && + mMode == nsIDNSService::MODE_TRRONLY) || + aRequestMode == nsIRequest::TRR_ONLY_MODE) { + // For TRR-only requests, or if the global mode is TRR-only, just say we're + // enabled. + return true; + } + + if ((aRequestMode == nsIRequest::TRR_DEFAULT_MODE && + mMode == nsIDNSService::MODE_TRRFIRST) || + aRequestMode == nsIRequest::TRR_FIRST_MODE) { + return mConfirmationState != CONFIRM_FAILED; + } + + return false; } void TRRService::GetPrefBranch(nsIPrefBranch** result) { @@ -691,7 +717,7 @@ bool TRRService::MaybeBootstrap(const nsACString& aPossible, bool TRRService::IsDomainBlocked(const nsACString& aHost, const nsACString& aOriginSuffix, bool aPrivateBrowsing) { - if (!Enabled(nsIRequest::TRR_DEFAULT_MODE)) { + if (!Enabled()) { return true; } diff --git a/netwerk/dns/TRRService.h b/netwerk/dns/TRRService.h index 2b19a116c431..19c91403e996 100644 --- a/netwerk/dns/TRRService.h +++ b/netwerk/dns/TRRService.h @@ -38,7 +38,7 @@ class TRRService : public TRRServiceBase, TRRService(); nsresult Init(); nsresult Start(); - bool Enabled(nsIRequest::TRRMode aMode = nsIRequest::TRR_FIRST_MODE); + bool Enabled(nsIRequest::TRRMode aRequestMode = nsIRequest::TRR_DEFAULT_MODE); bool IsConfirmed() { return mConfirmationState == CONFIRM_OK; } bool DisableIPv6() { return mDisableIPv6; } diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp index 25ac6f31a291..ddc472ef257e 100644 --- a/netwerk/dns/nsHostResolver.cpp +++ b/netwerk/dns/nsHostResolver.cpp @@ -1637,7 +1637,8 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec) { rv = TrrLookup(rec); } - bool serviceNotReady = !gTRRService || !gTRRService->IsConfirmed(); + bool serviceNotReady = + !gTRRService || !gTRRService->Enabled(rec->mEffectiveTRRMode); if (rec->mEffectiveTRRMode == nsIRequest::TRR_DISABLED_MODE || (rec->mEffectiveTRRMode == nsIRequest::TRR_FIRST_MODE && diff --git a/netwerk/test/unit/test_trr.js b/netwerk/test/unit/test_trr.js index fa2d915c209e..aef5be187294 100644 --- a/netwerk/test/unit/test_trr.js +++ b/netwerk/test/unit/test_trr.js @@ -195,6 +195,38 @@ class DNSListener { } } +async function waitForConfirmation(expectedResponseIP, confirmationShouldFail) { + // Check that the confirmation eventually completes. + let count = 100; + while (count > 0) { + if (count == 50 || count == 10) { + // At these two points we do a longer timeout to account for a slow + // response on the server side. This is usually a problem on the Android + // because of the increased delay between the emulator and host. + await new Promise(resolve => do_timeout(100 * (100 / count), resolve)); + } + let [, inRecord] = await new DNSListener( + `ip${count}.example.org`, + undefined, + false + ); + inRecord.QueryInterface(Ci.nsIDNSAddrRecord); + let responseIP = inRecord.getNextAddrAsString(); + Assert.ok(true, responseIP); + if (responseIP == expectedResponseIP) { + break; + } + count--; + } + + if (confirmationShouldFail) { + Assert.equal(count, 0, "Confirmation did not finish after 100 iterations"); + return; + } + + Assert.greater(count, 0, "Finished confirmation before 100 iterations"); +} + add_task(async function test0_nodeExecute() { // This test checks that moz-http2.js running in node is working. // This should always be the first test in this file (except for setup) @@ -1672,46 +1704,71 @@ add_task(async function test_redirect_post() { }); // confirmationNS set without confirmed NS yet -// checks that we properly fall back to DNS is confirmation is not ready yet -add_task(async function test_resolve_not_confirmed() { +// checks that we properly fall back to DNS is confirmation is not ready yet, +// and wait-for-confirmation pref is true +add_task(async function test_resolve_not_confirmed_wait_for_confirmation() { dns.clearCache(true); + Services.prefs.setBoolPref("network.trr.wait-for-confirmation", true); + Services.prefs.setIntPref("network.trr.mode", 2); // TRR-first + Services.prefs.setCharPref( + "network.trr.confirmationNS", + "confirm.example.com" + ); + Services.prefs.setCharPref( + "network.trr.uri", + `https://foo.example.com:${h2Port}/doh?responseIP=7.7.7.7&slowConfirm=true` + ); + + await new DNSListener("example.org", "127.0.0.1"); + await new Promise(resolve => do_timeout(1000, resolve)); + await waitForConfirmation("7.7.7.7"); + + Services.prefs.setCharPref("network.trr.confirmationNS", "skip"); + Services.prefs.clearUserPref("network.trr.wait-for-confirmation"); +}); + +// checks that we don't fall back to DNS is confirmation is not ready yet, and +// wait-for-confirmation pref is false +add_task(async function test_resolve_confirmation_pending() { + dns.clearCache(true); + Services.prefs.setBoolPref("network.trr.wait-for-confirmation", false); + Services.prefs.setIntPref("network.trr.mode", 2); // TRR-first + Services.prefs.setCharPref( + "network.trr.confirmationNS", + "confirm.example.com" + ); + Services.prefs.setCharPref( + "network.trr.uri", + `https://foo.example.com:${h2Port}/doh?responseIP=7.7.7.7&slowConfirm=true` + ); + + // DoH available immediately + await new DNSListener("example.org", "7.7.7.7"); + + Services.prefs.setCharPref("network.trr.confirmationNS", "skip"); + Services.prefs.clearUserPref("network.trr.wait-for-confirmation"); +}); + +// checks that we properly fall back to DNS if confirmation failed +add_task(async function test_resolve_confirm_failed() { + dns.clearCache(true); + Services.prefs.setBoolPref("network.trr.wait-for-confirmation", true); Services.prefs.setIntPref("network.trr.mode", 2); // TRR-first Services.prefs.setCharPref( "network.trr.uri", - `https://foo.example.com:${h2Port}/doh?responseIP=7.7.7.7` + `https://foo.example.com:${h2Port}/404` ); Services.prefs.setCharPref( "network.trr.confirmationNS", "confirm.example.com" ); + await waitForConfirmation("7.7.7.7", true); + await new DNSListener("example.org", "127.0.0.1"); - // Check that the confirmation eventually completes. - let count = 100; - while (count > 0) { - if (count == 50 || count == 10) { - // At these two points we do a longer timeout to account for a slow - // response on the server side. This is usually a problem on the Android - // because of the increased delay between the emulator and host. - await new Promise(resolve => do_timeout(100 * (100 / count), resolve)); - } - let [, inRecord] = await new DNSListener( - `ip${count}.example.org`, - undefined, - false - ); - inRecord.QueryInterface(Ci.nsIDNSAddrRecord); - let responseIP = inRecord.getNextAddrAsString(); - if (responseIP == "7.7.7.7") { - break; - } - count--; - } - - Assert.greater(count, 0, "Finished confirmation before 100 iterations"); - Services.prefs.setCharPref("network.trr.confirmationNS", "skip"); + Services.prefs.clearUserPref("network.trr.wait-for-confirmation"); }); // Tests that we handle FQDN encoding and decoding properly diff --git a/testing/xpcshell/moz-http2/moz-http2.js b/testing/xpcshell/moz-http2/moz-http2.js index 36b96d531b8f..eb476db4503d 100644 --- a/testing/xpcshell/moz-http2/moz-http2.js +++ b/testing/xpcshell/moz-http2/moz-http2.js @@ -782,11 +782,15 @@ function handleRequest(req, res) { } } - let delay = undefined; + let delay = 0; if (packet.questions[0].type == "A") { - delay = u.query.delayIPv4; + delay = parseInt(u.query.delayIPv4); } else if (packet.questions[0].type == "AAAA") { - delay = u.query.delayIPv6; + delay = parseInt(u.query.delayIPv6); + } + + if (u.query.slowConfirm && responseType() == "NS") { + delay += 1000; } if (delay) { @@ -794,7 +798,7 @@ function handleRequest(req, res) { arg => { writeResponse(arg[0], arg[1]); }, - parseInt(delay), + delay, [response, buf] ); return;