Bläddra i källkod

Don't force the IE11 upload workaround on Safari (#847)

* Stop forcing the IE11 upload workaround on Safari via user agent sniffing

The upload test reported low and unstable numbers on Safari (macOS and
iOS) while the download test was correct.

Cause: the "start" handler in speedtest_worker.js matched every Safari
user agent and set settings.forceIE11Workaround = true. In ulTest() that
flag selects a fundamentally different, less accurate measurement path:
instead of sending one xhr_ul_blob_megabytes blob (20 MB by default) and
accumulating event.loaded from upload.onprogress, it sends a stream of
256 KB requests and adds a fixed 256 KB to totLoaded on each onload.
Progress is therefore quantised to whole requests and each request pays
a full round trip before the next one starts, so on fast or high-latency
links the measured speed is both too low and noisy.

The workaround is not needed. Its comment claims it is only required for
MPOT, but the block never checked settings.mpot. The same block also
excludes crios and fxios, which on iOS run the very same WebKit
networking stack as Safari, so the user agent test cannot have been
guarding an engine-level bug in the first place. Current WebKit
implements xhr.upload.onprogress, and the existing feature detection in
ulTest() (the try/catch that probes xhr.upload.onprogress) already
selects the workaround for browsers that genuinely lack it, without
sniffing.

Changes:
- Remove the Safari user agent block. Safari now takes the regular
  upload path, the same one Chrome and Firefox on iOS already took.
- Guard the Edge and PlayStation 4 blocks with
  typeof s.forceIE11Workaround === "undefined", matching how the other
  quirks avoid clobbering explicitly passed settings. Previously an
  explicit forceIE11Workaround: false in the start command was silently
  overwritten, so the workaround was not switchable from outside the
  worker. Default behaviour for those browsers is unchanged.
- Add tests/e2e/worker-browser-quirks.spec.js, which loads the worker
  under spoofed user agents and asserts the resolved settings.
- Update doc.md so Safari is no longer listed as affected.

The Edge and PlayStation 4 blocks are deliberately left outside the
enable_quirks condition: those browsers have no working
upload.onprogress, so running them with enable_quirks: false would
report 0 or Fail rather than a less precise number.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S1bUKsok59shM69PwLMMRM

* docs: describe both detection paths for the IE11 upload workaround

The previous wording claimed affected browsers are detected by checking
whether xhr.upload is usable "not by sniffing the user agent", but Edge
and the PlayStation 4 browser are still matched by user agent regex.
Both mechanisms exist and for different reasons: feature detection only
sees a missing xhr.upload object, while those two expose the object and
simply never fire its events.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S1bUKsok59shM69PwLMMRM

---------

Co-authored-by: Claude <noreply@anthropic.com>
marcel1702 1 vecka sedan
förälder
incheckning
0603db7b37
3 ändrade filer med 100 tillägg och 9 borttagningar
  1. 1 1
      doc.md
  2. 8 8
      speedtest_worker.js
  3. 91 0
      tests/e2e/worker-browser-quirks.spec.js

+ 1 - 1
doc.md

@@ -732,7 +732,7 @@ To keep track of the amount of transferred data, the XHR Level 2 `upload.onprogr
 
 This test has a couple of complications:
 
-* Some browsers don't have a working `upload.onprogress` event. For this, we use a small blobs instead of a large one and we keep track of progress using the `onload` event. This is referred to as IE11 Workaround (but the same bug was also found in some versions of Edge and Safari)
+* Some browsers don't have a working `upload.onprogress` event. For this, we use a small blobs instead of a large one and we keep track of progress using the `onload` event. This is referred to as IE11 Workaround. Browsers that expose no usable `xhr.upload` object, such as IE11, select it automatically through feature detection. Some versions of Edge and the PlayStation 4 browser do have an `xhr.upload` object whose events never fire, which feature detection cannot see, so those two are still matched by user agent. Safari is __not__ affected and uses the regular, more accurate upload test
 * When `mpot` is set to `true`, an empty request must first be sent in order to load the CORS headers before the test can start
 
 See the code for more implementation details.

+ 8 - 8
speedtest_worker.js

@@ -146,22 +146,22 @@ this.addEventListener("message", function(e) {
 				}
 			}
 			if (/Edge.(\d+\.\d+)/i.test(ua)) {
-				//Edge 15 introduced a bug that causes onprogress events to not get fired, we have to use the "small chunks" workaround that reduces accuracy
-				settings.forceIE11Workaround = true;
+				if (typeof s.forceIE11Workaround === "undefined") {
+					//Edge 15 introduced a bug that causes onprogress events to not get fired, we have to use the "small chunks" workaround that reduces accuracy
+					settings.forceIE11Workaround = true;
+				}
 			}
 			if (/PlayStation 4.(\d+\.\d+)/i.test(ua)) {
-				//PS4 browser has the same bug as IE11/Edge
-				settings.forceIE11Workaround = true;
+				if (typeof s.forceIE11Workaround === "undefined") {
+					//PS4 browser has the same bug as IE11/Edge
+					settings.forceIE11Workaround = true;
+				}
 			}
 			if (/Chrome.(\d+)/i.test(ua) && /Android|iPhone|iPad|iPod|Windows Phone/i.test(ua)) {
 				//cheap af
 				//Chrome mobile introduced a limitation somewhere around version 65, we have to limit XHR upload size to 4 megabytes
 				settings.xhr_ul_blob_megabytes = 4;
 			}
-			if (/^((?!chrome|android|crios|fxios).)*safari/i.test(ua)) {
-				//Safari also needs the IE11 workaround but only for the MPOT version
-				settings.forceIE11Workaround = true;
-			}
 			//telemetry_level has to be parsed and not just copied
 			if (typeof s.telemetry_level !== "undefined") settings.telemetry_level = s.telemetry_level === "basic" ? 1 : s.telemetry_level === "full" ? 2 : s.telemetry_level === "debug" ? 3 : 0; // telemetry level
 			//transform test_order to uppercase, just in case

+ 91 - 0
tests/e2e/worker-browser-quirks.spec.js

@@ -0,0 +1,91 @@
+const fs = require("node:fs");
+const path = require("node:path");
+const { test, expect } = require("@playwright/test");
+
+const workerSource = fs.readFileSync(path.join(__dirname, "..", "..", "speedtest_worker.js"), "utf8");
+
+const userAgents = {
+  safariMac:
+    "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.4 Safari/605.1.15",
+  safariIos:
+    "Mozilla/5.0 (iPhone; CPU iPhone OS 17_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.4 Mobile/15E148 Safari/604.1",
+  chromeIos:
+    "Mozilla/5.0 (iPhone; CPU iPhone OS 17_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) CriOS/123.0.6312.52 Mobile/15E148 Safari/604.1",
+  firefoxIos:
+    "Mozilla/5.0 (iPhone; CPU iPhone OS 17_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) FxiOS/124.0 Mobile/15E148 Safari/605.1.15",
+  chromeAndroid:
+    "Mozilla/5.0 (Linux; Android 14; Pixel 8) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Mobile Safari/537.36",
+  edge16:
+    "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36 Edge/16.16299",
+  playstation4: "Mozilla/5.0 (PlayStation 4 5.55) AppleWebKit/601.2 (KHTML, like Gecko)"
+};
+
+/*
+  Loads speedtest_worker.js into a page that reports the given user agent, feeds it a "start"
+  command and reports back the settings the worker resolved. The "_" test step is a no-op delay,
+  so the worker settles its settings without performing any network I/O.
+*/
+async function resolveSettings(browser, userAgent, customSettings) {
+  const context = await browser.newContext({ userAgent });
+  try {
+    const page = await context.newPage();
+    await page.goto("about:blank");
+    await page.addScriptTag({ content: workerSource });
+    return await page.evaluate(custom => {
+      window.dispatchEvent(new MessageEvent("message", { data: "start " + JSON.stringify(custom) }));
+      return {
+        userAgent: navigator.userAgent,
+        forceIE11Workaround: settings.forceIE11Workaround,
+        xhr_ul_blob_megabytes: settings.xhr_ul_blob_megabytes
+      };
+    }, Object.assign({ test_order: "_" }, customSettings));
+  } finally {
+    await context.close();
+  }
+}
+
+test.describe("speedtest_worker browser quirks", () => {
+  test("Safari uses the accurate upload test instead of the IE11 workaround", async ({ browser }) => {
+    for (const ua of [userAgents.safariMac, userAgents.safariIos]) {
+      const resolved = await resolveSettings(browser, ua);
+      expect(resolved.userAgent, "context user agent should be applied").toBe(ua);
+      expect(resolved.forceIE11Workaround, `${ua} should not force the IE11 workaround`).toBe(false);
+      expect(resolved.xhr_ul_blob_megabytes).toBe(20);
+    }
+  });
+
+  test("Safari, Chrome and Firefox on iOS resolve to the same upload path", async ({ browser }) => {
+    const safari = await resolveSettings(browser, userAgents.safariIos);
+    const chrome = await resolveSettings(browser, userAgents.chromeIos);
+    const firefox = await resolveSettings(browser, userAgents.firefoxIos);
+    expect(safari.forceIE11Workaround).toBe(chrome.forceIE11Workaround);
+    expect(safari.forceIE11Workaround).toBe(firefox.forceIE11Workaround);
+  });
+
+  test("Safari in MPOT mode also uses the accurate upload test", async ({ browser }) => {
+    const resolved = await resolveSettings(browser, userAgents.safariMac, { mpot: true });
+    expect(resolved.forceIE11Workaround).toBe(false);
+  });
+
+  test("Chrome mobile still caps the upload blob at 4 megabytes", async ({ browser }) => {
+    const resolved = await resolveSettings(browser, userAgents.chromeAndroid);
+    expect(resolved.xhr_ul_blob_megabytes).toBe(4);
+    expect(resolved.forceIE11Workaround).toBe(false);
+  });
+
+  test("Edge and the PlayStation 4 browser keep the IE11 workaround by default", async ({ browser }) => {
+    for (const ua of [userAgents.edge16, userAgents.playstation4]) {
+      const resolved = await resolveSettings(browser, ua);
+      expect(resolved.forceIE11Workaround, `${ua} should keep the IE11 workaround`).toBe(true);
+    }
+  });
+
+  test("an explicitly passed forceIE11Workaround is not overwritten by the quirks", async ({ browser }) => {
+    for (const ua of [userAgents.edge16, userAgents.playstation4]) {
+      const off = await resolveSettings(browser, ua, { forceIE11Workaround: false });
+      expect(off.forceIE11Workaround, `${ua} should honour an explicit false`).toBe(false);
+    }
+    const on = await resolveSettings(browser, userAgents.safariMac, { forceIE11Workaround: true });
+    expect(on.forceIE11Workaround, "an explicit true should still force the workaround").toBe(true);
+  });
+});