Overview

If DNS over HTTPS (DoH) is enabled for Fx, sometimes IPv6 hosts lose priority and IPv4 hosts are chosen for HTTP(S) connections on their behalf.
Since the spec of IPv6 expects that IPv6 supersedes IPv4, this is an unexpected behaviour.

Prerequisites

The repro environment needs to have a v4/v6 dual-stack connection.

Repro steps

1. Enable DoH for Firefox as documented here: https://support.mozilla.org/en-US/kb/firefox-dns-over-https#w_manually-enabling-and-disabling-dns-over-https

2. Access to a webpage hosted over both IPv4 and IPv6, e.g. https://makotom.net/

Expected Behaviour

Every access should be made over IPv6.
Log collected with about:networking#logging will be attached as doh-v6-master.moz_log.

Actual Behaviour

Sometimes accesses are made over IPv4.
Log collected with about:networking#logging is attached as doh-v4-master.moz_log.

Analysis

DoH in Fx behaves as follows:

1. nsHostResolver tries to fetch both RRSets for IPv4 and IPv6 Even though it tries to do things concurrently, it actually issues queries sequentially, firstly for IPv4 and then next for IPv6 after that.
   Ref 1: https://github.com/mozilla/gecko-dev/blob/6fce0d1/netwerk/dns/nsHostResolver.cpp#L1256
   Ref 2: https://github.com/mozilla/gecko-dev/blob/6fce0d1/netwerk/dns/nsHostResolver.cpp#L1290

2. At the final stage of the name resolution, nsHostResolver::CompleteLookup appends the RRSets, which were received earlier, to the list of the RRSets which were received later. Namely, in the address list, RRSets received later precedes the RRSets received earlier.
   Ref 1: https://github.com/mozilla/gecko-dev/blob/6fce0d1/netwerk/dns/nsHostResolver.cpp#L1821
   Ref 2: https://github.com/mozilla/gecko-dev/blob/6fce0d1/netwerk/dns/nsHostResolver.cpp#L1632

3. The list is consumed from the head to the tail (like a queue, not a stack). Hence the RRSets received in the last response supersedes others and will be used for HTTP(S) connections for first.
   Ref: https://github.com/mozilla/gecko-dev/blob/6fce0d1/netwerk/dns/nsHostResolver.cpp#L1910

Therefore, if RRSets for IPv6 arrive before RRSets for IPv4, IPv4 addresses will come first in the list of the resolved addresses, preceding IPv6 addresses.
Thankfully this does not happen often because of the order of queries in the Step 1. This is why the issue appears intermittently.

Note that there is no option to prioritize IPv6 addresses over IPv4 addresses - although there is an option to wait for IPv4 RRSets (wait-for-A-and-AAAA), which effectively prioritizes IPv4.

This also affects Firefox ESR 68.7.0 - using network.trr.mode=3, most DNS responses are IPv4, test-ipv6.com reports "Your browser has real working IPv6 address - but is avoiding using it.". Tried with network.trr.early-AAAA false and true, I couldn't see any difference.

Hello @valentin

I've made a small patch that I've tested. Could you please review it?

I tested it by testing the exact opposite:
adding a '!' to if ((element = rrfrom->mAddresses.getFirst())) { so that IPv4 always comes before
IPv6 which did happen when I checked through about:networking for google.com

diff -r 73ee67c9a90d netwerk/dns/nsHostResolver.cpp
--- a/netwerk/dns/nsHostResolver.cpp Tue Jul 21 11:42:44 2020 +0000
+++ b/netwerk/dns/nsHostResolver.cpp Wed Jul 22 10:59:52 2020 +0000
@@ -1757,9 +1757,21 @@
return NS_ERROR_NULL_POINTER;
}
NetAddrElement* element;

• while ((element = rrfrom->mAddresses.getFirst())) {
• element->remove(); // unlist from old
• rrto->AddAddress(element); // enlist on new

• // We assume that each of the arguments are all-IPv4 or all-IPv6
• // hence judging by the first element
• if ((element = rrfrom->mAddresses.getFirst())) {
• if ((element->mAddress.raw.family == PR_AF_INET6)) {

•  // rrfrom has IPv6 so it should be first
  

•  do {
  

•    element->remove();
  

•    rrto->mAddresses.insertFront(element);
  

•  } while ((element = rrfrom->mAddresses.getFirst()));
  

• } else {

•  do {
  

•    element->remove();
  

•    rrto->mAddresses.insertBack(element);
  

•  } while ((element = rrfrom->mAddresses.getFirst()));
  

• }
  }
  return NS_OK;
  }

Oh no formatting

diff -r 73ee67c9a90d netwerk/dns/nsHostResolver.cpp
--- a/netwerk/dns/nsHostResolver.cpp	Tue Jul 21 11:42:44 2020 +0000
+++ b/netwerk/dns/nsHostResolver.cpp	Wed Jul 22 10:59:52 2020 +0000
@@ -1757,9 +1757,21 @@
     return NS_ERROR_NULL_POINTER;
   }
   NetAddrElement* element;
-  while ((element = rrfrom->mAddresses.getFirst())) {
-    element->remove();          // unlist from old
-    rrto->AddAddress(element);  // enlist on new
+  // We assume that each of the arguments are all-IPv4 or all-IPv6
+  // hence judging by the first element
+  if ((element = rrfrom->mAddresses.getFirst())) {
+    if ((element->mAddress.raw.family == PR_AF_INET6)) {
+      // rrfrom has IPv6 so it should be first
+      do {
+        element->remove();
+        rrto->mAddresses.insertFront(element);
+      } while ((element = rrfrom->mAddresses.getFirst()));
+    } else {
+      do {
+        element->remove();
+        rrto->mAddresses.insertBack(element);
+      } while ((element = rrfrom->mAddresses.getFirst()));
+    }
   }
   return NS_OK;
 }

Needinfo no longer needed

https://hg.mozilla.org/mozilla-central/rev/0a68cf3c2835