From a4e1fa95b96b525261c725a06af3873a6212ea19 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Thu, 17 Sep 2026 08:17:13 +0800 Subject: [PATCH] crash: fix a rare import crash Currently, our waitForImport blocks the caller, but continues to process any already-queued requests. This can result in new JavaScript running while v8 is linking modules and that JavaScript can itself import a module that is part of the still-being-linked graph. waitForImport now works like a syncRequest. While HttpClient will continue to make progress on all transfers, all other transfers will gate behind the waiting one (using the same infrastructure that exists for syncRequest). This crash was seen on an unknown srape URL. --- src/browser/ScriptManagerBase.zig | 61 +++++++++++++------ .../tests/page/module_reentrant_import.html | 25 ++++++++ src/browser/tests/page/modules/reentrant-a.js | 2 + src/browser/tests/page/modules/reentrant-c.js | 1 + src/network/HttpClient.zig | 28 +++++++-- 5 files changed, 95 insertions(+), 22 deletions(-) create mode 100644 src/browser/tests/page/module_reentrant_import.html create mode 100644 src/browser/tests/page/modules/reentrant-a.js create mode 100644 src/browser/tests/page/modules/reentrant-c.js diff --git a/src/browser/ScriptManagerBase.zig b/src/browser/ScriptManagerBase.zig index b26f118dc..5d46a85ef 100644 --- a/src/browser/ScriptManagerBase.zig +++ b/src/browser/ScriptManagerBase.zig @@ -212,24 +212,30 @@ pub fn preloadImport(self: *ScriptManagerBase, url: [:0]const u8, referrer: []co self.async_scripts.append(&script.node); const owner = self.owner; - owner.makeRequest(.{ - .ctx = script, - .url = url, - .method = .GET, - .origin = owner.origin(), - .request_mode = .cors, - .credentials_mode = .same_origin, - .resource_type = .script, - .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, - .header_callback = Script.headerCallback, - .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; + const transfer = blk: { + errdefer self.async_scripts.remove(&script.node); + const transfer = try owner.newRequest(.{ + .ctx = script, + .url = url, + .method = .GET, + .origin = owner.origin(), + .request_mode = .cors, + .credentials_mode = .same_origin, + .resource_type = .script, + .start_callback = if (log.enabled(.http, .debug)) Script.startCallback else null, + .header_callback = Script.headerCallback, + .data_callback = Script.dataCallback, + .done_callback = Script.doneCallback, + .error_callback = Script.errorCallback, + .shutdown_callback = Script.shutdownCallback, + }); + errdefer transfer.deinit(); + try owner.headersForRequest(transfer); + break :blk transfer; }; + gop.value_ptr.transfer_id = transfer.id; + // A synchronous failure is delivered through Script.errorCallback. + transfer.submit() catch {}; } // (element set) or the prescan finding a @@ -277,6 +283,24 @@ pub fn waitForImport(self: *ScriptManagerBase, url: [:0]const u8) !ModuleSource defer self.endEvaluationWindow(was_evaluating); var client = self.client; + + // We're inside V8's module instantiation. Nothing but this module's + // transfer may be delivered: any other callback can run JS (e.g. a fetch() + // resolving), and JS that import()s a module of the graph V8 is still + // linking re-enters instantiation and crashes V8. + const frame_id = self.owner.frameId(); + const blocked = blk: { + const entry = self.imported_modules.get(url) orelse break :blk false; + if (entry.state != .loading) { + break :blk false; + } + try client.blockOn(frame_id, entry.transfer_id); + break :blk true; + }; + defer if (blocked) { + client.releaseBlocking(frame_id); + }; + while (true) { // imported_modules can be mutated by client.tick, so we need to lookup // the entry on each iteration. @@ -1018,6 +1042,9 @@ const ImportedModule = struct { // will never collect it (see preloadModuleHint). A dynamic import may // adopt a hint entry outright (see getAsyncImport). hint: bool = false, + // The transfer fetching the module, which waitForImport lets through the + // HttpClient's gate. + transfer_id: u32 = 0, state: State, buffer: std.ArrayList(u8) = .empty, diff --git a/src/browser/tests/page/module_reentrant_import.html b/src/browser/tests/page/module_reentrant_import.html new file mode 100644 index 000000000..9a776b319 --- /dev/null +++ b/src/browser/tests/page/module_reentrant_import.html @@ -0,0 +1,25 @@ + + + + + + diff --git a/src/browser/tests/page/modules/reentrant-a.js b/src/browser/tests/page/modules/reentrant-a.js new file mode 100644 index 000000000..38ef85b4d --- /dev/null +++ b/src/browser/tests/page/modules/reentrant-a.js @@ -0,0 +1,2 @@ +import { c } from "./reentrant-c.js?delay_ms=400"; +export const a = c; diff --git a/src/browser/tests/page/modules/reentrant-c.js b/src/browser/tests/page/modules/reentrant-c.js new file mode 100644 index 000000000..93c974371 --- /dev/null +++ b/src/browser/tests/page/modules/reentrant-c.js @@ -0,0 +1 @@ +export const c = 'c'; diff --git a/src/network/HttpClient.zig b/src/network/HttpClient.zig index 59275c0e7..b33c0391d 100644 --- a/src/network/HttpClient.zig +++ b/src/network/HttpClient.zig @@ -172,9 +172,10 @@ test_inbox: if (lp.IS_TEST) ?*Inbox else void = if (lp.IS_TEST) null else {}, max_response_size: usize, -// While a frame has a blocking (synchronous) request in flight, dispatch -// holds back every other transfer for that frame so their callbacks can't -// run JS while the parser is on the stack. frame_id -> blocking transfer id. +// While a frame has a blocking (synchronous) request in flight, or waits for +// a module import, dispatch holds back every other transfer for that frame so +// their callbacks can't run JS while the parser is on the stack. +// frame_id -> blocking transfer id. blocking_requests: std.AutoHashMapUnmanaged(u32, u32) = .empty, // Count of transfers parked for CDP interception (request or auth phase). @@ -1424,9 +1425,26 @@ fn processTransfer(self: *Client, transfer: *Transfer) !void { transfer.state = .queued; } +// Until released, a sync tick delivers only `transfer_id` +pub fn blockOn(self: *Client, frame_id: u32, transfer_id: u32) !void { + try self.blocking_requests.putNoClobber(self.allocator, frame_id, transfer_id); + + // maybe the transfer was already gated + var node = self.gated_queue.first; + while (node) |n| : (node = n.next) { + const transfer: *Transfer = @fieldParentPtr("_queue_node", n); + if (transfer.id == transfer_id) { + transfer._gated = false; + self.gated_queue.remove(n); + self.dispatch_queue.append(n); + return; + } + } +} + // A blocking request is complete. Any completed transfer that was placed in the // gated_queue because of it can now be placed back in the dispatch queue. -fn releaseBlocking(self: *Client, frame_id: u32) void { +pub fn releaseBlocking(self: *Client, frame_id: u32) void { _ = self.blocking_requests.remove(frame_id); // items were added to the gate in order, so walking backwards restores that // order. (Order might not matter, but preserving it costs nothing) @@ -2492,7 +2510,7 @@ pub const Transfer = struct { req.shutdown_callback = SyncContext.shutdownCallback; const frame_id = req.frame_id; - client.blocking_requests.putNoClobber(client.allocator, frame_id, self.id) catch |err| { + client.blockOn(frame_id, self.id) catch |err| { self.deinit(); return err; };