From cc82b1ae2537b34e1045df9ad0b3f5d92ee53560 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 23 May 2025 17:11:14 +0800 Subject: [PATCH 1/3] Fix connection memory leak When the idle pool is full and the oldest connection is freed, free the connection instance. --- src/http/client.zig | 64 ++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/src/http/client.zig b/src/http/client.zig index a6fa1615..adf040d0 100644 --- a/src/http/client.zig +++ b/src/http/client.zig @@ -49,8 +49,7 @@ pub const Client = struct { http_proxy: ?Uri, root_ca: tls.config.CertBundle, tls_verify_host: bool = true, - idle_connections: IdleConnections, - connection_pool: std.heap.MemoryPool(Connection), + connection_manager: ConnectionManager, const Opts = struct { tls_verify_host: bool = true, @@ -65,17 +64,16 @@ pub const Client = struct { const state_pool = try StatePool.init(allocator, max_concurrent); errdefer state_pool.deinit(allocator); - const idle_connections = IdleConnections.init(allocator, opts.max_idle_connection); - errdefer idle_connections.deinit(); + const connection_manager = ConnectionManager.init(allocator, opts.max_idle_connection); + errdefer connection_manager.deinit(); return .{ .root_ca = root_ca, .allocator = allocator, .state_pool = state_pool, .http_proxy = opts.http_proxy, - .idle_connections = idle_connections, .tls_verify_host = opts.tls_verify_host, - .connection_pool = std.heap.MemoryPool(Connection).init(allocator), + .connection_manager = connection_manager, }; } @@ -85,8 +83,7 @@ pub const Client = struct { self.root_ca.deinit(allocator); } self.state_pool.deinit(allocator); - self.idle_connections.deinit(); - self.connection_pool.deinit(); + self.connection_manager.deinit(); } pub fn request(self: *Client, method: Request.Method, uri: *const Uri) !Request { @@ -307,11 +304,11 @@ pub const Request = struct { self._connection = null; if (self._keepalive == false) { - self.destroyConnection(connection); + self._client.connection_manager.destroy(connection); return; } - self._client.idle_connections.put(connection) catch |err| { + self._client.connection_manager.keepIdle(connection) catch |err| { self.destroyConnection(connection); log.err("failed to release connection to pool: {}", .{err}); }; @@ -319,24 +316,21 @@ pub const Request = struct { fn createConnection(self: *Request, socket: posix.socket_t, blocking: bool) !*Connection { const client = self._client; - const connection = try client.connection_pool.create(); - errdefer client.connection_pool.destroy(connection); + const connection, const owned_host = try client.connection_manager.create(self._connect_host); connection.* = .{ .tls = null, .socket = socket, .blocking = blocking, + .host = owned_host, .port = self._connect_port, - .host = try client.allocator.dupe(u8, self._connect_host), }; return connection; } fn destroyConnection(self: *Request, connection: *Connection) void { - const client = self._client; - connection.deinit(client.allocator); - client.connection_pool.destroy(connection); + self._client.connection_manager.destroy(connection); } const AddHeaderOpts = struct { @@ -403,9 +397,10 @@ pub const Request = struct { posix.close(socket); return err; }; + self._connection = connection; if (self._secure) { - connection.tls = .{ + self._connection.?.tls = .{ .blocking = try tls.client(std.net.Stream{ .handle = socket }, .{ .host = self._connect_host, .root_ca = self._client.root_ca, @@ -415,7 +410,6 @@ pub const Request = struct { }; } - self._connection = connection; self._connection_from_keepalive = false; } @@ -605,7 +599,7 @@ pub const Request = struct { return null; } - return self._client.idle_connections.get(self._secure, self._connect_host, self._connect_port, blocking); + return self._client.connection_manager.get(self._secure, self._connect_host, self._connect_port, blocking); } fn createSocket(self: *Request, blocking: bool) !struct { posix.socket_t, std.net.Address } { @@ -2219,18 +2213,19 @@ const StatePool = struct { // always re-use the connection (just toggle the socket's blocking status), but // for TLS, we'd need to see if the two different TLS objects (blocking and non // blocking) can be converted from each other. -const IdleConnections = struct { +const ConnectionManager = struct { max: usize, idle: List, count: usize, mutex: Thread.Mutex, allocator: Allocator, node_pool: std.heap.MemoryPool(Node), + connection_pool: std.heap.MemoryPool(Connection), const List = std.DoublyLinkedList(*Connection); const Node = List.Node; - fn init(allocator: Allocator, max: usize) IdleConnections { + fn init(allocator: Allocator, max: usize) ConnectionManager { return .{ .max = max, .count = 0, @@ -2238,10 +2233,11 @@ const IdleConnections = struct { .mutex = .{}, .allocator = allocator, .node_pool = std.heap.MemoryPool(Node).init(allocator), + .connection_pool = std.heap.MemoryPool(Connection).init(allocator), }; } - fn deinit(self: *IdleConnections) void { + fn deinit(self: *ConnectionManager) void { const allocator = self.allocator; self.mutex.lock(); @@ -2253,9 +2249,10 @@ const IdleConnections = struct { node = next; } self.node_pool.deinit(); + self.connection_pool.deinit(); } - fn get(self: *IdleConnections, secure: bool, host: []const u8, port: u16, blocking: bool) ?*Connection { + fn get(self: *ConnectionManager, secure: bool, host: []const u8, port: u16, blocking: bool) ?*Connection { self.mutex.lock(); defer self.mutex.unlock(); @@ -2273,7 +2270,7 @@ const IdleConnections = struct { return null; } - fn put(self: *IdleConnections, connection: *Connection) !void { + fn keepIdle(self: *ConnectionManager, connection: *Connection) !void { self.mutex.lock(); defer self.mutex.unlock(); @@ -2281,20 +2278,33 @@ const IdleConnections = struct { if (self.count == self.max) { const oldest = self.idle.popFirst() orelse { std.debug.assert(self.max == 0); - connection.deinit(self.allocator); + self.destroy(connection); return; }; - oldest.data.deinit(self.allocator); + self.destroy(oldest.data); // re-use the node node = oldest; } else { - self.count += 1; node = try self.node_pool.create(); + self.count += 1; } node.data = connection; self.idle.append(node); } + + fn create(self: *ConnectionManager, host: []const u8) !struct { *Connection, []const u8 } { + const connection = try self.connection_pool.create(); + errdefer self.connection_pool.destroy(connection); + + const owned_host = try self.allocator.dupe(u8, host); + return .{ connection, owned_host }; + } + + fn destroy(self: *ConnectionManager, connection: *Connection) void { + connection.deinit(self.allocator); + self.connection_pool.destroy(connection); + } }; const testing = @import("../testing.zig"); From 7adaa53f42099a49cb79fd7f327a6946194afe45 Mon Sep 17 00:00:00 2001 From: sjorsdonkers <72333389+sjorsdonkers@users.noreply.github.com> Date: Fri, 23 May 2025 11:37:46 +0200 Subject: [PATCH 2/3] image constructor --- src/browser/html/elements.zig | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/browser/html/elements.zig b/src/browser/html/elements.zig index b5f62f8a..c6a7bba7 100644 --- a/src/browser/html/elements.zig +++ b/src/browser/html/elements.zig @@ -563,6 +563,26 @@ pub const HTMLImageElement = struct { pub const Self = parser.Image; pub const prototype = *HTMLElement; pub const subtype = .node; + pub const js_name = "Image"; + + pub fn constructor(width: ?[]const u8, height: ?[]const u8, state: *const SessionState) !*parser.Image { + const element = try parser.documentCreateElement(parser.documentHTMLToDocument(state.window.document), "img"); + if (width) |width_| try parser.elementSetAttribute(element, "width", width_); + if (height) |height_| try parser.elementSetAttribute(element, "height", height_); + return @ptrCast(element); + } + + pub fn get_width(self: *parser.Image) !u32 { + const width = try parser.elementGetAttribute(@ptrCast(self), "width") orelse return 0; + return try std.fmt.parseInt(u32, width, 10); + } + pub fn get_height(self: *parser.Image) !u32 { + const width = try parser.elementGetAttribute(@ptrCast(self), "height") orelse return 0; + return try std.fmt.parseInt(u32, width, 10); + } + pub fn get_src(self: *parser.Image) ![]const u8 { + return try parser.elementGetAttribute(@ptrCast(self), "src") orelse return ""; + } }; pub const HTMLInputElement = struct { @@ -1059,4 +1079,14 @@ test "Browser.HTML.Element" { .{ "document.getElementById('content').click()", "undefined" }, .{ "click_count", "1" }, }, .{}); + + try runner.testCases(&.{ + .{ "(new Image).width", "0" }, + .{ "(new Image).height", "0" }, + .{ "(new Image).src", "" }, + .{ "(new Image(4)).width", "4" }, + .{ "(new Image(4, 6)).height", "6" }, + .{ "(new Image).width = 15", "15" }, + .{ "(new Image).src = 'abc'", "abc" }, + }, .{}); } From 5fc9cd7d486a62d06211a9b89e5d7e924811e06e Mon Sep 17 00:00:00 2001 From: sjorsdonkers <72333389+sjorsdonkers@users.noreply.github.com> Date: Fri, 23 May 2025 15:25:41 +0200 Subject: [PATCH 3/3] non deprecated netsurf image properties --- src/browser/html/elements.zig | 71 ++++++++++++++++++++++++------ src/browser/netsurf.zig | 81 +++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 14 deletions(-) diff --git a/src/browser/html/elements.zig b/src/browser/html/elements.zig index c6a7bba7..f84e8e0e 100644 --- a/src/browser/html/elements.zig +++ b/src/browser/html/elements.zig @@ -565,23 +565,49 @@ pub const HTMLImageElement = struct { pub const subtype = .node; pub const js_name = "Image"; - pub fn constructor(width: ?[]const u8, height: ?[]const u8, state: *const SessionState) !*parser.Image { + pub fn constructor(width: ?u32, height: ?u32, state: *const SessionState) !*parser.Image { const element = try parser.documentCreateElement(parser.documentHTMLToDocument(state.window.document), "img"); - if (width) |width_| try parser.elementSetAttribute(element, "width", width_); - if (height) |height_| try parser.elementSetAttribute(element, "height", height_); - return @ptrCast(element); + const image: *parser.Image = @ptrCast(element); + if (width) |width_| try parser.imageSetWidth(image, width_); + if (height) |height_| try parser.imageSetHeight(image, height_); + return image; } - pub fn get_width(self: *parser.Image) !u32 { - const width = try parser.elementGetAttribute(@ptrCast(self), "width") orelse return 0; - return try std.fmt.parseInt(u32, width, 10); + pub fn get_alt(self: *parser.Image) ![]const u8 { + return try parser.imageGetAlt(self); } - pub fn get_height(self: *parser.Image) !u32 { - const width = try parser.elementGetAttribute(@ptrCast(self), "height") orelse return 0; - return try std.fmt.parseInt(u32, width, 10); + pub fn set_alt(self: *parser.Image, alt: []const u8) !void { + try parser.imageSetAlt(self, alt); } pub fn get_src(self: *parser.Image) ![]const u8 { - return try parser.elementGetAttribute(@ptrCast(self), "src") orelse return ""; + return try parser.imageGetSrc(self); + } + pub fn set_src(self: *parser.Image, src: []const u8) !void { + try parser.imageSetSrc(self, src); + } + pub fn get_useMap(self: *parser.Image) ![]const u8 { + return try parser.imageGetUseMap(self); + } + pub fn set_useMap(self: *parser.Image, use_map: []const u8) !void { + try parser.imageSetUseMap(self, use_map); + } + pub fn get_height(self: *parser.Image) !u32 { + return try parser.imageGetHeight(self); + } + pub fn set_height(self: *parser.Image, height: u32) !void { + try parser.imageSetHeight(self, height); + } + pub fn get_width(self: *parser.Image) !u32 { + return try parser.imageGetWidth(self); + } + pub fn set_width(self: *parser.Image, width: u32) !void { + try parser.imageSetWidth(self, width); + } + pub fn get_isMap(self: *parser.Image) !bool { + return try parser.imageGetIsMap(self); + } + pub fn set_isMap(self: *parser.Image, is_map: bool) !void { + try parser.imageSetIsMap(self, is_map); } }; @@ -1080,13 +1106,30 @@ test "Browser.HTML.Element" { .{ "click_count", "1" }, }, .{}); + // Image try runner.testCases(&.{ + // Testing constructors .{ "(new Image).width", "0" }, .{ "(new Image).height", "0" }, - .{ "(new Image).src", "" }, .{ "(new Image(4)).width", "4" }, .{ "(new Image(4, 6)).height", "6" }, - .{ "(new Image).width = 15", "15" }, - .{ "(new Image).src = 'abc'", "abc" }, + + // Testing ulong property + .{ "let fruit = new Image", null }, + .{ "fruit.width", "0" }, + .{ "fruit.width = 5", "5" }, + .{ "fruit.width", "5" }, + .{ "fruit.width = '15'", "15" }, + .{ "fruit.width", "15" }, + .{ "fruit.width = 'apple'", "apple" }, + .{ "fruit.width;", "0" }, + + // Testing string property + .{ "let lyric = new Image", null }, + .{ "lyric.src", "" }, + .{ "lyric.src = 'okay'", "okay" }, + .{ "lyric.src", "okay" }, + .{ "lyric.src = 15", "15" }, + .{ "lyric.src", "15" }, }, .{}); } diff --git a/src/browser/netsurf.zig b/src/browser/netsurf.zig index 7a4d8056..756d8fb6 100644 --- a/src/browser/netsurf.zig +++ b/src/browser/netsurf.zig @@ -2509,3 +2509,84 @@ pub fn htmlCollectionItem(collection: *HTMLCollection, index: u32) !*Node { try DOMErr(err); return @ptrCast(node.?); } + +const ulongNegativeOne = 4294967295; + +// Image +// Image.name is deprecated +// Image.align is deprecated +// Image.border is deprecated +// Image.longDesc is deprecated +// Image.hspace is deprecated +// Image.vspace is deprecated + +pub fn imageGetAlt(image: *Image) ![]const u8 { + var s_: ?*String = null; + const err = c.dom_html_image_element_get_alt(image, &s_); + try DOMErr(err); + const s = s_ orelse return ""; + return strToData(s); +} +pub fn imageSetAlt(image: *Image, alt: []const u8) !void { + const err = c.dom_html_image_element_set_alt(image, try strFromData(alt)); + try DOMErr(err); +} + +pub fn imageGetSrc(image: *Image) ![]const u8 { + var s_: ?*String = null; + const err = c.dom_html_image_element_get_src(image, &s_); + try DOMErr(err); + const s = s_ orelse return ""; + return strToData(s); +} +pub fn imageSetSrc(image: *Image, src: []const u8) !void { + const err = c.dom_html_image_element_set_src(image, try strFromData(src)); + try DOMErr(err); +} + +pub fn imageGetUseMap(image: *Image) ![]const u8 { + var s_: ?*String = null; + const err = c.dom_html_image_element_get_use_map(image, &s_); + try DOMErr(err); + const s = s_ orelse return ""; + return strToData(s); +} +pub fn imageSetUseMap(image: *Image, use_map: []const u8) !void { + const err = c.dom_html_image_element_set_use_map(image, try strFromData(use_map)); + try DOMErr(err); +} + +pub fn imageGetHeight(image: *Image) !u32 { + var height: u32 = 0; + const err = c.dom_html_image_element_get_height(image, &height); + try DOMErr(err); + if (height == ulongNegativeOne) return 0; + return height; +} +pub fn imageSetHeight(image: *Image, height: u32) !void { + const err = c.dom_html_image_element_set_height(image, height); + try DOMErr(err); +} + +pub fn imageGetWidth(image: *Image) !u32 { + var width: u32 = 0; + const err = c.dom_html_image_element_get_width(image, &width); + try DOMErr(err); + if (width == ulongNegativeOne) return 0; + return width; +} +pub fn imageSetWidth(image: *Image, width: u32) !void { + const err = c.dom_html_image_element_set_width(image, width); + try DOMErr(err); +} + +pub fn imageGetIsMap(image: *Image) !bool { + var is_map: bool = false; + const err = c.dom_html_image_element_get_is_map(image, &is_map); + try DOMErr(err); + return is_map; +} +pub fn imageSetIsMap(image: *Image, is_map: bool) !void { + const err = c.dom_html_image_element_set_is_map(image, is_map); + try DOMErr(err); +}