From 79cdbd285df14348f1e603d29edf875275c0b766 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Sun, 31 May 2026 18:39:26 +0800 Subject: [PATCH] Improve forced terminate on CDP client disconnect. Depends on https://github.com/lightpanda-io/zig-v8-fork/pull/179 An improvement to https://github.com/lightpanda-io/browser/pull/2515 to prevent a v8 assertion if we terminate as an inspector dispatch is happening. The problem is that if we just immediately terminate, we aren't sure what the worker thread is doing, and, apparently, if we terminate then dispatch a message to the inspector, we fail an assertion. With the way the code was, the only safe solution would be to hold a mutex over the session dispatch, but that could block the network thread. So instead of terminating from the network thread, we now ask v8 to execute a callback. This gets executed on the worker thread, which can then terminate the execution. The initial version of 2515 delayed the termination from the network thread. It's possible that solution would "solve" the issue, simply because it's very unlikely that a worker would be "stuck" for 5 seconds and then get unstuck. More likely that it exits immediately, or is stuck in an endless loop. But that would still leave a window where we could terminate in network and then dispatch in the worker. Less likely, but still possible. Hopefully this new mechanism eliminates this from being a problem in all circumstances. --- .github/actions/install/action.yml | 2 +- Dockerfile | 2 +- build.zig.zon | 4 ++-- src/browser/js/Env.zig | 26 +++++++++++++++++++++++++- src/cdp/CDP.zig | 7 ++++++- 5 files changed, 35 insertions(+), 6 deletions(-) diff --git a/.github/actions/install/action.yml b/.github/actions/install/action.yml index e0ab9295..24593371 100644 --- a/.github/actions/install/action.yml +++ b/.github/actions/install/action.yml @@ -13,7 +13,7 @@ inputs: zig-v8: description: 'zig v8 version to install' required: false - default: 'v0.4.5' + default: 'v0.4.6' v8: description: 'v8 version to install' required: false diff --git a/Dockerfile b/Dockerfile index 24a99071..8cd5e1eb 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,7 +3,7 @@ FROM debian:stable-slim ARG MINISIG=0.12 ARG ZIG_MINISIG=RWSGOq2NVecA2UPNdBUZykf1CCb147pkmdtYxgb3Ti+JO/wCYvhbAb/U ARG V8=14.0.365.4 -ARG ZIG_V8=v0.4.5 +ARG ZIG_V8=v0.4.6 ARG TARGETPLATFORM RUN apt-get update -yq && \ diff --git a/build.zig.zon b/build.zig.zon index d891d903..b0ca9ea3 100644 --- a/build.zig.zon +++ b/build.zig.zon @@ -5,8 +5,8 @@ .minimum_zig_version = "0.15.2", .dependencies = .{ .v8 = .{ - .url = "https://github.com/lightpanda-io/zig-v8-fork/archive/refs/tags/v0.4.5.tar.gz", - .hash = "v8-0.0.0-xddH65CXBADLRFlCM_pECcwsoY-9P9mZ7VnYM-6V3mXW", + .url = "https://github.com/lightpanda-io/zig-v8-fork/archive/refs/tags/v0.4.6.tar.gz", + .hash = "v8-0.0.0-xddH67-YBABDa7EPYFKQsWLdpK8xJvVVmDsKIaGPGID7", }, // .v8 = .{ .path = "../zig-v8-fork" }, .brotli = .{ diff --git a/src/browser/js/Env.zig b/src/browser/js/Env.zig index 9e3ffcb9..cf69a6fa 100644 --- a/src/browser/js/Env.zig +++ b/src/browser/js/Env.zig @@ -96,6 +96,10 @@ microtask_queues_are_running: bool, // IsExecutionTerminating check and PerformCheckpoint trips a V8 debug assert. terminate_mutex: std.Thread.Mutex = .{}, +// Set from network thread, saying termination should happen. Read from worker +// thread making sure terminate hasn't been canceled. +terminate_requested: std.atomic.Value(bool) = .init(false), + pub const InitOpts = struct { with_inspector: bool = false, }; @@ -503,18 +507,38 @@ pub fn dumpMemoryStats(self: *Env) void { , .{ stats.total_heap_size, stats.total_heap_size_executable, stats.total_physical_size, stats.total_available_size, stats.used_heap_size, stats.heap_size_limit, stats.malloced_memory, stats.external_memory, stats.peak_malloced_memory, stats.number_of_native_contexts, stats.number_of_detached_contexts, stats.total_global_handles_size, stats.used_global_handles_size, stats.does_zap_garbage }); } +pub fn isExecutionTerminating(self: *const Env) bool { + return v8.v8__Isolate__IsExecutionTerminating(self.isolate.handle); +} + pub fn terminate(self: *Env) void { self.terminate_mutex.lock(); defer self.terminate_mutex.unlock(); v8.v8__Isolate__TerminateExecution(self.isolate.handle); } +// Called from the network thread, caused v8 to eventually call terminateInterrupt +pub fn requestTerminate(self: *Env) void { + self.terminate_requested.store(true, .release); + v8.v8__Isolate__RequestInterrupt(self.isolate.handle, terminateInterrupt, self); +} + +// Runs on the worker thread +fn terminateInterrupt(_: ?*v8.Isolate, data: ?*anyopaque) callconv(.c) void { + const self: *Env = @ptrCast(@alignCast(data.?)); + if (self.terminate_requested.load(.acquire)) { + v8.v8__Isolate__TerminateExecution(self.isolate.handle); + } +} + /// Clears a pending termination so V8 calls (e.g. those made during cleanup) /// don't keep tripping over the terminating-state asserts. Safe to call -/// unconditionally; a no-op if termination wasn't pending. +/// unconditionally; a no-op if termination wasn't pending. Also clears the +/// requestTerminate gate so any still-pending interrupt becomes a no-op. pub fn cancelTerminate(self: *Env) void { self.terminate_mutex.lock(); defer self.terminate_mutex.unlock(); + self.terminate_requested.store(false, .release); v8.v8__Isolate__CancelTerminateExecution(self.isolate.handle); } diff --git a/src/cdp/CDP.zig b/src/cdp/CDP.zig index 02810273..57bef94c 100644 --- a/src/cdp/CDP.zig +++ b/src/cdp/CDP.zig @@ -171,7 +171,7 @@ pub fn onLinkDisconnect(self: *CDP, err: ?anyerror) void { // Called by Network to try to force the Worker to shutdown. Protects against a // stuck worker. pub fn terminateFromNetwork(self: *CDP) void { - self.browser.env.terminate(); + self.browser.env.requestTerminate(); } // Called in the Worker to dispatch a single CDP message bubbled up by @@ -182,6 +182,11 @@ pub fn terminateFromNetwork(self: *CDP) void { // frees right after we return), so `c.input`'s string slices stay // valid for the duration of dispatch. pub fn onMessage(self: *CDP, c: *Inbox.Message.Cdp) anyerror!void { + // Once a terminate is pending, don't dispatch + if (self.browser.env.isExecutionTerminating()) { + return; + } + const arena = &self.message_arena; defer _ = arena.reset(.{ .retain_with_limit = 1024 * 16 }); return self.dispatchParsed(arena.allocator(), .{ .cdp = self }, c.raw, c.input);