From f86a9f60f945feb7a3e59ae6f029bc4a6e4586ca Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Tue, 8 Jul 2025 21:35:39 -0500 Subject: [PATCH 1/4] gtk: open non-file URLs using the XDG Desktop Portal Use the XDG Desktop Portal OpenURI DBus method to open non-file URLs. File URLs use a separate DBus method and are not handled here. If the DBus method call fails or the URL is a file URL, use the cross-platform fallback to open the URL. --- src/apprt/gtk/App.zig | 31 ++- src/apprt/gtk/portal.zig | 52 ++++ src/apprt/gtk/portal/OpenURI.zig | 400 +++++++++++++++++++++++++++++++ 3 files changed, 478 insertions(+), 5 deletions(-) create mode 100644 src/apprt/gtk/portal.zig create mode 100644 src/apprt/gtk/portal/OpenURI.zig diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index d6a50f0f6..8dc3aac2c 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -43,6 +43,7 @@ const ClipboardConfirmationWindow = @import("ClipboardConfirmationWindow.zig"); const CloseDialog = @import("CloseDialog.zig"); const GlobalShortcuts = @import("GlobalShortcuts.zig"); const Split = @import("Split.zig"); +const OpenURI = @import("portal.zig").OpenURI; const inspector = @import("inspector.zig"); const key = @import("key.zig"); const winprotopkg = @import("winproto.zig"); @@ -104,6 +105,8 @@ custom_css_providers: std.ArrayListUnmanaged(*gtk.CssProvider) = .{}, global_shortcuts: ?GlobalShortcuts, +open_uri: OpenURI = undefined, + /// The timer used to quit the application after the last window is closed. quit_timer: union(enum) { off: void, @@ -447,6 +450,8 @@ pub fn init(self: *App, core_app: *CoreApp, opts: Options) !void { .css_provider = css_provider, .global_shortcuts = .init(core_app.alloc, gio_app), }; + + try self.open_uri.init(self); } // Terminate the application. The application will not be restarted after @@ -470,6 +475,7 @@ pub fn terminate(self: *App) void { if (self.global_shortcuts) |*shortcuts| shortcuts.deinit(); self.config.deinit(); + self.open_uri.deinit(); } /// Perform a given action. Returns `true` if the action was able to be @@ -1793,17 +1799,32 @@ fn openConfig(self: *App) !bool { } fn openUrl( - app: *App, + self: *App, value: apprt.action.OpenUrl, ) void { - // TODO: use https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.OpenURI.html + if (std.mem.startsWith(u8, value.url, "/")) { + self.openUrlFallback(value.kind, value.url); + return; + } + if (std.mem.startsWith(u8, value.url, "file://")) { + self.openUrlFallback(value.kind, value.url); + return; + } + self.open_uri.start(value) catch |err| { + log.err("unable to open uri err={}", .{err}); + self.openUrlFallback(value.kind, value.url); + return; + }; +} + +pub fn openUrlFallback(self: *App, kind: apprt.action.OpenUrl.Kind, url: []const u8) void { // Fallback to the minimal cross-platform way of opening a URL. // This is always a safe fallback and enables for example Windows // to open URLs (GTK on Windows via WSL is a thing). internal_os.open( - app.core_app.alloc, - value.kind, - value.url, + self.core_app.alloc, + kind, + url, ) catch |err| log.warn("unable to open url: {}", .{err}); } diff --git a/src/apprt/gtk/portal.zig b/src/apprt/gtk/portal.zig new file mode 100644 index 000000000..7972a775f --- /dev/null +++ b/src/apprt/gtk/portal.zig @@ -0,0 +1,52 @@ +const std = @import("std"); + +const gio = @import("gio"); + +const Allocator = std.mem.Allocator; + +pub const OpenURI = @import("portal/OpenURI.zig"); + +/// Generate a token suitable for use in requests to the XDG Desktop Portal +pub fn generateToken() usize { + return std.crypto.random.int(usize); +} + +/// Get the XDG portal request path for the current Ghostty instance. +/// +/// If this sounds like nonsense, see `request` for an explanation as to +/// why we need to do this. +pub fn getRequestPath(alloc: Allocator, dbus: *gio.DBusConnection, token: usize) (Allocator.Error || error{NoDBusUniqueName})![:0]const u8 { + // See https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.Request.html + // for the syntax XDG portals expect. + + const token_string = try std.fmt.allocPrint(alloc, "{x:0>16}", .{token}); + defer alloc.free(token_string); + + const unique_name = try alloc.dupe(u8, std.mem.span(dbus.getUniqueName() orelse { + return error.NoDBusUniqueName; + })[1..]); + defer alloc.free(unique_name); + + // Sanitize the unique name by replacing every `.` with `_`. + // In effect, this will turn a unique name like `:1.192` into `1_192`. + std.mem.replaceScalar(u8, unique_name, '.', '_'); + + const object_path = try std.mem.joinZ( + alloc, + "/", + &.{ + "/org/freedesktop/portal/desktop/request", + unique_name, // Remove leading `:` + token_string, + }, + ); + + return object_path; +} + +/// Try and parse the token out of a request path. +pub fn parseRequestPath(request_path: []const u8) ?usize { + const index = std.mem.lastIndexOfScalar(u8, request_path, '/') orelse return null; + const token = request_path[index + 1 ..]; + return std.fmt.parseUnsigned(usize, token, 16) catch return null; +} diff --git a/src/apprt/gtk/portal/OpenURI.zig b/src/apprt/gtk/portal/OpenURI.zig new file mode 100644 index 000000000..ddf6fd6e3 --- /dev/null +++ b/src/apprt/gtk/portal/OpenURI.zig @@ -0,0 +1,400 @@ +//! Use DBus to call the XDG Desktop Portal to open an URI. +//! See: https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.OpenURI.html#org-freedesktop-portal-openuri-openuri +const OpenURI = @This(); + +const std = @import("std"); +const Allocator = std.mem.Allocator; + +const gio = @import("gio"); +const glib = @import("glib"); +const gobject = @import("gobject"); + +const App = @import("../App.zig"); +const portal = @import("../portal.zig"); +const apprt = @import("../../../apprt.zig"); + +const log = std.log.scoped(.openuri); + +/// The GTK app that we "belong" to. +app: *App, + +/// Connection to the D-Bus session bus that we'll use for all of our messaging. +dbus: *gio.DBusConnection, + +/// Mutex to protect modification of the entries map or the cleanup timer. +mutex: std.Thread.Mutex = .{}, + +/// Map to store data about any in-flight calls to the portal. +entries: std.AutoHashMapUnmanaged(usize, Entry) = .empty, + +/// Used to manage a timer to clean up any orphan entries in the map. +cleanup_timer: ?c_uint = null, + +/// Data about any in-flight calls to the portal. +pub const Entry = struct { + /// When the request started. + start: std.time.Instant, + /// A token used by the portal to identify requests and responses. The actual + /// format of the token does not really matter as long as it can be used as + /// part of a D-Bus object path. `usize` was chosen since it's easy to hash and + /// to generate random tokens. + token: usize, + /// The "kind" of URI. Unused here, but we may need to pass it on to the fallback + /// URL opener if the D-Bus method fails. + kind: apprt.action.OpenUrl.Kind, + /// A copy of the URI that we are opening. We need our own copy since the method + /// calls are asynchronous and the original may have been freed by the time we need + /// it. + uri: [:0]const u8, + /// Used to manage a scription to a D-Bus signal, which is how the Portal + /// reports results of the method call. + subscription: ?c_uint = null, + + pub fn deinit(self: *Entry, alloc: Allocator) void { + alloc.free(self.uri); + } +}; + +pub const Errors = error{ + DBusConnectionRequired, + NoDBusUniqueName, + PutFailed, +}; + +pub fn init( + self: *OpenURI, + app: *App, +) error{DBusConnectionRequired}!void { + const gio_app = app.app.as(gio.Application); + const dbus = gio_app.getDbusConnection() orelse { + return error.DBusConnectionRequired; + }; + + self.* = .{ + .app = app, + .dbus = dbus, + }; +} + +/// Send the D-Bus method call to the XDG Desktop portal. The result of the +/// method call will be reported asynchronously. +pub fn start(self: *OpenURI, value: apprt.action.OpenUrl) (Allocator.Error || Errors)!void { + const alloc = self.app.core_app.alloc; + + const token = portal.generateToken(); + + self.mutex.lock(); + defer self.mutex.unlock(); + + try self.entries.putNoClobber(alloc, token, .{ + .start = std.time.Instant.now() catch @panic("can't get the current time!"), + .token = token, + .kind = value.kind, + .uri = try alloc.dupeZ(u8, value.url), + }); + + const entry = self.entries.getPtr(token) orelse { + return error.PutFailed; + }; + + self.startCleanupTimer(); + + try self.subscribeToResponse(entry); + try self.sendRequest(entry); +} + +pub fn deinit(self: *OpenURI) void { + const alloc = self.app.core_app.alloc; + + var it = self.entries.valueIterator(); + while (it.next()) |value| { + value.deinit(alloc); + } + + self.entries.deinit(alloc); +} + +/// Subscribe to the D-Bus signal that will contain the results of our method +/// call to the portal. This must be called with the mutex locked. +fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || Errors)!void { + const alloc = self.app.core_app.alloc; + + if (entry.subscription != null) return; + + const request_path = try portal.getRequestPath(alloc, self.dbus, entry.token); + defer alloc.free(request_path); + + entry.subscription = self.dbus.signalSubscribe( + null, + "org.freedesktop.portal.Request", + "Response", + request_path, + null, + .{}, + responseReceived, + self, + null, + ); +} +// Unsubscribe to the D-Bus signal that contains the result of the method call. +// This will prevent a response from being processed multiple times. This must +// be called when the mutex is locked. +fn unsubscribeFromResponse(self: *OpenURI, entry: *Entry) void { + + // Unsubscribe from the response signal + if (entry.subscription) |subscription| { + self.dbus.signalUnsubscribe(subscription); + entry.subscription = null; + } +} + +/// Send the D-Bus method call to the portal. The mutex must be locked when this +/// is called. +fn sendRequest(self: *OpenURI, entry: *Entry) Allocator.Error!void { + const alloc = self.app.core_app.alloc; + + const payload = payload: { + const builder_type = glib.VariantType.new("(ssa{sv})"); + defer glib.free(builder_type); + + // Initialize our builder to build up our parameters + var builder: glib.VariantBuilder = undefined; + builder.init(builder_type); + + // parent window - empty string means we have no window + builder.add("s", ""); + + // URI to open + builder.add("s", entry.uri.ptr); + + // Options + { + const options = glib.VariantType.new("a{sv}"); + defer glib.free(options); + + builder.open(options); + defer builder.close(); + + { + const option = glib.VariantType.new("{sv}"); + defer glib.free(option); + + builder.open(option); + defer builder.close(); + + builder.add("s", "handle_token"); + + const token = try std.fmt.allocPrintZ(alloc, "{x:0<16}", .{entry.token}); + defer alloc.free(token); + + const handle_token = glib.Variant.newString(token.ptr); + builder.add("v", handle_token); + } + { + const option = glib.VariantType.new("{sv}"); + defer glib.free(option); + + builder.open(option); + defer builder.close(); + + builder.add("s", "writable"); + + const writable = glib.Variant.newBoolean(@intFromBool(false)); + builder.add("v", writable); + } + { + const option = glib.VariantType.new("{sv}"); + defer glib.free(option); + + builder.open(option); + defer builder.close(); + + builder.add("s", "ask"); + + const ask = glib.Variant.newBoolean(@intFromBool(false)); + builder.add("v", ask); + } + } + + break :payload builder.end(); + }; + + // We're expecting an object path back from the method call. + const reply_type = glib.VariantType.new("(o)"); + defer glib.free(reply_type); + + self.dbus.call( + "org.freedesktop.portal.Desktop", + "/org/freedesktop/portal/desktop", + "org.freedesktop.portal.OpenURI", + "OpenURI", + payload, + reply_type, + .{}, + -1, + null, + requestCallback, + self, + ); +} + +/// Process the result of the original method call. Receiving this result does +/// not indicate that the that the method call succeeded but it may contain an +/// error message that is useful to log for debugging purposes. +fn requestCallback( + _: ?*gobject.Object, + result: *gio.AsyncResult, + ud: ?*anyopaque, +) callconv(.c) void { + const self: *OpenURI = @ptrCast(@alignCast(ud orelse return)); + + var err_: ?*glib.Error = null; + defer if (err_) |err| err.free(); + + const reply_ = self.dbus.callFinish(result, &err_); + + if (err_) |err| { + log.err("Open URI request failed={s} ({})", .{ + err.f_message orelse "(unknown)", + err.f_code, + }); + return; + } + + const reply = reply_ orelse { + log.err("D-Bus method call returned a null value!", .{}); + return; + }; + defer reply.unref(); + + const reply_type = glib.VariantType.new("(o)"); + defer glib.free(reply_type); + + if (reply.isOfType(reply_type) == 0) { + log.warn("Reply from D-Bus method call does not contain an object path!", .{}); + return; + } + + var object_path_: ?[*:0]const u8 = null; + reply.get("(o)", &object_path_); + + const object_path = object_path_ orelse { + log.err("D-Bus method call did not return an object path", .{}); + return; + }; + + const token = portal.parseRequestPath(std.mem.span(object_path)) orelse { + log.warn("Unable to parse token from the object path {s}", .{object_path}); + return; + }; + + // Check to see if the request path returned matches a token that we sent. + self.mutex.lock(); + defer self.mutex.unlock(); + + if (!self.entries.contains(token)) { + log.warn("Token {x:0<16} not found in the map!", .{token}); + } +} + +/// Handle the response signal from the portal. This should contain the actual +/// results of the method call (success or failure). +fn responseReceived( + _: *gio.DBusConnection, + _: ?[*:0]const u8, + object_path: [*:0]const u8, + _: [*:0]const u8, + _: [*:0]const u8, + params: *glib.Variant, + ud: ?*anyopaque, +) callconv(.c) void { + const self: *OpenURI = @ptrCast(@alignCast(ud orelse { + log.err("OpenURI response received with null userdata", .{}); + return; + })); + + const alloc = self.app.core_app.alloc; + + const token = portal.parseRequestPath(std.mem.span(object_path)) orelse { + log.warn("invalid object path: {s}", .{std.mem.span(object_path)}); + return; + }; + + self.mutex.lock(); + defer self.mutex.unlock(); + + var entry = (self.entries.fetchRemove(token) orelse { + log.warn("no entry for token {x:0<16}", .{token}); + return; + }).value; + log.warn("removed {x:0<16} from the map", .{entry.token}); + defer entry.deinit(alloc); + + self.unsubscribeFromResponse(&entry); + + var response: u32 = 0; + var results: ?*glib.Variant = null; + params.get("(u@a{sv})", &response, &results); + + switch (response) { + 0 => { + log.debug("open uri successful", .{}); + }, + 1 => { + log.debug("open uri request was cancelled by the user", .{}); + }, + 2 => { + log.warn("open uri request ended unexpectedly", .{}); + self.app.openUrlFallback(entry.kind, entry.uri); + }, + else => { + log.err("unrecognized response code={}", .{response}); + self.app.openUrlFallback(entry.kind, entry.uri); + }, + } +} + +/// Wait this number of seconds and then clean up any orphaned entries. +const cleanup_timeout = 30; + +// this must be called with the mutex locked +fn stopCleanupTimer(self: *OpenURI) void { + if (self.cleanup_timer) |timer| { + if (glib.Source.remove(timer) == 0) { + log.warn("unable to remove cleanup timer source={d}", .{timer}); + } + self.cleanup_timer = null; + } +} + +// this must be called with the mutex locked +fn startCleanupTimer(self: *OpenURI) void { + self.stopCleanupTimer(); + self.cleanup_timer = glib.timeoutAddSeconds(cleanup_timeout + 1, cleanup, self); +} + +/// The cleanup timer is used to free up any entries that may have failed +/// to get a response in a timely manner. +fn cleanup(ud: ?*anyopaque) callconv(.c) c_int { + const self: *OpenURI = @ptrCast(@alignCast(ud orelse { + log.warn("cleanup called with null userdata", .{}); + return @intFromBool(glib.SOURCE_REMOVE); + })); + + self.mutex.lock(); + defer self.mutex.unlock(); + + self.cleanup_timer = null; + + const now = std.time.Instant.now() catch @panic("unable to get time!"); + + var it = self.entries.valueIterator(); + while (it.next()) |entry| { + if (now.since(entry.start) > cleanup_timeout * std.time.ns_per_s) { + self.unsubscribeFromResponse(entry); + _ = self.entries.remove(entry.token); + } + } + + return @intFromBool(glib.SOURCE_REMOVE); +} From 02069a54c470b072582db868d40962a2425e30f8 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Mon, 14 Jul 2025 11:41:03 -0500 Subject: [PATCH 2/4] gtk: improve error handling and memory management for OpenURI --- src/apprt/gtk/portal.zig | 27 ++++-- src/apprt/gtk/portal/OpenURI.zig | 138 ++++++++++++++++++++----------- 2 files changed, 107 insertions(+), 58 deletions(-) diff --git a/src/apprt/gtk/portal.zig b/src/apprt/gtk/portal.zig index 7972a775f..f4ad5bb4a 100644 --- a/src/apprt/gtk/portal.zig +++ b/src/apprt/gtk/portal.zig @@ -15,20 +15,23 @@ pub fn generateToken() usize { /// /// If this sounds like nonsense, see `request` for an explanation as to /// why we need to do this. -pub fn getRequestPath(alloc: Allocator, dbus: *gio.DBusConnection, token: usize) (Allocator.Error || error{NoDBusUniqueName})![:0]const u8 { +pub fn getRequestPath(alloc: Allocator, dbus: *gio.DBusConnection, token: usize) (Allocator.Error || std.fmt.BufPrintError || error{NoDBusUniqueName})![:0]const u8 { // See https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.Request.html // for the syntax XDG portals expect. - const token_string = try std.fmt.allocPrint(alloc, "{x:0>16}", .{token}); - defer alloc.free(token_string); + var token_buf: [16]u8 = undefined; + const token_string = try std.fmt.bufPrint(&token_buf, "{x:0>16}", .{token}); - const unique_name = try alloc.dupe(u8, std.mem.span(dbus.getUniqueName() orelse { - return error.NoDBusUniqueName; - })[1..]); + // Get the unique name from D-Bus and strip the leading `:` + const unique_name = try alloc.dupe(u8, std.mem.span( + dbus.getUniqueName() orelse { + return error.NoDBusUniqueName; + }, + )[1..]); defer alloc.free(unique_name); - // Sanitize the unique name by replacing every `.` with `_`. - // In effect, this will turn a unique name like `:1.192` into `1_192`. + // Sanitize the unique name by replacing every `.` with `_`. In effect, this + // will turn a unique name like `1.192` into `1_192`. std.mem.replaceScalar(u8, unique_name, '.', '_'); const object_path = try std.mem.joinZ( @@ -50,3 +53,11 @@ pub fn parseRequestPath(request_path: []const u8) ?usize { const token = request_path[index + 1 ..]; return std.fmt.parseUnsigned(usize, token, 16) catch return null; } + +test "parseRequestPath" { + const testing = std.testing; + + try testing.expectEqual(0x75af01a79c6fea34, parseRequestPath("/org/freedesktop/portal/desktop/request/1_42/75af01a79c6fea34").?); + try testing.expectEqual(null, parseRequestPath("/org/freedesktop/portal/desktop/request/1_42/75af01a79c6fGa34")); + try testing.expectEqual(null, parseRequestPath("75af01a79c6fea34")); +} diff --git a/src/apprt/gtk/portal/OpenURI.zig b/src/apprt/gtk/portal/OpenURI.zig index ddf6fd6e3..c1a6c4080 100644 --- a/src/apprt/gtk/portal/OpenURI.zig +++ b/src/apprt/gtk/portal/OpenURI.zig @@ -25,7 +25,7 @@ dbus: *gio.DBusConnection, mutex: std.Thread.Mutex = .{}, /// Map to store data about any in-flight calls to the portal. -entries: std.AutoHashMapUnmanaged(usize, Entry) = .empty, +entries: std.AutoArrayHashMapUnmanaged(usize, *Entry) = .empty, /// Used to manage a timer to clean up any orphan entries in the map. cleanup_timer: ?c_uint = null, @@ -34,31 +34,35 @@ cleanup_timer: ?c_uint = null, pub const Entry = struct { /// When the request started. start: std.time.Instant, - /// A token used by the portal to identify requests and responses. The actual - /// format of the token does not really matter as long as it can be used as - /// part of a D-Bus object path. `usize` was chosen since it's easy to hash and - /// to generate random tokens. + /// A token used by the portal to identify requests and responses. The + /// actual format of the token does not really matter as long as it can be + /// used as part of a D-Bus object path. `usize` was chosen since it's easy + /// to hash and to generate random tokens. token: usize, - /// The "kind" of URI. Unused here, but we may need to pass it on to the fallback - /// URL opener if the D-Bus method fails. + /// The "kind" of URI. Unused here, but we may need to pass it on to the + /// fallback URL opener if the D-Bus method fails. kind: apprt.action.OpenUrl.Kind, - /// A copy of the URI that we are opening. We need our own copy since the method - /// calls are asynchronous and the original may have been freed by the time we need - /// it. + /// A copy of the URI that we are opening. We need our own copy since the + /// method calls are asynchronous and the original may have been freed by + /// the time we need it. uri: [:0]const u8, - /// Used to manage a scription to a D-Bus signal, which is how the Portal - /// reports results of the method call. + /// Used to manage a scription to a D-Bus signal, which is how the XDG + /// Portal reports results of the method call. subscription: ?c_uint = null, - pub fn deinit(self: *Entry, alloc: Allocator) void { + pub fn deinit(self: *const Entry, alloc: Allocator) void { alloc.free(self.uri); } }; pub const Errors = error{ + /// Could not get a D-Bus connection DBusConnectionRequired, + /// The D-Bus connection did not have a unique name. This _should_ be + /// impossible, but is handled for safety's sake. NoDBusUniqueName, - PutFailed, + /// The system was unable to give us the time. + TimerUnavailable, }; pub fn init( @@ -76,9 +80,25 @@ pub fn init( }; } +pub fn deinit(self: *OpenURI) void { + const alloc = self.app.core_app.alloc; + + self.mutex.lock(); + defer self.mutex.unlock(); + + self.stopCleanupTimer(); + + for (self.entries.entries.items(.value)) |entry| { + entry.deinit(alloc); + alloc.destroy(entry); + } + + self.entries.deinit(alloc); +} + /// Send the D-Bus method call to the XDG Desktop portal. The result of the /// method call will be reported asynchronously. -pub fn start(self: *OpenURI, value: apprt.action.OpenUrl) (Allocator.Error || Errors)!void { +pub fn start(self: *OpenURI, value: apprt.action.OpenUrl) (Allocator.Error || std.fmt.BufPrintError || Errors)!void { const alloc = self.app.core_app.alloc; const token = portal.generateToken(); @@ -86,37 +106,39 @@ pub fn start(self: *OpenURI, value: apprt.action.OpenUrl) (Allocator.Error || Er self.mutex.lock(); defer self.mutex.unlock(); - try self.entries.putNoClobber(alloc, token, .{ - .start = std.time.Instant.now() catch @panic("can't get the current time!"), - .token = token, - .kind = value.kind, - .uri = try alloc.dupeZ(u8, value.url), - }); - - const entry = self.entries.getPtr(token) orelse { - return error.PutFailed; + // Create an entry that is used to track the results of the D-Bus method + // call. + const entry = entry: { + const entry = try alloc.create(Entry); + errdefer alloc.destroy(entry); + entry.* = .{ + .start = std.time.Instant.now() catch return error.TimerUnavailable, + .token = token, + .kind = value.kind, + .uri = try alloc.dupeZ(u8, value.url), + }; + errdefer entry.deinit(alloc); + try self.entries.putNoClobber(alloc, token, entry); + break :entry entry; }; + errdefer { + _ = self.entries.swapRemove(token); + entry.deinit(alloc); + alloc.destroy(entry); + } + self.startCleanupTimer(); try self.subscribeToResponse(entry); + errdefer self.unsubscribeFromResponse(entry); + try self.sendRequest(entry); } -pub fn deinit(self: *OpenURI) void { - const alloc = self.app.core_app.alloc; - - var it = self.entries.valueIterator(); - while (it.next()) |value| { - value.deinit(alloc); - } - - self.entries.deinit(alloc); -} - /// Subscribe to the D-Bus signal that will contain the results of our method /// call to the portal. This must be called with the mutex locked. -fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || Errors)!void { +fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || std.fmt.BufPrintError || Errors)!void { const alloc = self.app.core_app.alloc; if (entry.subscription != null) return; @@ -136,11 +158,11 @@ fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || Errors null, ); } + // Unsubscribe to the D-Bus signal that contains the result of the method call. // This will prevent a response from being processed multiple times. This must // be called when the mutex is locked. fn unsubscribeFromResponse(self: *OpenURI, entry: *Entry) void { - // Unsubscribe from the response signal if (entry.subscription) |subscription| { self.dbus.signalUnsubscribe(subscription); @@ -155,11 +177,12 @@ fn sendRequest(self: *OpenURI, entry: *Entry) Allocator.Error!void { const payload = payload: { const builder_type = glib.VariantType.new("(ssa{sv})"); - defer glib.free(builder_type); + defer builder_type.free(); // Initialize our builder to build up our parameters var builder: glib.VariantBuilder = undefined; builder.init(builder_type); + errdefer builder.clear(); // parent window - empty string means we have no window builder.add("s", ""); @@ -221,7 +244,7 @@ fn sendRequest(self: *OpenURI, entry: *Entry) Allocator.Error!void { // We're expecting an object path back from the method call. const reply_type = glib.VariantType.new("(o)"); - defer glib.free(reply_type); + defer reply_type.free(); self.dbus.call( "org.freedesktop.portal.Desktop", @@ -268,7 +291,7 @@ fn requestCallback( defer reply.unref(); const reply_type = glib.VariantType.new("(o)"); - defer glib.free(reply_type); + defer reply_type.free(); if (reply.isOfType(reply_type) == 0) { log.warn("Reply from D-Bus method call does not contain an object path!", .{}); @@ -323,14 +346,17 @@ fn responseReceived( self.mutex.lock(); defer self.mutex.unlock(); - var entry = (self.entries.fetchRemove(token) orelse { + const entry = (self.entries.fetchSwapRemove(token) orelse { log.warn("no entry for token {x:0<16}", .{token}); return; }).value; - log.warn("removed {x:0<16} from the map", .{entry.token}); - defer entry.deinit(alloc); - self.unsubscribeFromResponse(&entry); + defer { + entry.deinit(alloc); + alloc.destroy(entry); + } + + self.unsubscribeFromResponse(entry); var response: u32 = 0; var results: ?*glib.Variant = null; @@ -381,19 +407,31 @@ fn cleanup(ud: ?*anyopaque) callconv(.c) c_int { return @intFromBool(glib.SOURCE_REMOVE); })); + const alloc = self.app.core_app.alloc; + self.mutex.lock(); defer self.mutex.unlock(); self.cleanup_timer = null; - const now = std.time.Instant.now() catch @panic("unable to get time!"); + const now = std.time.Instant.now() catch { + // `now()` should never fail, but if it does, don't crash, just return. + // This might cause a small memory leak in rare circumstances but it + // should get cleaned up the next time a URL is clicked. + return @intFromBool(glib.SOURCE_REMOVE); + }; - var it = self.entries.valueIterator(); - while (it.next()) |entry| { - if (now.since(entry.start) > cleanup_timeout * std.time.ns_per_s) { - self.unsubscribeFromResponse(entry); - _ = self.entries.remove(entry.token); + loop: while (true) { + for (self.entries.entries.items(.value)) |entry| { + if (now.since(entry.start) > cleanup_timeout * std.time.ns_per_s) { + self.unsubscribeFromResponse(entry); + _ = self.entries.swapRemove(entry.token); + entry.deinit(alloc); + alloc.destroy(entry); + continue :loop; + } } + break :loop; } return @intFromBool(glib.SOURCE_REMOVE); From 573aef833bed8d11449400730284d2670c997bf5 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Mon, 14 Jul 2025 15:04:14 -0500 Subject: [PATCH 3/4] gtk: add asserts to check for locked mutex, improve doc comments --- src/apprt/gtk/portal/OpenURI.zig | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/apprt/gtk/portal/OpenURI.zig b/src/apprt/gtk/portal/OpenURI.zig index c1a6c4080..492319065 100644 --- a/src/apprt/gtk/portal/OpenURI.zig +++ b/src/apprt/gtk/portal/OpenURI.zig @@ -4,6 +4,7 @@ const OpenURI = @This(); const std = @import("std"); const Allocator = std.mem.Allocator; +const assert = std.debug.assert; const gio = @import("gio"); const glib = @import("glib"); @@ -139,6 +140,8 @@ pub fn start(self: *OpenURI, value: apprt.action.OpenUrl) (Allocator.Error || st /// Subscribe to the D-Bus signal that will contain the results of our method /// call to the portal. This must be called with the mutex locked. fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || std.fmt.BufPrintError || Errors)!void { + assert(!self.mutex.tryLock()); + const alloc = self.app.core_app.alloc; if (entry.subscription != null) return; @@ -163,6 +166,8 @@ fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || std.fm // This will prevent a response from being processed multiple times. This must // be called when the mutex is locked. fn unsubscribeFromResponse(self: *OpenURI, entry: *Entry) void { + assert(!self.mutex.tryLock()); + // Unsubscribe from the response signal if (entry.subscription) |subscription| { self.dbus.signalUnsubscribe(subscription); @@ -173,6 +178,8 @@ fn unsubscribeFromResponse(self: *OpenURI, entry: *Entry) void { /// Send the D-Bus method call to the portal. The mutex must be locked when this /// is called. fn sendRequest(self: *OpenURI, entry: *Entry) Allocator.Error!void { + assert(!self.mutex.tryLock()); + const alloc = self.app.core_app.alloc; const payload = payload: { @@ -383,8 +390,11 @@ fn responseReceived( /// Wait this number of seconds and then clean up any orphaned entries. const cleanup_timeout = 30; -// this must be called with the mutex locked +/// If there is an active cleanup timer, cancel it. This must be called with the +/// mutex locked fn stopCleanupTimer(self: *OpenURI) void { + assert(!self.mutex.tryLock()); + if (self.cleanup_timer) |timer| { if (glib.Source.remove(timer) == 0) { log.warn("unable to remove cleanup timer source={d}", .{timer}); @@ -393,8 +403,12 @@ fn stopCleanupTimer(self: *OpenURI) void { } } -// this must be called with the mutex locked +/// Start a timer to clean up any entries that have not received a timely +/// response. If there is already a timer it will be stopped and replaced with a +/// new one. This must be called with the mutex locked. fn startCleanupTimer(self: *OpenURI) void { + assert(!self.mutex.tryLock()); + self.stopCleanupTimer(); self.cleanup_timer = glib.timeoutAddSeconds(cleanup_timeout + 1, cleanup, self); } From abbc31a3cb7af5fac9ae63b7ec693d31bb8ae7f2 Mon Sep 17 00:00:00 2001 From: "Jeffrey C. Ollie" Date: Mon, 14 Jul 2025 15:05:18 -0500 Subject: [PATCH 4/4] gtk: improve doc comments --- src/apprt/gtk/portal/OpenURI.zig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/apprt/gtk/portal/OpenURI.zig b/src/apprt/gtk/portal/OpenURI.zig index 492319065..37b321559 100644 --- a/src/apprt/gtk/portal/OpenURI.zig +++ b/src/apprt/gtk/portal/OpenURI.zig @@ -162,9 +162,9 @@ fn subscribeToResponse(self: *OpenURI, entry: *Entry) (Allocator.Error || std.fm ); } -// Unsubscribe to the D-Bus signal that contains the result of the method call. -// This will prevent a response from being processed multiple times. This must -// be called when the mutex is locked. +/// Unsubscribe to the D-Bus signal that contains the result of the method call. +/// This will prevent a response from being processed multiple times. This must +/// be called when the mutex is locked. fn unsubscribeFromResponse(self: *OpenURI, entry: *Entry) void { assert(!self.mutex.tryLock());