From ae2359c1afb2c973c17c8117300535df63b49dce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Fri, 3 Jul 2026 23:52:29 +0200 Subject: [PATCH 1/2] script: fix segfault when a tool call triggers navigation A press/click that submits a form or follows a link commits a replacement Page inside the tool call itself (finalizeAction -> awaitQueuedNavigation), freeing the Frame that Session._tool_frame_override still pointed at; finalizeAction then dereferenced it via requireFrame -> currentFrame. Any script whose press/click navigated crashed on replay. Store a frame id instead of a pointer and resolve it at every currentFrame call. The replacement page keeps the frame id (see commitPendingPage), so the same handle stays routable across the swap. --- src/browser/Session.zig | 14 +++++++++----- src/script/Runtime.zig | 25 +++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 4d113012f..d9f0a636a 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -72,7 +72,10 @@ _nav_cursor: usize = 0, // Set by the agent script Runtime around one tool call so each `Page` handle's // tools act on its own frame, not `pages[0]`. Null for all other callers. -_tool_frame_override: ?*Frame = null, +// A frame id, not a pointer: a tool-triggered navigation commits a replacement +// Page mid-call, freeing the old Frame while keeping its frame id (see +// `commitPendingPage`). +_tool_frame_override: ?u32 = null, // Loader IDs are scoped to the Session: each new BrowserContext gets a // fresh counter. Frame IDs (`frame_id_gen`) live on `Browser` instead so @@ -444,8 +447,9 @@ pub fn primaryPage(self: *Session) ?PageHandle { // DEPRECATED. Exists during our transition to multi-page sessions. pub fn currentFrame(self: *Session) ?*Frame { - if (self._tool_frame_override) |frame| { - return frame; + if (self._tool_frame_override) |frame_id| { + // No pages[0] fallthrough: the override targets one specific page. + return self.findFrameByFrameId(frame_id); } if (self.pages.items.len == 0) { return null; @@ -458,8 +462,8 @@ pub fn currentFrame(self: *Session) ?*Frame { } /// See `_tool_frame_override`. Pass null to clear. -pub fn setToolFrameOverride(self: *Session, frame: ?*Frame) void { - self._tool_frame_override = frame; +pub fn setToolFrameOverride(self: *Session, frame_id: ?u32) void { + self._tool_frame_override = frame_id; } // Multi-page aware: frame ids are globally unique (monotonic on `Browser`). diff --git a/src/script/Runtime.zig b/src/script/Runtime.zig index 911b5d60a..2ab67361c 100644 --- a/src/script/Runtime.zig +++ b/src/script/Runtime.zig @@ -449,10 +449,10 @@ fn invoke(self: *Runtime, tool: BrowserTool, info: *const v8.FunctionCallbackInf // `pages[0]` is not necessarily this handle's page. const frame_id = self.receiverFrameId(context, info) orelse return self.throwError("page is not navigated or has been closed; call page.goto(url) first"); - const frame = self.session.findFrameByFrameId(frame_id) orelse + if (self.session.findFrameByFrameId(frame_id) == null) return self.throwError("page handle is no longer valid; the page was closed"); - self.session.setToolFrameOverride(frame); + self.session.setToolFrameOverride(frame_id); defer self.session.setToolFrameOverride(null); const result = self.callTool(arena, tool, args) catch |err| switch (err) { @@ -1083,6 +1083,27 @@ test "agent script runtime: parallel gotos coexist and route per page" { ); } +test "agent script runtime: a tool-triggered navigation keeps the handle routable" { + defer testing.reset(); + defer testing.test_session.closeAllPages(); + + var registry = CDPNode.Registry.init(testing.allocator); + defer registry.deinit(); + + const runtime = try Runtime.init(testing.allocator, testing.test_app, testing.test_session, ®istry); + defer runtime.deinit(); + + // click commits a replacement Page mid-call; the frame override and the + // handle's frame id must both survive the swap. + try runTestScript(runtime, + \\const page = new Page(); + \\await page.goto("http://localhost:9582/src/browser/tests/mcp_nav.html"); + \\page.click("#navlink"); + \\const href = page.evaluate("location.href"); + \\if (href !== "about:blank") throw new Error("expected about:blank, got: " + href); + ); +} + test "agent script runtime: re-goto on the same page object replaces its page" { defer testing.reset(); defer testing.test_session.closeAllPages(); From cfd61ca69e82ebe9fc6666a655a7f22e7a9915d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Arrufat?= Date: Fri, 3 Jul 2026 23:52:40 +0200 Subject: [PATCH 2/2] tools: resolve $LP_* placeholders in startGoto The script runtime's async goto path calls startGoto directly, bypassing the substituteStringArgs pass that call() applies to every other tool, so $LP_* placeholders in goto URLs (a base URL, a username in the path) stayed literal and the navigation failed on replay. --- src/browser/tools.zig | 3 ++- src/script/Runtime.zig | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/browser/tools.zig b/src/browser/tools.zig index b6587a3e4..f020a5fce 100644 --- a/src/browser/tools.zig +++ b/src/browser/tools.zig @@ -1856,7 +1856,8 @@ pub fn startGoto( arguments: ?std.json.Value, receiver_frame_id: ?u32, ) ToolError!StartedGoto { - const args = try parseArgs(GotoParams, arena, arguments); + // This path bypasses `call`, so it needs its own `$LP_*` substitution pass. + const args = try parseArgs(GotoParams, arena, try substituteStringArgs(arena, .goto, arguments)); if (receiver_frame_id) |fid| { if (session.livePage(fid)) |page| { registry.resetFrame(arena, &page.frame); diff --git a/src/script/Runtime.zig b/src/script/Runtime.zig index 2ab67361c..8e923b916 100644 --- a/src/script/Runtime.zig +++ b/src/script/Runtime.zig @@ -1083,6 +1083,30 @@ test "agent script runtime: parallel gotos coexist and route per page" { ); } +test "agent script runtime: goto resolves $LP_* placeholders" { + defer testing.reset(); + defer testing.test_session.closeAllPages(); + + var registry = CDPNode.Registry.init(testing.allocator); + defer registry.deinit(); + + const runtime = try Runtime.init(testing.allocator, testing.test_app, testing.test_session, ®istry); + defer runtime.deinit(); + + _ = setenv(@constCast("LP_RUNTIME_GOTO_BASE"), @constCast("http://localhost:9582"), 1); + defer _ = unsetenv(@constCast("LP_RUNTIME_GOTO_BASE")); + + try runTestScript(runtime, + \\const page = new Page(); + \\await page.goto("$LP_RUNTIME_GOTO_BASE/src/browser/tests/mcp_actions.html"); + \\const { btn } = page.extract({ btn: "#btn" }); + \\if (btn !== "Click Me") throw new Error("wrong page: " + btn); + ); +} + +extern fn setenv(name: [*:0]u8, value: [*:0]u8, override: c_int) c_int; +extern fn unsetenv(name: [*:0]u8) c_int; + test "agent script runtime: a tool-triggered navigation keeps the handle routable" { defer testing.reset(); defer testing.test_session.closeAllPages();