Skip to content

browser: update urls after redirection #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 4, 2025
Merged

browser: update urls after redirection #504

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4302be5
browser: update urls after redirection
krichprollsch Apr 3, 2025
5cbcb90
browser: fix buffer url usage w/ the arena
krichprollsch Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 32 additions & 18 deletions src/browser/browser.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,23 +351,7 @@ pub const Page = struct {
return;
}

// own the url
self.rawuri = try arena.dupe(u8, uri);
self.uri = std.Uri.parse(self.rawuri.?) catch try std.Uri.parseAfterScheme("", self.rawuri.?);

self.url = try URL.constructor(arena, self.rawuri.?, null);
self.location.url = &self.url.?;
try self.session.window.replaceLocation(&self.location);

// prepare origin value.
var buf: std.ArrayListUnmanaged(u8) = .{};
try self.uri.writeToStream(.{
.scheme = true,
.authority = true,
}, buf.writer(arena));
self.origin = buf.items;

// TODO handle fragment in url.
self.uri = std.Uri.parse(uri) catch try std.Uri.parseAfterScheme("", uri);

self.session.app.telemetry.record(.{ .navigate = .{
.proxy = false,
Expand All @@ -380,7 +364,37 @@ pub const Page = struct {

var response = try request.sendSync(.{});
const header = response.header;
try self.session.cookie_jar.populateFromResponse(self.uri, &header);
try self.session.cookie_jar.populateFromResponse(request.uri, &header);

// update uri after eventual redirection
var buf: std.ArrayListUnmanaged(u8) = .{};
defer buf.deinit(arena);
Copy link
Collaborator

@karlseguin karlseguin Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can treat the allocator like an ArenaAllocator or like an std.mem.Allocator.

If we treat it like an Arena, we can remove unnecessary frees, which has a super-tiny performance cost.

If we treat it like an Allocator, we gain some flexibility with respect to changing the implementation.

It isn't a system-wide decisions, there are places where we'll have an arena and want to treat it like a generic allocator, and there are places where we'll definitely want to take advantage of the arena's behavior. The reason I like to call these ArenaAllocators arena is that it's clear to the writer & reader what the behavior is.

TL;DR - The deinit is almost certainly a no-op and could be removed. It's up to you.


buf.clearRetainingCapacity();
Copy link
Collaborator

@karlseguin karlseguin Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does nothing

try request.uri.writeToStream(.{
.scheme = true,
.authentication = true,
.authority = true,
.path = true,
.query = true,
.fragment = true,
}, buf.writer(arena));
self.rawuri = try buf.toOwnedSlice(arena);
Copy link
Collaborator

@karlseguin karlseguin Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you re-use the buffer later, which I guess is why you copy the contents. But, toOwnedSlice frees the underlying memory, so the underlying slice won't get re-used. I think you can use buf.items to avoid the allocation + copy.


self.uri = try std.Uri.parse(self.rawuri.?);

// TODO handle fragment in url.
self.url = try URL.constructor(arena, self.rawuri.?, null);
self.location.url = &self.url.?;
try self.session.window.replaceLocation(&self.location);

// prepare origin value.
buf.clearRetainingCapacity();
Copy link
Collaborator

@karlseguin karlseguin Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As-above, if you stick with the toOwnedSlice(arena), then this code does nothing. toOwnedSlice(arena) calls clearAndFree().

If, above, you decide to do instead:

self.rawuri = buf.items

Then here you'll want to reset the buffer:

buf = .{};

as the clearRetainingCapacity() will break self.rawuri

try request.uri.writeToStream(.{
.scheme = true,
.authority = true,
}, buf.writer(arena));
self.origin = try buf.toOwnedSlice(arena);
Copy link
Collaborator

@karlseguin karlseguin Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the option to use buf.items here.


log.info("GET {any} {d}", .{ self.uri, header.status });

Expand Down