Bug 1885893 - Only collect HTTPS-First telemetry on successful request r=freddyb

This patch addresses the problem that we currently collect HTTPS-First telemetry
for sites that are not reachable at all, be it through always causing a error or
through always timing out.

- On a downgrade, do not collect telemetry instantly, but instead save the
  telemetry data in the load state for the downgraded request
- That telemetry data will then be copied over into the document load listener
  of the new request
- On a successful request, if we have downgrade data in the load listener, we
  collect the downgrade telemetry, as the downgrade seems to have been
  successful
- Similar to the downgrade case, we only count the upgrade metric once we
  encounter a successful request annotated with the information that it was
  upgraded by HTTPS-First, instead of counting it instantly on the decision to
  upgrade. This also means the upgrade metric will not include loads that are
  downgraded again anymore
- Add a testcase for a site which is neither reachable via HTTP nor HTTPS, and
  ensure no telemetry is collected

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

build/pgo/server-locations.txt

@@ -318,6 +318,7 @@ https://mismatch.badcertdomain.example.com:443                    privileged,cer
 # Hosts for HTTPS-First upgrades/downgrades
 http://httpsfirst.com:80                                          privileged
 https://httpsfirst.com:443                                        privileged,nocert
+https://invalid.example.com:443                                   privileged,nocert
 
 # Hosts for sha1 console warning tests
 https://sha1ee.example.com:443                                    privileged,cert=sha1_end_entity

docshell/base/nsDocShellLoadState.cpp

@@ -24,6 +24,7 @@
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/ContentParent.h"
 #include "mozilla/dom/LoadURIOptionsBinding.h"
+#include "mozilla/dom/nsHTTPSOnlyUtils.h"
 #include "mozilla/StaticPrefs_browser.h"
 #include "mozilla/StaticPrefs_fission.h"
 #include "mozilla/Telemetry.h"
@@ -165,6 +166,7 @@ nsDocShellLoadState::nsDocShellLoadState(const nsDocShellLoadState& aOther)
       mPartitionedPrincipalToInherit(aOther.mPartitionedPrincipalToInherit),
       mForceAllowDataURI(aOther.mForceAllowDataURI),
       mIsExemptFromHTTPSFirstMode(aOther.mIsExemptFromHTTPSFirstMode),
+      mHttpsFirstDowngradeData(aOther.GetHttpsFirstDowngradeData()),
       mOriginalFrameSrc(aOther.mOriginalFrameSrc),
       mIsFormSubmission(aOther.mIsFormSubmission),
       mLoadType(aOther.mLoadType),
@@ -632,6 +634,16 @@ void nsDocShellLoadState::SetIsExemptFromHTTPSFirstMode(
   mIsExemptFromHTTPSFirstMode = aIsExemptFromHTTPSFirstMode;
 }
 
+RefPtr<HTTPSFirstDowngradeData>
+nsDocShellLoadState::GetHttpsFirstDowngradeData() const {
+  return mHttpsFirstDowngradeData;
+}
+
+void nsDocShellLoadState::SetHttpsFirstDowngradeData(
+    RefPtr<HTTPSFirstDowngradeData> const& aHttpsFirstTelemetryData) {
+  mHttpsFirstDowngradeData = aHttpsFirstTelemetryData;
+}
+
 bool nsDocShellLoadState::OriginalFrameSrc() const { return mOriginalFrameSrc; }
 
 void nsDocShellLoadState::SetOriginalFrameSrc(bool aOriginalFrameSrc) {

docshell/base/nsDocShellLoadState.h

@@ -24,6 +24,7 @@ class nsIURI;
 class nsIDocShell;
 class nsIChannel;
 class nsIReferrerInfo;
+struct HTTPSFirstDowngradeData;
 namespace mozilla {
 class OriginAttributes;
 template <typename, class>
@@ -148,6 +149,11 @@ class nsDocShellLoadState final {
 
   void SetIsExemptFromHTTPSFirstMode(bool aIsExemptFromHTTPSFirstMode);
 
+  RefPtr<HTTPSFirstDowngradeData> GetHttpsFirstDowngradeData() const;
+
+  void SetHttpsFirstDowngradeData(
+      RefPtr<HTTPSFirstDowngradeData> const& aHttpsFirstTelemetryData);
+
   bool OriginalFrameSrc() const;
 
   void SetOriginalFrameSrc(bool aOriginalFrameSrc);
@@ -484,6 +490,10 @@ class nsDocShellLoadState final {
   // will be exempt from HTTPS-Only-Mode upgrades.
   bool mIsExemptFromHTTPSFirstMode;
 
+  // If set, this load is a HTTPS-First downgrade, and the downgrade data will
+  // be submitted to telemetry later if the load succeeds.
+  RefPtr<HTTPSFirstDowngradeData> mHttpsFirstDowngradeData;
+
   // If this attribute is true, this load corresponds to a frame
   // element loading its original src (or srcdoc) attribute.
   bool mOriginalFrameSrc;

dom/security/metrics.yaml

@@ -14,8 +14,9 @@ httpsfirst:
   upgraded:
     type: counter
     description: >
-      Counts how often a load is marked to be upgraded to HTTPS because of
-      HTTPS-First (`dom.security.https_first` enabled).
+      Counts how often a load is successfully upgraded to HTTPS because of
+      HTTPS-First (`dom.security.https_first` enabled). This does not include
+      loads that get downgraded again.
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
     data_reviews:
@@ -30,9 +31,10 @@ httpsfirst:
   upgraded_schemeless:
     type: counter
     description: >
-      Counts how often a load is marked to be upgraded to HTTPS because of
+      Counts how often a load is successfully upgraded to HTTPS because of
       schemeless HTTPS-First (`dom.security.https_first` disabled, but load
-      marked as schemeless).
+      marked as schemeless). This does not include loads that get downgraded
+      again.
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
     data_reviews:
@@ -48,7 +50,7 @@ httpsfirst:
     type: counter
     description: >
       How many regular HTTPS-First (`dom.security.https_first` enabled)
-      upgrades get downgraded again.
+      upgrades fail and get downgraded again.
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
     data_reviews:
@@ -64,7 +66,7 @@ httpsfirst:
     type: counter
     description: >
       How many schemeless HTTPS-First (`dom.security.https_first` disabled, but
-      load marked as schemeless) upgrades get downgraded again.
+      load marked as schemeless) upgrades fail and get downgraded again.
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
     data_reviews:
@@ -118,8 +120,7 @@ httpsfirst:
     description: >
       If a HTTPS-First (`dom.security.https_first` enabled) upgrade isn't
       successful, measures the timespan between the navigation start and the
-      downgrade. This does not include the case in which the https request times
-      out and the http request sent after 3s gets a response faster.
+      downgrade.
     time_unit: millisecond
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
@@ -137,9 +138,7 @@ httpsfirst:
     description: >
       If a schemeless HTTPS-First (`dom.security.https_first` disabled, but load
       marked as schemeless) upgrade isn't successful, measures the timespan
-      between the navigation start and the downgrade. This does not include the
-      case in which the https request times out and the http request sent after
-      3s gets a response faster.
+      between the navigation start and the downgrade.
     time_unit: millisecond
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380

dom/security/nsHTTPSOnlyUtils.cpp

@@ -448,8 +448,6 @@ bool nsHTTPSOnlyUtils::ShouldUpgradeHttpsFirstRequest(nsIURI* aURI,
     nsHTTPSOnlyUtils::LogLocalizedString("HTTPSFirstSchemeless", params,
                                          nsIScriptError::warningFlag, aLoadInfo,
                                          aURI, true);
-
-    mozilla::glean::httpsfirst::upgraded_schemeless.Add();
   } else {
     nsAutoCString scheme;
 
@@ -464,10 +462,6 @@ bool nsHTTPSOnlyUtils::ShouldUpgradeHttpsFirstRequest(nsIURI* aURI,
         isSpeculative ? "HTTPSOnlyUpgradeSpeculativeConnection"
                       : "HTTPSOnlyUpgradeRequest",
         params, nsIScriptError::warningFlag, aLoadInfo, aURI, true);
-
-    if (!isSpeculative) {
-      mozilla::glean::httpsfirst::upgraded.Add();
-    }
   }
 
   // Set flag so we know that we upgraded the request
@@ -594,7 +588,17 @@ nsHTTPSOnlyUtils::PotentiallyDowngradeHttpsFirstRequest(
                                        nsIScriptError::warningFlag, loadInfo,
                                        uri, true);
 
-  // Record telemety
+  return newURI.forget();
+}
+
+void nsHTTPSOnlyUtils::UpdateLoadStateAfterHTTPSFirstDowngrade(
+    mozilla::net::DocumentLoadListener* aDocumentLoadListener,
+    nsDocShellLoadState* aLoadState) {
+  // We have to exempt the load from HTTPS-First to prevent a upgrade-downgrade
+  // loop
+  aLoadState->SetIsExemptFromHTTPSFirstMode(true);
+
+  // Add downgrade data for later telemetry usage to load state
   nsDOMNavigationTiming* timing = aDocumentLoadListener->GetTiming();
   if (timing) {
     mozilla::TimeStamp navigationStart = timing->GetNavigationStartTimeStamp();
@@ -602,31 +606,60 @@ nsHTTPSOnlyUtils::PotentiallyDowngradeHttpsFirstRequest(
       mozilla::TimeDuration duration =
           mozilla::TimeStamp::Now() - navigationStart;
 
+      nsCOMPtr<nsIChannel> channel = aDocumentLoadListener->GetChannel();
+      nsCOMPtr<nsILoadInfo> loadInfo = channel->LoadInfo();
+
       bool isPrivateWin =
           loadInfo->GetOriginAttributes().mPrivateBrowsingId > 0;
       bool isSchemeless =
           loadInfo->GetWasSchemelessInput() &&
           !nsHTTPSOnlyUtils::IsHttpsFirstModeEnabled(isPrivateWin);
 
-      using namespace mozilla::glean::httpsfirst;
-      auto downgradedMetric = isSchemeless ? downgraded_schemeless : downgraded;
-      auto downgradedOnTimerMetric =
-          isSchemeless ? downgraded_on_timer_schemeless : downgraded_on_timer;
-      auto downgradeTimeMetric =
-          isSchemeless ? downgrade_time_schemeless : downgrade_time;
-
       nsresult channelStatus;
       channel->GetStatus(&channelStatus);
-      if (channelStatus == NS_ERROR_NET_TIMEOUT_EXTERNAL) {
-        downgradedOnTimerMetric.AddToNumerator();
-      } else {
-        downgradeTimeMetric.AccumulateRawDuration(duration);
-      }
-      downgradedMetric.Add();
+
+      RefPtr downgradeData = mozilla::MakeRefPtr<HTTPSFirstDowngradeData>();
+      downgradeData->downgradeTime = duration;
+      downgradeData->isOnTimer = channelStatus == NS_ERROR_NET_TIMEOUT_EXTERNAL;
+      downgradeData->isSchemeless = isSchemeless;
+      aLoadState->SetHttpsFirstDowngradeData(downgradeData);
     }
   }
+}
 
-  return newURI.forget();
+void nsHTTPSOnlyUtils::SubmitHTTPSFirstTelemetry(
+    nsCOMPtr<nsILoadInfo> const& aLoadInfo,
+    RefPtr<HTTPSFirstDowngradeData> const& aHttpsFirstDowngradeData) {
+  using namespace mozilla::glean::httpsfirst;
+  if (aHttpsFirstDowngradeData) {
+    // Successfully downgraded load
+
+    auto downgradedMetric = aHttpsFirstDowngradeData->isSchemeless
+                                ? downgraded_schemeless
+                                : downgraded;
+    auto downgradedOnTimerMetric = aHttpsFirstDowngradeData->isSchemeless
+                                       ? downgraded_on_timer_schemeless
+                                       : downgraded_on_timer;
+    auto downgradeTimeMetric = aHttpsFirstDowngradeData->isSchemeless
+                                   ? downgrade_time_schemeless
+                                   : downgrade_time;
+
+    if (aHttpsFirstDowngradeData->isOnTimer) {
+      downgradedOnTimerMetric.AddToNumerator();
+    }
+    downgradedMetric.Add();
+    downgradeTimeMetric.AccumulateRawDuration(
+        aHttpsFirstDowngradeData->downgradeTime);
+  } else if (aLoadInfo->GetHttpsOnlyStatus() &
+             nsILoadInfo::HTTPS_ONLY_UPGRADED_HTTPS_FIRST) {
+    // Successfully upgraded load
+
+    if (aLoadInfo->GetWasSchemelessInput()) {
+      upgraded_schemeless.Add();
+    } else {
+      upgraded.Add();
+    }
+  }
 }
 
 /* static */

dom/security/nsHTTPSOnlyUtils.h

@@ -176,6 +176,31 @@ class nsHTTPSOnlyUtils {
    */
   static uint32_t GetStatusForSubresourceLoad(uint32_t aHttpsOnlyStatus);
 
+  /**
+   * When a downgrade is happening because of HTTPS-First, this function will
+   * update the load state for the new load accordingly. This includes
+   * information about the downgrade for later telemetry use.
+   * @param aDocumentLoadListener The calling document load listener.
+   * @param aLoadState The load state to be updated
+   */
+  static void UpdateLoadStateAfterHTTPSFirstDowngrade(
+      mozilla::net::DocumentLoadListener* aDocumentLoadListener,
+      nsDocShellLoadState* aLoadState);
+
+  /**
+   * When a load is successful, this should be called by the document load
+   * listener. In two cases, telemetry is then recorded:
+   * a) If downgrade data has been passed, the passed load is a successful
+   *    downgrade, which means telemetry based on the downgrade data will be
+   *    submitted.
+   * b) If the passed load info indicates that this load has been upgraded by
+   *    HTTPS-First, this means the upgrade was successful, which will be
+   *    recorded to telemetry.
+   */
+  static void SubmitHTTPSFirstTelemetry(
+      nsCOMPtr<nsILoadInfo> const& aLoadInfo,
+      RefPtr<HTTPSFirstDowngradeData> const& aHttpsFirstDowngradeData);
+
  private:
   /**
    * Checks if it can be ruled out that the error has something
@@ -257,4 +282,17 @@ class TestHTTPAnswerRunnable final : public mozilla::Runnable,
   RefPtr<nsITimer> mTimer;
 };
 
+/**
+ * Data about a HTTPS-First downgrade used for Telemetry. We need to store this
+ * instead of directly submitting it when deciding to downgrade, because it is
+ * only interesting for us if the downgraded load is actually succesful.
+ */
+struct HTTPSFirstDowngradeData
+    : public mozilla::RefCounted<HTTPSFirstDowngradeData> {
+  MOZ_DECLARE_REFCOUNTED_TYPENAME(HTTPSFirstDowngradeData)
+  mozilla::TimeDuration downgradeTime;
+  bool isOnTimer = false;
+  bool isSchemeless = false;
+};
+
 #endif /* nsHTTPSOnlyUtils_h___ */

dom/security/test/https-first/browser_httpsfirst.js

@@ -90,8 +90,14 @@ add_task(async function () {
     "http://"
   );
 
+  await runPrefTest(
+    "http://invalid.example.com",
+    "Should downgrade non-reachable site.",
+    "http://"
+  );
+
   info("Checking expected telemetry");
-  is(Glean.httpsfirst.upgraded.testGetValue(), 5);
+  is(Glean.httpsfirst.upgraded.testGetValue(), 1);
   is(Glean.httpsfirst.upgradedSchemeless.testGetValue(), null);
   is(Glean.httpsfirst.downgraded.testGetValue(), 3);
   is(Glean.httpsfirst.downgradedSchemeless.testGetValue(), null);

netwerk/ipc/DocumentLoadListener.cpp

@@ -646,6 +646,7 @@ auto DocumentLoadListener::Open(nsDocShellLoadState* aLoadState,
   // See description of  mFileName in nsDocShellLoadState.h
   mIsDownload = !aLoadState->FileName().IsVoid();
   mIsLoadingJSURI = net::SchemeIsJavascript(aLoadState->URI());
+  mHTTPSFirstDowngradeData = aLoadState->GetHttpsFirstDowngradeData().forget();
 
   // Check for infinite recursive object or iframe loads
   if (aLoadState->OriginalFrameSrc() || !mIsDocumentLoad) {
@@ -2428,9 +2429,7 @@ bool DocumentLoadListener::MaybeHandleLoadErrorWithURIFixup(nsresult aStatus) {
   loadState->SetWasSchemelessInput(wasSchemelessInput);
 
   if (isHTTPSFirstFixup) {
-    // We have to exempt the load from HTTPS-First to prevent a
-    // upgrade-downgrade loop.
-    loadState->SetIsExemptFromHTTPSFirstMode(true);
+    nsHTTPSOnlyUtils::UpdateLoadStateAfterHTTPSFirstDowngrade(this, loadState);
   }
 
   // Ensure to set referrer information in the fallback channel equally to the
@@ -2572,6 +2571,17 @@ DocumentLoadListener::OnStartRequest(nsIRequest* aRequest) {
     return NS_OK;
   }
 
+  // If this is a successful load with a successful status code, we can possibly
+  // submit HTTPS-First telemetry.
+  if (NS_SUCCEEDED(status) && httpChannel) {
+    uint32_t responseStatus = 0;
+    if (NS_SUCCEEDED(httpChannel->GetResponseStatus(&responseStatus)) &&
+        responseStatus < 400) {
+      nsHTTPSOnlyUtils::SubmitHTTPSFirstTelemetry(
+          mChannel->LoadInfo(), mHTTPSFirstDowngradeData.forget());
+    }
+  }
+
   mStreamListenerFunctions.AppendElement(StreamListenerFunction{
       VariantIndex<0>{}, OnStartRequestParams{aRequest}});
 

netwerk/ipc/DocumentLoadListener.h

@@ -617,6 +617,8 @@ class DocumentLoadListener : public nsIInterfaceRequestor,
   bool mOpenPromiseResolved = false;
 
   const bool mIsDocumentLoad;
+
+  RefPtr<HTTPSFirstDowngradeData> mHTTPSFirstDowngradeData;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(DocumentLoadListener, DOCUMENT_LOAD_LISTENER_IID)