From a11ae912b4e3f61570f81df476c9e96ee32af2e0 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 26 Jan 2026 19:18:32 +0800 Subject: [PATCH] Add finalizer to Response and use an pooled arena Unlike XHR, Response is a bit more complicated as it can exist in Zig code without ever being given to v8. So we need to track this handoff to know who is responsible for freeing it (zig code, on error/shutdown) or v8 code after promise resolution. This also cleansup a bad merge for the XHR finalizer and adds cleaning up the `XMLHttpRequestEventTarget` callbacks. --- src/browser/Factory.zig | 6 +- src/browser/ScriptManager.zig | 2 +- src/browser/js/bridge.zig | 2 +- src/browser/webapi/net/Fetch.zig | 68 ++++++++++++++----- src/browser/webapi/net/Response.zig | 34 ++++++++-- src/browser/webapi/net/XMLHttpRequest.zig | 42 +++++++++--- .../webapi/net/XMLHttpRequestEventTarget.zig | 42 ++++++------ src/http/Client.zig | 8 ++- 8 files changed, 147 insertions(+), 57 deletions(-) diff --git a/src/browser/Factory.zig b/src/browser/Factory.zig index 205e70b7..ba4e3ddd 100644 --- a/src/browser/Factory.zig +++ b/src/browser/Factory.zig @@ -37,6 +37,8 @@ const XMLHttpRequestEventTarget = @import("webapi/net/XMLHttpRequestEventTarget. const Blob = @import("webapi/Blob.zig"); const AbstractRange = @import("webapi/AbstractRange.zig"); +const Allocator = std.mem.Allocator; + const IS_DEBUG = builtin.mode == .Debug; const assert = std.debug.assert; @@ -344,9 +346,7 @@ pub fn svgElement(self: *Factory, tag_name: []const u8, child: anytype) !*@TypeO return chain.get(4); } -pub fn xhrEventTarget(self: *Factory, child: anytype) !*@TypeOf(child) { - const allocator = self._slab.allocator(); - +pub fn xhrEventTarget(_: *const Factory, allocator: Allocator, child: anytype) !*@TypeOf(child) { return try AutoPrototypeChain( &.{ EventTarget, XMLHttpRequestEventTarget, @TypeOf(child) }, ).create(allocator, child); diff --git a/src/browser/ScriptManager.zig b/src/browser/ScriptManager.zig index b2b64b86..95ad89cc 100644 --- a/src/browser/ScriptManager.zig +++ b/src/browser/ScriptManager.zig @@ -754,7 +754,7 @@ pub const Script = struct { return; } - switch (self.mode) { + switch (self.mode) { .import_async => |ia| ia.callback(ia.data, error.FailedToLoad), .import => { const entry = manager.imported_modules.getPtr(self.url).?; diff --git a/src/browser/js/bridge.zig b/src/browser/js/bridge.zig index 27359f88..9860d98f 100644 --- a/src/browser/js/bridge.zig +++ b/src/browser/js/bridge.zig @@ -92,7 +92,7 @@ pub fn Builder(comptime T: type) type { return entries; } - pub fn finalizer(comptime func: *const fn (self: *T, comptime shutdown: bool) void) Finalizer { + pub fn finalizer(comptime func: *const fn (self: *T, shutdown: bool) void) Finalizer { return .{ .from_zig = struct { fn wrap(ptr: *anyopaque) void { diff --git a/src/browser/webapi/net/Fetch.zig b/src/browser/webapi/net/Fetch.zig index 98aaf1a4..6be804f4 100644 --- a/src/browser/webapi/net/Fetch.zig +++ b/src/browser/webapi/net/Fetch.zig @@ -37,22 +37,26 @@ _url: []const u8, _buf: std.ArrayList(u8), _response: *Response, _resolver: js.PromiseResolver.Global, +_owns_response: bool, pub const Input = Request.Input; pub const InitOpts = Request.InitOpts; pub fn init(input: Input, options: ?InitOpts, page: *Page) !js.Promise { const request = try Request.init(input, options, page); + const response = try Response.init(null, .{ .status = 0 }, page); + errdefer response.deinit(true); const resolver = page.js.local.?.createPromiseResolver(); - const fetch = try page.arena.create(Fetch); + const fetch = try response._arena.create(Fetch); fetch.* = .{ ._page = page, ._buf = .empty, - ._url = try page.arena.dupe(u8, request._url), + ._url = try response._arena.dupe(u8, request._url), ._resolver = try resolver.persist(), - ._response = try Response.init(null, .{ .status = 0 }, page), + ._response = response, + ._owns_response = true, }; const http_client = page._session.browser.http_client; @@ -74,26 +78,30 @@ pub fn init(input: Input, options: ?InitOpts, page: *Page) !js.Promise { .headers = headers, .resource_type = .fetch, .cookie_jar = &page._session.cookie_jar, + .start_callback = httpStartCallback, .header_callback = httpHeaderDoneCallback, .data_callback = httpDataCallback, .done_callback = httpDoneCallback, .error_callback = httpErrorCallback, + .shutdown_callback = httpShutdownCallback, }); return resolver.promise(); } -pub fn deinit(self: *Fetch) void { - if (self.transfer) |transfer| { - transfer.abort(); - self.transfer = null; +fn httpStartCallback(transfer: *Http.Transfer) !void { + const self: *Fetch = @ptrCast(@alignCast(transfer.ctx)); + if (comptime IS_DEBUG) { + log.debug(.http, "request start", .{ .url = self._url, .source = "fetch" }); } + self._response._transfer = transfer; } fn httpHeaderDoneCallback(transfer: *Http.Transfer) !bool { const self: *Fetch = @ptrCast(@alignCast(transfer.ctx)); + const arena = self._response._arena; if (transfer.getContentLength()) |cl| { - try self._buf.ensureTotalCapacity(self._page.arena, cl); + try self._buf.ensureTotalCapacity(arena, cl); } const res = self._response; @@ -108,12 +116,12 @@ fn httpHeaderDoneCallback(transfer: *Http.Transfer) !bool { } res._status = header.status; - res._url = try self._page.arena.dupeZ(u8, std.mem.span(header.url)); + res._url = try arena.dupeZ(u8, std.mem.span(header.url)); res._is_redirected = header.redirect_count > 0; // Determine response type based on origin comparison - const page_origin = URL.getOrigin(self._page.call_arena, self._page.url) catch null; - const response_origin = URL.getOrigin(self._page.call_arena, res._url) catch null; + const page_origin = URL.getOrigin(arena, self._page.url) catch null; + const response_origin = URL.getOrigin(arena, res._url) catch null; if (page_origin) |po| { if (response_origin) |ro| { @@ -139,17 +147,19 @@ fn httpHeaderDoneCallback(transfer: *Http.Transfer) !bool { fn httpDataCallback(transfer: *Http.Transfer, data: []const u8) !void { const self: *Fetch = @ptrCast(@alignCast(transfer.ctx)); - try self._buf.appendSlice(self._page.arena, data); + try self._buf.appendSlice(self._response._arena, data); } fn httpDoneCallback(ctx: *anyopaque) !void { const self: *Fetch = @ptrCast(@alignCast(ctx)); - self._response._body = self._buf.items; + var response = self._response; + response._transfer = null; + response._body = self._buf.items; log.info(.http, "request complete", .{ .source = "fetch", .url = self._url, - .status = self._response._status, + .status = response._status, .len = self._buf.items.len, }); @@ -157,12 +167,23 @@ fn httpDoneCallback(ctx: *anyopaque) !void { self._page.js.localScope(&ls); defer ls.deinit(); - return ls.toLocal(self._resolver).resolve("fetch done", self._response); + const js_val = try ls.local.zigValueToJs(self._response, .{}); + self._owns_response = false; + return ls.toLocal(self._resolver).resolve("fetch done", js_val); } fn httpErrorCallback(ctx: *anyopaque, err: anyerror) void { const self: *Fetch = @ptrCast(@alignCast(ctx)); - self._response._type = .@"error"; // Set type to error for network failures + + var response = self._response; + response._transfer = null; + // the response is only passed on v8 on success, if we're here, it's safe to + // clear this. (defer since `self is in the response's arena). + + defer if (self._owns_response) { + response.deinit(err == error.Abort); + self._owns_response = false; + }; var ls: js.Local.Scope = undefined; self._page.js.localScope(&ls); @@ -171,6 +192,21 @@ fn httpErrorCallback(ctx: *anyopaque, err: anyerror) void { ls.toLocal(self._resolver).reject("fetch error", @errorName(err)); } +fn httpShutdownCallback(ctx: *anyopaque) void { + const self: *Fetch = @ptrCast(@alignCast(ctx)); + if (comptime IS_DEBUG) { + // should always be true + std.debug.assert(self._owns_response); + } + + if (self._owns_response) { + var response = self._response; + response._transfer = null; + response.deinit(true); + self._owns_response = false; + } +} + const testing = @import("../../../testing.zig"); test "WebApi: fetch" { try testing.htmlRunner("net/fetch.html", .{}); diff --git a/src/browser/webapi/net/Response.zig b/src/browser/webapi/net/Response.zig index c413afbe..d4088c66 100644 --- a/src/browser/webapi/net/Response.zig +++ b/src/browser/webapi/net/Response.zig @@ -18,10 +18,12 @@ const std = @import("std"); const js = @import("../../js/js.zig"); +const Http = @import("../../../http/Http.zig"); const Page = @import("../../Page.zig"); const Headers = @import("Headers.zig"); const ReadableStream = @import("../streams/ReadableStream.zig"); + const Allocator = std.mem.Allocator; const Response = @This(); @@ -34,6 +36,7 @@ pub const Type = enum { opaqueredirect, }; +_page: *Page, _status: u16, _arena: Allocator, _headers: *Headers, @@ -42,6 +45,7 @@ _type: Type, _status_text: []const u8, _url: [:0]const u8, _is_redirected: bool, +_transfer: ?*Http.Transfer = null, const InitOpts = struct { status: u16 = 200, @@ -50,14 +54,19 @@ const InitOpts = struct { }; pub fn init(body_: ?[]const u8, opts_: ?InitOpts, page: *Page) !*Response { + const arena = try page.getArena(.{ .debug = "Response" }); + errdefer page.releaseArena(arena); + const opts = opts_ orelse InitOpts{}; // Store empty string as empty string, not null - const body = if (body_) |b| try page.arena.dupe(u8, b) else null; - const status_text = if (opts.statusText) |st| try page.dupeString(st) else ""; + const body = if (body_) |b| try arena.dupe(u8, b) else null; + const status_text = if (opts.statusText) |st| try arena.dupe(u8, st) else ""; - return page._factory.create(Response{ - ._arena = page.arena, + const self = try arena.create(Response); + self.* = .{ + ._page = page, + ._arena = arena, ._status = opts.status, ._status_text = status_text, ._url = "", @@ -65,7 +74,20 @@ pub fn init(body_: ?[]const u8, opts_: ?InitOpts, page: *Page) !*Response { ._type = .basic, ._is_redirected = false, ._headers = try Headers.init(opts.headers, page), - }); + }; + return self; +} + +pub fn deinit(self: *Response, shutdown: bool) void { + if (self._transfer) |transfer| { + if (shutdown) { + transfer.terminate(); + } else { + transfer.abort(error.Abort); + } + self._transfer = null; + } + self._page.releaseArena(self._arena); } pub fn getStatus(self: *const Response) u16 { @@ -134,6 +156,8 @@ pub const JsApi = struct { pub const name = "Response"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; + pub const weak = true; + pub const finalizer = bridge.finalizer(Response.deinit); }; pub const constructor = bridge.constructor(Response.init, .{}); diff --git a/src/browser/webapi/net/XMLHttpRequest.zig b/src/browser/webapi/net/XMLHttpRequest.zig index 35773921..dc75b6f7 100644 --- a/src/browser/webapi/net/XMLHttpRequest.zig +++ b/src/browser/webapi/net/XMLHttpRequest.zig @@ -81,7 +81,7 @@ const ResponseType = enum { pub fn init(page: *Page) !*XMLHttpRequest { const arena = try page.getArena(.{ .debug = "XMLHttpRequest" }); errdefer page.releaseArena(arena); - return page._factory.xhrEventTarget(XMLHttpRequest{ + return page._factory.xhrEventTarget(arena, XMLHttpRequest{ ._page = page, ._arena = arena, ._proto = undefined, @@ -89,7 +89,7 @@ pub fn init(page: *Page) !*XMLHttpRequest { }); } -pub fn deinit(self: *XMLHttpRequest, comptime shutdown: bool) void { +pub fn deinit(self: *XMLHttpRequest, shutdown: bool) void { if (self._transfer) |transfer| { if (shutdown) { transfer.terminate(); @@ -103,8 +103,33 @@ pub fn deinit(self: *XMLHttpRequest, comptime shutdown: bool) void { if (self._on_ready_state_change) |func| { page.js.release(func); } + + { + const proto = self._proto; + if (proto._on_abort) |func| { + page.js.release(func); + } + if (proto._on_error) |func| { + page.js.release(func); + } + if (proto._on_load) |func| { + page.js.release(func); + } + if (proto._on_load_end) |func| { + page.js.release(func); + } + if (proto._on_load_start) |func| { + page.js.release(func); + } + if (proto._on_progress) |func| { + page.js.release(func); + } + if (proto._on_timeout) |func| { + page.js.release(func); + } + } + page.releaseArena(self._arena); - page._factory.destroy(self); } fn asEventTarget(self: *XMLHttpRequest) *EventTarget { @@ -161,7 +186,6 @@ pub fn send(self: *XMLHttpRequest, body_: ?[]const u8) !void { if (self._ready_state != .opened) { return error.InvalidStateError; } - self._page.js.strongRef(self); if (body_) |b| { if (self._method != .GET and self._method != .HEAD) { @@ -319,7 +343,11 @@ fn httpHeaderDoneCallback(transfer: *Http.Transfer) !bool { if (header.contentType()) |ct| { self._response_mime = Mime.parse(ct) catch |e| { - log.info(.http, "invalid content type", .{.content_Type = ct, .err = e, .url = self._url,}); + log.info(.http, "invalid content type", .{ + .content_Type = ct, + .err = e, + .url = self._url, + }); return false; }; } @@ -399,8 +427,6 @@ fn httpDoneCallback(ctx: *anyopaque) !void { .total = loaded, .loaded = loaded, }, local, page); - - page.js.weakRef(self); } fn httpErrorCallback(ctx: *anyopaque, err: anyerror) void { @@ -408,7 +434,6 @@ fn httpErrorCallback(ctx: *anyopaque, err: anyerror) void { // http client will close it after an error, it isn't safe to keep around self._transfer = null; self.handleError(err); - self._page.js.weakRef(self); } pub fn abort(self: *XMLHttpRequest) void { @@ -417,7 +442,6 @@ pub fn abort(self: *XMLHttpRequest) void { transfer.abort(error.Abort); self._transfer = null; } - self._page.js.weakRef(self); } fn handleError(self: *XMLHttpRequest, err: anyerror) void { diff --git a/src/browser/webapi/net/XMLHttpRequestEventTarget.zig b/src/browser/webapi/net/XMLHttpRequestEventTarget.zig index 2a051f36..6b8abb19 100644 --- a/src/browser/webapi/net/XMLHttpRequestEventTarget.zig +++ b/src/browser/webapi/net/XMLHttpRequestEventTarget.zig @@ -26,13 +26,13 @@ const XMLHttpRequestEventTarget = @This(); _type: Type, _proto: *EventTarget, -_on_abort: ?js.Function.Global = null, -_on_error: ?js.Function.Global = null, -_on_load: ?js.Function.Global = null, -_on_load_end: ?js.Function.Global = null, -_on_load_start: ?js.Function.Global = null, -_on_progress: ?js.Function.Global = null, -_on_timeout: ?js.Function.Global = null, +_on_abort: ?js.Function.Temp = null, +_on_error: ?js.Function.Temp = null, +_on_load: ?js.Function.Temp = null, +_on_load_end: ?js.Function.Temp = null, +_on_load_start: ?js.Function.Temp = null, +_on_progress: ?js.Function.Temp = null, +_on_timeout: ?js.Function.Temp = null, pub const Type = union(enum) { request: *@import("XMLHttpRequest.zig"), @@ -71,85 +71,85 @@ pub fn dispatch(self: *XMLHttpRequestEventTarget, comptime event_type: DispatchT ); } -pub fn getOnAbort(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnAbort(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_abort; } pub fn setOnAbort(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_abort = try cb.persistWithThis(self); + self._on_abort = try cb.tempWithThis(self); } else { self._on_abort = null; } } -pub fn getOnError(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnError(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_error; } pub fn setOnError(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_error = try cb.persistWithThis(self); + self._on_error = try cb.tempWithThis(self); } else { self._on_error = null; } } -pub fn getOnLoad(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnLoad(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_load; } pub fn setOnLoad(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_load = try cb.persistWithThis(self); + self._on_load = try cb.tempWithThis(self); } else { self._on_load = null; } } -pub fn getOnLoadEnd(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnLoadEnd(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_load_end; } pub fn setOnLoadEnd(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_load_end = try cb.persistWithThis(self); + self._on_load_end = try cb.tempWithThis(self); } else { self._on_load_end = null; } } -pub fn getOnLoadStart(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnLoadStart(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_load_start; } pub fn setOnLoadStart(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_load_start = try cb.persistWithThis(self); + self._on_load_start = try cb.tempWithThis(self); } else { self._on_load_start = null; } } -pub fn getOnProgress(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnProgress(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_progress; } pub fn setOnProgress(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_progress = try cb.persistWithThis(self); + self._on_progress = try cb.tempWithThis(self); } else { self._on_progress = null; } } -pub fn getOnTimeout(self: *const XMLHttpRequestEventTarget) ?js.Function.Global { +pub fn getOnTimeout(self: *const XMLHttpRequestEventTarget) ?js.Function.Temp { return self._on_timeout; } pub fn setOnTimeout(self: *XMLHttpRequestEventTarget, cb_: ?js.Function) !void { if (cb_) |cb| { - self._on_timeout = try cb.persistWithThis(self); + self._on_timeout = try cb.tempWithThis(self); } else { self._on_timeout = null; } diff --git a/src/http/Client.zig b/src/http/Client.zig index 68a2d919..9d497597 100644 --- a/src/http/Client.zig +++ b/src/http/Client.zig @@ -186,7 +186,7 @@ pub fn abort(self: *Client) void { while (n) |node| { n = node.next; const transfer: *Transfer = @fieldParentPtr("_node", node); - self.transfer_pool.destroy(transfer); + transfer.kill(); } self.queue = .{}; @@ -392,6 +392,8 @@ fn requestFailed(self: *Client, transfer: *Transfer, err: anyerror, comptime exe if (execute_callback) { transfer.req.error_callback(transfer.ctx, err); + } else if (transfer.req.shutdown_callback) |cb| { + cb(transfer.ctx); } } @@ -781,6 +783,7 @@ pub const Request = struct { data_callback: *const fn (transfer: *Transfer, data: []const u8) anyerror!void, done_callback: *const fn (ctx: *anyopaque) anyerror!void, error_callback: *const fn (ctx: *anyopaque, err: anyerror) void, + shutdown_callback: ?*const fn (ctx: *anyopaque) void = null, const ResourceType = enum { document, @@ -995,6 +998,9 @@ pub const Transfer = struct { if (self._handle != null) { self.client.endTransfer(self); } + if (self.req.shutdown_callback) |cb| { + cb(self.ctx); + } self.deinit(); }