Document cookie #529
Document cookie #529
Conversation
src/html/document.zig
Outdated
| var buf: std.ArrayListUnmanaged(u8) = .{}; | ||
| defer buf.deinit(allocator); | ||
| try userctx.cookie_jar.forRequest(&userctx.url.uri, buf.writer(allocator), .{ .navigation = true }); | ||
| return buf.toOwnedSlice(allocator); |
There was a problem hiding this comment.
i know this is consistent with a lot of the existing APIs, but if you know that allocator is an arena, that this an unnecessary/extra allocation + copy. Plus, we never free the dupe, so it's a bit like the worst of both: don't assume it's an arena, but also don't free the allocation.
There are comments like https://github.com/lightpanda-io/browser/blob/main/src/url/url.zig#L75 which imply this should be fixed. But I think relying on the page_arena is fine (though, we should rename the allocator parameter arena, IMO)
There was a problem hiding this comment.
I agree, I changed.
| @@ -145,7 +145,7 @@ pub const Jar = struct { | |||
| if (first) { | |||
| first = false; | |||
| } else { | |||
| try writer.writeAll(", "); | |||
| try writer.writeAll("; "); | |||
There was a problem hiding this comment.
The problem with unit test: the test asserts the wrong behavior. Nice catch :)
src/html/document.zig
Outdated
| pub fn set_cookie(_: *parser.DocumentHTML, _: []const u8) ![]const u8 { | ||
| return error.NotImplemented; | ||
| pub fn set_cookie(_: *parser.DocumentHTML, arena: std.mem.Allocator, userctx: UserContext, cookie_str: []const u8) ![]const u8 { | ||
| const c = try Cookie.parse(arena, &userctx.url.uri, cookie_str); |
There was a problem hiding this comment.
This is a problem, the cookie has to outlive the arena (cookies live for the duration of the session, the arena is for the page). You can parse with userctx.cookie_jar.allocator instead of arena.
Not the first case where I think most web apis need the page_arena, but a few places need something with a longer lifetime. In jsuruntime, there is no special parameter for Allocator (or Loop), there's simply a session_state.arena. We could add a session_state.session_arena and/or a session_state.allocator
There was a problem hiding this comment.
Good catch 👍
Did you see other places in that case in web api implementations?
There was a problem hiding this comment.
Somewhat related, we see this problem with XHR, where the page_arena is disposed of, clearing out the XHR object, but there are still inflight IO events. It crashes the app. It happens if the page load is slow, and the CDP client disconnects due to a timeout. It's on my radar.
The only other thing which I think currently outlives the page is the storage shed, but I just looked at the code again, and it seems OK as it's using the Session's arena.
fix #525