From 5b994c10cd5b48bd0a7ad852c84c9f0e307151b1 Mon Sep 17 00:00:00 2001
From: Karl Seguin <k@openmymind.io>
Date: Thu, 24 Sep 2026 12:51:51 +0800
Subject: [PATCH] http: preserve preload script errors

Preloaded modules would discard errors in the hope that re-fetching the script
when it was needed, might work. This reverses that decision and stores the
error state. When a preload fails, that failure sticks.

This does mean that a transient failure doesn't get a second chance.

But it also means we don't log the failure multiple times (I noticed this when
using adblocking and seeing duplicate failures for the same endpoint). Since we
don't keep trying, it cuts down the CDP flow too.
---
 src/browser/ScriptManager.zig                 | 61 +++++++++++++++++--
 src/browser/ScriptManagerBase.zig             | 47 +++++++++++---
 .../tests/fixtures/preload_failed.html        |  8 +++
 src/browser/tests/fixtures/preload_failed.js  |  1 +
 .../tests/fixtures/preload_failed_module.js   |  1 +
 5 files changed, 105 insertions(+), 13 deletions(-)
 create mode 100644 src/browser/tests/fixtures/preload_failed.html
 create mode 100644 src/browser/tests/fixtures/preload_failed.js
 create mode 100644 src/browser/tests/fixtures/preload_failed_module.js

diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig
index a0437c64a..08fced465 100644
--- a/src/browser/ScriptManager.zig
+++ b/src/browser/ScriptManager.zig
@@ -199,7 +199,7 @@ fn waitForPreload(self: *ScriptManager, url: [:0]const u8) ?*Script {
                 _ = client.tickSync(200) catch return null;
                 continue;
             },
-            .done => |script| {
+            .done, .failed => |script| {
                 // Preload scripts are single-use. We return it and it becomes
                 // the caller's responsibility to free.
                 _ = self.preloaded_scripts.remove(url);
@@ -318,7 +318,7 @@ pub fn addFromElement(self: *ScriptManager, comptime from_parser: bool, script_e
     if (mode != .normal) {
         var preloaded = self.takePreload(remote_url);
         if (preloaded == null and kind == .module) {
-            preloaded = self.base.takeModuleHint(remote_url);
+            preloaded = try self.base.takeModuleHint(remote_url);
         }
         if (preloaded) |pre| {
             if (comptime lp.IS_DEBUG) {
@@ -545,6 +545,7 @@ const PreloadedScript = struct {
     const State = union(enum) {
         loading: *Script,
         done: *Script,
+        failed: *Script,
     };
 
     pub fn deinit(self: PreloadedScript) void {
@@ -583,10 +584,12 @@ const PreloadedScript = struct {
             log.warn(.http, "script fetch error", .{ .err = err, .req = script.url, .extra = "preload", .status = script.status });
         }
 
+        script.status = 0; // status == 0 is correctly treated as an error throughout
+        script.complete = true;
+
         const self: *ScriptManager = @fieldParentPtr("base", script.manager);
-        _ = self.preloaded_scripts.remove(script.url);
+        self.preloaded_scripts.getPtr(script.url).?.state = .{ .failed = script };
         script.queueHintEvent(.@"error");
-        script.deinit();
     }
 
     // Owner-driven teardown killed this preload fetch via Transfer.kill, which
@@ -726,6 +729,52 @@ test "ScriptManager: preload whose submit fails synchronously releases its arena
     const url = "http://127.0.0.1:9582/fails-at-submit.js";
     // A fetch was started (and failed), so the hint's error event fires.
     try testing.expectEqual(true, try sm.preloadScript(null, url));
-    // errorCallback consumed the entry; nothing dangles in the map.
-    try testing.expectEqual(false, sm.preloaded_scripts.contains(url));
+    // The failed entry stays for a <script> to consume; reset() frees it.
+    try testing.expect(sm.preloaded_scripts.getPtr(url).?.state == .failed);
+}
+
+// A failed preload used to be dropped, so the <script> consuming it fetched
+// again: a blocked script logged "blocked url" and counted in
+// adblock_verdicts twice. Unblocking before the <script>s and import() run
+// makes a refetch observable: it would succeed and run the script.
+test "ScriptManager: a failed preload is consumed, not refetched" {
+    const client = &testing.test_session.browser.http_client;
+    try client.setBlockedUrls(&.{ "*/preload_failed.js", "*/preload_failed_module.js" });
+    defer client.setBlockedUrls(&.{}) catch unreachable;
+
+    // Both hints' fetch errors, and nothing else.
+    testing.expectLog(&.{ .http, .http });
+
+    const page = try testing.pageTest("fixtures/preload_failed.html", .{});
+    defer page.close();
+    try client.setBlockedUrls(&.{});
+
+    {
+        const frame = page.frame().?;
+        var ls: js.Local.Scope = undefined;
+        frame.js.localScope(&ls);
+        defer ls.deinit();
+
+        try ls.local.eval(
+            \\const classic = document.createElement('script');
+            \\classic.src = 'preload_failed.js';
+            \\classic.onerror = () => window.classic_error = true;
+            \\document.head.appendChild(classic);
+            \\const module = document.createElement('script');
+            \\module.type = 'module';
+            \\module.src = 'preload_failed_module.js';
+            \\module.onerror = () => window.module_error = true;
+            \\document.head.appendChild(module);
+            \\const dynamic = document.createElement('script');
+            \\dynamic.textContent = "import('./preload_failed_module.js').catch(() => window.import_error = true);";
+            \\document.head.appendChild(dynamic);
+        , null);
+    }
+
+    var runner = testing.test_session.runner(.{});
+    try runner.waitForScript(page.frame_id,
+        \\window.classic_hint_error && window.module_hint_error &&
+        \\window.classic_error && window.module_error && window.import_error &&
+        \\!window.failed_classic_ran && !window.failed_module_ran
+    , 2000);
 }
diff --git a/src/browser/ScriptManagerBase.zig b/src/browser/ScriptManagerBase.zig
index 95598f301..6dbdab3f8 100644
--- a/src/browser/ScriptManagerBase.zig
+++ b/src/browser/ScriptManagerBase.zig
@@ -253,8 +253,14 @@ pub fn preloadModuleHint(self: *ScriptManagerBase, element: ?*Element.Html, url:
 
 // A <script type=module src=...> whose URL was hinted (modulepreload link or
 // prescan)
-pub fn takeModuleHint(self: *ScriptManagerBase, url: [:0]const u8) ?*Script {
+pub fn takeModuleHint(self: *ScriptManagerBase, url: [:0]const u8) !?*Script {
     const entry = self.imported_modules.getEntry(url) orelse return null;
+    if (entry.value_ptr.state == .err) {
+        // for loading/done, we'll remove the entry (because the script will
+        // get consumed). For err, we can keep the failure in the map to
+        // prevent a 2nd loader from needlessly trying to load this script
+        return try self.failedScript(url, .import);
+    }
     if (entry.value_ptr.hint == false) {
         // The script was preloaded, but not because of a hint. It came from v8
         // telling us to preload the module. We cannot take it here because we know
@@ -268,15 +274,31 @@ pub fn takeModuleHint(self: *ScriptManagerBase, url: [:0]const u8) ?*Script {
             break :blk script;
         },
         .done => |script| script,
-        // The hint's fetch failed; give the script its own attempt.
-        // I'm not sure if this is the right behavior. Why would a preload fail
-        // but the "real" load work? But it's definetly safer.
-        .err => return null,
+        .err => unreachable, // handled above
     };
     self.imported_modules.removeByPtr(entry.key_ptr);
     return script;
 }
 
+// A dummy script for a module whose fetch already failed, to trigger the
+// consumer's failure path (Script.eval fails on status == 0)
+fn failedScript(self: *ScriptManagerBase, url: [:0]const u8, extra: Script.Extra) !*Script {
+    const arena = try self.acquireArena(.tiny, "SM.failedScript");
+    errdefer arena.release();
+    const script = try arena.create(Script);
+    script.* = .{
+        .arena = arena,
+        .url = url,
+        .status = 0,
+        .node = .{},
+        .manager = self,
+        .complete = true,
+        .source = .{ .remote = .empty },
+        .extra = extra,
+    };
+    return script;
+}
+
 pub fn waitForImport(self: *ScriptManagerBase, url: [:0]const u8) !ModuleSource {
     const was_evaluating = self.is_evaluating;
     self.is_evaluating = true;
@@ -354,6 +376,12 @@ pub fn releaseImport(self: *ScriptManagerBase, url: [:0]const u8) void {
 pub fn getAsyncImport(self: *ScriptManagerBase, url: [:0]const u8, cb: ImportAsync.Callback, cb_data: *anyopaque, referrer: []const u8) !void {
     // A <link rel=modulepreload> hint may already be fetching/fetched this module
     if (self.imported_modules.getEntry(url)) |entry| {
+        if (entry.value_ptr.state == .err) {
+            const script = try self.failedScript(url, .{ .import_async = .{ .callback = cb, .data = cb_data } });
+            self.ready_scripts.append(&script.node);
+            self.evaluate();
+            return;
+        }
         if (entry.value_ptr.hint) {
             switch (entry.value_ptr.state) {
                 .loading => |script| {
@@ -381,8 +409,7 @@ pub fn getAsyncImport(self: *ScriptManagerBase, url: [:0]const u8, cb: ImportAsy
                     self.evaluate();
                     return;
                 },
-                // The hint's fetch failed; give the import its own attempt.
-                .err => {},
+                .err => unreachable, // handled above
             }
         }
     }
@@ -852,6 +879,12 @@ pub const Script = struct {
             return;
         }
 
+        if (self.source == .remote and (self.status < 200 or self.status > 299)) {
+            // An adopted preload / module hint that had already failed.
+            self.executeCallback(comptime .wrap("error"));
+            return;
+        }
+
         const previous_script = frame.document._current_script;
         frame.document._current_script = fe.script_element;
         defer frame.document._current_script = previous_script;
diff --git a/src/browser/tests/fixtures/preload_failed.html b/src/browser/tests/fixtures/preload_failed.html
new file mode 100644
index 000000000..9746bc3df
--- /dev/null
+++ b/src/browser/tests/fixtures/preload_failed.html
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<!--
+  Driven by the "a failed preload is consumed, not refetched" test in
+  ScriptManager.zig, which blocks these URLs while the hints fetch, then
+  unblocks them and inserts the matching <script> elements.
+-->
+<link rel="preload" as="script" href="preload_failed.js" onerror="window.classic_hint_error = true">
+<link rel="modulepreload" href="preload_failed_module.js" onerror="window.module_hint_error = true">
diff --git a/src/browser/tests/fixtures/preload_failed.js b/src/browser/tests/fixtures/preload_failed.js
new file mode 100644
index 000000000..4154d458b
--- /dev/null
+++ b/src/browser/tests/fixtures/preload_failed.js
@@ -0,0 +1 @@
+window.failed_classic_ran = true;
diff --git a/src/browser/tests/fixtures/preload_failed_module.js b/src/browser/tests/fixtures/preload_failed_module.js
new file mode 100644
index 000000000..d9998e62f
--- /dev/null
+++ b/src/browser/tests/fixtures/preload_failed_module.js
@@ -0,0 +1 @@
+window.failed_module_ran = true;