Provides an optional resolver mechanism for Firefox that allows running together with or instead of the native resolver. TRR offers resolving of host names using a dedicated DNS-over-HTTPS server (HTTPS is required, HTTP/2 is preferable). DNS-over-HTTPS (DOH) allows DNS resolves with enhanced privacy, secure transfers and improved performance. To keep the failure rate at a minimum, the TRR system manages a dynamic persistent blacklist for host names that can't be resolved with DOH but works with the native resolver. Blacklisted entries will not be retried over DOH for a couple of days. "localhost" and names in the ".local" TLD will never be resolved via DOH. TRR is preffed OFF by default and you need to set a URI for an available DOH server to be able to use it. Since the URI for DOH is set with a name itself, it may have to use the native resolver for bootstrapping. (Optionally, the user can set the IP address of the DOH server in a pref to avoid the required initial native resolve.) When TRR starts up, it will first verify that it works by first checking a "confirmation" domain name. This confirmation domain is a pref by default set to "example.com". TRR will also by default await the captive-portal detection to raise its green flag before getting activated. All prefs for TRR are under the "network.trr" hierarchy. The DNS-over-HTTPS spec: https://tools.ietf.org/html/draft-ietf-doh-dns-over-https-02

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review222938 Static analysis found 2 defects in this patch. - 2 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/dns/nsHostResolver.cpp:1128 (Diff revision 1) > + if (gTRRService && gTRRService->IsTRRBlacklisted(rec->host, rec->pb, true)) { > + Telemetry::Accumulate(Telemetry::DNS_TRR_BLACKLISTED, true); > + MOZ_ASSERT(!rec->mTRRUsed); > + // not really an error but no TRR is issued > + return NS_ERROR_UNKNOWN_HOST; > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return] ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 1) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223198 Static analysis found 1 defect in this patch. - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 2) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223414 I didn't read anything in tests as those are a wip. overall no huge issues ::: commit-message-17ade:24 (Diff revision 2) > +server to be able to use it. Since the URI for DOH is set with a name itself, > +it may have to use the native resolver for bootstrapping. (Optionally, the > +user can set the IP address of the DOH server in a pref to avoid the required > +initial native resolve.) > + > +When TRR starts up, it will first verify that it works by first checking a double first ::: commit-message-17ade:31 (Diff revision 2) > +to "example.com". TRR will also by default await the captive-portal detection > +to raise its green flag before getting activated. > + > +All prefs for TRR are under the "network.trr" hierarchy. > + > +The DNS-over-HTTPS spec: https://tools.ietf.org/html/draft-ietf-doh-dns-over-https-02 -03 now (the differences are trivial and will be noted in the review) ::: modules/libpref/init/all.js:5400 (Diff revision 2) > pref("network.captive-portal-service.enabled", false); > > +// DNS Trusted Recursive Resolver > +// 0 - off, 1 - race, 2 TRR first, 3 TRR only, 4 shadow > +pref("network.trr.mode", 0); > +// DEBUG-only default URI remove DEBUG comment ::: netwerk/build/nsNetCID.h:495 (Diff revision 2) > 0x4f7b, \ > { 0x92, 0x05, 0xc3, 0x09, 0xce, 0xb2, 0xd6, 0x41 } \ > } > > +// TRR service > +#define NS_TRR_CONTRACTID \ I think this is orphaned code, right? It should be removed. ::: netwerk/dns/DNS.cpp:324 (Diff revision 2) > } > > AddrInfo::AddrInfo(const char *host, const char *cname) > : mHostName(nullptr) > , mCanonicalName(nullptr) > , ttl(NO_TTL_DATA) it looks like mFromTRR is not initialized here ::: netwerk/dns/DNSRequestChild.cpp:87 (Diff revision 2) > } > > NS_IMETHODIMP > +ChildDNSRecord::IsTRR(bool *retval) > +{ > + fprintf(stderr, "TODO: ChildDNSRecord::IsTRR doesn't know!\n"); remove stderr a child shouldn't need this, so return NS_ERROR_NOT_AVAILBLE instead of making up an answer ::: netwerk/dns/TRR.h:39 (Diff revision 2) > +class DOHresp { > +public: > + virtual ~DOHresp() { } > + nsresult Add(uint32_t TTL, unsigned char *dns, int index, uint16_t len, > + bool aLocalAllowed); > + uint16_t mNumAddresses; I believe mNumAddresses is unused. if it is used then it is uninitialized :) ::: netwerk/dns/TRR.h:58 (Diff revision 2) > + NS_DECL_NSIINTERFACEREQUESTOR > + NS_DECL_NSIREQUESTOBSERVER > + NS_DECL_NSISTREAMLISTENER > + > + static const unsigned int kMaxSize = 3200; > + explicit TRR(AHostResolver *aResolver, it would be nice to have comments about what each ctor is used for ::: netwerk/dns/TRR.cpp:19 (Diff revision 2) > +#include "nsISupportsBase.h" > +#include "nsISupportsUtils.h" > +#include "nsIUploadChannel2.h" > +#include "nsNetUtil.h" > +#include "nsThreadUtils.h" > +#include "nsStringStream.h" total nit but nsStringStream is misordered in the list ::: netwerk/dns/TRR.cpp:85 (Diff revision 2) > + } else { > + labelLength = mHost.Length() - offset; > + } > + if (labelLength > 63) { > + // too long label! > + return NS_ERROR_UNEXPECTED; should probably be NS_ERROR_FAILURE, its just a failure of the API caller not an internal consistency issue ::: netwerk/dns/TRR.cpp:129 (Diff revision 2) > + // limit the calling interface becase nsHostResolver has explicit slots for > + // these types > + return NS_ERROR_FAILURE; > + } > + > + // 'host' should be converted to a DNS query packet for QTYPE "A" or this comment is specific to get ::: netwerk/dns/TRR.cpp:136 (Diff revision 2) > + // > + nsresult rv; > + nsCOMPtr<nsIIOService> ios(do_GetIOService(&rv)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // body=q80BAAABAAAAAAAAA3d3dwdleGFtcGxlA2NvbQAAAQAB this comment is also specific to get, and the example comment is a bit dated as it doesn't use id 0 (-03 has an updated exmaple) ::: netwerk/dns/TRR.cpp:158 (Diff revision 2) > + Base64URLEncodePaddingPolicy::Omit, body); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCString uri; > + mTRRService->GetURI(uri); > + uri.Append(NS_LITERAL_CSTRING("?ct=application/udp-wireformat&body=")); the query parameter named body is changed to dns in -03 ::: netwerk/dns/TRR.cpp:387 (Diff revision 2) > + return (static_cast<uint8_t>(aData[index]) << 24) | > + (static_cast<uint8_t>(aData[index+1])<<16) | > + (static_cast<uint8_t>(aData[index+2]) << 8) | > + static_cast<uint8_t>(aData[index+3]); > +} > +// add an empty line ::: netwerk/dns/TRR.cpp:412 (Diff revision 2) > + uint8_t length; > + nsAutoCString host; > + > + LOG(("doh decode %s %d bytes\n", mHost.get(), mUsed)); > + > + if (mUsed < 12 || mResponse[0] || mResponse[1]) { mUsed is a terrible name (I chose it :)).. rename? ::: netwerk/dns/TRR.cpp:435 (Diff revision 2) > + if (length) { > + if (host.Length()) { > + host.Append("."); > + } > + if (mUsed < (index + 1 + length)) { > + return NS_ERROR_UNEXPECTED; most of these UNEXPECTED returns in this method should probably be more generic FAILURE. (I know, I put most of them in.. sorry). UNEXPECTED is a little more akin to asserting an invariant with a runtime return code.. while these are just data integrity errors from the internet ::: netwerk/dns/TRR.cpp:505 (Diff revision 2) > + if (mUsed < (index + 2)) { > + LOG(("TRR: Dohdecode:%d fail at index %d\n", __LINE__, index + 2)); > + return NS_ERROR_UNEXPECTED; > + } > + uint16_t CLASS = get16bit(mResponse, index); > + if (1 != CLASS) { instead of the magic const 1, use DNS_CLASS_IN and change its scope as necessary.. ::: netwerk/dns/TRR.cpp:548 (Diff revision 2) > + return NS_ERROR_UNEXPECTED; > + } > + rv = mDNS.Add(TTL, mResponse, index, RDLENGTH, > + mTRRService->AllowRFC1918()); > + if (NS_FAILED(rv)) { > + LOG(("TRR got local IPv4 address!\n")); I'm not sure that log is accurate.. there are other reasons for fialure, no? ::: netwerk/dns/TRR.cpp:692 (Diff revision 2) > + LOG(("done with additional rr now %u of %u\n", index, mUsed)); > + arRecords--; > + } > + > + if (index != mUsed) { > + LOG(("TRRRRRR: bad DNS parser (%u != %d)!\n", index, (int)mUsed)); we can do better ::: netwerk/dns/TRR.cpp:804 (Diff revision 2) > + nsIInputStream *aInputStream, > + uint64_t aOffset, > + const uint32_t aCount) > +{ > + // receive DNS response into the local buffer > + if mFailed this should probably return right away ::: netwerk/dns/TRR.cpp:842 (Diff revision 2) > + addr->inet6.scope_id = 0; // unknown > + for(int i = 0; i < 16; i++, index++) { > + addr->inet6.ip.u8[i] = dns[index]; > + } > + } else { > + return NS_ERROR_UNEXPECTED; leaks doh.. I think you can easily fix by making doh an nsAutoPtr and setting it to nullptr after inserting it in the list ::: netwerk/dns/TRR.cpp:846 (Diff revision 2) > + } else { > + return NS_ERROR_UNEXPECTED; > + } > + > + if (IsIPAddrLocal(addr) && !aLocalAllowed) { > + return NS_ERROR_FAILURE; leaks doh ::: netwerk/dns/TRRService.h:13 (Diff revision 2) > + > +#include "mozilla/Atomics.h" > +#include "mozilla/DataStorage.h" > +#include "nsHostResolver.h" > +#include "nsWeakReference.h" > +#include "nsIObserver.h" mis alphabetized ::: netwerk/dns/TRRService.h:68 (Diff revision 2) > + Atomic<bool, Relaxed> mRfc1918; > + Atomic<bool, Relaxed> mCaptiveIsPassed; > + Atomic<bool, Relaxed> mUseGET; > + > + RefPtr<DataStorage> mStorage; > + bool mClearStorage; looks like this is on multiple thraeds, so it should be relaxed atomic bool ::: netwerk/dns/TRRService.cpp:19 (Diff revision 2) > + > +static const char kOpenCaptivePortalLoginEvent[] = "captive-portal-login"; > +static const char kClearPrivateData[] = "clear-private-data"; > +static const char kPurge[] = "browser:purge-session-history"; > + > +const static uint32_t kTRRBlacklistExpireTime = 3600*24*3; // three days this should be a pref ::: netwerk/dns/TRRService.cpp:108 (Diff revision 2) > +nsresult > +TRRService::ReadPrefs(const char *name) > +{ > + MOZ_ASSERT(NS_IsMainThread(), "wrong thread"); > + if (!name || !strcmp(name, TRR_PREF("mode"))) { > + // 0 - off, 1 - parallel, 2 TRR first, 3 TRR only also shadow.. ::: netwerk/dns/TRRService.cpp:115 (Diff revision 2) > + if (NS_SUCCEEDED(Preferences::GetUint(TRR_PREF("mode"), &tmp))) { > + mMode = tmp; > + } > + } > + if (!name || !strcmp(name, TRR_PREF("uri"))) { > + // Base URI, appends "?ct&body=..." s/body/dns/ ::: netwerk/dns/TRRService.cpp:273 (Diff revision 2) > +{ > + if ((mMode == MODE_NATIVEONLY) || mConfirmer || > + mConfirmationState != CONFIRM_TRYING) { > + return; > + } > + nsCString host; nsAutoCString would be a touch better.. ::: netwerk/dns/TRRService.cpp:473 (Diff revision 2) > + } > + > + // when called without a host record, this is a domain name check response. > + if (NS_SUCCEEDED(status)) { > + LOG(("TRR verified %s to be fine!\n", newRRSet->mHostName)); > + // whitelist? remove irrelevant comment ::: netwerk/dns/moz.build:38 (Diff revision 2) > ] > > SOURCES += [ > 'nsEffectiveTLDService.cpp', # Excluded from UNIFIED_SOURCES due to special build flags. > 'nsHostResolver.cpp', # Redefines LOG > + 'TRR.cpp', is there a reason this isn't in unified sources? ::: netwerk/dns/nsDNSService2.cpp:112 (Diff revision 2) > > NS_IMETHODIMP > +nsDNSRecord::IsTRR(bool *retval) > +{ > + if (mHostRecord->addr_info) { > + mHostRecord->addr_info_lock.Lock(); you need to get the lock before you check the pointer for null ::: netwerk/dns/nsDNSService2.cpp:658 (Diff revision 2) > } > > RegisterWeakMemoryReporter(this); > > + mTrrService = new TRRService(); > + if (mTrrService && NS_FAILED(mTrrService->Init())) { we use infallible new, so don't check if mTrrService is not null ::: netwerk/dns/nsHostResolver.h:275 (Diff revision 2) > + virtual LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb) = 0; > + virtual nsresult GetHostRecord(const char *host, > + uint16_t flags, uint16_t af, bool pb, > + const nsCString &netInterface, > + const nsCString &originSuffix, > + nsHostRecord **result) trailing whitespace ::: netwerk/dns/nsHostResolver.h:279 (Diff revision 2) > + const nsCString &originSuffix, > + nsHostRecord **result) > + { > + return NS_ERROR_FAILURE; > + } > + virtual nsresult TrrLookup_unlocked(nsHostRecord *, mozilla::net::TRR *pushedTRR = nullptr) trailing whitespace ::: netwerk/dns/nsHostResolver.cpp:17 (Diff revision 2) > #define RES_RETRY_ON_FAILURE > #endif > > #include <stdlib.h> > #include <ctime> > +#include "prtime.h" prtime is in the include list twice ::: netwerk/dns/nsHostResolver.cpp:969 (Diff revision 2) > LOG((" DNS lookup for host [%s%s%s] blocking " > "pending 'getaddrinfo' query: callback [%p]", > LOG_HOST(host, netInterface), callback.get())); > } > } > - } > + } else { you might as well fix the other instances in this if-else-cascade that don't have their else on the same line as the preceeding } ::: netwerk/dns/nsHostResolver.cpp:1515 (Diff revision 2) > + } else if (newRRSet->isTRR() == TRRTYPE_AAAA) { > + MOZ_ASSERT(rec->mTrrAAAA); > + rec->mTrrAAAA = nullptr; > + rec->mTrrAAAAUsed = NS_SUCCEEDED(status) ? nsHostRecord::OK : nsHostRecord::FAILED; > + } else { > + MOZ_ASSERT(0); LOG(()) would be good ::: netwerk/protocol/http/nsHttpChannel.cpp:3744 (Diff revision 2) > bool lookupAppCache = (mChooseApplicationCache || (mLoadFlags & LOAD_CHECK_OFFLINE_CACHE)) && > !mPostID && > MOZ_LIKELY(allowApplicationCache); > // Try to race only if we use disk cache storage and we don't lookup > // app cache first > - maybeRCWN = !lookupAppCache; > + maybeRCWN = (!lookupAppCache) && mRequestHead.IsSafeMethod(); this one line is actually a ride along.. valentin might want to commit it separately ::: netwerk/protocol/http/nsHttpConnection.cpp:1053 (Diff revision 2) > uint32_t > nsHttpConnection::TimeToLive() > { > - if (IdleTime() >= mIdleTimeout) > + LOG(("nsHttpConnection::TTL: %p %s idle %d timeout %d\n", > + this, mConnInfo->Origin(), IdleTime(), mIdleTimeout)); > + whitespace ::: toolkit/components/telemetry/Histograms.json:3648 (Diff revision 2) > + "expires_in_version": "never", > + "kind": "exponential", > + "high": 60000, > + "releaseChannelCollection": "opt-out", > + "alert_emails": ["necko@mozilla.com", "dstenberg@mozilla.com"], > + "bug_numbers": [9999999], there is a bugzilla entry now so you can set these numbers correctly

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223414 test_trr.js only makes a single test so far, but shows what general approach I consider to use: I setup the http2-server to serve as DNS-over-HTTPS server and I have that respond with an A 127.0.0.1 record for bar.example.com, so that the test then connects to 127.0.0.1 and gets the HTTP content correctly from there. I first wanted to get the actual content from the http2 server too, but I ran into some bootstrapping issues then as I don't want to use the same host name for the DOH server as for the origin server. I don't think it matters for the TRR tests if we get the final contents over h1 or h2. > mUsed is a terrible name (I chose it :)).. rename? Ack, going with 'mBodySize' instead... > most of these UNEXPECTED returns in this method should probably be more generic FAILURE. (I know, I put most of them in.. sorry). UNEXPECTED is a little more akin to asserting an invariant with a runtime return code.. while these are just data integrity errors from the internet I'm replacing most of the "bad input DNS packet" errors with NS_ERROR_ILLEGAL_VALUE. I believe the http2 parser uses that at times. > this should be a pref I'm creating "network.trr.blacklist-duration", for the expiry time in number of hours. Default is 72. > is there a reason this isn't in unified sources? I believe I had a reason originally when I put it there, but I can't think of a good one now! =) > this one line is actually a ride along.. valentin might want to commit it separately I've removed it from my patch since it isn't strictly TRR related.

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223572 Static analysis found 1 defect in this patch. - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 3) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223916 can you also start the data collection review process - https://wiki.mozilla.org/Firefox/Data_Collection .. i.e. file a bug and r? francois? ::: netwerk/dns/TRR.cpp:85 (Diff revisions 2 - 3) > } else { > labelLength = mHost.Length() - offset; > } > if (labelLength > 63) { > // too long label! > - return NS_ERROR_UNEXPECTED; > + return NS_ERROR_FAILURE; you're right that illegal_value is the best choice here ::: netwerk/dns/TRR.cpp:150 (Diff revisions 2 - 3) > Base64URLEncodePaddingPolicy::Omit, body); > NS_ENSURE_SUCCESS(rv, rv); > > nsCString uri; > mTRRService->GetURI(uri); > - uri.Append(NS_LITERAL_CSTRING("?ct=application/udp-wireformat&body=")); > + uri.Append(NS_LITERAL_CSTRING("?ct&dns=")); the short form of ct maybe (probably?) breaks existing test partner.. test it out plz. if that's true just leave the long form for now (its still legal) ::: netwerk/dns/TRR.cpp:541 (Diff revisions 2 - 3) > return NS_ERROR_UNEXPECTED; > } > rv = mDNS.Add(TTL, mResponse, index, RDLENGTH, > mTRRService->AllowRFC1918()); > if (NS_FAILED(rv)) { > - LOG(("TRR got local IPv4 address!\n")); > + // got local IP addresses or unknown IP family oh do log.. even if its just "dns.add failed" ::: netwerk/dns/TRR.cpp:822 (Diff revisions 2 - 3) > > nsresult > DOHresp::Add(uint32_t TTL, unsigned char *dns, int index, uint16_t len, > bool aLocalAllowed) > { > - DOHaddr *doh = new DOHaddr; > + nsAutoPtr<DOHaddr> doh; no allocation. this is broken. ::: netwerk/dns/TRR.cpp:839 (Diff revisions 2 - 3) > addr->inet6.scope_id = 0; // unknown > for(int i = 0; i < 16; i++, index++) { > addr->inet6.ip.u8[i] = dns[index]; > } > } else { > + doh = nullptr; you don't need this.. the semantics of the autoptr are to free it if the autoptr goes out of scope and its not null ::: netwerk/dns/TRR.cpp:844 (Diff revisions 2 - 3) > + doh = nullptr; > return NS_ERROR_UNEXPECTED; > } > > if (IsIPAddrLocal(addr) && !aLocalAllowed) { > + doh = nullptr; as above, you don't need this ::: netwerk/dns/TRR.cpp:849 (Diff revisions 2 - 3) > + doh = nullptr; > return NS_ERROR_FAILURE; > } > doh->mTtl = TTL; > mAddresses.insertBack(doh); > + doh = nullptr; you want doh.forget()

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223998 Found a few bugs and a bunch of nits. I opened issues for them too, but you can drop them if you don't agree. ::: commit-message-17ade:24 (Diff revision 3) > +server to be able to use it. Since the URI for DOH is set with a name itself, > +it may have to use the native resolver for bootstrapping. (Optionally, the > +user can set the IP address of the DOH server in a pref to avoid the required > +initial native resolve.) > + > +When TRR starts up, it will first verify that it works bychecking a nit: bychecking ::: modules/libpref/init/all.js:5417 (Diff revision 3) > +pref("network.trr.confirmationNS", "example.com"); > +// hardcode the resolution of the hostname in network.trr.uri instead of > +// relying on the system resolver to do it for you > +pref("network.trr.bootstrapAddress", ""); > +// TRR blacklist entry expire time (in hours) > +pref("network.trr.blacklist-duration", 72) I have no _big_ problem with this being hours instead of seconds, but if we make it seconds it will be a lot easier to write automated tests for it. ::: netwerk/base/nsSocketTransport2.cpp:1809 (Diff revision 3) > mState = STATE_CLOSED; > mConnectionFlags &= ~(DISABLE_IPV6 | DISABLE_IPV4); > tryAgain = true; > + } else if (!(mConnectionFlags & DISABLE_TRR)) { > + bool trrEnabled; > + mDNSRecord->IsTRR( &trrEnabled); nit: odd spacing ::: netwerk/dns/DNS.h:142 (Diff revision 3) > // to initialize the host and the cname. > AddrInfo(const char *host, const PRAddrInfo *prAddrInfo, bool disableIPv4, > bool filterNameCollision, const char *cname); > > // Creates a basic AddrInfo object (initialize only the host and the cname). > AddrInfo(const char *host, const char *cname); Can we make all constructors explicit? ::: netwerk/dns/DNS.h:160 (Diff revision 3) > char *mCanonicalName; > uint32_t ttl; > static const uint32_t NO_TTL_DATA = (uint32_t) -1; > > LinkedList<NetAddrElement> mAddresses; > - > + unsigned int isTRR() { return mFromTRR; } nit: should be capitalized to IsTRR() ::: netwerk/dns/DNS.cpp:334 (Diff revision 3) > > +AddrInfo::AddrInfo(const char *host, unsigned int aTRR) > + : mHostName(nullptr) > + , mCanonicalName(nullptr) > + , ttl(NO_TTL_DATA) > + , mFromTRR(aTRR) We should either assert that aTRR is 1,2,5 or 28 or even better, add an optional argument to the other constructor: AddrInfo(const char *host, const char *cname, bool isTRR = false) ::: netwerk/dns/TRR.h:36 (Diff revision 3) > +class TRRService; > +extern TRRService *gTRRService; > + > +class DOHresp { > +public: > + virtual ~DOHresp() { } nit: = default; ::: netwerk/dns/TRR.h:46 (Diff revision 3) > + > +class TRR > + : public Runnable > + , public nsIHttpPushListener > + , public nsIInterfaceRequestor > +// , public nsIRequestObserver from nsIStreamListener Do we need to specifically mention that it also inherits from nsIRequestObserver? Also, if you want TRR to .QI to nsIRequestObserver you need to list it in NS_IMPL_ISUPPORTS ::: netwerk/dns/TRR.h:56 (Diff revision 3) > + NS_DECL_NSIHTTPPUSHLISTENER > + NS_DECL_NSIINTERFACEREQUESTOR > + NS_DECL_NSIREQUESTOBSERVER > + NS_DECL_NSISTREAMLISTENER > + > + static const unsigned int kMaxSize = 3200; nit: A comment would be helpful in figuring out the size of what this is. ::: netwerk/dns/TRR.h:77 (Diff revision 3) > + } > + > + // used on push > + explicit TRR(nsIHttpChannel *pushedChannel, > + AHostResolver *aResolver, > + bool aPB, nsHostRecord *pushedRec); I opened an issue in the Push() method. This needs to be split into constructor & method that does the push. ::: netwerk/dns/TRR.h:81 (Diff revision 3) > + AHostResolver *aResolver, > + bool aPB, nsHostRecord *pushedRec); > + > + // to verify a domain > + explicit TRR(AHostResolver *aResolver, > + nsCString aHost, use nsACString& ::: netwerk/dns/TRR.h:96 (Diff revision 3) > + , mPB(aPB) > + { } > + > + NS_IMETHOD Run() override; > + void Cancel(); > + enum TrrType Type() {return mType;} nit: spaces around braces ::: netwerk/dns/TRR.h:98 (Diff revision 3) > + > + NS_IMETHOD Run() override; > + void Cancel(); > + enum TrrType Type() {return mType;} > + nsCString mHost; > + RefPtr<nsHostRecord> mRec; nit: weird indentation ::: netwerk/dns/TRR.h:103 (Diff revision 3) > + RefPtr<nsHostRecord> mRec; > + RefPtr<AHostResolver> mHostResolver; > + TRRService *mTRRService; > + > +private: > + ~TRR() {} nit: = default; ::: netwerk/dns/TRR.cpp:39 (Diff revision 3) > +#undef LOG > +extern mozilla::LazyLogModule gHostResolverLog; > +#define LOG(args) MOZ_LOG(gHostResolverLog, mozilla::LogLevel::Debug, args) > +#define LOG_ENABLED() MOZ_LOG_TEST(mozilla::net::gHostResolverLog, mozilla::LogLevel::Debug) > + > + NS_IMPL_ISUPPORTS(TRR, nsIHttpPushListener, nsIInterfaceRequestor, nsIStreamListener, nsIRunnable) nit: unneeded spaces before NS_IMPL ::: netwerk/dns/TRR.cpp:72 (Diff revision 3) > + // octets. The domain name terminates with the zero length octet for the > + // null label of the root. > + // Followed by 16 bit QTYPE and 16 bit QCLASS > + > + PRInt32 index = 0; > + PRInt32 offset = 0; nit: can we use int32_t instead? ::: netwerk/dns/TRR.cpp:95 (Diff revision 3) > + if(!dotFound) { > + aBody += '\0'; // terminate with a final zero > + break; > + } > + offset += labelLength + 1; // move over label and dot > + } while(1); nit: this code might look nicer if we used mozilla::Tokenizer ::: netwerk/dns/TRR.cpp:118 (Diff revision 3) > + } > + return NS_OK; > +} > + > +nsresult > +TRR::DNSoverHTTPS() nit: A more descriptive name maybe? Such as PerformLookup, or SendRequest? ::: netwerk/dns/TRR.cpp:124 (Diff revision 3) > +{ > + // This is essentially the "run" method - created from nsHostResolver > + MOZ_ASSERT(NS_IsMainThread(), "wrong thread"); > + > + if ((mType != TRRTYPE_A) && (mType != TRRTYPE_AAAA) && (mType != TRRTYPE_NS)) { > + // limit the calling interface becase nsHostResolver has explicit slots for typo: becase ::: netwerk/dns/TRR.cpp:148 (Diff revision 3) > + then appended to the end of the URI. */ > + rv = Base64URLEncode(tmp.Length(), reinterpret_cast<const unsigned char *>(tmp.get()), > + Base64URLEncodePaddingPolicy::Omit, body); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCString uri; nit: nsAutoCString ::: netwerk/dns/TRR.cpp:159 (Diff revision 3) > + rv = DohEncode(body); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCString uri; > + mTRRService->GetURI(uri); > + NS_NewURI(getter_AddRefs(dnsURI), uri); Check return codes for NS_NewURI ::: netwerk/dns/TRR.cpp:162 (Diff revision 3) > + nsCString uri; > + mTRRService->GetURI(uri); > + NS_NewURI(getter_AddRefs(dnsURI), uri); > + } > + > + NS_NewChannel(getter_AddRefs(mChannel), Check return code ::: netwerk/dns/TRR.cpp:182 (Diff revision 3) > + rv = httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"), > + NS_LITERAL_CSTRING("application/dns-udpwireformat"), > + false); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCString cred; nit: nsAutoCString ::: netwerk/dns/TRR.cpp:184 (Diff revision 3) > + false); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCString cred; > + mTRRService->GetCredentials(cred); > + if (cred.Length()){ nit: !cred.IsEmpty() is a bit nicer ::: netwerk/dns/TRR.cpp:224 (Diff revision 3) > + NS_LITERAL_CSTRING("POST"), false); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + // set the *default* response content type > + httpChannel->SetContentType(NS_LITERAL_CSTRING("application/dns-udpwireformat")); nit: handle failure? ::: netwerk/dns/TRR.cpp:226 (Diff revision 3) > + } > + > + // set the *default* response content type > + httpChannel->SetContentType(NS_LITERAL_CSTRING("application/dns-udpwireformat")); > + > + if (NS_SUCCEEDED(httpChannel->AsyncOpen2(this))) { nit: if (NS_FAILED(rv)) { handle failure } is better IMO than the opposite. ::: netwerk/dns/TRR.cpp:308 (Diff revision 3) > + , mPB(aPB) > +{ > + if (!mHostResolver) { > + return; > + } > + Please split this into a constructor part, and a "do stuff part". ::: netwerk/dns/TRR.cpp:353 (Diff revision 3) > + if (!mRec) { > + return NS_ERROR_FAILURE; > + } > + > + // make a trr > + new TRR(pushed, mHostResolver, mPB, mRec); RefPtr<TRR> trr = newTRR(...); trr->DoStuff(); // Just so we don't leak if any call fails in the constructor. ::: netwerk/dns/TRR.cpp:372 (Diff revision 3) > +{ > + return (static_cast<uint8_t>(aData[index]) << 8) | > + static_cast<uint8_t>(aData[index + 1]); > +} > + > +static uint32_t get32bit(unsigned char *aData, int index) Do we need to worry about endianess? Wouldn't a call to ntohl be enough? ::: netwerk/dns/TRR.cpp:719 (Diff revision 3) > + ttl = item->mTtl; > + } > + } > + ai->ttl = ttl; > + if (!mHostResolver) { > + return NS_ERROR_FAILURE; Leaks ai on error. Use an AutoPtr and forget it in CompleteLookup? ::: netwerk/dns/TRR.cpp:755 (Diff revision 3) > + this, mHost.get(), mType, mFailed, (unsigned int)aStatusCode)); > + nsCOMPtr<nsIChannel> channel; > + channel.swap(mChannel); > + > + // if status was "fine", parse the response and pass on the answer > + if (!mFailed && NS_SUCCEEDED(aStatusCode)) { nit: if (mFailed || NS_FAILED ) handle failure. Makes the code easier to follow. ::: netwerk/dns/TRR.cpp:823 (Diff revision 3) > +nsresult > +DOHresp::Add(uint32_t TTL, unsigned char *dns, int index, uint16_t len, > + bool aLocalAllowed) > +{ > + nsAutoPtr<DOHaddr> doh; > + NetAddr *addr = &doh->mNet; doh is null here ::: netwerk/dns/TRR.cpp:859 (Diff revision 3) > + LOG(("DOHresp:Add %s\n", buf)); > + } > + return NS_OK; > +} > + > +class proxyCancel : public Runnable nit: ProxyCancel (uppercase) ::: netwerk/dns/TRRService.h:69 (Diff revision 3) > + Atomic<bool, Relaxed> mRfc1918; > + Atomic<bool, Relaxed> mCaptiveIsPassed; > + Atomic<bool, Relaxed> mUseGET; > + > + RefPtr<DataStorage> mStorage; > + Atomic<bool, Relaxed> mClearStorage; Comment what these do. And what does mStorage hold? ::: netwerk/dns/TRRService.h:71 (Diff revision 3) > + Atomic<bool, Relaxed> mUseGET; > + > + RefPtr<DataStorage> mStorage; > + Atomic<bool, Relaxed> mClearStorage; > + > + enum confirmationState { nit: ConfirmationState ::: netwerk/dns/TRRService.cpp:178 (Diff revision 3) > + if (hours > 10000) { > + // capped to avoid integer overflow and mistakes; a full year is 8760 > + // hours > + hours = 10000; > + } > + mTRRBlacklistExpireTime = hours * 360; Pretty sure this was supposed to be 3600. :) We should turn the pref to seconds, and avoid doing _any_ math :D ::: netwerk/dns/TRRService.cpp:212 (Diff revision 3) > + } > + return NS_OK; > +} > + > +nsresult > +TRRService::Stop() Shouldn't this method actually do something? ::: netwerk/dns/TRRService.cpp:314 (Diff revision 3) > + if (!mBootstrapAddr.Length()) { > + return false; > + } > + > + RefPtr<nsStandardURL> url = new nsStandardURL(); > + url->Init(nsIStandardURL::URLTYPE_AUTHORITY, 443, You can't call init anymore. Use the mutator and check the return code https://github.com/valenting/gecko/wiki/Migrating-nsIURI-code-to-use-mutators#c-instantiate-new-uri-via-contract-id ::: netwerk/dns/TRRService.cpp:335 (Diff revision 3) > + mStorage->Clear(); > + mClearStorage = false; > + } > + > + if (mMode == MODE_TRRONLY) { > + return false; // might as well try Does TRRONLY attempt to resolve localhost and *.local as well? Worth a comment. ::: netwerk/dns/TRRService.cpp:391 (Diff revision 3) > + } > + } > + return false; > +} > + > +class proxyBlacklist : public Runnable nit: ProxyBlacklist ::: netwerk/dns/nsHostResolver.h:159 (Diff revision 3) > static DnsPriority GetPriority(uint16_t aFlags); > > bool RemoveOrRefresh(); // Mark records currently being resolved as needed > // to resolve again. > + bool IsTRR() { return mTRRUsed; } > + void Complete(); We need a comment for what it does, or a better name for the method. ::: netwerk/dns/nsHostResolver.h:170 (Diff revision 3) > friend class nsHostResolver; > > explicit nsHostRecord(const nsHostKey& key); > mozilla::LinkedList<RefPtr<nsResolveHostCallback>> mCallbacks; > > - bool resolving; /* true if this record is being resolved, which means > + int mResolving; /* counter of outstanding resolving calls */ Let's only add C++ style comments. ::: netwerk/dns/nsHostResolver.h:398 (Diff revision 3) > nsresult Init(); > - nsresult IssueLookup(nsHostRecord *); > + void AssertOnQ(nsHostRecord *, PRCList *); > + mozilla::net::ResolverMode Mode(); > + nsresult NativeLookup(nsHostRecord *); > + nsresult TrrLookup(nsHostRecord *, mozilla::net::TRR *pushedTRR = nullptr); > + nsresult NameLookup(nsHostRecord *); Comment what this one does. ::: netwerk/test/unit/test_trr.js:85 (Diff revision 3) > + prefs.setBoolPref("network.trr.wait-for-portal", portalpref); > + prefs.setBoolPref("network.trr.useGET", getpref); > + prefs.setCharPref("network.trr.confirmationNS", confirmationpref); > +} > + > +function resetPrefs() { Use registerCleanupFunction() instead of calling it from testsDone. If possible, Cu.import Services.jsm and use Services.prefs.clearUserPref() ::: netwerk/test/unit/test_trr.js:144 (Diff revision 3) > + response.finish(); > +} > + > +function test1() > +{ > + num = 1; nit: several trailing whitespace instances in this file. ::: netwerk/test/unit/test_trr.js:150 (Diff revision 3) > + dump("execute doTest1 - basic TRR\n"); > + testPath = testPathBase + num; > + httpserver.registerPathHandler(testPath, "handler" + num); > + var channel = setupChannel(testPath); > + flags = 0; > + channel.asyncOpen2(new ChannelListener(eval("completeTest" + num), is the eval needed? ::: testing/xpcshell/moz-http2/moz-http2.js:538 (Diff revision 3) > + res.setHeader('Content-Type', 'application/dns-udpwireformat'); > + res.setHeader('Content-Length', content.length); > + res.writeHead(200); > + res.write(content); > + res.end(""); > + return; nit: trailing whitespace

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223916 > the short form of ct maybe (probably?) breaks existing test partner.. test it out plz. if that's true just leave the long form for now (its still legal) The short form 'ct' works with our test partner as well as other current public servers

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review223998 > I have no _big_ problem with this being hours instead of seconds, but if we make it seconds it will be a lot easier to write automated tests for it. Fair point. I'll change. > We should either assert that aTRR is 1,2,5 or 28 or even better, add an optional argument to the other constructor: AddrInfo(const char *host, const char *cname, bool isTRR = false) I'm not 100% convinced this is a good idea since the DNS.cpp code doesn't actually have any knowledge about what TYPEs the TRR code supports or not - it just holds the values without knowing what they're for. I've now added an assert for the numerical values in there. I don't see how an optional argument helps here, the other constructors set mFromTRR(false) (which arguably should use 0 instead). > nit: this code might look nicer if we used mozilla::Tokenizer Agreed, but unless you feel strongly that it has to be there in the first version, I'd rather postpone rewriting the parsing logic to mozilla::Tokenizer to a follow-up patch. > nit: A more descriptive name maybe? Such as PerformLookup, or SendRequest? SendHTTPRequest! > Do we need to worry about endianess? Wouldn't a call to ntohl be enough? I wanted to avoid 32 bit unaligned memory accesses and avoid an ugly cast like this: `uint32_t TTL = ntohl(* reinterpret_cast<uint32_t *>(&mResponse[index]));` ... but speaking of typecasts, I can remove them all from the get16/32bit functions (they're leftovers from an earlier iteration of the TRR code) Or did you have something nicer i mind? > nit: if (mFailed || NS_FAILED ) handle failure. Makes the code easier to follow. Generally, yes, but this method also uses the full-through for errors within that block so reversing the logic doesn't seem to improve the logic in this case! > is the eval needed? This pattern is commonly used in several other test files, so I've copied it. I plan to enable different functions to verify each test function as I extend this file - so I think it useful.

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review224160 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 4) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review224204 getting close. I won't really read the tests until you declare them done (or maybe valentin can do that alone.) ::: netwerk/dns/DNS.cpp:336 (Diff revisions 3 - 4) > : mHostName(nullptr) > , mCanonicalName(nullptr) > , ttl(NO_TTL_DATA) > , mFromTRR(aTRR) > { > + MOZ_ASSERT((aTRR == 1) || (aTRR == 2) || (aTRR == 5) || (aTRR == 28)); I agree with daniel's response here. This class can carry any RR type so it shouldn't assert. ::: netwerk/dns/TRR.cpp:232 (Diff revisions 3 - 4) > NS_LITERAL_CSTRING("POST"), false); > NS_ENSURE_SUCCESS(rv, rv); > } > > // set the *default* response content type > - httpChannel->SetContentType(NS_LITERAL_CSTRING("application/dns-udpwireformat")); > + if (NS_SUCCEEDED(httpChannel->SetContentType(NS_LITERAL_CSTRING("application/dns-udpwireformat"))) && this bit is wrong. setting the default response content type failing shouldn't create a situation where we don't proceed. That's why the error wasn't checked the first time around. its ok if you want to log a fail or something, but its not a hard error. ::: netwerk/dns/TRR.cpp:743 (Diff revisions 3 - 4) > } > // create and populate an TRR AddrInfo instance to pass on to signal that > // this comes from TRR > - AddrInfo *ai = new AddrInfo(mHost.get(), mType); > + nsAutoPtr<AddrInfo> ai(new AddrInfo(mHost.get(), mType)); > > - (void)mHostResolver->CompleteLookup(mRec, NS_ERROR_FAILURE, ai, mPB); > + (void)mHostResolver->CompleteLookup(mRec, NS_ERROR_FAILURE, ai.forget(), mPB); there is really no point in the autoPtr here - its just overhead. ::: netwerk/test/unit/test_trr.js:80 (Diff revisions 3 - 4) > } > > function resetTRRPrefs() { > - prefs.setIntPref("network.trr.mode", modepref); > - prefs.setCharPref("network.trr.uri", uripref); > - prefs.setBoolPref("network.trr.wait-for-portal", portalpref); > + prefs.clearUserPref("network.trr.mode"); > + prefs.clearUserPref("network.trr.uri"); > + prefs.clearUserPref("network.trr.wait-for-portal"); if you're not going to pop prefs back in at the end, you don't need to get and store them at the start. I think clearuserpref is ok here.. I actually prefer push/pop even though its a bit uglier because it allows for the rest of the test harness to have localized these values before running this test. (i.e. it doesn't assume the userpref is cleared before the test runs)

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review224306 Just a few questions that need to be answered, and the issues Patrick mentioned. Overall this looks really good. Great job! I'd like us to have a lot more tests for it, as there are a lot of corner cases and failure modes that we don't cover. I expect we'll have them by the time this is ready for our users. ::: netwerk/dns/TRR.cpp:233 (Diff revision 4) > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + // set the *default* response content type > + if (NS_SUCCEEDED(httpChannel->SetContentType(NS_LITERAL_CSTRING("application/dns-udpwireformat"))) && > + NS_SUCCEEDED(httpChannel->AsyncOpen2(this))) { Does mIdleTimeout deal with connections that stall after being asyncOpened? Native DNS calls have a timer - from my testing a few weeks ago, if you block the connection in a middlebox, it just keeps waiting forever (one hour at least). ::: netwerk/test/unit/test_trr.js:67 (Diff revision 4) > + > + // The moz-http2 cert is for foo.example.com and is signed by CA.cert.der > + // so add that cert to the trust list as a signing cert. // the foo.example.com domain name. > + let certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > + addCertFromFile(certdb, "CA.cert.der", "CTu,u,u"); not sure if this is the case, but do we need to remove the cert after the test? ::: netwerk/test/unit/test_trr.js:124 (Diff revision 4) > +{ > + dump("testDone\n"); > + httpserver.stop(do_test_finished); > +} > + > +function test_setup1() empty method

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review224306 > Does mIdleTimeout deal with connections that stall after being asyncOpened? > Native DNS calls have a timer - from my testing a few weeks ago, if you block the connection in a middlebox, it just keeps waiting forever (one hour at least). Ah yes, thanks for reminding me about that. Yes it really should have a fairly strict idle timeout. I'll leave this open for now. > not sure if this is the case, but do we need to remove the cert after the test? I didn't see any removal done in any of the other tests that add this cert to the db so I don't even know how to do it... =) > empty method This is fixed in my next iteration of test_trr when I'll switch to a 100% DNS-interface instead of bolting on the HTTP requests on top of it.

Some words about the updated patch version I just pushed for another round of reviews. Datareview: Got a + over in bug 1436338 Bugs I found and fixed several bugs since the previous review, which explains some changes that I think you'll spot. TRR Push Receiving DOH push was broken and is now fixed. One of the new tests verify that it works. Also, to get the push to work, Http2Session.cpp was modified with help and advice from Nick - that change needs a special eye. timeout: I added a per-request timeout with a dedicated pref ("request-timeout"), defaulting to 3000 milliseconds. tests: I changed the approach to now only use the DNS API and I've added a bunch of tests. There's a test10 in there that isn't called (yet) that's an embryo for testing confirmationNS but I haven't yet figured out a good way to make it work... These tests fail the "--verify" flag when run with xpcshell tests which makes the 'TV' (test verification) flag it red on try. It seems to be related to the http2 server since I see "connection refused" errors to the TRR requests, but I have not figured out how to A) fix the issue or B) disable the red. Prefs: I modified the "blacklist-duration" to instead use seconds (and I added the "request-timeout" as mentioned above already.) More logging: Yeps, in more places. To make it easier to debug this and understand the decision flow within TRR. I think we might want to trim some of them at a later point when things have been proven to work in real life somewhat.

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225090 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 5) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225176 this is extremely close ::: modules/libpref/init/all.js:91 (Diff revisions 4 - 5) > // after the shutdown has been signalled. Past that time data are never written > // and files are left open given up to the OS to do the cleanup. > pref("browser.cache.max_shutdown_io_lag", 2); > > pref("browser.cache.offline.enable", true); > + There are lots of changes to all.js that I do not believe you mean to make between 4 and 5. revert them. ::: netwerk/dns/TRR.cpp:282 (Diff revisions 4 - 5) > { > FallibleTArray<uint8_t> binary; > - nsresult rv = Base64URLDecode(query, > + bool found_dns = false; > + LOG(("TRR::DohDecodeQuery %s!\n", query.get())); > + > + // extract "dns=" from the query string good :) ::: netwerk/dns/TRRService.cpp:146 (Diff revisions 4 - 5) > MutexAutoLock lock(mLock); > Preferences::GetCString(TRR_PREF("credentials"), mPrivateCred); > } > if (!name || !strcmp(name, TRR_PREF("confirmationNS"))) { > MutexAutoLock lock(mLock); > + nsCString old(mConfirmationNS); autocstring ::: netwerk/dns/TRRService.cpp:148 (Diff revisions 4 - 5) > } > if (!name || !strcmp(name, TRR_PREF("confirmationNS"))) { > MutexAutoLock lock(mLock); > + nsCString old(mConfirmationNS); > Preferences::GetCString(TRR_PREF("confirmationNS"), mConfirmationNS); > + if (name && old.Length() && !mConfirmationNS.Equals(old) && !empty rather than length

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225176 > There are lots of changes to all.js that I do not believe you mean to make between 4 and 5. revert them. I have no idea how they ended up there, I assume I borked up my git mozreview push or something because those other changes don't exist in my local commit! :-O > !empty rather than length I'll make sure to use !IsEmpty() on this and the few other places in the same code using the same pattern.

This new version includes: - s/.Length/!.IsEmpty for string checks - xpschell.ini: the skip-if needs to be *after* the test to skip! - fix and verify network.trr.bootstrapAddress

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225210 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 6) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225218 please get another r+ from valentin when the tests are coplete

I fixed the timeout test case and added a test that gets a CNAME back when expecting an A record. I haven't come up with a good test for shadow mode requests, as I should need to resolve some real names then with the native resolver and since the tests shouldn't access the Internet I haven't figured out a way forward here... I'm listening if someone has an idea on how to do this. I removed the http2session fix from this patch since it has been landed separately in bug 1437513. Also notably: the test_trr.js xpcshell tests will fail in "TV" mode on try until bug 1437529 lands. (they work with --verify for me with that patch applied locally)

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225398 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 7) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225446 The tests look good. I have just a couple of minor questions. ::: netwerk/dns/nsHostResolver.cpp:1790 (Diff revision 7) > RefPtr<nsHostRecord> rec; > AddrInfo *ai = nullptr; > > - while (rec || resolver->GetHostToLookup(getter_AddRefs(rec))) { > + do { > + if (!rec) { > + nsHostRecord *tmpRec = nullptr; nit: use RefPtr for tmpRec ::: netwerk/test/unit/test_trr.js:87 (Diff revision 7) > +function testsDone() > +{ > + do_test_finished(); > +} > + > +var test_answer="127.0.0.1"; nit: spaces around = Several instances in this file. ::: netwerk/test/unit/test_trr.js:200 (Diff revision 7) > + // cache only. Set back the URI to a path that fails. > + prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/404"); > + dump("test5b - resolve push.example.now please\n"); > + test_answer="2018::2018"; > + listen = dns.asyncResolve("push.example.com", 0, listenerFine, mainThread, defaultOriginAttributes); > + do_test_finished(); Is there a reason for the extra test_pending/finished in this test? ::: netwerk/test/unit/test_trr.js:270 (Diff revision 7) > + prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns-cname"); > + prefs.setIntPref("network.trr.request-timeout", 10); > + listen = dns.asyncResolve("test12.example.com", 0, listenerFails, mainThread, defaultOriginAttributes); > +} > + > +function test11post() function doesn't seem to be used. ::: testing/xpcshell/moz-http2/moz-http2.js:539 (Diff revision 7) > + // get.example.com has A entry 1.2.3.4 > + var content= new Buffer("00000100000100010000000003676574076578616D706C6503636F6D0000010001C00C0001000100000037000401020304", "hex"); > + res.setHeader('Content-Type', 'application/dns-udpwireformat'); > + res.setHeader('Content-Length', content.length); > + //res.setHeader('X-path', u.path); > + //res.setHeader('X-pathname', u.pathname); Are the commented lines useful? ::: testing/xpcshell/moz-http2/moz-http2.js:567 (Diff revision 7) > + res.write(content); > + res.end(""); > + return; > + } > + else if (u.pathname === '/dns-750ms') { > + // it's just meant to be this slow - the test doesn't care about the actual response Do we need to make this slow by calling runlater?

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225446 > Is there a reason for the extra test_pending/finished in this test? Yes. since 'test5b' is invoked by a timer from within test5 it doesn't get the `do_test_*` set called from the parent function. > Do we need to make this slow by calling runlater? I started this out using runlater to actually have the http2 server respond correctly but with 750ms delay - but that turned out troublesome since the http2 server doesn't like when we exit all the tests before those 750ms have expired. Simply not answering to the request - like it does now - still excercises the timeout from Firefox's side (since it times out already after 10 milliseconds) and yet doesn't cause any problems with the test server that I've noticed. I therefore consider that good enough for this test.

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review225472 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 8) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Changes: - The new pref called "network.dns.native-is-localhost" which if set will make the native resolver *actually* resolve 'localhost' no matter what the host name argument asks it to resolve. This makes it easy for test cases to use whatever host names they need to without ever hitting the internet and reliably return a known IP for the host name. - I added CNAME "chasing" support to TRR to make it more useful when browsing the internet. With this, I see virtually no use of the TRR blacklist, while it was very frequently used when CNAMEs were not handled. The code currently allows 64 levels of CNAMEs and will report error if more are used. - I added a bunch of new tests, both for the previous functionality but also now for the CNAME support - I fixed a linked list memory leak. - I cancel the timers slightly more aggressively than before, as previously they were only cancelled in the destructor but since it could take a while until called I decided to cancel them off as soon as we have finished the TRR handling. We could potentially end up with a fair amount of simultaneous timers running...

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226262 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 9) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Grr. It seems resolving 'localhost' on the try servers occasionally returns "::1", which then breaks some assumptions in my tests! :-O

My try runs (with a little patch for the localhost thing mentioned above) consistently get reds on "Windows 10 x64 cc" and the tests called "R-e10s1" and "R-e10s2". I'm *suspecting* this could be the same reason why 'test_http2.js' has a skip-if for "(os == 'win' && ccov)" and a reference to Bug 1423667 (in xpcshell.ini). But the logs don't say anything of importance that I can see, so this leaves me utterly confused and a bit lost. Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5782b722a7a310c7856fcd54ef43244fb65c4121

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226480

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226484 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 10) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226482 This looks pretty good. ::: netwerk/dns/TRR.cpp:882 (Diff revisions 8 - 10) > rv = httpChannel->GetResponseStatus(&httpStatus); > if (NS_SUCCEEDED(rv) && httpStatus == 200) { > // decode body and create an AddrInfo struct for the response > - rv = DohDecode(mType); > + rv = DohDecode(); > > if (NS_SUCCEEDED(rv)) { We are starting to have too many levels of indentation here. Can we do something about it? Maybe even split into another method? ::: netwerk/dns/TRR.cpp:256 (Diff revision 10) > + // set the *default* response content type > + if (NS_FAILED(httpChannel->SetContentType(NS_LITERAL_CSTRING("application/dns-udpwireformat")))) { > + LOG(("TRR::SendHTTPRequest: couldn't set content-type!\n")); > + } > + if (NS_SUCCEEDED(httpChannel->AsyncOpen2(this))) { > + NS_NewTimerWithObserver(getter_AddRefs(mTimeout), this, Any reason we're using NS_NewTimerWithObserver instead of NS_NewTimerWithCallback? It has a cleaner API IMO.

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226546 ::: modules/libpref/init/all.js:2092 (Diff revisions 6 - 10) > > // Get TTL; not supported on all platforms; nop on the unsupported ones. > pref("network.dns.get-ttl", true); > > +// Makes the native resolver resolve IPv4 "localhost" instead of the actual > +// given name indicate in the comment that this is to support testing ::: netwerk/dns/GetAddrInfo.cpp:338 (Diff revisions 6 - 10) > if (aGetTtl) { > aFlags |= nsHostResolver::RES_CANON_NAME; > } > #endif > > + if (sNativeIsLocalhost) { sNativeIsLocalhost is a global, so its prefix should be g not s (s is generally static) ::: netwerk/dns/TRR.cpp:628 (Diff revisions 6 - 10) > case TRRTYPE_CNAME: > - break; > + if (mCname.IsEmpty()) { > + uint8_t clength = 0; > + unsigned int cindex = index; > + do { > + clength = static_cast<uint8_t>(mResponse[cindex]); I think cindex could be out of bounds ::: netwerk/dns/TRR.cpp:631 (Diff revisions 6 - 10) > + unsigned int cindex = index; > + do { > + clength = static_cast<uint8_t>(mResponse[cindex]); > + if ((clength & 0xc0) == 0xc0) { > + // name pointer, get the new offset (14 bits) > + uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1]; cindex + 1 could be out of bounds ::: netwerk/dns/TRR.cpp:632 (Diff revisions 6 - 10) > + do { > + clength = static_cast<uint8_t>(mResponse[cindex]); > + if ((clength & 0xc0) == 0xc0) { > + // name pointer, get the new offset (14 bits) > + uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1]; > + if (newpos >= mBodySize) { I think newpos is a label length.. it needs to be considered against index for the bounds check, right? ::: netwerk/dns/TRR.cpp:636 (Diff revisions 6 - 10) > + uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1]; > + if (newpos >= mBodySize) { > + LOG(("TRR: bad cname packet\n")); > + return NS_ERROR_ILLEGAL_VALUE; > + } > + cindex = newpos; this is weird. cindex the first time through the look is based on index, which is an offset from the beginning of the dns record. but here it is being assigned a label length (not an offset of the record) ::: netwerk/dns/TRR.cpp:645 (Diff revisions 6 - 10) > + } > + if (clength) { > + if (!mCname.IsEmpty()) { > + mCname.Append("."); > + } > + mCname.Append((const char *)(&mResponse[cindex]), clength); I'm not sure this is bounds checked in all cases

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226546 > I think cindex could be out of bounds Ack. Adding several bounds checks in there... > I think newpos is a label length.. it needs to be considered against index for the bounds check, right? No, 'newpos' is the new position where to continue reading the host name from; where the next label starts. At that index there will be a length. I'll add a comment to the code about it. > this is weird. cindex the first time through the look is based on index, which is an offset from the beginning of the dns record. but here it is being assigned a label length (not an offset of the record) No, cindex is the index within the body where it reads cname data from. In this case it has gotten a new index position and moves to read from there. Note the 'continue' following this assignment.

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226594 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 11) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226596 ::: netwerk/dns/TRR.cpp:633 (Diff revisions 10 - 11) > + return NS_ERROR_ILLEGAL_VALUE; > + } > clength = static_cast<uint8_t>(mResponse[cindex]); > if ((clength & 0xc0) == 0xc0) { > // name pointer, get the new offset (14 bits) > - uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1]; > + if (cindex >= (mBodySize-1)) { its just a nit, but this is better as cindex + 1 >= mBodySize because it makes clear that you are deref'ing against cindex+1 below ::: netwerk/dns/TRR.cpp:651 (Diff revisions 10 - 11) > } > if (clength) { > if (!mCname.IsEmpty()) { > mCname.Append("."); > } > + if ((cindex + clength) >= mBodySize) { gt, not gte right?

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226596 > gt, not gte right? Oh, correct!

Pushed by daniel@haxx.se: https://hg.mozilla.org/integration/autoland/rev/6776d69d2f03 introducing TRR (DOH); r=mcmanus,valentin

Comment on attachment 8947406 [details] bug 1434852 - introducing TRR (DOH); https://reviewboard.mozilla.org/r/217134/#review226610 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/protocol/http/HttpBaseChannel.cpp:3606 (Diff revision 12) > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetAllowAltSvc(mAllowAltSvc); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > rv = httpInternal->SetBeConservative(mBeConservative); > MOZ_ASSERT(NS_SUCCEEDED(rv)); > + rv = httpInternal->SetTrr(mTRR); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

FYI, bug 1458168: Could Mozilla operate a DNS-over-HTTPS server?