Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1820,6 +1820,12 @@ pub struct RuntimeHooks {
/// never lazily created. Called from `WebWorker::shutdown` / `global_exit`
/// right after `close_all_socket_groups`.
pub close_dns_for_terminate: fn(),
/// `ObjectURLRegistry::singleton().sweep_owner(owner)` — drop every
/// `URL.createObjectURL` entry registered by the given script-execution
/// context. Called from `WebWorker::shutdown` so a worker that never calls
/// `revokeObjectURL` does not pin its blob payloads for the lifetime of
/// the process. The registry lives in `bun_runtime::webcore` (forward-dep).
Comment thread
robobun marked this conversation as resolved.
pub sweep_object_urls_for_owner: fn(owner: i32),
}

/// Canonical `EventLoopCtx` vtable for a `*mut VirtualMachine` owner — the JS
Expand Down
7 changes: 7 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1313,6 +1313,13 @@ impl WebWorker {
if let Some(rare) = vm.rare_data.as_deref_mut() {
rare.release_js_handles();
}
if let Some(hooks) = runtime_hooks() {
// The process-global blob-URL registry has no per-context
// teardown path of its own; without this sweep a worker that
// calls URL.createObjectURL and exits without revoking pins
// the blob payload for the lifetime of the process.
Comment thread
robobun marked this conversation as resolved.
Outdated
(hooks.sweep_object_urls_for_owner)(vm.initial_script_execution_context_identifier);
}
exit_code = i32::from(vm.exit_handler.exit_code);
global_object = Some(vm.global);
}
Expand Down
7 changes: 7 additions & 0 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,7 @@ pub(crate) static __BUN_RUNTIME_HOOKS: RuntimeHooks = RuntimeHooks {
retroactively_report_discovered_tests,
cancel_all_timers,
close_dns_for_terminate,
sweep_object_urls_for_owner,
};

// ════════════════════════════════════════════════════════════════════════════
Expand Down Expand Up @@ -1636,6 +1637,12 @@ fn close_dns_for_terminate() {
}
}

/// `RuntimeHooks::sweep_object_urls_for_owner` — drop every blob-URL registry
/// entry whose recording context is `owner`.
Comment thread
robobun marked this conversation as resolved.
fn sweep_object_urls_for_owner(owner: i32) {
crate::webcore::object_url_registry::ObjectURLRegistry::singleton().sweep_owner(owner);
}

pub(crate) fn close_isolation_handles(vm: &mut VirtualMachine) {
let state = runtime_state();
if state.is_null() {
Expand Down
31 changes: 27 additions & 4 deletions src/runtime/webcore/ObjectURLRegistry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,25 @@ impl Default for ObjectURLRegistry {

pub struct Entry {
blob: Blob,
/// `VirtualMachine::initial_script_execution_context_identifier` of the
/// registering context. `WebWorker::shutdown` sweeps entries owned by the
/// dying context so a worker that never calls `revokeObjectURL` does not
/// pin its blob payloads for the lifetime of the process.
Comment thread
robobun marked this conversation as resolved.
Outdated
owner: i32,
}

// `Entry` is auto-`Send`: its sole field is `Blob`, which already asserts
// `Send + Sync` (see `webcore_types::Blob`). No `unsafe impl` needed.
// `Entry` is auto-`Send`: `Blob` already asserts `Send + Sync`
// (see `webcore_types::Blob`). No `unsafe impl` needed.
Comment thread
robobun marked this conversation as resolved.
const _: fn() = || {
fn assert_send<T: Send>() {}
assert_send::<Entry>();
};

impl Entry {
pub fn init(blob: &Blob) -> Box<Entry> {
pub fn init(blob: &Blob, owner: i32) -> Box<Entry> {
Box::new(Entry {
blob: blob.dupe_with_content_type(true),
owner,
})
}
}
Expand All @@ -54,8 +60,9 @@ impl Drop for Entry {

impl ObjectURLRegistry {
pub fn register(&self, vm: &mut VirtualMachine, blob: &Blob) -> UUID {
let owner = vm.initial_script_execution_context_identifier;
let uuid = vm.rare_data().next_uuid();
let entry = Entry::init(blob);
let entry = Entry::init(blob, owner);

self.map.lock().insert(uuid.bytes, entry);
uuid
Expand Down Expand Up @@ -97,6 +104,22 @@ impl ObjectURLRegistry {
};
self.map.lock().contains_key(&uuid.bytes)
}

/// Drop every entry registered by `owner`. Called from
/// `WebWorker::shutdown` (via `RuntimeHooks`) so blob URLs minted inside a
/// worker are released when that worker exits. Entries from the main
/// context (or other still-live workers) are left untouched.
Comment thread
robobun marked this conversation as resolved.
pub fn sweep_owner(&self, owner: i32) {
let mut map = self.map.lock();
let keys: Vec<[u8; 16]> = map
.iter()
.filter(|(_, e)| e.owner == owner)
.map(|(k, _)| *k)
.collect();
for k in keys {
let _ = map.remove(&k);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

fn uuid_from_pathname(pathname: &[u8]) -> Option<UUID> {
Expand Down
91 changes: 91 additions & 0 deletions test/js/web/workers/worker_blob.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows } from "harness";

test("Worker from a Blob", async () => {
const worker = new Worker(
Expand Down Expand Up @@ -124,3 +125,93 @@

expect(revoked).toBe("revoked.");
});

// The blob-URL registry is process-global. Before the per-owner sweep, an entry
// registered inside a worker stayed in the map after the worker exited, and the
// duped blob store pinned the payload for the lifetime of the process (roughly
// 1 MB retained per MB minted per dead worker). After the sweep, a dead
// worker's URL resolves like a revoked URL and the payload is released.
//
// Skipped on Windows: RSS there does not drop after worker threads exit (the
// per-thread mimalloc arenas stay committed), so allocator residue alone
// exceeds the threshold regardless of whether the entries are released.
test.skipIf(isWindows)(
"blob URLs created inside a Worker are released when the Worker exits",

Check warning on line 139 in test/js/web/workers/worker_blob.test.ts

View check run for this annotation

Claude / Claude Code Review

Windows skip drops all functional coverage of the sweep, not just the RSS check

The `test.skipIf(isWindows)` gates the entire test because RSS doesn't reliably drop on Windows, but that also skips the two RSS-independent behavioral assertions — `deadUrlServed === false` and `parentUrlStillResolves === true` — which are the direct checks of the sweep semantics. Consider running the test on all platforms and gating only the `expect(growthMB).toBeLessThan(40)` line on `!isWindows`, so Windows CI still covers the functional behavior (per REVIEW.md: "branch per-platform rather t
Comment thread
robobun marked this conversation as resolved.
Outdated
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const workerBody = ${JSON.stringify(`
const urls = [];
for (let i = 0; i < 4; i++) {
const u8 = new Uint8Array(1 << 20);
u8.fill(65);
urls.push(URL.createObjectURL(new Blob([u8])));
}
postMessage(urls);
`)};
const src = URL.createObjectURL(
new Blob([workerBody], { type: "application/javascript" }),
);

async function runWorker() {
const w = new Worker(src);
const urls = await new Promise(r => (w.onmessage = e => r(e.data)));

Check warning on line 161 in test/js/web/workers/worker_blob.test.ts

View check run for this annotation

Claude / Claude Code Review

runWorker() promise has no rejection path — hangs on worker error

The `runWorker()` helper awaits `new Promise(r => (w.onmessage = e => r(e.data)))` with no rejection path — if a worker errors before `postMessage` (thread spawn failure, OOM on the 4×1 MB allocs under ASAN, etc.), the promise never settles and the subprocess hangs until the outer 60 s timeout with no diagnostic. REVIEW.md ('Tests reviewers reject') is categorical: *'Wire EVERY failure event (`error`, `close`, `abort`, process exit) to reject the awaited promise'*. Add `w.onerror = reject` (usin
Comment thread
robobun marked this conversation as resolved.
Outdated
await new Promise(r => w.addEventListener("close", r, { once: true }));
return urls;
}

// Establish the allocator high-water mark before measuring.
for (let i = 0; i < 3; i++) await runWorker();
Bun.gc(true);
Bun.gc(true);

const rss0 = process.memoryUsage.rss();
let deadUrl;
for (let i = 0; i < 30; i++) {
const urls = await runWorker();
deadUrl ??= urls[0];
}
Bun.gc(true);
Bun.gc(true);
const growthMB = (process.memoryUsage.rss() - rss0) / 2 ** 20;

let deadUrlServed = false;
try {
await fetch(deadUrl);
deadUrlServed = true;
} catch {}

// The parent-minted worker-source URL must survive the sweep.
const parentUrlStillResolves = (await (await fetch(src)).text()) === workerBody;

console.log(JSON.stringify({ growthMB, deadUrlServed, parentUrlStillResolves }));
`,
],
env: {
...bunEnv,
// Under ASAN the 1 MB payload frees go into the quarantine (default
// quarantine_size_mb=256) instead of being returned, so the RSS delta
// measures the quarantine rather than the registry. Disable it so the
// threshold is meaningful on ASAN builds.
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0"].filter(Boolean).join(":"),
},
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
const { growthMB, deadUrlServed, parentUrlStillResolves } = JSON.parse(stdout);
// 30 workers * 4 MB: when leaking, growth is ~120 MB; when swept, growth
// is allocator noise (typically under 15 MB even on debug builds).
expect(growthMB).toBeLessThan(40);
expect(deadUrlServed).toBe(false);
expect(parentUrlStillResolves).toBe(true);
expect(exitCode).toBe(0);
},
60_000,
);
Loading