Merge pull request #1412 from lightpanda-io/response_arena

Add finalizer to Response and use an pooled arena

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);

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 {

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", .{});

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, .{});

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 {

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;
     }

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();
     }