Bug 1714506 - Force a transaction to wait for HTTPS RR, r=necko-reviewers,valentin

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

modules/libpref/init/StaticPrefList.yaml

@@ -9756,6 +9756,12 @@
   value: false
   mirror: always
 
+# Whether to force a transaction to wait https rr.
+- name: network.dns.force_waiting_https_rr
+  type: RelaxedAtomicBool
+  value: @IS_NIGHTLY_BUILD@
+  mirror: always
+
 # The TTL for negative responses of TXT and HTTPS records.
 - name: network.dns.negative_ttl_for_type_record
   type: RelaxedAtomicUint32

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -5547,7 +5547,7 @@ void HttpBaseChannel::MaybeFlushConsoleReports() {
 void HttpBaseChannel::DoDiagnosticAssertWhenOnStopNotCalledOnDestroy() {}
 
 NS_IMETHODIMP HttpBaseChannel::SetWaitForHTTPSSVCRecord() {
-  mCaps |= NS_HTTP_WAIT_HTTPSSVC_RESULT;
+  mCaps |= NS_HTTP_FORCE_WAIT_HTTP_RR;
   return NS_OK;
 }
 

netwerk/protocol/http/nsHttp.h

@@ -120,9 +120,9 @@ extern const nsCString kHttp3Versions[];
 // such as HTTP upgrade which are not supported by HTTP3.
 #define NS_HTTP_DISALLOW_HTTP3 (1 << 22)
 
-// Force a transaction to stay in pending queue until the HTTPSSVC record is
+// Force a transaction to stay in pending queue until the HTTPS RR is
 // available.
-#define NS_HTTP_WAIT_HTTPSSVC_RESULT (1 << 23)
+#define NS_HTTP_FORCE_WAIT_HTTP_RR (1 << 23)
 
 // This is used for a temporary workaround for a web-compat issue. The flag is
 // only set on CORS preflight request to allowed sending client certificates

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -1368,7 +1368,7 @@ nsresult nsHttpConnectionMgr::TryDispatchTransaction(
 
   // Don't dispatch if this transaction is waiting for HTTPS RR.
   // Note that this is only used in test currently.
-  if (caps & NS_HTTP_WAIT_HTTPSSVC_RESULT) {
+  if (caps & NS_HTTP_FORCE_WAIT_HTTP_RR) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -371,16 +371,24 @@ nsresult nsHttpTransaction::Init(
     nsCOMPtr<nsIEventTarget> target;
     Unused << gHttpHandler->GetSocketThreadTarget(getter_AddRefs(target));
     if (target) {
+      if (StaticPrefs::network_dns_force_waiting_https_rr()) {
+        mCaps |= NS_HTTP_FORCE_WAIT_HTTP_RR;
+        mHTTPSRRQueryStart = TimeStamp::Now();
+      }
+
       mResolver = new HTTPSRecordResolver(this);
       nsCOMPtr<nsICancelable> dnsRequest;
-      if (NS_SUCCEEDED(mResolver->FetchHTTPSRRInternal(
-              target, getter_AddRefs(dnsRequest)))) {
+      rv = mResolver->FetchHTTPSRRInternal(target, getter_AddRefs(dnsRequest));
+      if (NS_SUCCEEDED(rv)) {
         mHTTPSSVCReceivedStage = HTTPSSVC_NOT_PRESENT;
       }
 
       {
         MutexAutoLock lock(mLock);
         mDNSRequest.swap(dnsRequest);
+        if (NS_FAILED(rv)) {
+          MakeDontWaitHTTPSRR();
+        }
       }
     }
   }
@@ -3007,23 +3015,31 @@ nsresult nsHttpTransaction::OnHTTPSRRAvailable(
        mActivated));
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
 
+  {
+    MutexAutoLock lock(mLock);
+    MakeDontWaitHTTPSRR();
+    mDNSRequest = nullptr;
+  }
+
   if (!mResolver) {
     LOG(("The transaction is not interested in HTTPS record anymore."));
     return NS_OK;
   }
 
-  {
-    MutexAutoLock lock(mLock);
-    mDNSRequest = nullptr;
-  }
-
   uint32_t receivedStage = HTTPSSVC_NO_USABLE_RECORD;
   // Make sure we set the correct value to |mHTTPSSVCReceivedStage|, since we
   // also use this value to indicate whether HTTPS RR is used or not.
-  auto updateHTTPSSVCReceivedStage =
-      MakeScopeExit([&] { mHTTPSSVCReceivedStage = receivedStage; });
+  auto updateHTTPSSVCReceivedStage = MakeScopeExit([&] {
+    mHTTPSSVCReceivedStage = receivedStage;
 
-  MakeDontWaitHTTPSSVC();
+    if (!mHTTPSRRQueryStart.IsNull()) {
+      AccumulateTimeDelta(Telemetry::HTTPS_RR_WAITING_TIME,
+                          HTTPS_RR_IS_USED(mHTTPSSVCReceivedStage)
+                              ? "with_https_rr"_ns
+                              : "no_https_rr"_ns,
+                          mHTTPSRRQueryStart, TimeStamp::Now());
+    }
+  });
 
   nsCOMPtr<nsIDNSHTTPSSVCRecord> record = aHTTPSSVCRecord;
   if (!record) {

netwerk/protocol/http/nsHttpTransaction.h

@@ -83,7 +83,7 @@ class nsHttpTransaction final : public nsAHttpTransaction,
   void MakeSticky() { mCaps |= NS_HTTP_STICKY_CONNECTION; }
   void MakeNonSticky() override { mCaps &= ~NS_HTTP_STICKY_CONNECTION; }
 
-  void MakeDontWaitHTTPSSVC() { mCaps &= ~NS_HTTP_WAIT_HTTPSSVC_RESULT; }
+  void MakeDontWaitHTTPSRR() { mCaps &= ~NS_HTTP_FORCE_WAIT_HTTP_RR; }
 
   // SetPriority() may only be used by the connection manager.
   void SetPriority(int32_t priority) { mPriority = priority; }
@@ -440,6 +440,7 @@ class nsHttpTransaction final : public nsAHttpTransaction,
 
   // The time when the transaction was submitted to the Connection Manager
   TimeStamp mPendingTime;
+  TimeStamp mHTTPSRRQueryStart;
   TimeDuration mPendingDurationTime;
 
   uint64_t mTopBrowsingContextId{0};

toolkit/components/telemetry/Histograms.json

@@ -16729,5 +16729,17 @@
       "Fetch",
       "Unexpected"
     ]
+  },
+  "HTTPS_RR_WAITING_TIME": {
+    "record_in_processes": ["main", "socket"],
+    "products": ["firefox"],
+    "alert_emails": ["necko@mozilla.com", "kershaw@mozilla.com"],
+    "bug_numbers": [1714506],
+    "expires_in_version": "93",
+    "kind": "exponential",
+    "high": 5000,
+    "n_buckets": 100,
+    "keyed": true,
+    "description": "The time that a transaction is waiting for HTTPS RR"
   }
 }