Bug 1680605 - Make https upgrade with https rr properly r=necko-reviewers,dragana

Differential Revision: https://phabricator.services.mozilla.com/D98687
2021-01-05 19:30:18 +00:00 · 2021-01-05 19:30:18 +00:00 · 8b760c522f
--- a/netwerk/base/nsDNSPrefetch.cpp
+++ b/netwerk/base/nsDNSPrefetch.cpp
@ -80,7 +80,41 @@ nsresult nsDNSPrefetch::PrefetchHigh(bool refreshDNS) {
  return Prefetch(refreshDNS ? nsIDNSService::RESOLVE_BYPASS_CACHE : 0);
 }

-nsresult nsDNSPrefetch::FetchHTTPSSVC(bool aRefreshDNS) {
+namespace {
+
+class HTTPSRRListener final : public nsIDNSListener {
+ public:
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSIDNSLISTENER
+
+  explicit HTTPSRRListener(
+      std::function<void(nsIDNSHTTPSSVCRecord*)>&& aCallback)
+      : mResultCallback(std::move(aCallback)) {}
+
+ private:
+  ~HTTPSRRListener() = default;
+  std::function<void(nsIDNSHTTPSSVCRecord*)> mResultCallback;
+};
+
+NS_IMPL_ISUPPORTS(HTTPSRRListener, nsIDNSListener)
+
+NS_IMETHODIMP
+HTTPSRRListener::OnLookupComplete(nsICancelable* aRequest, nsIDNSRecord* aRec,
+                                  nsresult aStatus) {
+  if (NS_FAILED(aStatus)) {
+    mResultCallback(nullptr);
+    return NS_OK;
+  }
+
+  nsCOMPtr<nsIDNSHTTPSSVCRecord> httpsRecord = do_QueryInterface(aRec);
+  mResultCallback(httpsRecord);
+  return NS_OK;
+}
+
+};  // namespace
+
+nsresult nsDNSPrefetch::FetchHTTPSSVC(
+    bool aRefreshDNS, std::function<void(nsIDNSHTTPSSVCRecord*)>&& aCallback) {
  if (!sDNSService) {
    return NS_ERROR_NOT_AVAILABLE;
  }
@ -93,8 +127,9 @@ nsresult nsDNSPrefetch::FetchHTTPSSVC(bool aRefreshDNS) {
  }

  nsCOMPtr<nsICancelable> tmpOutstanding;
+  nsCOMPtr<nsIDNSListener> listener = new HTTPSRRListener(std::move(aCallback));
  return sDNSService->AsyncResolveNative(
-      mHostname, nsIDNSService::RESOLVE_TYPE_HTTPSSVC, flags, nullptr, this,
+      mHostname, nsIDNSService::RESOLVE_TYPE_HTTPSSVC, flags, nullptr, listener,
      target, mOriginAttributes, getter_AddRefs(tmpOutstanding));
 }

@ -103,9 +138,7 @@ NS_IMPL_ISUPPORTS(nsDNSPrefetch, nsIDNSListener)
 NS_IMETHODIMP
 nsDNSPrefetch::OnLookupComplete(nsICancelable* request, nsIDNSRecord* rec,
                                nsresult status) {
-  nsCOMPtr<nsIDNSHTTPSSVCRecord> httpsRecord = do_QueryInterface(rec);
-
-  if (mStoreTiming && !httpsRecord) {
+  if (mStoreTiming) {
    mEndTimestamp = mozilla::TimeStamp::Now();
  }
  nsCOMPtr<nsIDNSListener> listener = do_QueryReferent(mListener);
--- a/netwerk/base/nsDNSPrefetch.h
+++ b/netwerk/base/nsDNSPrefetch.h
@ -6,6 +6,8 @@
 #ifndef nsDNSPrefetch_h___
 #define nsDNSPrefetch_h___

+#include <functional>
+
 #include "nsIWeakReferenceUtils.h"
 #include "nsString.h"
 #include "mozilla/TimeStamp.h"
@ -44,7 +46,8 @@ class nsDNSPrefetch final : public nsIDNSListener {
  nsresult PrefetchMedium(bool refreshDNS = false);
  nsresult PrefetchLow(bool refreshDNS = false);

-  nsresult FetchHTTPSSVC(bool aRefreshDNS);
+  nsresult FetchHTTPSSVC(
+      bool aRefreshDNS, std::function<void(nsIDNSHTTPSSVCRecord*)>&& aCallback);

 private:
  nsCString mHostname;
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@ -572,16 +572,7 @@ nsresult nsHttpChannel::OnBeforeConnect() {
      auto resultCallback = [self(self)](bool aResult, nsresult aStatus) {
        MOZ_ASSERT(NS_IsMainThread());

-        // We need to wait for HTTPSSVC record if there is no AltSvc or HSTS
-        // upgrade for this request.
-        if (!aResult && NS_SUCCEEDED(aStatus) && self->LoadUseHTTPSSVC()) {
-          LOG(("nsHttpChannel Wait for HTTPSSVC record [this=%p]\n",
-               self.get()));
-          self->StoreWaitHTTPSSVCRecord(true);
-          return;
-        }
-
-        nsresult rv = self->ContinueOnBeforeConnect(aResult, aStatus);
+        nsresult rv = self->MaybeUseHTTPSRRForUpgrade(aResult, aStatus);
        if (NS_FAILED(rv)) {
          self->CloseCacheEntry(false);
          Unused << self->AsyncAbort(rv);
@ -618,7 +609,30 @@ nsresult nsHttpChannel::OnBeforeConnect() {
    }
  }

-  return ContinueOnBeforeConnect(shouldUpgrade, NS_OK);
+  return MaybeUseHTTPSRRForUpgrade(shouldUpgrade, NS_OK);
+}
+
+nsresult nsHttpChannel::MaybeUseHTTPSRRForUpgrade(bool aShouldUpgrade,
+                                                  nsresult aStatus) {
+  if (NS_FAILED(aStatus)) {
+    return aStatus;
+  }
+
+  if (mURI->SchemeIs("https") || aShouldUpgrade || !LoadUseHTTPSSVC()) {
+    return ContinueOnBeforeConnect(aShouldUpgrade, aStatus);
+  }
+
+  if (mHTTPSSVCRecord) {
+    LOG(("nsHttpChannel::MaybeUseHTTPSRRForUpgrade [%p] already got HTTPS RR",
+         this));
+    StoreWaitHTTPSSVCRecord(false);
+    return ContinueOnBeforeConnect(true, aStatus);
+  }
+
+  LOG(("nsHttpChannel::MaybeUseHTTPSRRForUpgrade [%p] wait for HTTPS RR",
+       this));
+  StoreWaitHTTPSSVCRecord(true);
+  return NS_OK;
 }

 nsresult nsHttpChannel::ContinueOnBeforeConnect(bool aShouldUpgrade,
@ -628,6 +642,8 @@ nsresult nsHttpChannel::ContinueOnBeforeConnect(bool aShouldUpgrade,
       "[this=%p aShouldUpgrade=%d rv=%" PRIx32 "]\n",
       this, aShouldUpgrade, static_cast<uint32_t>(aStatus)));

+  MOZ_ASSERT(!LoadWaitHTTPSSVCRecord());
+
  if (NS_FAILED(aStatus)) {
    return aStatus;
  }
@ -6817,6 +6833,12 @@ nsresult nsHttpChannel::BeginConnect() {
 }

 nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
+  bool httpssvcQueried = false;
+  // If https rr is not queried sucessfully, we have to reset mUseHTTPSSVC to
+  // false. Otherwise, this channel may wait https rr forever.
+  auto resetUsHTTPSSVC =
+      MakeScopeExit([&] { StoreUseHTTPSSVC(httpssvcQueried); });
+
  // Start a DNS lookup very early in case the real open is queued the DNS can
  // happen in parallel. Do not do so in the presence of an HTTP proxy as
  // all lookups other than for the proxy itself are done by the proxy.
@ -6867,10 +6889,22 @@ nsresult nsHttpChannel::MaybeStartDNSPrefetch() {

    if (LoadUseHTTPSSVC() ||
        gHttpHandler->UseHTTPSRRForSpeculativeConnection()) {
-      rv = mDNSPrefetch->FetchHTTPSSVC(mCaps & NS_HTTP_REFRESH_DNS);
+      nsWeakPtr weakPtrThis(
+          do_GetWeakReference(static_cast<nsIHttpChannel*>(this)));
+      rv = mDNSPrefetch->FetchHTTPSSVC(
+          mCaps & NS_HTTP_REFRESH_DNS,
+          [weakPtrThis](nsIDNSHTTPSSVCRecord* aRecord) {
+            nsCOMPtr<nsIHttpChannel> channel = do_QueryReferent(weakPtrThis);
+            RefPtr<nsHttpChannel> httpChannelImpl = do_QueryObject(channel);
+            if (httpChannelImpl) {
+              httpChannelImpl->OnHTTPSRRAvailable(aRecord);
+            }
+          });
      if (NS_FAILED(rv)) {
        LOG(("  FetchHTTPSSVC failed with 0x%08" PRIx32,
             static_cast<uint32_t>(rv)));
+      } else {
+        httpssvcQueried = true;
      }
    }
  }
@ -9007,52 +9041,72 @@ nsHttpChannel::OnLookupComplete(nsICancelable* request, nsIDNSRecord* rec,
                                nsresult status) {
  MOZ_ASSERT(NS_IsMainThread(), "Expecting DNS callback on main thread.");

-  nsCOMPtr<nsIDNSHTTPSSVCRecord> httpSSVCRecord = do_QueryInterface(rec);
  LOG(
      ("nsHttpChannel::OnLookupComplete [this=%p] prefetch complete%s: "
-       "%s status[0x%" PRIx32 "], isHTTPSSVC=%d\n",
+       "%s status[0x%" PRIx32 "]\n",
       this, mCaps & NS_HTTP_REFRESH_DNS ? ", refresh requested" : "",
       NS_SUCCEEDED(status) ? "success" : "failure",
-       static_cast<uint32_t>(status), !!httpSSVCRecord));
+       static_cast<uint32_t>(status)));

-  if (!httpSSVCRecord) {
-    // Unset DNS cache refresh if it was requested,
-    if (mCaps & NS_HTTP_REFRESH_DNS) {
-      mCaps &= ~NS_HTTP_REFRESH_DNS;
-      if (mTransaction) {
-        mTransaction->SetDNSWasRefreshed();
-      }
+  // We no longer need the dns prefetch object. Note: mDNSPrefetch could be
+  // validly null if OnStopRequest has already been called.
+  // We only need the domainLookup timestamps when not loading from cache
+  if (mDNSPrefetch && mDNSPrefetch->TimingsValid() && mTransaction) {
+    TimeStamp connectStart = mTransaction->GetConnectStart();
+    TimeStamp requestStart = mTransaction->GetRequestStart();
+    // We only set the domainLookup timestamps if we're not using a
+    // persistent connection.
+    if (requestStart.IsNull() && connectStart.IsNull()) {
+      mTransaction->SetDomainLookupStart(mDNSPrefetch->StartTimestamp());
+      mTransaction->SetDomainLookupEnd(mDNSPrefetch->EndTimestamp());
    }
-
-    if (!mDNSBlockingPromise.IsEmpty()) {
-      if (NS_SUCCEEDED(status)) {
-        nsCOMPtr<nsIDNSRecord> record(rec);
-        mDNSBlockingPromise.Resolve(record, __func__);
-      } else {
-        mDNSBlockingPromise.Reject(status, __func__);
-      }
-    }
-
-    return NS_OK;
  }
+  mDNSPrefetch = nullptr;
+
+  // Unset DNS cache refresh if it was requested,
+  if (mCaps & NS_HTTP_REFRESH_DNS) {
+    mCaps &= ~NS_HTTP_REFRESH_DNS;
+    if (mTransaction) {
+      mTransaction->SetDNSWasRefreshed();
+    }
+  }
+
+  if (!mDNSBlockingPromise.IsEmpty()) {
+    if (NS_SUCCEEDED(status)) {
+      nsCOMPtr<nsIDNSRecord> record(rec);
+      mDNSBlockingPromise.Resolve(record, __func__);
+    } else {
+      mDNSBlockingPromise.Reject(status, __func__);
+    }
+  }
+
+  return NS_OK;
+}
+
+void nsHttpChannel::OnHTTPSRRAvailable(nsIDNSHTTPSSVCRecord* aRecord) {
+  MOZ_ASSERT(NS_IsMainThread(), "Expecting DNS callback on main thread.");
+
+  LOG(("nsHttpChannel::OnHTTPSRRAvailable [this=%p, aRecord=%p]\n", this,
+       aRecord));
+  // This record will be used in the new redirect channel.
+  MOZ_ASSERT(!mHTTPSSVCRecord);
+  mHTTPSSVCRecord = aRecord;

  if (LoadWaitHTTPSSVCRecord()) {
    MOZ_ASSERT(mURI->SchemeIs("http"));
-    MOZ_ASSERT(!mHTTPSSVCRecord);

-    // This record will be used in the new redirect channel.
-    mHTTPSSVCRecord = httpSSVCRecord;
-    nsresult rv = ContinueOnBeforeConnect(true, status);
+    StoreWaitHTTPSSVCRecord(false);
+    nsresult rv = ContinueOnBeforeConnect(!!mHTTPSSVCRecord, mStatus);
    if (NS_FAILED(rv)) {
      CloseCacheEntry(false);
      Unused << AsyncAbort(rv);
    }
  } else {
    // This channel is not canceled and the transaction is not created.
-    if (NS_SUCCEEDED(mStatus) && !mTransaction &&
+    if (mHTTPSSVCRecord && NS_SUCCEEDED(mStatus) && !mTransaction &&
        (mFirstResponseSource != RESPONSE_FROM_CACHE)) {
      bool hasIPAddress = false;
-      Unused << httpSSVCRecord->GetHasIPAddresses(&hasIPAddress);
+      Unused << mHTTPSSVCRecord->GetHasIPAddresses(&hasIPAddress);
      Telemetry::Accumulate(Telemetry::DNS_HTTPSSVC_RECORD_RECEIVING_STAGE,
                            hasIPAddress
                                ? HTTPSSVC_WITH_IPHINT_RECEIVED_STAGE_0
@ -9060,8 +9114,6 @@ nsHttpChannel::OnLookupComplete(nsICancelable* request, nsIDNSRecord* rec,
      StoreHTTPSSVCTelemetryReported(true);
    }
  }
-
-  return NS_OK;
 }

 //-----------------------------------------------------------------------------
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@ -334,6 +334,8 @@ class nsHttpChannel final : public HttpBaseChannel,
  [[nodiscard]] nsresult OnBeforeConnect();
  [[nodiscard]] nsresult ContinueOnBeforeConnect(bool aShouldUpgrade,
                                                 nsresult aStatus);
+  nsresult MaybeUseHTTPSRRForUpgrade(bool aShouldUpgrade, nsresult aStatus);
+  void OnHTTPSRRAvailable(nsIDNSHTTPSSVCRecord* aRecord);
  void OnBeforeConnectContinue();
  [[nodiscard]] nsresult Connect();
  void SpeculativeConnect();
--- a/netwerk/test/unit/test_httpssvc_https_upgrade.js
+++ b/netwerk/test/unit/test_httpssvc_https_upgrade.js
@ -0,0 +1,213 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
+
+let prefs;
+let h2Port;
+let listen;
+
+const dns = Cc["@mozilla.org/network/dns-service;1"].getService(
+  Ci.nsIDNSService
+);
+const certOverrideService = Cc[
+  "@mozilla.org/security/certoverride;1"
+].getService(Ci.nsICertOverrideService);
+const threadManager = Cc["@mozilla.org/thread-manager;1"].getService(
+  Ci.nsIThreadManager
+);
+const mainThread = threadManager.currentThread;
+
+const defaultOriginAttributes = {};
+
+function setup() {
+  let env = Cc["@mozilla.org/process/environment;1"].getService(
+    Ci.nsIEnvironment
+  );
+  h2Port = env.get("MOZHTTP2_PORT");
+  Assert.notEqual(h2Port, null);
+  Assert.notEqual(h2Port, "");
+
+  // Set to allow the cert presented by our H2 server
+  do_get_profile();
+  prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
+
+  prefs.setBoolPref("network.http.spdy.enabled", true);
+  prefs.setBoolPref("network.http.spdy.enabled.http2", true);
+  // the TRR server is on 127.0.0.1
+  prefs.setCharPref("network.trr.bootstrapAddress", "127.0.0.1");
+
+  // make all native resolve calls "secretly" resolve localhost instead
+  prefs.setBoolPref("network.dns.native-is-localhost", true);
+
+  // 0 - off, 1 - race, 2 TRR first, 3 TRR only, 4 shadow
+  prefs.setIntPref("network.trr.mode", 2); // TRR first
+  prefs.setBoolPref("network.trr.wait-for-portal", false);
+  // don't confirm that TRR is working, just go!
+  prefs.setCharPref("network.trr.confirmationNS", "skip");
+
+  // So we can change the pref without clearing the cache to check a pushed
+  // record with a TRR path that fails.
+  Services.prefs.setBoolPref("network.trr.clear-cache-on-pref-change", false);
+
+  Services.prefs.setBoolPref("network.dns.upgrade_with_https_rr", true);
+  Services.prefs.setBoolPref("network.dns.use_https_rr_as_altsvc", true);
+
+  Services.prefs.setCharPref(
+    "network.trr.uri",
+    "https://foo.example.com:" + h2Port + "/httpssvc_as_altsvc"
+  );
+
+  // The moz-http2 cert is for foo.example.com and is signed by http2-ca.pem
+  // so add that cert to the trust list as a signing cert.  // the foo.example.com domain name.
+  const certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(
+    Ci.nsIX509CertDB
+  );
+  addCertFromFile(certdb, "http2-ca.pem", "CTu,u,u");
+}
+
+setup();
+registerCleanupFunction(() => {
+  prefs.clearUserPref("network.http.spdy.enabled");
+  prefs.clearUserPref("network.http.spdy.enabled.http2");
+  prefs.clearUserPref("network.dns.localDomains");
+  prefs.clearUserPref("network.dns.native-is-localhost");
+  prefs.clearUserPref("network.trr.mode");
+  prefs.clearUserPref("network.trr.uri");
+  prefs.clearUserPref("network.trr.credentials");
+  prefs.clearUserPref("network.trr.wait-for-portal");
+  prefs.clearUserPref("network.trr.allow-rfc1918");
+  prefs.clearUserPref("network.trr.useGET");
+  prefs.clearUserPref("network.trr.confirmationNS");
+  prefs.clearUserPref("network.trr.bootstrapAddress");
+  prefs.clearUserPref("network.trr.request-timeout");
+  prefs.clearUserPref("network.trr.clear-cache-on-pref-change");
+  prefs.clearUserPref("network.dns.upgrade_with_https_rr");
+  prefs.clearUserPref("network.dns.use_https_rr_as_altsvc");
+});
+
+class DNSListener {
+  constructor() {
+    this.promise = new Promise(resolve => {
+      this.resolve = resolve;
+    });
+  }
+  onLookupComplete(inRequest, inRecord, inStatus) {
+    this.resolve([inRequest, inRecord, inStatus]);
+  }
+  // So we can await this as a promise.
+  then() {
+    return this.promise.then.apply(this.promise, arguments);
+  }
+}
+
+DNSListener.prototype.QueryInterface = ChromeUtils.generateQI([
+  "nsIDNSListener",
+]);
+
+function makeChan(url) {
+  let chan = NetUtil.newChannel({
+    uri: url,
+    loadUsingSystemPrincipal: true,
+  }).QueryInterface(Ci.nsIHttpChannel);
+  return chan;
+}
+
+function channelOpenPromise(chan, flags) {
+  return new Promise(resolve => {
+    function finish(req, buffer) {
+      certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
+        false
+      );
+      resolve([req, buffer]);
+    }
+    certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
+      true
+    );
+    chan.asyncOpen(new ChannelListener(finish, null, flags));
+  });
+}
+
+class EventSinkListener {
+  getInterface(iid) {
+    if (iid.equals(Ci.nsIChannelEventSink)) {
+      return this;
+    }
+  }
+  asyncOnChannelRedirect(oldChan, newChan, flags, callback) {
+    Assert.equal(oldChan.URI.hostPort, newChan.URI.hostPort);
+    Assert.equal(oldChan.URI.scheme, "http");
+    Assert.equal(newChan.URI.scheme, "https");
+    callback.onRedirectVerifyCallback(Cr.NS_OK);
+  }
+}
+
+EventSinkListener.prototype.QueryInterface = ChromeUtils.generateQI([
+  "nsIInterfaceRequestor",
+  "nsIChannelEventSink",
+]);
+
+// Test if the request is upgraded to https with a HTTPSSVC record.
+add_task(async function testUseHTTPSSVCAsHSTS() {
+  dns.clearCache(true);
+
+  let dnsListener = new DNSListener();
+
+  // Do DNS resolution before creating the channel, so the HTTPSSVC record will
+  // be resolved from the cache.
+  let request = dns.asyncResolve(
+    "test.httpssvc.com",
+    dns.RESOLVE_TYPE_HTTPSSVC,
+    0,
+    null, // resolverInfo
+    dnsListener,
+    mainThread,
+    defaultOriginAttributes
+  );
+
+  let [inRequest, inRecord, inStatus] = await dnsListener;
+  Assert.equal(inRequest, request, "correct request was used");
+  Assert.equal(inStatus, Cr.NS_OK, "status OK");
+
+  // Since the HTTPS RR should be served from cache, the DNS record is available
+  // before nsHttpChannel::MaybeUseHTTPSRRForUpgrade() is called.
+  let chan = makeChan(`http://test.httpssvc.com:80/server-timing`);
+  let listener = new EventSinkListener();
+  chan.notificationCallbacks = listener;
+
+  let [req, resp] = await channelOpenPromise(chan);
+
+  req.QueryInterface(Ci.nsIHttpChannel);
+  Assert.equal(req.getResponseHeader("x-connection-http2"), "yes");
+
+  chan = makeChan(`http://test.httpssvc.com:80/server-timing`);
+  listener = new EventSinkListener();
+  chan.notificationCallbacks = listener;
+
+  [req, resp] = await channelOpenPromise(chan);
+
+  req.QueryInterface(Ci.nsIHttpChannel);
+  Assert.equal(req.getResponseHeader("x-connection-http2"), "yes");
+});
+
+// Test the case that we got an invalid DNS response.
+add_task(async function testInvalidDNSResult() {
+  let chan = makeChan(`http://foo.notexisted.com:80/server-timing`);
+  let [req, resp] = await channelOpenPromise(
+    chan,
+    CL_EXPECT_LATE_FAILURE | CL_ALLOW_UNKNOWN_CL
+  );
+  Assert.equal(req.status, Cr.NS_ERROR_CONNECTION_REFUSED);
+});
+
+add_task(async function testLiteralIP() {
+  let chan = makeChan(`http://127.0.0.1:80/server-timing`);
+  let [req, resp] = await channelOpenPromise(
+    chan,
+    CL_EXPECT_LATE_FAILURE | CL_ALLOW_UNKNOWN_CL
+  );
+  Assert.equal(req.status, Cr.NS_ERROR_CONNECTION_REFUSED);
+});
--- a/netwerk/test/unit/test_use_httpssvc.js
+++ b/netwerk/test/unit/test_use_httpssvc.js
@ -123,52 +123,14 @@ function channelOpenPromise(chan) {
  });
 }

-// This is for testing when the HTTPSSVC record is already available before
+// This is for testing when the HTTPSSVC record is not available when
 // the transaction is added in connection manager.
-add_task(async function testUseHTTPSSVC() {
+add_task(async function testUseHTTPSSVCForHttpsUpgrade() {
  // use the h2 server as DOH provider
  prefs.setCharPref(
    "network.trr.uri",
    "https://foo.example.com:" + h2Port + "/httpssvc_as_altsvc"
  );
-
-  let listener = new DNSListener();
-
-  // Do DNS resolution before creating the channel, so the HTTPSSVC record will
-  // be resolved from the cache.
-  let request = dns.asyncResolve(
-    "test.httpssvc.com",
-    dns.RESOLVE_TYPE_HTTPSSVC,
-    0,
-    null, // resolverInfo
-    listener,
-    mainThread,
-    defaultOriginAttributes
-  );
-
-  let [inRequest, inRecord, inStatus] = await listener;
-  Assert.equal(inRequest, request, "correct request was used");
-  Assert.equal(inStatus, Cr.NS_OK, "status OK");
-
-  // We need to skip the security check, since our test cert is signed for
-  // foo.example.com, not test.httpssvc.com.
-  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
-    true
-  );
-
-  let chan = makeChan(`https://test.httpssvc.com:8888`);
-  let [req, resp] = await channelOpenPromise(chan);
-  // Test if this request is done by h2.
-  Assert.equal(req.getResponseHeader("x-connection-http2"), "yes");
-
-  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
-    false
-  );
-});
-
-// This is for testing when the HTTPSSVC record is not available when
-// the transaction is added in connection manager.
-add_task(async function testUseHTTPSSVC1() {
  dns.clearCache(true);

  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
@ -205,12 +167,19 @@ EventSinkListener.prototype.QueryInterface = ChromeUtils.generateQI([

 // Test if the request is upgraded to https with a HTTPSSVC record.
 add_task(async function testUseHTTPSSVCAsHSTS() {
+  // use the h2 server as DOH provider
+  prefs.setCharPref(
+    "network.trr.uri",
+    "https://foo.example.com:" + h2Port + "/httpssvc_as_altsvc"
+  );
  dns.clearCache(true);

  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
    true
  );

+  // At this time, the DataStorage is not ready, so MaybeUseHTTPSRRForUpgrade()
+  // is called from the callback of NS_ShouldSecureUpgrade().
  let chan = makeChan(`http://test.httpssvc.com:80/`);
  let listener = new EventSinkListener();
  chan.notificationCallbacks = listener;
@ -220,6 +189,60 @@ add_task(async function testUseHTTPSSVCAsHSTS() {
  req.QueryInterface(Ci.nsIHttpChannel);
  Assert.equal(req.getResponseHeader("x-connection-http2"), "yes");

+  // At this time, the DataStorage is ready, so MaybeUseHTTPSRRForUpgrade()
+  // is called from nsHttpChannel::OnBeforeConnect().
+  chan = makeChan(`http://test.httpssvc.com:80/`);
+  listener = new EventSinkListener();
+  chan.notificationCallbacks = listener;
+
+  [req, resp] = await channelOpenPromise(chan);
+
+  req.QueryInterface(Ci.nsIHttpChannel);
+  Assert.equal(req.getResponseHeader("x-connection-http2"), "yes");
+
+  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
+    false
+  );
+});
+
+// This is for testing when the HTTPSSVC record is already available before
+// the transaction is added in connection manager.
+add_task(async function testUseHTTPSSVC() {
+  // use the h2 server as DOH provider
+  prefs.setCharPref(
+    "network.trr.uri",
+    "https://foo.example.com:" + h2Port + "/httpssvc_as_altsvc"
+  );
+
+  let listener = new DNSListener();
+
+  // Do DNS resolution before creating the channel, so the HTTPSSVC record will
+  // be resolved from the cache.
+  let request = dns.asyncResolve(
+    "test.httpssvc.com",
+    dns.RESOLVE_TYPE_HTTPSSVC,
+    0,
+    null, // resolverInfo
+    listener,
+    mainThread,
+    defaultOriginAttributes
+  );
+
+  let [inRequest, inRecord, inStatus] = await listener;
+  Assert.equal(inRequest, request, "correct request was used");
+  Assert.equal(inStatus, Cr.NS_OK, "status OK");
+
+  // We need to skip the security check, since our test cert is signed for
+  // foo.example.com, not test.httpssvc.com.
+  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
+    true
+  );
+
+  let chan = makeChan(`https://test.httpssvc.com:8888`);
+  let [req, resp] = await channelOpenPromise(chan);
+  // Test if this request is done by h2.
+  Assert.equal(req.getResponseHeader("x-connection-http2"), "yes");
+
  certOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData(
    false
  );
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@ -511,3 +511,4 @@ run-sequentially = node server exceptions dont replay well
 [test_httpssvc_retry_without_ech.js]
 skip-if = asan || tsan || os == 'win' || os =='android'
 run-sequentially = node server exceptions dont replay well
+[test_httpssvc_https_upgrade.js]
--- a/testing/xpcshell/moz-http2/moz-http2.js
+++ b/testing/xpcshell/moz-http2/moz-http2.js
@ -912,6 +912,12 @@ function handleRequest(req, res) {
      let packet = dnsPacket.decode(payload);
      let answers = [];
      if (packet.questions[0].type == "HTTPS") {
+        let priority = 1;
+        // Set an invalid priority to test the case when receiving a corrupted
+        // response.
+        if (packet.questions[0].name === "foo.notexisted.com") {
+          priority = 0;
+        }
        answers.push({
          name: packet.questions[0].name,
          type: packet.questions[0].type,
@ -919,7 +925,7 @@ function handleRequest(req, res) {
          class: "IN",
          flush: false,
          data: {
-            priority: 1,
+            priority,
            name: "foo.example.com",
            values: [
              { key: "alpn", value: "h2" },