From b9823b89190345c1a1e477249411505fa2885f74 Mon Sep 17 00:00:00 2001 From: Pierre Tachoire Date: Mon, 22 Jun 2026 11:42:46 +0200 Subject: [PATCH] fail pending module/preload fetches killed on teardown A queued module/preload transfer aborted during a page-swap teardown fired shutdown_callback (unset on these requests), not error_callback, leaving its entry stuck `.loading` and hanging waitForImport/waitForPreload. Register a JS-free shutdown_callback that moves the entry off `.loading` so the waiter unblocks. --- src/browser/ScriptManager.zig | 46 ++++++++++++++++++++++++++ src/browser/ScriptManagerBase.zig | 54 +++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig index 157abf557..dd6346485 100644 --- a/src/browser/ScriptManager.zig +++ b/src/browser/ScriptManager.zig @@ -150,6 +150,7 @@ pub fn preloadScript(self: *ScriptManager, element: *Element.Html, url: []const .data_callback = Script.dataCallback, .done_callback = PreloadedScript.doneCallback, .error_callback = PreloadedScript.errorCallback, + .shutdown_callback = PreloadedScript.shutdownCallback, }); return true; } @@ -459,4 +460,49 @@ const PreloadedScript = struct { script.queueHintEvent(.@"error"); script.deinit(); } + + // Owner-driven teardown killed this preload fetch via Transfer.kill, which + // fires shutdown_callback — not error_callback. Drop the entry (so a + // synchronous waitForPreload's getPtr returns null and it falls back to a + // normal fetch) and free the Script. No JS / hint events here, unlike + // errorCallback, since the owner is being torn down. + fn shutdownCallback(ctx: *anyopaque) void { + const script: *Script = @ptrCast(@alignCast(ctx)); + const self: *ScriptManager = @fieldParentPtr("base", script.manager); + _ = self.preloaded_scripts.remove(script.url); + script.deinit(); + } }; + +const testing = @import("../testing.zig"); + +test "ScriptManager: PreloadedScript.shutdownCallback drops a .loading preload" { + defer testing.reset(); + const frame = try testing.pageTest("mcp_nav.html", .{}); + defer frame._session.removePage(); + + const sm = &frame._script_manager; + const url: [:0]const u8 = "http://127.0.0.1:9582/killed-preload.js"; + + // Build a `.loading` preload entry directly (mirroring preloadScript) so the + // test doesn't depend on the network. shutdownCallback frees the Script. + const arena = try frame.getArena(.large, "test.shutdown"); + const script = try arena.create(Script); + script.* = .{ + .arena = arena, + .url = url, + .node = .{}, + .manager = &sm.base, + .complete = false, + .source = .{ .remote = .{} }, + .extra = .preload, + .hint_element = null, + }; + try sm.preloaded_scripts.put(sm.base.allocator, url, .{ .state = .{ .loading = script } }); + + // Transfer.kill fires this on owner teardown. The entry must be dropped so a + // synchronous waitForPreload's getPtr returns null and it falls back. + PreloadedScript.shutdownCallback(script); + + try testing.expect(sm.preloaded_scripts.getPtr(url) == null); +} diff --git a/src/browser/ScriptManagerBase.zig b/src/browser/ScriptManagerBase.zig index dd8f550c2..2a9a92d0e 100644 --- a/src/browser/ScriptManagerBase.zig +++ b/src/browser/ScriptManagerBase.zig @@ -289,6 +289,7 @@ pub fn preloadImport(self: *ScriptManagerBase, url: [:0]const u8, referrer: []co .data_callback = Script.dataCallback, .done_callback = Script.doneCallback, .error_callback = Script.errorCallback, + .shutdown_callback = Script.shutdownCallback, }) catch |err| { self.async_scripts.remove(&script.node); return err; @@ -819,6 +820,23 @@ pub const Script = struct { manager.evaluate(); } + // Owner-driven teardown (Frame / WorkerGlobalScope) killed this module + // fetch via Transfer.kill, which fires shutdown_callback — NOT + // error_callback (error_callback runs JS via manager.evaluate(), unsafe + // mid-teardown). Registered only on `.import` requests (preloadImport). + // Move the imported_modules entry off `.loading` to `.err` so a + // synchronous waitForImport returns error.Failed instead of spinning + // forever. We must not run JS or touch lists here; the orphaned Script is + // reaped by manager.reset()'s clearList over async_scripts. + pub fn shutdownCallback(ctx: *anyopaque) void { + const self: *Script = @ptrCast(@alignCast(ctx)); + const entry = self.manager.imported_modules.getPtr(self.url) orelse return; + switch (entry.state) { + .loading => entry.state = .err, + .done, .err => {}, + } + } + // Frame-only. Asserts extra == .frame; callers from the worker path never // reach here (workers only produce .import / .import_async). pub fn eval(self: *Script) void { @@ -1007,3 +1025,39 @@ pub const ImportedModule = struct { done: *Script, }; }; + +const testing = @import("../testing.zig"); + +test "ScriptManagerBase: shutdownCallback fails a .loading module" { + defer testing.reset(); + const frame = try testing.pageTest("mcp_nav.html", .{}); + defer frame._session.removePage(); + + const sm = &frame._script_manager.base; + const url: [:0]const u8 = "http://127.0.0.1:9582/killed-module.js"; + + // Build a `.loading` import entry directly (mirroring preloadImport) so the + // test doesn't depend on the network. The orphaned Script is reaped by + // reset()'s clearList over async_scripts. + const arena = try sm.acquireArena(.large, "test.shutdown"); + const script = try arena.create(Script); + script.* = .{ + .arena = arena, + .url = url, + .node = .{}, + .manager = sm, + .complete = false, + .source = .{ .remote = .{} }, + .extra = .import, + .hint_element = null, + }; + try sm.imported_modules.put(sm.allocator, url, .{ .state = .{ .loading = script } }); + sm.async_scripts.append(&script.node); + + // Transfer.kill fires this on owner teardown. It must move the entry off + // `.loading` so a synchronous waitForImport returns instead of hanging. + Script.shutdownCallback(script); + + try testing.expect(sm.imported_modules.getPtr(url).?.state == .err); + try testing.expectError(error.Failed, sm.waitForImport(url)); +}