diff --git a/src/bun.js/bindings/webcore/Worker.cpp b/src/bun.js/bindings/webcore/Worker.cpp index 8db7b462b33a..988eb44b3726 100644 --- a/src/bun.js/bindings/webcore/Worker.cpp +++ b/src/bun.js/bindings/webcore/Worker.cpp @@ -71,6 +71,8 @@ WTF_MAKE_TZONE_ALLOCATED_IMPL(Worker); extern "C" void WebWorker__notifyNeedTermination( void* worker); +extern "C" void WebWorker__destroy( + void* worker); static Lock allWorkersLock; static HashMap& allWorkers() WTF_REQUIRES_LOCK(allWorkersLock) @@ -228,6 +230,9 @@ Worker::~Worker() Locker locker { allWorkersLock }; allWorkers().remove(m_clientIdentifier); } + if (impl_) { + WebWorker__destroy(impl_); + } // m_contextProxy.workerObjectDestroyed(); } @@ -262,7 +267,8 @@ ExceptionOr Worker::postMessage(JSC::JSGlobalObject& state, JSC::JSValue m void Worker::terminate() { // m_contextProxy.terminateWorkerGlobalScope(); - m_terminationFlags.fetch_or(TerminateRequestedFlag); + if (m_terminationFlags.fetch_or(TerminateRequestedFlag)) + return; WebWorker__notifyNeedTermination(impl_); } diff --git a/src/bun.js/web_worker.zig b/src/bun.js/web_worker.zig index e55241a7e856..50fc984e703f 100644 --- a/src/bun.js/web_worker.zig +++ b/src/bun.js/web_worker.zig @@ -402,14 +402,22 @@ pub fn start( /// Deinit will clean up vm and everything. /// Early deinit may be called from caller thread, but full vm deinit will only be called within worker's thread. +/// The struct itself is freed separately by the owning C++ `Worker` via `WebWorker__destroy` so +/// that `WebWorker__notifyNeedTermination` never touches freed memory while JS still holds the +/// wrapper. fn deinit(this: *WebWorker) void { log("[{d}] deinit", .{this.execution_context_id}); this.parent_poll_ref.unrefConcurrently(this.parent); bun.default_allocator.free(this.unresolved_specifier); + this.unresolved_specifier = ""; for (this.preloads) |preload| { bun.default_allocator.free(preload); } bun.default_allocator.free(this.preloads); + this.preloads = &.{}; +} + +export fn WebWorker__destroy(this: *WebWorker) void { bun.default_allocator.destroy(this); } @@ -644,6 +652,12 @@ pub fn exitAndDeinit(this: *WebWorker) noreturn { } var arena = this.arena; + // Release owned resources before dispatching exit. `WebWorker__dispatchExit` drops the + // Zig-held ref on the C++ `Worker`; once the parent processes the close task and GC + // collects the wrapper, `~Worker()` will free this struct, so it must not be touched + // afterwards. + this.deinit(); + WebWorker__dispatchExit(globalObject, cpp_worker, exit_code); if (loop) |loop_| { loop_.internal_loop_data.jsc_vm = null; @@ -659,8 +673,6 @@ pub fn exitAndDeinit(this: *WebWorker) noreturn { bun.windows.libuv.Loop.shutdown(); } - this.deinit(); - if (vm_to_deinit) |vm| { vm.deinit(); // NOTE: deinit here isn't implemented, so freeing workers will leak the vm. } diff --git a/test/js/web/workers/worker-terminate-after-exit.test.ts b/test/js/web/workers/worker-terminate-after-exit.test.ts new file mode 100644 index 000000000000..5647c1d72fd5 --- /dev/null +++ b/test/js/web/workers/worker-terminate-after-exit.test.ts @@ -0,0 +1,47 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isDebug } from "harness"; + +// The Zig `WebWorker` struct used to be destroyed on the worker thread in +// `exitAndDeinit`, while `Worker::terminate()` on the main thread could still +// call `WebWorker__notifyNeedTermination(impl_)` afterwards, reading freed +// memory. Calling `terminate()` immediately sets `requested_terminate`, so the +// worker thread takes the fast early-exit path (before its VM is created) and +// the struct was freed right after the close event was posted. Calling +// `terminate()` again from the close handler then touched the freed struct. +test( + "Worker.terminate() after the worker thread has exited does not use freed memory", + async () => { + const code = ` + for (let i = 0; i < 10; i++) { + const w = new Worker("nonexistent-entrypoint-58146"); + const { promise, resolve } = Promise.withResolvers(); + w.addEventListener("close", resolve); + w.addEventListener("error", () => {}); + w.terminate(); + await promise; + w.terminate(); + } + Bun.gc(true); + `; + const concurrency = 5; + for (let batch = 0; batch < 4; batch++) { + const runs = Array.from({ length: concurrency }, async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", code], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + return { stderr, exitCode }; + }); + for (const { stderr, exitCode } of await Promise.all(runs)) { + if (exitCode !== 0) { + expect(stderr).toBe(""); + } + expect(exitCode).toBe(0); + } + } + }, + isDebug ? 60_000 : undefined, +);