Merge pull request #3622 from lightpanda-io/keep-preload-script-errors

http: preserve preload script errors

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);
 }

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;

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">

src/browser/tests/fixtures/preload_failed.js

@@ -0,0 +1 @@
+window.failed_classic_ran = true;

src/browser/tests/fixtures/preload_failed_module.js

@@ -0,0 +1 @@
+window.failed_module_ran = true;