Merge pull request #296 from lightpanda-io/memory_leak

cdp: fix memory leak in msg parsing of the JSON

src/cdp/browser.zig

@@ -23,6 +23,7 @@ const Ctx = server.Ctx;
 const cdp = @import("cdp.zig");
 const result = cdp.result;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -62,7 +63,8 @@ fn getVersion(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "browser.getVersion" });
 
     // ouput
@@ -89,7 +91,8 @@ fn setDownloadBehavior(
         downloadPath: ?[]const u8 = null,
         eventsEnabled: ?bool = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("REQ > id {d}, method {s}", .{ input.id, "browser.setDownloadBehavior" });
 
     // output
@@ -109,7 +112,8 @@ fn getWindowForTarget(
     const Params = struct {
         targetId: ?[]const u8 = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     std.debug.assert(input.sessionId != null);
     log.debug("Req > id {d}, method {s}", .{ input.id, "browser.getWindowForTarget" });
 
@@ -133,9 +137,10 @@ fn setWindowBounds(
     msg: *IncomingMessage,
     _: *Ctx,
 ) ![]const u8 {
-    const input = try msg.getInput(alloc, void);
 
     // input
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "browser.setWindowBounds" });
 
     // output

src/cdp/emulation.zig

@@ -24,6 +24,7 @@ const cdp = @import("cdp.zig");
 const result = cdp.result;
 const stringify = cdp.stringify;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -67,7 +68,8 @@ fn setEmulatedMedia(
         media: ?[]const u8 = null,
         features: ?[]MediaFeature = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setEmulatedMedia" });
 
     // output
@@ -84,7 +86,8 @@ fn setFocusEmulationEnabled(
     const Params = struct {
         enabled: bool,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setFocusEmulationEnabled" });
 
     // output
@@ -98,7 +101,8 @@ fn setDeviceMetricsOverride(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setDeviceMetricsOverride" });
 
     // output
@@ -111,7 +115,8 @@ fn setTouchEmulationEnabled(
     msg: *IncomingMessage,
     _: *Ctx,
 ) ![]const u8 {
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "emulation.setTouchEmulationEnabled" });
 
     return result(alloc, input.id, null, null, input.sessionId);

src/cdp/fetch.zig

@@ -23,6 +23,7 @@ const Ctx = server.Ctx;
 const cdp = @import("cdp.zig");
 const result = cdp.result;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -50,7 +51,8 @@ fn disable(
     msg: *IncomingMessage,
     _: *Ctx,
 ) ![]const u8 {
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "fetch.disable" });
 
     return result(alloc, input.id, null, null, input.sessionId);

src/cdp/log.zig

@@ -23,6 +23,7 @@ const Ctx = server.Ctx;
 const cdp = @import("cdp.zig");
 const result = cdp.result;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 const stringify = cdp.stringify;
 
 const log_cdp = std.log.scoped(.cdp);
@@ -50,7 +51,8 @@ fn enable(
     msg: *IncomingMessage,
     _: *Ctx,
 ) ![]const u8 {
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log_cdp.debug("Req > id {d}, method {s}", .{ input.id, "log.enable" });
 
     return result(alloc, input.id, null, null, input.sessionId);

src/cdp/msg.zig

@@ -136,8 +136,9 @@ pub const IncomingMessage = struct {
     }
 
     // getParams restart the JSON parsing
-    fn getParams(self: *IncomingMessage, alloc: std.mem.Allocator, T: type) !T {
+    fn getParams(self: *IncomingMessage, alloc: ?std.mem.Allocator, T: type) !T {
         if (T == void) return void{};
+        std.debug.assert(alloc != null); // if T is not void, alloc should not be null
 
         if (self.params_skip) {
             // TODO if the params have been skipped, we have to retart the
@@ -150,20 +151,56 @@ pub const IncomingMessage = struct {
         // parse "params"
         const options = std.json.ParseOptions{
             .max_value_len = self.scanner.input.len,
-            .allocate = .alloc_if_needed,
+            .allocate = .alloc_always,
-        };
-        return try std.json.innerParse(T, alloc, &self.scanner, options);
-    }
-
-    pub fn getInput(self: *IncomingMessage, alloc: std.mem.Allocator, T: type) !struct { id: u16, sessionId: ?[]const u8, params: T } {
-        return .{
-            .params = try self.getParams(alloc, T),
-            .id = try self.getId(),
-            .sessionId = try self.getSessionId(),
         };
+        return try std.json.innerParse(T, alloc.?, &self.scanner, options);
     }
 };
 
+pub fn Input(T: type) type {
+    return struct {
+        arena: ?*std.heap.ArenaAllocator = null,
+        id: u16,
+        params: T,
+        sessionId: ?[]const u8,
+
+        const Self = @This();
+
+        pub fn get(alloc: std.mem.Allocator, msg: *IncomingMessage) !Self {
+            var arena: ?*std.heap.ArenaAllocator = null;
+            var allocator: ?std.mem.Allocator = null;
+
+            if (T != void) {
+                arena = try alloc.create(std.heap.ArenaAllocator);
+                arena.?.* = std.heap.ArenaAllocator.init(alloc);
+                allocator = arena.?.allocator();
+            }
+
+            errdefer {
+                if (arena) |_arena| {
+                    _arena.deinit();
+                    alloc.destroy(_arena);
+                }
+            }
+
+            return .{
+                .arena = arena,
+                .params = try msg.getParams(allocator, T),
+                .id = try msg.getId(),
+                .sessionId = try msg.getSessionId(),
+            };
+        }
+
+        pub fn deinit(self: Self) void {
+            if (self.arena) |arena| {
+                const allocator = arena.child_allocator;
+                arena.deinit();
+                allocator.destroy(arena);
+            }
+        }
+    };
+}
+
 test "read incoming message" {
     const inputs = [_][]const u8{
         \\{"id":1,"method":"foo","sessionId":"bar","params":{"bar":"baz"}}
@@ -185,11 +222,12 @@ test "read incoming message" {
         try std.testing.expectEqualSlices(u8, "bar", (try msg.getSessionId()).?);
 
         const T = struct { bar: []const u8 };
-        const in = msg.getInput(std.testing.allocator, T) catch |err| {
+        const in = Input(T).get(std.testing.allocator, &msg) catch |err| {
             if (err != error.SkippedParams) return err;
             // TODO remove this check when params in the beginning is handled.
             continue;
         };
+        defer in.deinit();
         try std.testing.expectEqualSlices(u8, "baz", in.params.bar);
     }
 }

src/cdp/network.zig

@@ -23,6 +23,7 @@ const Ctx = server.Ctx;
 const cdp = @import("cdp.zig");
 const result = cdp.result;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -52,7 +53,8 @@ fn enable(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "network.enable" });
 
     return result(alloc, input.id, null, null, input.sessionId);
@@ -65,7 +67,8 @@ fn setCacheDisabled(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "network.setCacheDisabled" });
 
     return result(alloc, input.id, null, null, input.sessionId);

src/cdp/page.zig

@@ -25,6 +25,7 @@ const result = cdp.result;
 const stringify = cdp.stringify;
 const sendEvent = cdp.sendEvent;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -63,7 +64,8 @@ fn enable(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "page.enable" });
 
     return result(alloc, input.id, null, null, input.sessionId);
@@ -90,7 +92,8 @@ fn getFrameTree(
     ctx: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "page.getFrameTree" });
 
     // output
@@ -142,7 +145,8 @@ fn setLifecycleEventsEnabled(
     const Params = struct {
         enabled: bool,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "page.setLifecycleEventsEnabled" });
 
     ctx.state.page_life_cycle_events = true;
@@ -171,7 +175,8 @@ fn addScriptToEvaluateOnNewDocument(
         includeCommandLineAPI: bool = false,
         runImmediately: bool = false,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "page.addScriptToEvaluateOnNewDocument" });
 
     // output
@@ -205,7 +210,8 @@ fn createIsolatedWorld(
         worldName: []const u8,
         grantUniveralAccess: bool,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     std.debug.assert(input.sessionId != null);
     log.debug("Req > id {d}, method {s}", .{ input.id, "page.createIsolatedWorld" });
 
@@ -247,7 +253,8 @@ fn navigate(
         frameId: ?[]const u8 = null,
         referrerPolicy: ?[]const u8 = null, // TODO: enum
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     std.debug.assert(input.sessionId != null);
     log.debug("Req > id {d}, method {s}", .{ input.id, "page.navigate" });
 

src/cdp/performance.zig

@@ -23,6 +23,7 @@ const Ctx = server.Ctx;
 const cdp = @import("cdp.zig");
 const result = cdp.result;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -50,7 +51,8 @@ fn enable(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "performance.enable" });
 
     return result(alloc, input.id, null, null, input.sessionId);

src/cdp/runtime.zig

@@ -26,6 +26,7 @@ const Ctx = server.Ctx;
 const cdp = @import("cdp.zig");
 const result = cdp.result;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 const stringify = cdp.stringify;
 
 const log = std.log.scoped(.cdp);
@@ -77,9 +78,13 @@ fn sendInspector(
                 userGesture: ?bool = null,
             };
 
-            const input = try msg.getInput(alloc, Params);
+            const input = try Input(Params).get(alloc, msg);
+            defer input.deinit();
             log.debug("Req > id {d}, method {s} (script saved on cache)", .{ input.id, "runtime.evaluate" });
-            script = input.params.expression;
+            const params = input.params;
+            const func = try alloc.alloc(u8, params.expression.len);
+            @memcpy(func, params.expression);
+            script = func;
             id = input.id;
         } else if (method == .callFunctionOn) {
             const Params = struct {
@@ -95,14 +100,19 @@ fn sendInspector(
                 userGesture: ?bool = null,
             };
 
-            const input = try msg.getInput(alloc, Params);
+            const input = try Input(Params).get(alloc, msg);
+            defer input.deinit();
             log.debug("Req > id {d}, method {s} (script saved on cache)", .{ input.id, "runtime.callFunctionOn" });
-            script = input.params.functionDeclaration;
+            const params = input.params;
+            const func = try alloc.alloc(u8, params.functionDeclaration.len);
+            @memcpy(func, params.functionDeclaration);
+            script = func;
             id = input.id;
         }
 
         if (script) |src| {
             try cdp.dumpFile(alloc, id, src);
+            alloc.free(src);
         }
     }
 
@@ -163,7 +173,8 @@ fn runIfWaitingForDebugger(
     msg: *IncomingMessage,
     _: *Ctx,
 ) ![]const u8 {
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "runtime.runIfWaitingForDebugger" });
 
     return result(alloc, input.id, null, null, input.sessionId);

src/cdp/target.zig

@@ -24,6 +24,7 @@ const cdp = @import("cdp.zig");
 const result = cdp.result;
 const stringify = cdp.stringify;
 const IncomingMessage = @import("msg.zig").IncomingMessage;
+const Input = @import("msg.zig").Input;
 
 const log = std.log.scoped(.cdp);
 
@@ -72,7 +73,8 @@ fn setDiscoverTargets(
     _: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.setDiscoverTargets" });
 
     // output
@@ -111,7 +113,8 @@ fn setAutoAttach(
         flatten: bool = true,
         filter: ?[]TargetFilter = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.setAutoAttach" });
 
     // attachedToTarget event
@@ -144,7 +147,8 @@ fn attachToTarget(
         targetId: []const u8,
         flatten: bool = true,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.attachToTarget" });
 
     // attachedToTarget event
@@ -180,7 +184,8 @@ fn getTargetInfo(
     const Params = struct {
         targetId: ?[]const u8 = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.getTargetInfo" });
 
     // output
@@ -213,7 +218,8 @@ fn getBrowserContexts(
     ctx: *Ctx,
 ) ![]const u8 {
     // input
-    const input = try msg.getInput(alloc, void);
+    const input = try Input(void).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.getBrowserContexts" });
 
     // ouptut
@@ -246,7 +252,8 @@ fn createBrowserContext(
         proxyBypassList: ?[]const u8 = null,
         originsWithUniversalNetworkAccess: ?[][]const u8 = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.createBrowserContext" });
 
     ctx.state.contextID = ContextID;
@@ -279,7 +286,8 @@ fn disposeBrowserContext(
     const Params = struct {
         browserContextId: []const u8,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.disposeBrowserContext" });
 
     // output
@@ -309,7 +317,8 @@ fn createTarget(
         background: bool = false,
         forTab: ?bool = null,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.createTarget" });
 
     // change CDP state
@@ -326,7 +335,7 @@ fn createTarget(
             .targetId = ctx.state.frameID,
             .title = "",
             .url = ctx.state.url,
-            .browserContextId = msg.params.?.browserContextId orelse ContextID,
+            .browserContextId = input.params.browserContextId orelse ContextID,
         },
         .waitingForDebugger = true,
     };
@@ -360,7 +369,8 @@ fn closeTarget(
     const Params = struct {
         targetId: []const u8,
     };
-    const input = try msg.getInput(alloc, Params);
+    const input = try Input(Params).get(alloc, msg);
+    defer input.deinit();
     log.debug("Req > id {d}, method {s}", .{ input.id, "target.closeTarget" });
 
     // output

src/main.zig

@@ -158,12 +158,27 @@ fn printUsageExit(execname: []const u8, res: u8) void {
 pub fn main() !void {
 
     // allocator
-    var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator);
+    // - in Debug mode we use the General Purpose Allocator to detect memory leaks
-    defer arena.deinit();
+    // - in Release mode we use the page allocator
-    const alloc = arena.allocator();
+    var alloc: std.mem.Allocator = undefined;
+    var _gpa: ?std.heap.GeneralPurposeAllocator(.{}) = null;
+    if (builtin.mode == .Debug) {
+        _gpa = std.heap.GeneralPurposeAllocator(.{}){};
+        alloc = _gpa.?.allocator();
+    } else {
+        alloc = std.heap.page_allocator;
+    }
+    defer {
+        if (_gpa) |*gpa| {
+            switch (gpa.deinit()) {
+                .ok => std.debug.print("No memory leaks\n", .{}),
+                .leak => @panic("Memory leak"),
+            }
+        }
+    }
 
     // args
-    var args = try std.process.argsWithAllocator(arena.allocator());
+    var args = try std.process.argsWithAllocator(alloc);
     defer args.deinit();
 
     const execname = args.next().?;
@@ -263,7 +278,7 @@ pub fn main() !void {
         std.log.info("Listening on: {s}:{d}...", .{ host, port });
 
         // loop
-        var loop = try jsruntime.Loop.init(arena.allocator());
+        var loop = try jsruntime.Loop.init(alloc);
         defer loop.deinit();
 
         // listen
@@ -279,7 +294,7 @@ pub fn main() !void {
         const vm = jsruntime.VM.init();
         defer vm.deinit();
 
-        var loop = try jsruntime.Loop.init(arena.allocator());
+        var loop = try jsruntime.Loop.init(alloc);
         defer loop.deinit();
 
         var browser = Browser{};