From b93092a9f4ffa16e584e73358c9c7a9065fc9962 Mon Sep 17 00:00:00 2001
From: Malte Juergens <mjurgens@mozilla.com>
Date: Mon, 29 Apr 2024 13:52:15 +0000
Subject: [PATCH] Bug 1891185 - Collect different downgrade time on timer for
 HTTPS-First telemetry r=freddyb

It does not make sense to collect the "downgrade time" as we currently do it
when a upgrade was caused by a faster http request on a timer. We can not
compare the navigation start timestamp with the current time, as the replacing
http channel has started loading earlier than the current time. So leave out the
case where the downgrade happens because of a faster http request on the timer,
as we already know that the time overhead there is exactly 3s.

Differential Revision: https://phabricator.services.mozilla.com/D207338
---
 dom/security/metrics.yaml                     | 14 ++++----
 dom/security/nsHTTPSOnlyUtils.cpp             | 35 ++++++++-----------
 .../test/https-first/browser_httpsfirst.js    |  9 +++--
 3 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/dom/security/metrics.yaml b/dom/security/metrics.yaml
index 02084407b11b..d48069e4b1ed 100644
--- a/dom/security/metrics.yaml
+++ b/dom/security/metrics.yaml
@@ -118,8 +118,8 @@ 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 is essentially the overhead caused by HTTPS-First if a
-      site does not support HTTPS.
+      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.
     time_unit: millisecond
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
@@ -135,11 +135,11 @@ httpsfirst:
   downgrade_time_schemeless:
     type: timing_distribution
     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 is
-      essentially the overhead caused by HTTPS-First if a site does not support
-      HTTPS.
+      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.
     time_unit: millisecond
     bugs:
       - https://bugzilla.mozilla.org/show_bug.cgi?id=1868380
diff --git a/dom/security/nsHTTPSOnlyUtils.cpp b/dom/security/nsHTTPSOnlyUtils.cpp
index 31c7408a3769..0bc99179dc9a 100644
--- a/dom/security/nsHTTPSOnlyUtils.cpp
+++ b/dom/security/nsHTTPSOnlyUtils.cpp
@@ -601,35 +601,28 @@ nsHTTPSOnlyUtils::PotentiallyDowngradeHttpsFirstRequest(
     if (navigationStart) {
       mozilla::TimeDuration duration =
           mozilla::TimeStamp::Now() - navigationStart;
+
       bool isPrivateWin =
           loadInfo->GetOriginAttributes().mPrivateBrowsingId > 0;
+      bool isSchemeless =
+          loadInfo->GetWasSchemelessInput() &&
+          !nsHTTPSOnlyUtils::IsHttpsFirstModeEnabled(isPrivateWin);
 
-      if (loadInfo->GetWasSchemelessInput() &&
-          !IsHttpsFirstModeEnabled(isPrivateWin)) {
-        mozilla::glean::httpsfirst::downgraded_schemeless.Add();
-        if (timing) {
-          mozilla::glean::httpsfirst::downgrade_time_schemeless
-              .AccumulateRawDuration(duration);
-        }
-      } else {
-        mozilla::glean::httpsfirst::downgraded.Add();
-        if (timing) {
-          mozilla::glean::httpsfirst::downgrade_time.AccumulateRawDuration(
-              duration);
-        }
-      }
+      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) {
-        if (loadInfo->GetWasSchemelessInput() &&
-            !nsHTTPSOnlyUtils::IsHttpsFirstModeEnabled(isPrivateWin)) {
-          mozilla::glean::httpsfirst::downgraded_on_timer_schemeless
-              .AddToNumerator();
-        } else {
-          mozilla::glean::httpsfirst::downgraded_on_timer.AddToNumerator();
-        }
+        downgradedOnTimerMetric.AddToNumerator();
+      } else {
+        downgradeTimeMetric.AccumulateRawDuration(duration);
       }
+      downgradedMetric.Add();
     }
   }
 
diff --git a/dom/security/test/https-first/browser_httpsfirst.js b/dom/security/test/https-first/browser_httpsfirst.js
index c4437f6051a9..e0bba26f733f 100644
--- a/dom/security/test/https-first/browser_httpsfirst.js
+++ b/dom/security/test/https-first/browser_httpsfirst.js
@@ -99,11 +99,10 @@ add_task(async function () {
   is(Glean.httpsfirst.downgradedOnTimerSchemeless.testGetValue(), null);
   const downgradeSeconds =
     Glean.httpsfirst.downgradeTime.testGetValue().sum / 1_000_000_000;
-  ok(
-    downgradeSeconds > 2 && downgradeSeconds < 30,
-    `Summed downgrade time should be above 2 and below 30 seconds (is ${downgradeSeconds.toFixed(
-      2
-    )}s)`
+  Assert.less(
+    downgradeSeconds,
+    10,
+    "Summed downgrade time should be below 10 seconds"
   );
   is(null, Glean.httpsfirst.downgradeTimeSchemeless.testGetValue());
 });