From f7a32c05a8603699372d681fc2e6f34508ebb008 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 3 Jun 2026 08:24:47 +0800 Subject: [PATCH 1/9] Improve / fix InterceptLayer.intercepted count tracking The intercept state is currently split and hard to keep consistent and even just reason about. InterceptLayer keeps the `intercepted` count, but CDP's `BrowserContext` has its intercepted lookup. This isn't a problem per se, but you BrowserContext.deinit tries to decrement `InterceptLayer.intercepted` which is only safe if we can guarantee that the two are in sync. Which we can't. This commit simplifies the upkeep of `InterceptLayer.intercepted` and uses the Transfer's state on unpark/deinit to decrement it. The CDP layer no longer cares about / has to maintain the count. Driven by this crash report: BrowserContext.deinit.intercepted --- value: 0 /home/runner/work/browser/browser/src/lightpanda.zig:279:25: 0x2871842 in deinit (lightpanda) /home/runner/work/browser/browser/src/cdp/CDP.zig:127:18: 0x28c3f45 in deinit (lightpanda) /home/runner/work/browser/browser/src/Server.zig:186:21: 0x2827997 in handleConnection (lightpanda) /home/runner/work/_temp/6dc322a8-c74f-4990-9660-4cc6dcfb9352/zig-x86_64-linux-0.15.2/lib/std/Thread.zig:509:13: 0x269c233 in entryFn (lightpanda) ???:?:?: 0x7fce7ccabd57 in ??? (libc.so.6) Unwind information for `libc.so.6:0x7fce7ccabd57` was not available, trace may be incomplete on 1.0.0-nightly.6542+94ba0791 --- src/browser/HttpClient.zig | 31 ++++++++++++++++++---- src/cdp/CDP.zig | 11 +------- src/cdp/domains/fetch.zig | 34 ++++++++++++------------- src/network/layer/InterceptionLayer.zig | 3 --- 4 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/browser/HttpClient.zig b/src/browser/HttpClient.zig index 874ed98b..e62e9f20 100644 --- a/src/browser/HttpClient.zig +++ b/src/browser/HttpClient.zig @@ -1564,10 +1564,28 @@ pub const Transfer = struct { // post-perform would need to be improved), pub fn unpark(self: *Transfer) void { lp.assert(self.state == .parked, "Transfer.unpark", .{ .state = self.state }); + self.leaveIntercept(); self.state = .created; } + // Decrement the interception counter iff this transfer is currently + // parked for CDP interception. + fn leaveIntercept(self: *Transfer) void { + if (self.state != .parked) { + return; + } + switch (self.state.parked) { + .robots => {}, + .intercept_request, .intercept_auth => { + const intercept_layer = &self.client.interception_layer; + lp.assert(intercept_layer.intercepted > 0, "Transfer.leaveIntercept", .{ .value = intercept_layer.intercepted }); + intercept_layer.intercepted -= 1; + }, + } + } + pub fn deinit(self: *Transfer) void { + self.leaveIntercept(); if (self._conn) |c| { self.client.removeConn(c); self._conn = null; @@ -1904,9 +1922,9 @@ pub const Transfer = struct { log.debug(.http, "abort auth transfer", .{ .intercepted = self.client.interception_layer.intercepted }); } - self.client.interception_layer.intercepted -= 1; + // The transfer is still .parked(.intercept_auth) + // abort -> deinit -> leaveIntercept decrements the counter. self.abort(error.AbortAuthChallenge); - return; } // headerDoneCallback is called once the headers have been read. @@ -2067,13 +2085,16 @@ pub const Transfer = struct { pub fn continueTransfer(self: *Client, transfer: *Transfer) !void { if (comptime IS_DEBUG) { - lp.assert(self.interception_layer.intercepted > 0, "HttpClient.continueTransfer", .{ .value = self.interception_layer.intercepted }); log.debug(.http, "continue transfer", .{ .intercepted = self.interception_layer.intercepted }); } - self.interception_layer.intercepted -= 1; transfer.unpark(); - return self.process(transfer); + self.process(transfer) catch |err| { + if (transfer.state == .created) { + transfer.abort(err); + } + return err; + }; } const Noop = struct { diff --git a/src/cdp/CDP.zig b/src/cdp/CDP.zig index 57bef94c..05e66933 100644 --- a/src/cdp/CDP.zig +++ b/src/cdp/CDP.zig @@ -618,18 +618,9 @@ pub const BrowserContext = struct { // abort all intercepted requests before closing the session/page // since some of these might callback into the page/scriptmanager. - // intercept_state stores ids — look each one up; if it's already - // gone (out-of-band destroy), there's nothing to abort, but the - // intercepted counter still needs decrementing because we - // incremented it on pause. + // intercept_state stores ids. const http_client = &browser.http_client; for (self.intercept_state.pendingIntercepts()) |transfer_id| { - lp.assert( - http_client.interception_layer.intercepted > 0, - "BrowserContext.deinit.intercepted", - .{ .value = http_client.interception_layer.intercepted }, - ); - http_client.interception_layer.intercepted -= 1; if (http_client.findTransfer(transfer_id)) |transfer| { transfer.abort(error.ClientDisconnect); } diff --git a/src/cdp/domains/fetch.zig b/src/cdp/domains/fetch.zig index 0d8f2347..cc62921d 100644 --- a/src/cdp/domains/fetch.zig +++ b/src/cdp/domains/fetch.zig @@ -198,6 +198,7 @@ pub fn requestIntercept(bc: *CDP.BrowserContext, intercept: *const Notification. const transfer = intercept.transfer; try bc.intercept_state.put(transfer.id); + errdefer _ = bc.intercept_state.remove(transfer.id); try bc.cdp.sendEvent("Fetch.requestPaused", .{ .requestId = &id.toInterceptId(transfer.id), @@ -332,23 +333,21 @@ fn continueWithAuth(cmd: *CDP.Command) !void { return cmd.sendResult(null, .{}); } - // TODO: double-decrement of interception_layer.intercepted if - // continueTransfer fails: continueTransfer decrements unconditionally, - // and the errdefer below decrements again via abortAuthChallenge. - // Worse: if continueTransfer's failure path destroys the transfer - // (start_callback fail in makeRequest), this errdefer hits a freed - // transfer. Pre-existing; needs makeRequest failure-semantics cleanup. - errdefer transfer.abortAuthChallenge(); - - transfer.updateCredentials(try std.fmt.allocPrintSentinel( - transfer.arena, - "{s}:{s}", - .{ - params.authChallengeResponse.username, - params.authChallengeResponse.password, - }, - 0, - )); + { + // The transfer is still parked here; if building the credentials + // fails, release it. Scoped so the errdefer does NOT cover + // continueTransfer (which owns its failures). + errdefer transfer.abortAuthChallenge(); + transfer.updateCredentials(try std.fmt.allocPrintSentinel( + transfer.arena, + "{s}:{s}", + .{ + params.authChallengeResponse.username, + params.authChallengeResponse.password, + }, + 0, + )); + } try client.continueTransfer(transfer); return cmd.sendResult(null, .{}); @@ -440,6 +439,7 @@ pub fn requestAuthRequired(bc: *CDP.BrowserContext, intercept: *const Notificati const transfer = intercept.transfer; try bc.intercept_state.put(transfer.id); + errdefer _ = bc.intercept_state.remove(transfer.id); const request = &transfer.req; const challenge = transfer._auth_challenge orelse return error.NullAuthChallenge; diff --git a/src/network/layer/InterceptionLayer.zig b/src/network/layer/InterceptionLayer.zig index 25c49ae9..9c391572 100644 --- a/src/network/layer/InterceptionLayer.zig +++ b/src/network/layer/InterceptionLayer.zig @@ -194,7 +194,6 @@ pub fn continueRequest(self: *InterceptionLayer, transfer: *Transfer) anyerror!v lp.assert(self.intercepted > 0, "InterceptionLayer.continueRequest", .{ .value = self.intercepted }); log.debug(.http, "continue transfer", .{ .intercepted = self.intercepted }); } - self.intercepted -= 1; // Resume the layer chain. Ownership is re-handed to whichever subsequent // layer commits the transfer (queue, multi, or another park). If the @@ -214,7 +213,6 @@ pub fn abortRequest(self: *InterceptionLayer, transfer: *Transfer) void { lp.assert(self.intercepted > 0, "InterceptionLayer.abortRequest", .{ .value = self.intercepted }); log.debug(.http, "abort transfer", .{ .intercepted = self.intercepted }); } - self.intercepted -= 1; transfer.abort(error.Abort); } @@ -229,7 +227,6 @@ pub fn fulfillRequest( lp.assert(self.intercepted > 0, "InterceptionLayer.fulfillRequest", .{ .value = self.intercepted }); log.debug(.http, "fulfill transfer", .{ .intercepted = self.intercepted }); } - self.intercepted -= 1; // `done` flips true once we've called the user's done_callback. If // done_callback itself throws, the user already saw their end-of-flow From 5487174e2b61c6f4fb3ed13312cb67acd86c6108 Mon Sep 17 00:00:00 2001 From: Rohit <71192000+rohitsux@users.noreply.github.com> Date: Wed, 3 Jun 2026 06:12:55 +0530 Subject: [PATCH 2/9] feat(webapi): reflect src, type, width and height on HTMLEmbedElement Add the reflected attributes src, type, width and height to HTMLEmbedElement per HTML 4.8.6 (https://html.spec.whatwg.org/multipage/iframe-embed-object.html#htmlembedelement). src is a URL-reflecting attribute (resolved against the document base, mirroring HTMLImageElement.src); type, width and height are plain DOMString reflections. Mirrors the reflection idiom used by HTMLMarqueeElement / HTMLImageElement. --- src/browser/tests/element/html/embed.html | 56 +++++++++++++++++++++++ src/browser/webapi/element/html/Embed.zig | 51 +++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 src/browser/tests/element/html/embed.html diff --git a/src/browser/tests/element/html/embed.html b/src/browser/tests/element/html/embed.html new file mode 100644 index 00000000..b4e689db --- /dev/null +++ b/src/browser/tests/element/html/embed.html @@ -0,0 +1,56 @@ + + + + + + + + + + + diff --git a/src/browser/webapi/element/html/Embed.zig b/src/browser/webapi/element/html/Embed.zig index a3292cb1..6c00c7aa 100644 --- a/src/browser/webapi/element/html/Embed.zig +++ b/src/browser/webapi/element/html/Embed.zig @@ -17,6 +17,7 @@ // along with this program. If not, see . const js = @import("../../../js/js.zig"); +const Frame = @import("../../../Frame.zig"); const Node = @import("../../Node.zig"); const Element = @import("../../Element.zig"); const HtmlElement = @import("../Html.zig"); @@ -27,10 +28,50 @@ _proto: *HtmlElement, pub fn asElement(self: *Embed) *Element { return self._proto._proto; } +pub fn asConstElement(self: *const Embed) *const Element { + return self._proto._proto; +} pub fn asNode(self: *Embed) *Node { return self.asElement().asNode(); } +pub fn getSrc(self: *const Embed, frame: *Frame) ![]const u8 { + const element = self.asConstElement(); + const src = element.getAttributeSafe(comptime .wrap("src")) orelse return ""; + if (src.len == 0) { + return ""; + } + return element.asConstNode().resolveURL(src, frame, .{}); +} + +pub fn setSrc(self: *Embed, value: []const u8, frame: *Frame) !void { + try self.asElement().setAttributeSafe(comptime .wrap("src"), .wrap(value), frame); +} + +pub fn getType(self: *const Embed) []const u8 { + return self.asConstElement().getAttributeSafe(comptime .wrap("type")) orelse ""; +} + +pub fn setType(self: *Embed, value: []const u8, frame: *Frame) !void { + try self.asElement().setAttributeSafe(comptime .wrap("type"), .wrap(value), frame); +} + +pub fn getWidth(self: *const Embed) []const u8 { + return self.asConstElement().getAttributeSafe(comptime .wrap("width")) orelse ""; +} + +pub fn setWidth(self: *Embed, value: []const u8, frame: *Frame) !void { + try self.asElement().setAttributeSafe(comptime .wrap("width"), .wrap(value), frame); +} + +pub fn getHeight(self: *const Embed) []const u8 { + return self.asConstElement().getAttributeSafe(comptime .wrap("height")) orelse ""; +} + +pub fn setHeight(self: *Embed, value: []const u8, frame: *Frame) !void { + try self.asElement().setAttributeSafe(comptime .wrap("height"), .wrap(value), frame); +} + pub const JsApi = struct { pub const bridge = js.Bridge(Embed); @@ -39,4 +80,14 @@ pub const JsApi = struct { pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; }; + + pub const height = bridge.accessor(Embed.getHeight, Embed.setHeight, .{ .ce_reactions = true }); + pub const src = bridge.accessor(Embed.getSrc, Embed.setSrc, .{ .ce_reactions = true }); + pub const @"type" = bridge.accessor(Embed.getType, Embed.setType, .{ .ce_reactions = true }); + pub const width = bridge.accessor(Embed.getWidth, Embed.setWidth, .{ .ce_reactions = true }); }; + +const testing = @import("../../../../testing.zig"); +test "WebApi: HTML.Embed" { + try testing.htmlRunner("element/html/embed.html", .{}); +} From 1c70fcb8c50a38e3b7db584403fa83a728254f20 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 3 Jun 2026 10:10:26 +0800 Subject: [PATCH 3/9] page.findFrame searches popups too If you run WPT tests, you'll see a fair number of `FrameNotFound`. This comes from the cdp frameNavigated notification handler and it happens because our frame lookup only considers iframes, not popups. This commit includes popup in the search. I don't expect this to change WPT results. --- src/browser/Page.zig | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index f9d63947..a6fa6749 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -244,7 +244,10 @@ pub fn scheduleNavigation(self: *Page, frame: *Frame) !void { } pub fn findFrameByFrameId(self: *Page, frame_id: u32) ?*Frame { - return findFrameBy(&self.frame, "_frame_id", frame_id); + if (findFrameBy(&self.frame, "_frame_id", frame_id)) |found| { + return found; + } + return self.findPopupBy("_frame_id", frame_id); } // Returns the popup Frame registered under `name`, or null. @@ -258,11 +261,16 @@ pub fn findPopupByName(self: *Page, name: []const u8) ?*Frame { } pub fn findFrameByLoaderId(self: *Page, loader_id: u32) ?*Frame { - return findFrameBy(&self.frame, "_loader_id", loader_id); + if (findFrameBy(&self.frame, "_loader_id", loader_id)) |found| { + return found; + } + return self.findPopupBy("_loader_id", loader_id); } fn findFrameBy(frame: *Frame, comptime field: []const u8, id: u32) ?*Frame { - if (@field(frame, field) == id) return frame; + if (@field(frame, field) == id) { + return frame; + } for (frame.child_frames.items) |f| { if (findFrameBy(f, field, id)) |found| { return found; @@ -270,3 +278,12 @@ fn findFrameBy(frame: *Frame, comptime field: []const u8, id: u32) ?*Frame { } return null; } + +fn findPopupBy(self: *Page, comptime field: []const u8, id: u32) ?*Frame { + for (self.popups.items) |frame| { + if (findFrameBy(frame, field, id)) |found| { + return found; + } + } + return null; +} From 24e379f29a445f3806a07a5113876cc059736db5 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 3 Jun 2026 10:40:29 +0800 Subject: [PATCH 4/9] Don't overlog ClientDisconnected Since the CDP rework, error.ClientDisconnected surfaces to the Runner. There's no reason to log this (especially as at an error level). It's perfectly normal and has already been logged at the CDP level. --- src/browser/Runner.zig | 1 + 1 file changed, 1 insertion(+) diff --git a/src/browser/Runner.zig b/src/browser/Runner.zig index 592f9dd5..a8b7bf53 100644 --- a/src/browser/Runner.zig +++ b/src/browser/Runner.zig @@ -93,6 +93,7 @@ fn _wait(self: *Runner, comptime is_cdp: bool, opts: WaitOpts) !void { const tick_result = self._tick(is_cdp, tick_opts) catch |err| { switch (err) { error.JsError => {}, // already logged (with hopefully more context) + error.ClientDisconnected => {}, // CDP layer already logged this else => log.err(.browser, "session wait", .{ .err = err, .url = self.frame.url, From 2fc9d6107027619ca5f23ff34f6abe3f7d16aabc Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Wed, 3 Jun 2026 11:06:24 +0800 Subject: [PATCH 5/9] Support for "module" type workers. Pretty basic, worker started with the {type: 'module'} option has 2 practical differences: 1 - importScript isn't allowed (TypeError) 2 - the initial script is loaded as a module Seen in various WPT tests, but also saw this usage on mastodon. --- src/browser/js/Env.zig | 2 +- src/browser/tests/worker/module-worker.js | 25 +++++++++++++++ src/browser/tests/worker/worker.html | 23 ++++++++++++++ src/browser/webapi/Worker.zig | 38 ++++++++++++++++++----- src/browser/webapi/WorkerGlobalScope.zig | 6 ++++ 5 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 src/browser/tests/worker/module-worker.js diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index 45452ffc..1fa807f8 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -620,7 +620,7 @@ test "Env: Worker context " { const frame = try session.createPage(); defer session.removePage(); - const worker = try @import("../webapi/Worker.zig").init("http://localhost:9582/src/browser/tests/testing.js", frame); + const worker = try @import("../webapi/Worker.zig").init("http://localhost:9582/src/browser/tests/testing.js", null, frame); var ls: js.Local.Scope = undefined; worker._worker_scope.js.localScope(&ls); diff --git a/src/browser/tests/worker/module-worker.js b/src/browser/tests/worker/module-worker.js new file mode 100644 index 00000000..1b1aa7b6 --- /dev/null +++ b/src/browser/tests/worker/module-worker.js @@ -0,0 +1,25 @@ +// A module worker (`new Worker(url, { type: "module" })`). Unlike a classic +// worker, the entry script may use top-level static `import`/`export`, and +// `importScripts()` is not supported (it throws a TypeError). +import { baseValue } from './modules/base.js'; +import { importedValue, localValue } from './modules/importer.js'; + +export const exported = 'top-level-export-ok'; + +let importScriptsError = null; +try { + importScripts('./import-script1.js'); +} catch (e) { + importScriptsError = e.constructor.name; +} + +onmessage = function (event) { + postMessage({ + echo: event.data, + baseValue: baseValue, + importedValue: importedValue, + localValue: localValue, + importScriptsError: importScriptsError, + from: 'module-worker', + }); +}; diff --git a/src/browser/tests/worker/worker.html b/src/browser/tests/worker/worker.html index cabe5d14..881c6b6c 100644 --- a/src/browser/tests/worker/worker.html +++ b/src/browser/tests/worker/worker.html @@ -380,6 +380,29 @@ } + +